Skip to content

feat(placement): solve each capture's merged scene instead of a reference - #273

Merged
SamGu-NRX merged 1 commit into
alphafrom
obv/burst7-bs-capture-scene-stage
Oct 16, 2026
Merged

SamGu-NRX merged 1 commit into
alphafrom
obv/burst7-bs-capture-scene-stage

Conversation

@obvious-autobuild

Copy link
Copy Markdown

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:

  • The solver's whole input is scene/scene.json from this run, converted by the scene bridge and re-serialized deterministically (sorted keys, no NaN/Inf), so stats.input_sha256 binds to the exact bytes solved. The committed reference fixture is deleted.
  • Placement now waits for objects_3d to be terminal, not just scene done, and result waits for a terminal placement (graph.py + STAGE_DEPS, kept aligned).
  • objects/objects_3d.json is read only as readiness evidence — stage done, declared hss-objects-3d/v0 format, an explicit empty object list being the one valid "no hazards" shape. The coordinator's _merge_if_ready already merged detected objects into the source scene, so the stage never merges again; 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 / 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/placements of 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| solve
Loading

The dashed edge is what changed structurally: objects_3d gates 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 (make targets / 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.sha256 is git-ignored and absent in a fresh clone — the check exits on FileNotFoundError before 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 --check clean; lint-imports → 5 kept, 0 broken.
  • Targeted: the reworked stage tests (12) cover the refusal paths by mutation-style construction (missing/malformed/unsupported scene; objects_3d failed/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 -q

Material coverage limits: no Modal/GPU run; the real (non-stub) capture stages were not exercised — placement itself is stubs_exempt and runs the real solver on this path.

Risk

  • Runs whose scene stage finishes but whose objects_3d stage 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 terminates manual_review.
  • The staged W6 receipt (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

  • The scope is focused; related changes belong together
  • I explained what changed and why
  • N/A — no visible changes (backend only), no before/after images
  • N/A — no motion or interaction to record

…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>
@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9cc519bd-35f6-4664-93dd-e70357cc16fb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@obvious-autobuild

Copy link
Copy Markdown
Author

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)

  1. [low, non-blocking] backend/server/placement/scene_bridge.py:7-8 — stale module docstring on this branch. It still says the bridge is "the placement stage's placement/scene.json input, which nothing publishes yet (backend/server/placement/adapter.py falls back to a committed reference fixture until a stage hydrates it through this bridge)". After this PR none of that is true: the stage's hydrated input is scene/scene.json (STAGE_INPUTS["placement"]), placement/scene.json is an output the stage publishes, the reference-fixture fallback is deleted in this diff, and this PR is the hydration that sentence was waiting for. Evidence: the quoted lines at scene_bridge.py:7-8 at head 38a5735 vs adapter.py's rewritten docstring and _read_input (refusal, no fallback). Smallest correct fix: rewrite the sentence to the new contract — input is the merged scene/scene.json metres document, converted wire bytes are published as placement/scene.json, no reference fallback. The file is owned by the base branch's PR (the scene-converter work), so per AGENTS.md's one-writer-per-file note the one-line edit can land there instead.

No other findings. Specifically probed and found correct:

  • Merge-before-spawn ordering (the load-bearing claim): coordinator.tick() calls _merge_if_ready before _spawn_ready in the same tick, and stage inputs are hydrated inside _spawn_ready via services.stage_context, so placement only ever sees post-merge scene bytes. A merge failure closes the run rather than spawning placement.
  • Readiness gate: is_ready treats terminal as done|failed|skipped; the adapter refuses unless the objects_3d stage record is done, so failed/skipped/missing records all refuse → the optional stage degrades the run to manual_review instead of implying no hazards. An explicit empty object list with the declared format is the one valid "no hazards" shape (tested).
  • One-time merge precedence: the adapter treats objects/objects_3d.json as readiness evidence only and never re-merges; same-id detector records can't duplicate hazards (tested with a same-id record present).
  • Typed refusals survive to the stage record: SceneBridgeError(ValueError) is raised in _source_scene_bytes, outside run()'s except ValueError handler, and its code reaches the runner — verified end-to-end by the coordinator-path test asserting placement_scene_unsupported on the stage record, plus error_parts/is_retryable assertions for the three new codes.
  • No untyped-escape hole: the adapter's allow_nan=False re-serialization can't raise an untyped ValueError in practice — the bridge refuses non-finite numbers itself (_refuse_nonfinite inside convert_scene, typed placement_scene_invalid), so that line is unreachable defense in depth.
  • No hang risk from the new result dep: placement is always in STAGES (never filtered), optional-degradable, and terminal on failure, so result cannot wait forever.

What I verified (fresh clone, head 38a5735, Python 3.12.15)

Followed AGENTS.md / CLAUDE.md / CONTRIBUTING.md / backend/CONTRIBUTING.md and the deslop review skill. Read the PR body, the only comment (a CodeRabbit skip notice; no reviews), and the full diff vs merge-base 48de156 — one commit on the base tip, +459/−150 across 7 files (contracts/jobs.py, orchestrator/graph.py, placement/adapter.py, deleted placement/reference_scene.json, and three test files).

Command Result
HSS_ORCH_STAGE_STUBS=1 uv run --frozen pytest server/orchestrator/tests/test_placement_stage.py -q (the body's suggested repro) 12 passed in 3.72s
HSS_ORCH_STAGE_STUBS=1 uv run --frozen pytest tests/test_capture_placement_frontdoor.py tests/test_backend_w6_gate.py -q 12 passed in 2.99s
HSS_ORCH_STAGE_STUBS=1 uv run --frozen pytest tests/ server/orchestrator/tests/ server/api/tests/ server/common/tests/ -q (the body's claimed suite) 413 passed, 1 failed in 39.36s — the failure is tests/test_ssr_overlap_check.py::test_repo_has_no_ssr_shingle_overlap, exactly as the body discloses
Same SSR test on a base-commit worktree (48de156) 1 failed, 5 passed — same FileNotFoundError on data/hygiene/ssr_shingles.sha256 (git-ignored via .gitignore:25, absent in a fresh clone). Pre-existing, unrelated to this diff, as the body claims
make server (with uv 0.11.33 — the top-level server/ pins uv >=0.10.11,<0.12) ruff check + ruff format --check + pytest -q: 312 passed in 92.71s, as the body claims
make schemas · make hygiene · ruff check . · ruff format --check . · lint-imports (backend) clean · hygiene ok (3112 tracked files) · clean · 472 files clean · Contracts: 5 kept, 0 broken — all match the body verbatim

New tests have bite (mutation-style, in a disposable clone, tree restored clean after — git status empty):

  • Reverting server/placement/adapter.py to base 48de156 → 8 of 12 stage tests fail (every refusal path, merge precedence, source-derived hashing, determinism); the 4 survivors test unchanged behavior.
  • Reverting server/orchestrator/graph.py + contracts/jobs.py → test_dependency_ordering_is_aligned_across_graph_and_contract fails.

The tests exercise the real path: the coordinator-path test walks RunCoordinator with real services, state, artifact overlay and events, not the adapter in isolation; the front-door and W6 tests drive full capture journeys through the API.

Scope and body accuracy: grep -rn reference_scene finds zero stale references to the deleted fixture. The staged W6 receipt is retained as a historical record and no longer gates fresh journeys, exactly as the Risk section discloses. Every number in the Verification section that can be reproduced locally reproduced exactly, including the suggested repro command.

Couldn't verify

  • No Modal/GPU run on the review VM, so the real (non-stub) capture stages were not exercised — the body discloses this. Placement itself is stubs_exempt and runs the real top-level solver on this path, so the solved-geometry path is real.
  • _merge_objects (the coordinator's production merge) was exercised only through stub stage outputs in the coordinator-path test, not with real reconstruction output.

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.

@SamGu-NRX
SamGu-NRX changed the base branch from obv/burst7-bs-scene-converter to alpha October 16, 2026 04:32
@SamGu-NRX
SamGu-NRX marked this pull request as ready for review October 16, 2026 04:32
@SamGu-NRX
SamGu-NRX merged commit 7206266 into alpha Oct 16, 2026
5 checks passed
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T04:34:27.130124Z 38a5735 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@codemetrics-ai

Copy link
Copy Markdown

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.

@greptile-apps

greptile-apps Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[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.

Fix All in Claude CodeFindings

  1. P1 Missing hazards look empty ▶
Fix with agent prompt
### Issue 1
backend/server/placement/adapter.py:236-238
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.

Summary

Placement now solves each capture’s converted scene instead of a committed reference.

  • Placement scores each capture’s merged scene.
  • Placement waits for hazard detection before the result stage runs.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  S["scene/scene.json"] --> M["Coordinator merges detected objects"]
  O["objects_3d finishes"] --> M
  M --> B["convert_scene"]
  B --> P["Placement solver"]
  P --> R["result stage"]
  O -. "Readiness evidence" .-> P
Loading

Reviews (1) · Last reviewed commit: "feat(placement): solve each capture's me..." · Reviewed by Greptile

Comment on lines +236 to +238
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}')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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.

Fix in Claude Code Fix in Codex

obvious-autobuild Bot pushed a commit that referenced this pull request Oct 16, 2026
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>
obvious-autobuild Bot added a commit that referenced this pull request Oct 16, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 effective changed lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant