fix: close out the issue #160 review-residual bundle - #177
Conversation
Six residuals from the PR #156/#159/#168 reviews, each with citing tests: 1. nav stamp hardening (item 1): nav_action reuses the guard's TOTAL parse_sim_stamp (BG-3) instead of bare int() -- a malformed stamp no longer kills nav's event loop (base_pose is its only clock). The deliberate unstamped-budget semantics: the machine's clock HOLDS on unstamped poses (budgets freeze, control keeps steering); a fully unstamped source never sim-times-out and the guard wall net is the stop (item 2, documented residual). 2. validator dedup (item 3): one _input_source() helper replaces four hand-rolled extended-form unwraps; the guard-tick check uses the shared _parse_timer_hz, which REJECTS millis/0 (the ad-hoc regex accepted it) -- new golden case. 3. relaunch numbering (item 5): the wall-clamp relaunch passes AISLE_EPISODE_BASE so the new client continues run-global episode numbering -- goal_ids never repeat, fidelity.load_sidecar's duplicate refusal can no longer void a relaunched A7/both run; the sidecar wait counts DISTINCT goal_ids, not lines, so duplicates cannot mask a missing episode. 4. A7 wall budget (item 6): in --verifier realistic every episode runs the full sim budget (nothing ends early) + the judge -- the ADR-23 per-episode clamp is raised to cover it (a7_per_episode_budget_s). 5. RECORDER_AWAIT ported to base_recorder via a shared PURE CaptureWindow (unit-covered), and the ADR-29 wall-net + sweep tests opt in (await violation:1) -- the issue #94 truncation class closed for the mobile guard tests too. 6. ADR-29 status (item 4): already flipped RATIFIED in PR #171. Gates: ruff clean; unit 1080 passed (19 new); trace_check ok; graph: test_guard_mutex 6 passed, test_recorder_await + test_mobile_bridge pass (one suite-load flake passed standalone -- the pre-existing class this bundle mitigates). Closes #160 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| # A7/both run would lose its whole VER-6 comparison. The runner passes | ||
| # the count of episodes already recorded; goal_ids/request_ids/records | ||
| # continue the run-global sequence | ||
| episode_base = int(os.environ.get("AISLE_EPISODE_BASE", "0")) |
There was a problem hiding this comment.
[P1] Do not let the first launch inherit an ambient episode offset
The runner builds the initial child environment from os.environ and only sets AISLE_EPISODE_BASE on a relaunch. Consequently, an ambient value reaches this read on the first launch and silently changes every episode index, reset id, and goal id without appearing in the graph/environment attestation; a malformed value also crashes the rollout client. This is the same CON-5 ambient-state channel that scrub_bringup_env exists to close. Have the runner explicitly set the initial base to 0 (or scrub the variable before doing so), and add a regression with the ambient variable pre-set.
| self.pose = [float(v) for v in pose] | ||
| self._sim_ns = int(sim_time_ns) | ||
| if sim_time_ns is not None: | ||
| self._sim_ns = int(sim_time_ns) |
There was a problem hiding this comment.
[P1] Reset the held simulation clock between navigation goals
_sim_ns survives _finish() and on_goal() does not clear it. With this new hold-on-None behavior, goal 2 can therefore anchor its budgets to goal 1: after goal 1 completed at 1.0 s, I fed goal 2 an unstamped pose and on_tick() set both _t0_ns and _progress_ns to the stale 1.0 s value; when a valid 2.0 s stamp arrived, goal 2 immediately returned blocked with t_end: 1.0. That contradicts the new fully-unstamped/held-clock semantics and can terminate a healthy sequential action after a transient malformed stamp. Clear _sim_ns when a new goal is accepted and cover the sequential-goal case.
| episode = 0 | ||
| episode = 0 # index into THIS launch's seeds/targets | ||
| # global numbering offset (issue #160 item 5): after a wall-clamp | ||
| # relaunch (ADR-23) this client is the run's SECOND writer, and a |
There was a problem hiding this comment.
[P2] Amend ADR-23 to match the new run-global numbering
This intentionally reverses the accepted ADR’s documented consequence: docs/decisions/ADR-23.md currently says “Episode indices and goal ids restart per launch,” while this implementation makes both continue across launches. Leaving the ADR unchanged makes the authoritative rationale instruct future contributors to restore the exact duplicate-ID behavior being fixed here. Update that consequence (and its trace-window implications) in this PR.
|
Fixes for all three review findings are ready in #178. It targets this PR branch (rather than pushing directly) to preserve CON-16 cross-review ownership. Gates: format and lint green; 1,081 unit tests passed; MOB-2 live acceptance passed. |
1 participant
Fixes the six review residuals tracked in #160 (from the PR #156/#159/#168 review passes), each with citing tests.
nav_action.pybareint(sim_time_ns)— malformed stamp kills nav's event loopparse_sim_stamp(BG-3); the machine's clock holds on unstamped poses: budgets freeze rather than anchor at 0, control keeps steeringvalidate.pyad-hoc timer regex + 4 unwrap copies_input_source()helper; the guard-tick check now uses_parse_timer_hz, which rejectsmillis/0(the regex accepted it) — new golden caseAISLE_EPISODE_BASE(run-global numbering continues,fidelity.load_sidecar's duplicate refusal can't void a relaunched A7/both run);await_realistic_sidecarcounts DISTINCT goal_idsa7_per_episode_budget_s: the ADR-23 clamp covers full-sim-budget episodes at pessimistic rtf + the judge — healthy A7 runs no longer trip the clampRECORDER_AWAITmachinery extracted to a shared pureCaptureWindow(unit-covered, 13 tests), ported tobase_recorder.py; the ADR-29 wall-net + sweep tests opt in (violation:1) — the issue #94 truncation class closed for the mobile guard testsIDs: MOB-2, BG-3, ADR-23, ADR-29, VER-6/VER-14, HAR-1. No frozen-set files touched.
Gates: ruff format/check clean;
pytest -m unit1080 passed (19 new);tools/trace_check.pyok; graph:test_guard_mutex6 passed (~3 min),test_recorder_await+test_mobile_bridgepass — one pre-existing suite-load flake (test_mobile_reset_rehomes...) failed under parallel load and passed standalone; it is the same truncation class this PR mitigates and can opt into the await if it recurs.🤖 Generated with Claude Code