fix(session-recorder): defense-in-depth against impossible turn timing (#23 postmortem)#24
Merged
Merged
Conversation
#23 postmortem) PR #23 fixed a real bug where a long wrapping prompt fooled the pixel-based turn-boundary detector into producing a turn whose "hard" segment was mathematically impossible (shorter than the deterministic scripted typing time alone) — silently corrupting the ledger. It was only caught by a human eyeballing video frames, not by the pipeline itself. This adds two independent, complementary guards so the SAME class of bug (and others shaped like it) gets caught automatically next time, instead of waiting for another reactive patch. Layer 1 (detect_anchors.py, detect_turns()): a deterministic self-consistency floor requiring no external data. submit - typing_start must equal exactly type_dur + pre_enter (the scripted time to type the prompt and reach Enter) — a real submit can never be faster than that. The existing `max(0.0, ...)` clamp used to silently absorb a violation of this; now it raises immediately, naming the turn and the impossible numbers. Layer 2 (author.py, main()): a best-effort cross-check of the pixel-detected per-turn response window (done - submit) against session-timeline.jsonl's real UserPromptSubmit->Stop wall-clock delta for the same turn, when that file is available and its turns can be unambiguously correlated (mirrors panel.py's existing instant-turn-aware matching). This catches a DIFFERENT class of error Layer 1 cannot: a wrong-but-internally-self-consistent group selection that still satisfies Layer 1's floor. It degrades silently on a missing/malformed/uncorrelated timeline — never a hard requirement — and only raises on a wild disagreement (tolerance: max(5.0s, 50% of the real delta), chosen generously against detect_turns()'s own documented "many seconds" video/wall-clock drift risk during heavy output, while still comfortably catching a mismatch of the #23 bug's scale; validated against one real 110s-recording data point showing ~7% pixel-vs-wallclock agreement). Layer 2 intentionally lives in author.py, not detect_anchors.py: detect_turns already argues wall-clock timing can't be the PRIMARY source of truth (video drift), and author.py is the orchestration layer that already knows the demo path, per CONTRIBUTING.md's pure-logic/thin-I/O split. 12 new regression tests (3 Layer 1, 9 Layer 2); full suite green (151 passed, up from the pre-existing 139), including every #16-#23 recovery-path regression test unmodified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EwptawkV4dcGhoGefzxgV
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
PR #23 fixed a real bug where a long wrapping prompt fooled the pixel-based
turn-boundary detector into producing a turn whose "hard" segment was
mathematically impossible (shorter than the deterministic scripted typing
time alone) — silently corrupting the ledger. It was only caught by a human
eyeballing video frames, not by the pipeline itself.
The ask was to make this class of failure structurally harder to reintroduce
— "double robust" (雙重穩健) — rather than just patching the next reactive
report. This PR adds two independent, complementary guards instead of one.
Layer 1 — deterministic self-consistency floor (
detect_anchors.py,detect_turns())No external dependency; always available.
gen_capture_tape.py's own turnloop is:
Sleep(PAD)[box empty] → type the prompt (type_dur, box fills) →Sleep(pre_enter)[box full, beat] →Enter(submit, box clears). Sosubmit - typing_startmust equal exactlytype_dur + pre_enter— thereis no way to submit before finishing typing.
typing_startwas already computed asmax(0.0, submit - type_dur - pre_enter). Thatmax(0.0, ...)clamp is precisely where the old bug hid:if
submititself is wrong (e.g. a wrap/spinner artifact mistaken for thereal Enter-clear — the #23 shape), the clamp silently absorbs the
contradiction into a
typing_startof0.0instead of surfacing it. Now,the moment the clamp would fire,
detect_turns()raises immediately, namingthe turn index and the impossible numbers.
Note:
PAD(the pad before typing starts) deliberately does not appearin this floor — it belongs to the preceding soft/pre segment, not this hard
one (see
raw_segments's docstring), so including it would make theinvariant wrong, not stronger. Considered and rejected.
Layer 2 — best-effort cross-check against
session-timeline.jsonl(author.py,main())detect_turns()'s own docstring is explicit that wall-clock hook time can'tbe the primary source of truth for frame positions (VHS video time can
drift from it by many seconds during heavy output) — so this is deliberately
a secondary, best-effort signal, not a replacement.
When
<demo>/session-timeline.jsonlexists and a turn'sUserPromptSubmit/Stopevents can be unambiguously correlated (position-matched amongnon-instant turns, mirroring
panel.py's existing_turn_stop_eventslogic), this compares that turn's real
Stop - UserPromptSubmitwall-clockdelta against the pixel-detected
done - submitresponse window for thesame turn. A wild disagreement independently signals the same class of
corruption Layer 1 guards against — importantly, it also catches a
different failure Layer 1 structurally cannot: a wrong-but-internally-
self-consistent group selection that still happens to satisfy Layer 1's own
floor (submit/typing_start/done can all be mutually consistent and still be
picked from the wrong pixels).
Tolerance:
max(5.0s, 50% of the real delta). Chosen deliberatelygenerous against
detect_turns()'s own documented "many seconds duringheavy output" drift risk, so it only fires on a genuinely wild mismatch, not
ordinary detection noise (
done's own+0.3s/+0.5sscan slop). Validatedagainst one real ~110s recording: a turn's real
Stop - UserPromptSubmitdelta was 8.115s; the pixel-detected equivalent agreed within ~0.6s (~7%
relative) — comfortably inside this tolerance with a lot of headroom to
spare. This is one data point, not proof the tolerance is perfectly tuned for
every recording length/output-heaviness — flagged as an area to revisit if a
future legitimate heavy-output recording ever trips it.
Layering decision: Layer 2 lives in
author.py, notdetect_anchors.py.detect_anchors.pyis meant to stay "pure logic + thin video I/O" perCONTRIBUTING.md's "pure logic is unit-tested / I/O stays thin" split, withno awareness of the timeline file.
author.pyis the orchestration layerthat already knows the demo path and calls
detect_anchors.detect(...), soit's the natural home for this additional, optional I/O — mirroring
panel.py's existing (already pure, already tested)_turn_stop_events-style matching rather than inventing a new formatreader. The timeline-reading/matching helpers are duplicated (not imported)
rather than pulled in from
panel.py, sinceauthor.pyruns upstream ofpanel.pyin the pipeline and importing "forward" would be a layeringinversion.
Degrades gracefully by design: missing file, malformed JSON, or turns
that can't be correlated (fewer hook events than turns — a dropped/retried
hook, or no timeline at all) are all silently skipped — never a hard
requirement, never a new single point of failure. It only ever raises for a
turn it could correlate whose numbers wildly disagree.
An interesting edge case surfaced during this work
Layer 1's invariant (
submit - typing_start == type_dur + pre_enter) istautological by construction unless the clamp fires — so it only catches
a bug that pushes the raw unclamped value negative. A wrong group selection
that still yields internally-consistent (but simply wrong) numbers (e.g. a
guided-selection swap between two similarly-timed groups) would sail through
Layer 1 untouched. Layer 2 is the layer that can catch that case — but only
when
session-timeline.jsonlhappens to be available and correlatable forthat recording. For an older/partial capture with no timeline, that specific
failure shape would currently only be caught by... a human eyeballing frames
again. Flagging this rather than pretending both layers give identical
coverage.
Tests
12 new regression tests (3 for Layer 1 in
tests/test_detect_anchors.py, 9for Layer 2 in
tests/test_author.py), following each file's existingsynthetic-signal patterns. Full suite: 151 passed (up from the
pre-existing 139), including every #16–#23 recovery-path regression test
unmodified — no false positives introduced on instant-turn recovery, wrap-
merge recovery, or any other existing path.
Test plan
pytest tests/ -q→ 151 passed, 0 failedregression test (ran full suite, all green)
timeline files
record-sessionrun (skipped — needsvhs/tmux/edge-tts/liveclaudecredentials and several minutes; unit tests arethis repo's primary CI bar per
CONTRIBUTING.md)https://claude.ai/code/session_012EwptawkV4dcGhoGefzxgV