Repository navigation
feat(placement): solve each capture's merged scene instead of a reference - #273
Conversation
…ence The placement stage solved one committed reference scene and only warned when the run's own artifact was absent, so every capture scored the same geometry. It now refuses anything but this capture's merged scene: `scene/scene.json` converted by the scene bridge is the solver's whole input, re-serialized deterministically (sorted keys, no NaN) so `stats.input_sha256` binds to the exact bytes solved. The committed reference fixture is deleted. Placement now also waits for `objects_3d` to be terminal, not just `scene` done, and `result` waits for a terminal placement. The stage reads `objects/objects_3d.json` only as readiness evidence — stage done, declared `hss-objects-3d/v0` format, an explicit empty object list being the one valid "no hazards" shape — because the coordinator's `_merge_if_ready` already merged detected objects into the source scene; a second merge would duplicate same-id hazards. Missing inputs or an unfinished objects_3d stage are typed non-retryable errors (`placement_scene_input_missing`, `placement_objects_3d_unavailable`) that degrade the run to manual_review instead of implying no hazards; bridge refusals keep flowing through `placement_scene_invalid` and `placement_scene_unsupported`. STAGE_DEPS gains the same placement and result entries as the runtime graph, the input-selection contract admits the two stage inputs, and the tests move from the fixture to the journey's own bytes: the stage tests pin source-derived hashing, dependency ordering, every refusal path and one-time merge precedence; the front-door and W6 fresh-journey tests compare against a direct POST of the same converted bytes, keeping the staged receipt as a historical record. Co-authored-by: obvious-autobuild[bot] <obvious-autobuild[bot]@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
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 |
|
Independent review (Obvious, Autobuild + Max) Verdict: ready (the PR is a draft; nothing here blocks marking it ready for review). One low, non-blocking finding. Every claim in the body that can be checked locally reproduced exactly. Nothing was changed, pushed, approved, or merged for this review; the only write is this comment. Findings (ranked by severity)
No other findings. Specifically probed and found correct:
What I verified (fresh clone, head 38a5735, Python 3.12.15)Followed AGENTS.md / CLAUDE.md / CONTRIBUTING.md / backend/CONTRIBUTING.md and the
New tests have bite (mutation-style, in a disposable clone, tree restored clean after —
The tests exercise the real path: the coordinator-path test walks Scope and body accuracy: Couldn't verify
Review environment: fresh clone at head 38a5735; uv 0.13.1 (backend) / 0.11.33 (top-level server). No code, workflow, or PR state was modified; the worktree was restored clean after the revert checks. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Review skipped Codemetrics is installed on this repository, but it hasn't been added to your team yet. To enable AI code reviews for this repo, go to your dashboard and add BaseScanning to your team's repositories. |
[Critical impact] Changes how the placement solver receives scene data. Reject incomplete object artifacts before merging so placement cannot mistake missing evidence for an empty hazard list.
|
| fmt = document.get("format") if isinstance(document, dict) else None | ||
| if fmt != "hss-objects-3d/v0": | ||
| raise PlacementObjectsError(f'{_OBJECTS_3D} must carry format "hss-objects-3d/v0", got {fmt!r}') |
There was a problem hiding this comment.
The new readiness check accepts {"format": "hss-objects-3d/v0"} without an objects list. The coordinator treats missing or null objects as an empty list, so this malformed artifact reaches placement without detected hazards or a warning that the evidence is incomplete.
Require objects to be a list, and add refusal tests for missing, null, and non-list values. Only an explicit empty list should mean that the completed pass found nothing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/placement/adapter.py
Line: 236-238
Comment:
**Missing hazards look empty**
The new readiness check accepts `{"format": "hss-objects-3d/v0"}` without an `objects` list. The coordinator treats missing or null `objects` as an empty list, so this malformed artifact reaches placement without detected hazards or a warning that the evidence is incomplete.
Require `objects` to be a list, and add refusal tests for missing, null, and non-list values. Only an explicit empty list should mean that the completed pass found nothing.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.The base absorbed burst5-7 PRs (#262-#273, plus alpha). Ours is the reviewed superset in every conflicted file: the strict file-index lease, manifest-first packet hydration (amendment same-name bytes), the confirmation recompose hook, and the evolved regression tests. Base's packet_key plumbing and reads persistence were already carried by this branch's fork point. Co-authored-by: Sam Gu <sgu07966@gmail.com>
The base absorbed burst5-7 PRs (#262-#273, plus alpha). Ours is the reviewed superset in every conflicted file: the strict file-index lease, manifest-first packet hydration (amendment same-name bytes), the confirmation recompose hook, and the evolved regression tests. Base's packet_key plumbing and reads persistence were already carried by this branch's fork point. Co-authored-by: Sam Gu <sgu07966@gmail.com> Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
What changed
The placement stage solved one committed reference scene and only warned when the run's own artifact was absent — every capture scored the same geometry. It now refuses anything but this capture's merged scene:
scene/scene.jsonfrom this run, converted by the scene bridge and re-serialized deterministically (sorted keys, no NaN/Inf), sostats.input_sha256binds to the exact bytes solved. The committed reference fixture is deleted.objects_3dto be terminal, not justscenedone, andresultwaits for a terminal placement (graph.py+STAGE_DEPS, kept aligned).objects/objects_3d.jsonis read only as readiness evidence — stagedone, declaredhss-objects-3d/v0format, an explicit empty object list being the one valid "no hazards" shape. The coordinator's_merge_if_readyalready merged detected objects into the source scene, so the stage never merges again; a second merge would duplicate same-id hazards.objects_3dstage are typed non-retryable errors (placement_scene_input_missing,placement_objects_3d_unavailable) that degrade the run tomanual_reviewinstead of implying no hazards. Bridge refusals keep flowing throughplacement_scene_invalid/placement_scene_unsupported.Tests move from the fixture to the journey's own bytes: the stage tests pin source-derived hashing, dependency ordering, every refusal path and one-time merge precedence; the front-door and W6 fresh-journey tests now compare against a direct
POST /v1/placementsof the same converted bytes, keeping the staged receipt as a historical record.Why
Solving a reference made the stage's output independent of the capture, and the warning was invisible to the verdict. With #272's bridge available, the stage can do what Wave 6 promised: score the scene this capture actually produced, with the merge boundary in the coordinator — the stage validates readiness only, so merge precedence stays in one place.
How it works
flowchart LR scene[scene stage\nscene/scene.json] --> merge[coordinator\n_merge_if_ready] o3d[objects_3d stage\nobjects/objects_3d.json] -->|terminal| merge merge -->|merged metres scene| bridge[scene_bridge.convert_scene] bridge -->|deterministic wire bytes| solve[placement solver] solve -->|placement.json + scene.json| result[result stage\nwaits for terminal placement] o3d -.->|readiness evidence only| solveThe dashed edge is what changed structurally:
objects_3dgates when placement may run and contributes no geometry after the merge.UI changes
N/A — backend stage behavior only.
Verification
Run on this branch's head (
maketargets /uv run --frozen), fresh sandbox clone:pytest tests/ server/orchestrator/tests/ server/api/tests/ server/common/tests/→ 413 passed; the only failure is the documented pre-existing SSR environment failure (data/hygiene/ssr_shingles.sha256is git-ignored and absent in a fresh clone — the check exits onFileNotFoundErrorbefore comparing anything; identical on prior branches, unrelated to this diff).make server(top-level solver suite) → 312 passed, ruff clean.make schemas→ ok;make hygiene→ ok (3112 tracked files);ruff check+format --checkclean;lint-imports→ 5 kept, 0 broken.objects_3dfailed/skipped/running/absent/malformed-format), the empty-list-is-valid case, distinct-scene hashing and same-input determinism.Suggested reproduction:
cd backend && HSS_ORCH_STAGE_STUBS=1 uv run --frozen pytest server/orchestrator/tests/test_placement_stage.py -qMaterial coverage limits: no Modal/GPU run; the real (non-stub) capture stages were not exercised — placement itself is
stubs_exemptand runs the real solver on this path.Risk
scenestage finishes but whoseobjects_3dstage fails now degrade with no placement artifact instead of shipping a placement on possibly incomplete hazards — that is the intended conservative behavior, and the run still terminatesmanual_review.data/w6/frontdoor-receipt.json,placement-result.json) still records the fixture-based run; the gate no longer compares fresh journeys against it, so a stale receipt cannot mask a regression but also pins nothing current.Checklist