diff --git a/docs/07-annotation-model.md b/docs/07-annotation-model.md index 166826c..43821e8 100644 --- a/docs/07-annotation-model.md +++ b/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. | --- diff --git a/paces/edits.py b/paces/edits.py index 64f05e0..04b2e2b 100644 --- a/paces/edits.py +++ b/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) diff --git a/paces/model.py b/paces/model.py index 4a2fd7d..bd252b7 100644 --- a/paces/model.py +++ b/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,6 +373,61 @@ 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. @@ -353,9 +435,11 @@ def validate_document(doc: StepDocument) -> list[str]: 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() diff --git a/tests/test_edit_protection_residuals.py b/tests/test_edit_protection_residuals.py new file mode 100644 index 0000000..d0abecf --- /dev/null +++ b/tests/test_edit_protection_residuals.py @@ -0,0 +1,254 @@ +"""Residuals from PR #11's adversarial review, tracked as issue #12. + +Each test below covers one item from the issue. Items declined (documented +instead of fixed, or explicitly deferred) are not tested here — see the issue +for the one-line reasons. +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from paces.edits import apply_edits, merge_regenerated +from paces.model import ( + Lock, + Measure, + Source, + SourceSpan, + Step, + StepDocument, + validate_document, +) + + +def _doc(**overrides) -> StepDocument: + fields = dict( + id="guide", + title="A guide", + steps=[Step(id="a", name="Step A", duration=Measure(value="2", unit="eight"))], + ) + fields.update(overrides) + return StepDocument(**fields) + + +# ── Lock.by / Lock.at validation ───────────────────────────────────────────── + + +def test_lock_by_rejects_empty_string(): + with pytest.raises(ValidationError): + Lock(path="/name", by="", at="2026-08-30T00:00:00Z") + + +def test_lock_by_rejects_whitespace_only(): + with pytest.raises(ValidationError): + Lock(path="/name", by=" ", at="2026-08-30T00:00:00Z") + + +def test_lock_at_rejects_junk_timestamp(): + with pytest.raises(ValidationError): + Lock(path="/name", by="user:thor", at="not-a-timestamp") + + +def test_lock_at_rejects_non_utc_offset(): + with pytest.raises(ValidationError): + Lock(path="/name", by="user:thor", at="2026-08-30T00:00:00+02:00") + + +def test_lock_at_accepts_the_format_edits_writes(): + Lock(path="/name", by="user:thor", at="2026-08-30T00:00:00Z") + + +# ── Lock.was float leak (validate_document) ────────────────────────────────── + + +def test_validate_document_flags_float_leaked_into_lock_was(): + doc = _doc( + steps=[ + Step( + id="a", + name="Step A", + duration=Measure(value="2", unit="eight"), + locks=[ + Lock( + path="/name", by="user:thor", at="2026-08-30T00:00:00Z", was=1.5 + ) + ], + ) + ] + ) + issues = validate_document(doc) + assert any("float" in issue for issue in issues) + + +def test_validate_document_flags_float_leaked_into_attrs(): + doc = _doc( + steps=[ + Step( + id="a", + name="Step A", + duration=Measure(value="2", unit="eight"), + attrs={"render.web": {"hue": 0.5}}, + ) + ] + ) + issues = validate_document(doc) + assert any("float" in issue for issue in issues) + + +def test_validate_document_does_not_flag_the_typed_confidence_float(): + doc = _doc( + steps=[ + Step( + id="a", + name="Step A", + duration=Measure(value="2", unit="eight"), + spans=[SourceSpan(source="vid", start="10", confidence=0.8)], + ) + ] + ) + issues = validate_document(doc) + assert not any("float" in issue for issue in issues) + + +def test_validate_document_does_not_flag_a_lock_was_holding_the_old_confidence(): + # apply_edits itself writes exactly this: a Lock.was carrying the + # pre-edit value of a schema-typed float field (SourceSpan.confidence). + doc = _doc( + steps=[ + Step( + id="a", + name="Step A", + duration=Measure(value="2", unit="eight"), + spans=[SourceSpan(source="vid", start="10", confidence=0.8)], + ) + ] + ) + edited = apply_edits( + doc, + [{"op": "set", "path": "/steps/a/spans/0/confidence", "value": 0.9}], + by="user:thor", + ) + issues = validate_document(edited) + assert not any("float" in issue for issue in issues) + + +def test_validate_document_clean_document_has_no_float_issues(): + assert validate_document(_doc()) == [] + + +# ── rename onto an existing id is refused at edit time ─────────────────────── + + +def _two_step_doc(**overrides) -> StepDocument: + fields = dict( + id="guide", + title="A guide", + sources=[Source(id="vid", kind="video", uri="https://example.com/v")], + steps=[ + Step( + id="a", + name="Step A", + duration=Measure(value="2", unit="eight"), + spans=[SourceSpan(source="vid", start="10")], + ), + Step( + id="b", + name="Step B", + duration=Measure(value="2", unit="eight"), + spans=[SourceSpan(source="vid", start="20")], + ), + ], + ) + fields.update(overrides) + return StepDocument(**fields) + + +def test_apply_edits_refuses_rename_onto_an_existing_id(): + with pytest.raises(ValueError, match="already used"): + apply_edits( + _two_step_doc(), + [{"op": "set", "path": "/steps/a/id", "value": "b"}], + by="user:thor", + ) + + +def test_apply_edits_allows_rename_onto_a_free_id(): + doc = apply_edits( + _two_step_doc(), + [{"op": "set", "path": "/steps/a/id", "value": "a2"}], + by="user:thor", + ) + assert [s.id for s in doc.steps] == ["a2", "b"] + + +def test_apply_edits_allows_renaming_id_to_itself(): + doc = apply_edits( + _two_step_doc(), + [{"op": "set", "path": "/steps/a/id", "value": "a"}], + by="user:thor", + ) + assert [s.id for s in doc.steps] == ["a", "b"] + + +def test_apply_edits_refuses_a_nested_rename_onto_a_top_level_id(): + doc = _two_step_doc( + steps=[ + Step( + id="a", + name="Step A", + duration=Measure(value="2", unit="eight"), + steps=[ + Step(id="c", name="Sub", duration=Measure(value="2", unit="eight")) + ], + ), + Step(id="b", name="Step B", duration=Measure(value="2", unit="eight")), + ] + ) + # "b" isn't a sibling of "c" (it lives one level up) — the collision must + # still be caught document-wide, matching validate_document's own scope. + with pytest.raises(ValueError, match="already used"): + apply_edits( + doc, + [{"op": "set", "path": "/steps/a/steps/c/id", "value": "b"}], + by="user:thor", + ) + + +# ── merge after an id rename does not resurrect the old id ────────────────── + + +def test_merge_regenerated_does_not_resurrect_the_pre_rename_id(): + committed = apply_edits( + _two_step_doc(), + [{"op": "set", "path": "/steps/a/id", "value": "a2"}], + by="user:thor", + ) + # Analysis re-runs without knowledge of the rename: it still emits "a". + fresh = _two_step_doc() + merged = merge_regenerated(committed, fresh) + assert [s.id for s in merged.steps] == ["a2", "b"] + assert merged.steps[0].name == "Step A" + + +def test_merge_regenerated_does_not_double_match_when_fresh_has_both_ids(): + committed = apply_edits( + _two_step_doc(), + [{"op": "set", "path": "/steps/a/id", "value": "a2"}], + by="user:thor", + ) + # Fresh carries BOTH the old id and the already-renamed id (e.g. analysis + # caught up and emitted "a2", but a stray "a" is also present) — the + # renamed committed step must not be matched twice. + fresh = _two_step_doc( + steps=[ + Step(id="a", name="Step A", duration=Measure(value="2", unit="eight")), + Step(id="a2", name="Step A", duration=Measure(value="2", unit="eight")), + Step(id="b", name="Step B", duration=Measure(value="2", unit="eight")), + ] + ) + merged = merge_regenerated(committed, fresh) + ids = [s.id for s in merged.steps] + assert ids == ["a", "a2", "b"] + assert len(ids) == len(set(ids)) + assert validate_document(merged) == []