fix: make T2 reads sim-time deterministic

[heyong4725]

Spec concern

Issue #153 — T2 scan/read scheduling and the invalid single-episode success gate.

The read park now carries its completed sim_time_ns through
move_done -> read_request. The label reader buffers timestamped wrist
frames and selects the earliest frame strictly after that barrier in either
request/frame arrival order. Stale or unstamped frames cannot consume a
barriered request, and reset clears both the request and frame buffer.

The live T2 graph test is now the safety/integration smoke the baseline can
actually support: it rejects wrong_object while leaving success-rate
acceptance to the committed 25-episode curve. CON-5 layer (d) explicitly
classifies full-episode outcomes as statistical; the measured expert T2
baseline is 2/25, so a one-seed success assertion was not a valid gate.

Closes #153.

Requirement IDs implemented or affected

• CON-5
• TC-2

Gates run

• Manual review + simplify equivalent (repo skills were unavailable in this Codex session)
• uv run ruff format --check .
• uv run ruff check .
• uv run pytest -m unit — 1064 passed, 79 deselected
• uv run pytest -q tests/graph/test_expert_graph.py::test_expert_t2_episode_closes_without_wrong_object -s — 1 passed
• uv run python tools/trace_check.py — ok, no uncovered or unknown citations

The first diagnostic live run reproduced the issue's honest statistical
failure: it read and promoted cetirizine, completed a grasp/place attempt,
then timed out during an in-context retry. The corrected safety gate passed
on the subsequent live run.

ADRs added (if any)

None. CON-5 layer (d) already governs the acceptance interpretation.

[heyong4725]

Pre-landing review (/review) — 4 blocking, 2 precision

Scope check: CLEAN — the diff is the barrier mechanism plus its tests; no unrelated changes. The direction is right (the read must be chosen by sim order, not by which queued RGB event wins a wall race). Every finding below was verified by reading the branch code, not pattern-matched.

Blocking

1. queue_size: 1 on rgb_wrist defeats the "earliest eligible frame" guarantee — the wall-coupling survives transport. (confidence 9/10)
graphs/expert_t2.yaml:100-102 wires the reader's wrist input at depth 1, and dora's default delivery is latest-wins/drop-oldest. recent_frames can only buffer what the transport hands over, so min(eligible, ...) (label_reader.py:338) selects the earliest surviving frame, not the earliest frame after the park. rgb_wrist is 30 Hz (TC table, specs/010-topic-contract.md:39) and a read costs ~2 s of OWLv2 — under exactly the host load issue #153 documents ("the 1/3 failure followed a 2-minute unit-suite run"), a descheduled reader drops the frames the barrier was meant to select.

This repo already solved this half of the problem once: issue #120's stamp-routed verifier paired select-by-stamp with CAMERA_QUEUE_DEPTH = 400 (src/aisle/harness/rollout.py:477-479) precisely because a latest-wins camera queue voids an arrival proof. This PR adopts the selection pattern without the queue depth.

Note the governance catch: graphs/expert_*.yaml is in the CON-7 frozen set, so raising that queue needs an env-change PR with tools/env_hash.py --write. That is probably why it wasn't done — but without it the PR's title claim is not achieved.

2. A missing stamp fails OPEN to pre-park imagery. (confidence 9/10)
ik_trajectory.py:1174 defaults a missing stamp to 0: "frame_after_sim_time_ns": int(metadata.get("sim_time_ns", 0)). The reader arms on barrier >= 0 (label_reader.py:333, :357), so barrier 0 is a live barrier that every stamped frame clears — including frames captured mid-park, which is the stale-frame class this PR exists to prevent.

The sibling branch just settled this exact semantic: parse_sim_stamp maps zero to None because "topics.stamp() defaults missing stamps to 0, so a genuine 0 is indistinguishable from an unstamped source" (src/aisle/mobility/guard.py:98-109). Same rule belongs here, and the fail direction should be closed (refuse and reply), not open.

3. A barriered request can hang forever, and the test that would have caught it was removed in the same PR. (confidence 9/10)
Both entry points retain pending when nothing is eligible (label_reader.py:333-337, :356-360). The old invariant was "the first wrist frame after the request is read once", so a reply always followed — the docstring still advertises "a refused eligible read still REPLIES, so the tour cannot hang", which now holds only if an eligible frame ever arrives. The state machine has no read deadline: its only retry timer is retry_due_tick, armed by plan_done (task_state_machine.py:63), not while awaiting read_result. So the sole bound is the verifier's episode timeout.

Meanwhile the new gate accepts never_grasped and timeout (tests/graph/test_expert_graph.py:167-170), so a permanently silent or crashed reader now passes the live test. Fix: a per-read sim-time deadline in the reader that refuses and replies.

4. Bare int() on the stamp trust boundary — the class this repo fixed last week. (confidence 9/10)
label_reader.py:327, :349, :477 and ik_trajectory.py:1174 all call int() directly on metadata. A malformed stamp (None, a string, a container) raises out of the node event loop and kills the reader mid-tour. This is issue #160 item 1 verbatim, fixed on the open branch fix/issue-160-review-residuals (PR #177) by reusing parse_sim_stamp. Reuse it here too — and note #177 also touches stamp semantics, so landing order matters.

Precision

5. Reset is not episode-fenced (confidence 6/10, medium — I verified the wiring, not a concrete interleaving). reset_done and read_request arrive on independent queues (expert_t2.yaml:103-108); on_reset_done clears unconditionally and on_read_request rearms unconditionally, so nothing prevents a delayed reset from clearing a fresh request or a delayed request from rearming after reset. The unit test imposes ideal ordering. A generation/episode counter carried on both would close it.

6. The statistical justification for dropping the success assertion is defensible, but it leaves the fix unproven. (confidence 8/10)
Under ADR-26 layer (d) a fixed seed's outcome really can flip on irreducible Metal ULP noise, so "one seed is not a success-rate gate" is a fair reading — the 2/25 figure is about the population, and the old test pinned one seed, so those are different claims and the PR body slightly conflates them. The real cost is different: after this change nothing demonstrates the determinism fix works end to end. The unit tests pin ReaderSession logic against synthetic stamps; the live test now passes on almost any non-wrong_object outcome, including the never_grasped mode issue #153 describes as the failure signature. Combined with finding 1, there is no evidence the live wall-coupling is gone. Suggest a live assertion on the mechanism instead of the outcome: e.g. the reader's read_result stamps satisfy sim_time_ns > frame_after_sim_time_ns for every read in the episode, which is deterministic, cheap, and fails loudly if the barrier stops working.

Verdict

The mechanism is the right idea and the unit tests around ReaderSession are good. But as it stands the PR does not deliver its title: the transport can still drop the frame the barrier selects (1), a missing stamp reopens the stale-frame path it closes (2), it introduces a hang with no bound (3) while removing the detector that would have shown either problem (6). Findings 1-4 are all small fixes; 1 requires an env-change epoch for the frozen graph.

Adversarial coverage: Codex (5 findings, all independently verified here). The Claude adversarial pass died mid-run on a model rate limit and did not deliver — cross-model coverage is thinner than usual on this review.

🤖 Generated with Claude Code