feat(planner): add entity-ID-keyed ScenarioBelief snapshot projection - #8437
feat(planner): add entity-ID-keyed ScenarioBelief snapshot projection#8437ll7 wants to merge 12 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #8437
Reviewed exact head 6b036f38b8fe3ae8c66dde4e594078cc081a6bfe against current
origin/main=f891abee5523b40ff7863fb3bdfc218a1526c056.
The bounded draft adds an opt-in BeliefAwarePlannerInput and immutable
PlannerTrackBelief records to the existing ScenarioBelief adapter. The projection retains
visible and non-visible canonical agents by ID, keeps the legacy observation separate, rejects
malformed complete projections with a finite legacy fallback, and exposes deterministic JSON
diagnostics. The existing stream_gap path and default planner roster remain unchanged.
Local proof passed: 60 focused representation/planner tests, Ruff check/format, Python compile,
and git diff --check.
This review is diagnostic contract evidence only. The draft remains held because the live #8050
preparation packet has implementation_admitted: False and the current ScenarioBelief owner has no
retirement/generation epoch. The implementation explicitly reports that lifecycle limitation and
does not claim reuse-safe identity, benchmark improvement, safety improvement, or paper-facing
results.
🔍 PR Contract Check SummaryOverall Status: 🟢 PASSED
ℹ️ Info
This check is mechanized. Please resolve any blockers to pass CI. |
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #8437
Reviewed exact head cd35400a8836ac5b73225128a3e7fd9ce6ebd31d against current
origin/main=f891abee5523b40ff7863fb3bdfc218a1526c056.
The bounded draft adds an opt-in BeliefAwarePlannerInput and immutable
PlannerTrackBelief records to the existing ScenarioBelief adapter. The projection retains
visible and non-visible canonical agents by ID, keeps the legacy observation separate, rejects
malformed complete projections with a finite legacy fallback, and exposes deterministic JSON
diagnostics. The existing stream_gap path and default planner roster remain unchanged.
Local proof passed: 60 focused representation/planner tests, Ruff check/format, Python compile,
and git diff --check.
The first exact-head CI run exposed and the current head fixes a Python 3.11 import failure caused
by the immutable diagnostics mapping proxy being used as a dataclass default. The replacement uses
default_factory=dict; a fresh exact-head CI run is required before any readiness decision.
This review is diagnostic contract evidence only. The draft remains held because the live #8050
preparation packet has implementation_admitted: False and the current ScenarioBelief owner has no
retirement/generation epoch. The implementation explicitly reports that lifecycle limitation and
does not claim reuse-safe identity, benchmark improvement, safety improvement, or paper-facing
results.
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #8437
Reviewed exact head c88dfc94ae15d14a90d16885492d032c5edbf7ba against current
origin/main=f891abee5523b40ff7863fb3bdfc218a1526c056.
The bounded draft adds an opt-in BeliefAwarePlannerInput and immutable
PlannerTrackBelief records to the existing ScenarioBelief adapter. The projection retains
visible and non-visible canonical agents by ID, keeps the legacy observation separate, rejects
malformed complete projections with a finite legacy fallback, and exposes deterministic JSON
diagnostics. The existing stream_gap path and default planner roster remain unchanged.
Local proof passed: 61 focused representation/planner tests, Ruff check/format, Python compile,
and git diff --check.
The replacement head also rejects negative radius values in direct typed-record construction,
loads the optional ScenarioBelief/SciPy-backed representation lazily so legacy adapter imports
remain dependency-light, and counts only explicit occluded visibility as occluded. The refreshed
focused proof is 61 tests, with the blocked-SciPy adapter-import probe passing.
The first exact-head CI run exposed and the current head fixes a Python 3.11 import failure caused
by the immutable diagnostics mapping proxy being used as a dataclass default. The replacement uses
default_factory=dict; a fresh exact-head CI run is required before any readiness decision.
This review is diagnostic contract evidence only. The draft remains held because the live #8050
preparation packet has implementation_admitted: False and the current ScenarioBelief owner has no
retirement/generation epoch. The implementation explicitly reports that lifecycle limitation and
does not claim reuse-safe identity, benchmark improvement, safety improvement, or paper-facing
results.
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #8437
Reviewed exact head 8817151e638cad904e2d16ba57027839f46053a7 against current
origin/main=f891abee5523b40ff7863fb3bdfc218a1526c056.
The bounded draft adds an opt-in BeliefAwarePlannerInput and immutable
PlannerTrackBelief records to the existing ScenarioBelief adapter. The projection retains
visible and non-visible canonical agents by ID, keeps the legacy observation separate, rejects
malformed complete projections with a finite legacy fallback, and exposes deterministic JSON
diagnostics. The existing stream_gap path and default planner roster remain unchanged.
Local proof passed: 68 focused representation/planner tests, exact changed-line coverage at
97.5% (357/366), Ruff check/format, Python compile, and git diff --check.
The deterministic projection, uncertainty-gate, and stream-gap tests are now explicitly routed
through PR fast shards; the initial hosted changed-coverage failure was therefore repaired at the
test-lane owner rather than waived.
The replacement head also rejects negative radius values in direct typed-record construction,
loads the optional ScenarioBelief/SciPy-backed representation lazily so legacy adapter imports
remain dependency-light, and counts only explicit occluded visibility as occluded. The refreshed
focused proof is 61 tests, with the blocked-SciPy adapter-import probe passing.
The first exact-head CI run exposed and the earlier replacement head fixes a Python 3.11 import failure caused
by the immutable diagnostics mapping proxy being used as a dataclass default. The replacement uses
default_factory=dict; a fresh exact-head CI run for 8817151e638cad904e2d16ba57027839f46053a7
is required before any readiness decision.
This review is diagnostic contract evidence only. The draft remains held because the live #8050
preparation packet has implementation_admitted: False and the current ScenarioBelief owner has no
retirement/generation epoch. The implementation explicitly reports that lifecycle limitation and
does not claim reuse-safe identity, benchmark improvement, safety improvement, or paper-facing
results.
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #8437
Reviewed exact head 8817151e638cad904e2d16ba57027839f46053a7 against current
origin/main=f891abee5523b40ff7863fb3bdfc218a1526c056.
The bounded draft adds an opt-in BeliefAwarePlannerInput and immutable
PlannerTrackBelief records to the existing ScenarioBelief adapter. The projection retains
visible and non-visible canonical agents by ID, keeps the legacy observation separate, rejects
malformed complete projections with a finite legacy fallback, and exposes deterministic JSON
diagnostics. The existing stream_gap path and default planner roster remain unchanged.
Local proof passed: 68 focused representation/planner tests, exact changed-line coverage at
97.5% (357/366), Ruff check/format, Python compile, and git diff --check.
The deterministic projection, uncertainty-gate, and stream-gap tests are now explicitly routed
through PR fast shards; the initial hosted changed-coverage failure was therefore repaired at the
test-lane owner rather than waived.
The replacement head also rejects negative radius values in direct typed-record construction,
loads the optional ScenarioBelief/SciPy-backed representation lazily so legacy adapter imports
remain dependency-light, and counts only explicit occluded visibility as occluded. The refreshed
focused proof is 68 tests, with the blocked-SciPy adapter-import probe passing.
The first exact-head CI run exposed a Python 3.11 import failure caused by the immutable diagnostics
mapping proxy being used as a dataclass default; the replacement uses
default_factory=dict; the replacement exact-head hosted checks and changed-coverage gate are now
terminally green.
This review is diagnostic contract evidence only. The draft remains held because the live #8050
preparation packet has implementation_admitted: False and the current ScenarioBelief owner has no
retirement/generation epoch. The implementation explicitly reports that lifecycle limitation and
does not claim reuse-safe identity, benchmark improvement, safety improvement, or paper-facing
results.
8817151 to
186c992
Compare
Exact-head implementation review — PR #8437Reviewed exact head The bounded draft adds an opt-in Local proof passed: 68 focused representation/planner tests, exact changed-line coverage at The deterministic projection, uncertainty-gate, and stream-gap tests are now explicitly routed The replacement head also rejects negative radius values in direct typed-record construction, The prior exact-head CI run exposed a Python 3.11 import failure caused by the immutable diagnostics This review is diagnostic contract evidence only. The draft remains held because the live #8050 |
## Summary This successor closes the remaining boundedness gap in the CI `ci_uv_sync_diag` probe. It removes the unbounded `uv cache size` traversal and makes both advisory `du` probes deadline-aware and diagnostically explicit. ## Linked Issues - Relates to #8249 (successor follow-up to merged PR #8365; the issue is reopened for this work). ## Stack / Dependency - Base dependency: none. - Required prior PRs and stack follow-up issues: none. - Safe to review independently: yes; the branch is based on current `main`. ## What Changed - Removed the redundant `uv cache size` cache-root traversal. - Require a positive `ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS` value, defaulting invalid values (including zero) to 10 seconds. - Detect GNU `timeout(1)` explicitly and use `--kill-after=2s` after the configured deadline. - Emit `ok`, `timed-out`, or `error` sizing status markers and exit codes for cache and virtualenv probes. - Added deterministic tests for the unbounded-`uv` regression, zero timeout, hard timeout timing, non-timeout errors, no-GNU fallback behavior, and the script contract. ## Why It Matters The diagnostic remains advisory and exits zero, while its expensive cache and virtual-environment walks no longer hide failures or consume the full readiness test budget. Hosts without GNU `timeout(1)` retain the prior direct-`du` fallback and ordinary output keys. ## Research / Evidence Notes Not applicable — this is a CI/tooling reliability change with no research, benchmark, metric, or paper-facing claim. ## Validation / Proof - `tests/dev/test_ci_uv_sync_diag.py`: 12 passed. - `tests/test_ci_script_contract.py`: 153 passed. - `bash -n scripts/dev/ci_uv_sync_diag.sh`: passed. - Ruff check and format checks for both changed Python test files: passed. - `BASE_REF=origin/main PR_READY_MODE=final ... scripts/dev/pr_ready_check.sh`: passed on committed head `8e7015c578279c1d4e1dd31279b0143bf9161eec` against base `5f476625eba9c4ae2fa5770781852b78c865fd8b`. ## Risks / Rollback The no-GNU fallback remains intentionally unbounded for compatibility with stock macOS-style hosts. Revert commit `8e7015c57` to restore the preceding diagnostic implementation if needed. ## Docs / Provenance No durable benchmark or model artifacts were produced. Readiness receipts remain worktree-local ignored output; the source contract and reproducible test fixtures are tracked in this PR. ## Downstream Propagation Not applicable — support/tooling change; no benchmark, metric, model, claim-map, registry, or durable evidence update is required. ## Follow-Up / Residual Scope No deferred work remains for this implementation slice. Issue #8249 is reopened for this PR; this is the immediate successor requested for the remaining `uv sync` diagnostic gaps. ## Reviewer Notes - Verify that `uv cache size` is absent from the executable path and that the later bounded `du` remains the sole cache-size traversal. - Verify that timeout expirations are not attributed to host contention without evidence and that non-timeout tool failures remain visible. <!-- Keep this marker and its keys unchanged. Update values to match the human narrative and changed files. --> <!-- pr-contract:v2 change_class: tooling linked_issues: closes: [] relates: [8249] deferred_work: status: none issues: [] reason: "" evidence: applicability: na tier: null result: na domain_approval: required: false status: not_required domains: [] note: "NA - support/tooling change; no experimental claim." performance: claimed: false -->
…//github.com/ll7/robot_sf_ll7 into autopilot/review-pr8437-20260905-cycle44
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review
Reviewed PR #8437 at exact head 4b2e3d5620d416a4ab979d3e6de19eb6a9d015f1 against fresh
origin/main a2bf33373e5775eacbeed5049c15b987ad5af9df. The branch was synchronized from a
fresh-base linked worktree before the bounded follow-up repair.
Findings and repairs
- The issue #8050 contract requires a
retained_track_countdiagnostic; the draft previously
exposed onlyprojected_track_count. The diagnostic now reports both current-snapshot counts,
with a regression assertion. _age_stepsdocumented conservative upward rounding but could return zero for a positive age
smaller than its epsilon. The calculation now preserves positive ages while still collapsing
floating-point values close to a positive integral step; a regression covers the edge.
Validation
- Focused ScenarioBelief/planner suite:
69 passed. - Ruff 0.16.5 check/format,
git diff --check, and clean merge-tree againstorigin/mainpassed. - Hosted checks for this new head are not yet terminal; no hosted success is inferred.
The projection remains diagnostic-only and additive. It does not admit a planner implementation,
stateful identity continuity, retirement/reuse safety, benchmark evidence, or a paper-facing claim.
The PR should remain draft/blocked pending a lifecycle owner for generation/retirement semantics
and the already-declared downstream runtime/integration proof. No merge-ready label or merge is
appropriate for this head.
Decision: hold at state:blocked/draft after the bounded repair; no further P0–P3 repair is safely
isolated in this review slice.
gate-verdict: blocked @ 4b2e3d5
pr-metadata: reconciled @ 4bef7d160332fed84343bce6a1f7c6bbe596d13919b58de9ed127c8663604fae
|
review-claim: codex-goal-autopilot-20260905 @ 4b2e3d5 until 2026-09-05T00:23:52Z |
ll7
left a comment
There was a problem hiding this comment.
Exact-head research/implementation review
Reviewed draft PR #8437 at exact head 4b2e3d5620d416a4ab979d3e6de19eb6a9d015f1 against current origin/main e7b6f9bfde9f80ca65d62bb1d85c681946e2a3dd in a dedicated linked review worktree.
Findings and disposition
No actionable P0-P3 implementation defect was verified for the declared diagnostic, additive ScenarioBelief projection slice. The projection is keyed by snapshot entity ID and explicitly reports that generation/retirement continuity is unavailable; it does not admit stateful planner identity, benchmark improvement, safety effects, or paper evidence.
Focused exact-head proof passed:
tests/planner/test_scenario_belief_track_projection.py: 16 passed;- scoped Ruff check and format check: passed;
git diff --check origin/main...HEAD: passed;git merge-tree --write-tree origin/main HEAD: conflict-free.
Hosted checks at this head are terminal with 34 successes and 2 intentional skips, but CI does not establish lifecycle semantics or research validity. The PR remains draft and blocked by linked issue #8050's implementation_admitted:false state and its required retirement/generation evidence. The current exact-head route must not be promoted to planner admission or benchmark evidence without that lifecycle-owner decision.
base-policy: ordinary-cas @ 4b2e3d5620d416a4ab979d3e6de19eb6a9d015f1
pr-metadata: reconciled @ 4bef7d160332fed84343bce6a1f7c6bbe596d13919b58de9ed127c8663604fae
gate-verdict: blocked @ 4b2e3d5620d416a4ab979d3e6de19eb6a9d015f1
merge-ready: no @ 4b2e3d5620d416a4ab979d3e6de19eb6a9d015f1
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review
Reviewed PR #8437 at exact head f6c71c1a7962244cb9f9f0b7db8d36b072855591 against fresh
origin/main 1cb110f36cbcc96f781c3e5e8f99b13e60a0cb3e in linked worktrees. The current head
adds bounded validation hardening to the previously reviewed diagnostic projection.
Current-head repairs
- Complex arrays are rejected before float coercion, exact
(2,)entity vector shape is required,
and overflowing finite time ratios fail closed asinvalid_beliefinstead of reaching integer
conversion. - Nested diagnostics are recursively copied into immutable mappings/sequences and read-only arrays,
preventing caller mutation from changing a constructed planner input or its JSON. - The stable identity/lifecycle limitation remains explicit:
stateful_identity_admitted:false,
no generation or retirement token is claimed, andretired_track_count:nullremains honest.
Validation
- Focused current-base ScenarioBelief/planner/representation suite:
65 passed. - Ruff 0.16.5 check/format, Python compilation, and
git diff --checkpassed for the changed
implementation and tests. - Hosted checks for this exact head are pending and supersede prior-head results; no hosted success
is inferred.
The projection remains diagnostic-only and additive. It does not admit a planner implementation,
stateful identity continuity, retirement/reuse safety, benchmark evidence, or a paper-facing claim.
The PR should remain draft/blocked pending a lifecycle owner for generation/retirement semantics and
the already-declared downstream runtime/integration proof. No merge-ready label or merge is
appropriate for this head.
Decision: hold at state:blocked/draft. The remaining lifecycle/identity contract is not safely
solved by this bounded input-validation repair.
gate-verdict: blocked @ f6c71c1
base-policy: ordinary-cas @ f6c71c1
pr-metadata: reconciled @ 709e8fa6e92a45a0483d92ee2510ae271cfed6d25b085836230f486c157c9cf2
Summary
Add an opt-in, deterministic entity-ID-keyed projection of one
ScenarioBeliefsnapshot for thecurrently supported projection target,
BeliefGuidedLocalPlanner. The projection retains everyScenarioBelief.agentsentry in immutable typed track records and keeps the legacy visibleobservation separate, preventing uncertainty metadata from being joined by visible-row position.
This draft deliberately does not implement or operationally admit
BeliefGuidedLocalPlanner,establish physical-agent continuity, or close #8050.
Linked issues
implementation_admitted:false.Scope and contract
PlannerTrackBeliefexposes the snapshot-suppliedtrack_id, validated state and covariance,existence/visibility, age, source, and adapter-derived confidence.
track_idis an entity identifier supplied by thisScenarioBeliefsnapshot, not avisible-observation row number. It is not a generation or continuity token, and no cross-lifecycle
continuity is implied across retirement, disappearance, reappearance, ID reuse, or episodes.
or stateful identity semantics. Diagnostics report
stateful_identity_admitted:false, unavailableretirement tracking,
retired_track_count:null, and the required lifecycle reset boundary.projected_track_countdescribes thecurrent snapshot only; no historical retained-track count is inferred.
position and velocity confidences, while the zero radius covariance block means radius uncertainty
is unavailable as modelled, not measured to be zero.
complete projections fail closed to a finite legacy observation when one is available.
time ratios. Nested diagnostics are copied into immutable containers so later caller mutation
cannot change the serialized input.
stream_gapadapter,ScenarioBelief.to_socnav_struct(), planner defaults,registries, benchmark rosters, and campaign evidence are unchanged.
Validation / proof
65 passedacross the projection, uncertainty-gate, and canonicalScenarioBelieftests, including ID-reuse, malformed-input, overflow, and deep-immutabilityregressions. This proof was run in a linked worktree at current
origin/main.git diff --checkpass for the changed implementationand focused tests.
result is treated as current until the exact head is terminal.
Research / evidence / domain
Diagnostic interface evidence only. This change does not run a benchmark, change a metric, claim a
planner or safety improvement, use simulator-private future state, or establish paper-facing
results. Domain approval is not required for this contract-only slice.
Risks / rollback
The API is additive and opt-in. It cannot distinguish physical-agent retirement or ID reuse at the
ScenarioBeliefboundary, so stateful consumers must reset at an externally supplied lifecycleboundary. Rollback is a revert of the projection commits.
Downstream propagation
Not applicable: this is a support/diagnostic interface only. No planner registration, default roster,
benchmark, campaign, metric, or paper-facing claim changed. No downstream planner work is requested.
Residual scope / hold
Keep this PR draft. #8050 remains open with
implementation_admitted:false; a lifecycle owner mustprovide retirement/generation evidence before any stateful planner admission. Do not treat this
projection as providing cross-snapshot identity continuity or as benchmark evidence.