[obvious-autobuild]

PR: Make capture replay enforce every reported acceptance check

Repo: SamGu-NRX/BaseScanning
Branch: obv/burst8-bs-replay-acceptance → base obv/burst7-bs-capture-scene-stage (stacked on PR #274's head, obv/burst7-bs-capture-placement-runner @ bdfbb3f)
Commit: 5f9e295 — "Enforce every reported acceptance check in the placement replay runner"
Draft: yes — never merges, pushes to a default branch, or deploys.

Load-bearing assumption

The premise: a replay runner's worth comes from checks that would fail if the thing they describe broke — an acceptance suite whose success path is unreachable, or whose pass depends on the runner trusting the code it audits, reports trust that was never earned. Evidence that confirms this: any fault injection in the new tests breaking the suite. Evidence that would break it: the committed manifest's positive fixtures becoming convertible (the temporary-manifest fixtures in the tests would then be redundant but harmless).

Problem

The runner from burst 7 (capture_placement_cases.py) walked the real capture front door but reported aggregate verdicts it could not have earned:

1. No success path. Both committed positive sources are refused by the bridge (/objects/0/geometry/standoff_m), so ok: true was unreachable; the suite reported that honestly as blockers, but nothing proved the runner could ever pass.
2. Trusting the audited code. The runner recorded solver responses without binding stats.input_sha256 to the exact bytes each path sent — two replies could agree with each other and both be wrong.
3. No wire-truth oracle. Converted scenes were never compared field-by-field against independently authored wire scenes (metres → feet), so a silently wrong conversion passed.
4. Weak device-boundary and replay checks. Served-result HTTP status, outcome kind, and run identity were not all verified against what is actually served; finalize-replay idempotence was recorded, not enforced as an aggregate blocker.
5. Stamps are not evidence. Cross-capture distinctness was checked on raw merged-scene hashes, which differ by identity stamp alone — one geometry under two case ids passed.
6. Vacuous passes. Positive refusals and negative solutions were not uniformly blockers; empty or unknown --case-ids selections did not refuse to run.

Solution

• Runner (backend/scripts/capture_placement_cases.py, +~50 lines net):
  • Each solver response's stats.input_sha256 must equal the sha256 of the exact request bytes on both paths (staged capture and direct POST /v1/placements).
  • The converted scene is compared field-by-field (canonical normalized geometry) against the independently authored wire fixture, with a metre-to-feet sentinel, chain-ends, and tap-hazard-converted-exactly-once checks.
  • Served device result checked for HTTP 200, manual_review, and the journey's own run id; finalize replay must stay idempotent or the suite blocks.
  • Positive cases must publish a canonical normalized-geometry hash with identity stamps stripped; cross-capture distinctness is judged on that, not on raw stamps (twin-identity runs are blockers).
  • Positive refusals and negative solutions are blockers either way; empty/unknown case selections exit with an error instead of passing.
• Tests (backend/tests/test_capture_placement_cases.py, rewritten): the success path runs on a temporary manifest whose two positive sources, wire scenes, and pinned expectations are authored by hand in the test module — the same geometry in metres and in feet, never produced by running the converter (the committed manifest stays pinned to its refusal state, reported loudly). Fault-injection tests prove the checks are load-bearing: a corrupted converter, a forged input hash in both replies, a lying served result (kind or run id), a non-idempotent replay, a declared negative that solves, a substituted boundary scene, a removed stage, one geometry under two identities, and CLI exit codes through a real subprocess.

Verification

• Focused suite: 17 passed (tests/test_capture_placement_cases.py).
• Placement contract suite: 14 passed (server/placement/tests).
• Full backend suite: run at PR time (result noted in the PR comment).
• ruff check + ruff format --check: clean on both files.

Assumptions

• Draft PR stacked on [obvious-fleet] Merge origin/alpha into the capture-API placement-case replay runner (resolve stale early-#271 conflicts) #274 (same base branch), matching the review-train pattern; nothing merged, no default-branch pushes, no deploys.
• The committed manifest's positive fixtures remain bridge-incompatible as of 2026-10-15 — the runner reports that state as blockers and exits nonzero by design; the new tests carry the success path on a temp manifest instead of weakening the pinned fixtures.
• The success-path expectations (spot.span_ft, checks, missing-evidence bands) are pinned from the solver's deterministic output on exactly the merged scenes the temp manifest produces; they are geometry facts, not policy.

Merged alpha

Merged origin/alpha (c2d4bb2) into this branch as merge commit 2a1dfd5 (a normal merge — no rebase, no force-push) so the PR can land on alpha now that #274 and the alpha-side fixes are in.

Conflicts and resolutions

• backend/scripts/capture_placement_cases.py — kept this branch's acceptance enforcement (independent merge oracle, stats.input_sha256 bindings, field-by-field wire comparison, served-result HTTP-200/kind/run-id checks, applicable/not_applicable crossCapture/finalizeReplay aggregates, full-run positive-solve requirement, empty-selection refusal) and adopted alpha's env-hygiene fix: _apply_dev_env now returns a restore callable and run_cases restores the process environment in a finally. Alpha's merged-scene _geometry_sha256 was superseded by this branch's canonicalized normalized-geometry hash (identity fully stripped, floats canonicalized) — both implement the same post-[obvious-fleet] Merge origin/alpha into the capture-API placement-case replay runner (resolve stale early-#271 conflicts) #274 review fix, and where they disagreed this feature branch's semantics stand.
• backend/tests/test_capture_placement_cases.py — took this branch's rewritten suite verbatim and kept two of alpha's three added tests verbatim (test_a_failed_finalize_replay_fails_the_cli — its faked replay dict is compatible with the branch's applicable-aware blocker, whose message contains "finalize replay"; and test_run_cases_restores_the_process_environment). Alpha's twin-negative cross-capture test is superseded by this branch's test_one_geometry_under_two_identities_fails_cross_capture, which pins the same intent — geometry compared, never identity stamps — through the solving path. One adaptation was required: the twin manifest's positive expectations changed from {"decision": "manual_review"} to _expected_a(), because alpha's hardened case_suite now rejects positive cases without geometric expectations.

Verification (in backend/, at merge commit 2a1dfd5)

• uv run pytest tests/test_capture_placement_cases.py -q → 19 passed (this branch's 17 + alpha's 2)
• uv run pytest server/placement/tests -q → 14 passed
• uvx ruff check + uvx ruff format --check on both files → clean
• uv run lint-imports → Contracts: 5 kept, 0 broken
• CLI smoke on the committed fixtures → exit 1, ok: false, 7 blockers, 4 cases — the pinned refusal state is still reported loudly, by design
• Full backend suite (uv run pytest -q) → 4 failed, 1426 passed, 18 skipped, 14 errors in 594s — the 4 failures are the documented pre-existing eval-gate failures (server/eval/tests/test_gates.py, expired SD-96 pins) and the 14 errors are the pre-existing tests/test_capture_cold_restart.py collection errors that pure alpha already ships (load_receipt import); identical to the two parents' baselines, nothing new from this merge.

[coderabbitai]

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: 8374e006-3ad2-4dab-a1ef-e969046eb4ef

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[obvious-autobuild]

Full backend suite at 5f9e295: 1280 passed / 17 skipped / 5 failed (4:26).

The 5 failures are the pre-existing baseline set, none on the PR's paths:

• tests/test_ssr_overlap_check.py::test_repo_has_no_ssr_shingle_overlap (SSR hygiene; needs git-ignored data/hygiene/ssr_shingles.sha256)
• server/eval/tests/test_gates.py eval gates x4 (test_committed_fixtures_are_scored_against_their_labels, test_offline_table_skips_the_engine_and_passes_the_unpatched_reference, test_route_over_the_bar_is_known_until_expiry, test_refresh_rewrites_pins_after_a_committed_edit)

Focused suite (tests/test_capture_placement_cases.py): 17 passed. Placement contract suite (server/placement/tests): 14 passed. ruff check + ruff format --check clean on both changed files.

[obvious-autobuild]

Independent review (Obvious, Autobuild + Max)

Verdict: ready — it does what the body claims, the fault-injection tests bite, and the full suite shows zero regressions. All findings below are non-blocking.

Findings (ranked by severity)

1. [low] _assert_merge_precedence is dead code while the module docstring claims it is "asserted explicitly". backend/scripts/capture_placement_cases.py:507 defines the function; it is never called anywhere (grep across scripts/, tests/, server/ finds only the definition), yet the docstring at :38 promises "same-id precedence asserted explicitly". No detection hole: the whole-document equality at :702 (json.loads(merged_bytes) == expected_merged) is stricter and subsumes the same fault class — a coordinator merge that failed same-id precedence would leave the merged copy ≠ the injected object, and that check fires. What is lost is only the targeted diagnostic. Smallest fix: call it in _collect_case right after the merged_matches block (_assert_merge_precedence(json.loads(merged_bytes), journey, case["mismatches"])), or delete the function and drop the parenthetical from the docstring.
2. [low] Body inaccuracy — runner size. The body says the runner change is "+~50 lines net"; the actual numstat is backend/scripts/capture_placement_cases.py +360/−33 (net +327, ~6.5× the claim). The tests line ("rewritten", +781/−38) is accurate.
3. [low] Body names the wrong base branch. The header says "→ base obv/burst7-bs-capture-scene-stage"; the PR's actual base — the one the same body's parenthetical correctly identifies — is obv/burst7-bs-capture-placement-runner @ bdfbb3f. obv/burst7-bs-capture-scene-stage points at 38a5735, several commits behind the real base.
4. [informational] The identity-strip in _normalized_geometry is unpinned by the suite. I removed the {"scene_id", "provenance", "schema_version"} strip (keeping all keys) and all 17 tests still pass, including test_one_geometry_under_two_identities_fails_cross_capture. Reason: the converted wire scene carries no per-run identity fields (the twins' converted outputs are identical even unstripped; scene_bridge.py mentions provenance only in a docstring), so today the strip is defense-in-depth. If the bridge ever starts stamping identity into the converted scene, nothing fails if the strip regresses. Smallest fix: a twin-style test whose two sources differ only in scene_id/provenance (same geometry), which the strip alone makes equal.

What I verified (commands and results)

Environment: detached worktree at the PR head 5f9e295, uv sync --frozen --extra api (uv 0.13.1, Python 3.12).

• uv run --frozen --extra api pytest -q tests/test_capture_placement_cases.py → 17 passed (19.1s) — matches the body's "17 passed".
• uv run --frozen --extra api pytest -q server/placement/tests → 14 passed — matches the body.
• uv run --frozen ruff check + ruff format --check on both changed files → clean — matches the body. (lint-imports: 5 kept, 0 broken.)
• Full suite at the head: uv run --frozen --extra api pytest -q → 5 failed / 1280 passed / 17 skipped (5:33) — exactly the counts in the existing bot comment; the failures are test_ssr_overlap_check.py::test_repo_has_no_ssr_shingle_overlap plus the 4 server/eval/tests/test_gates.py gates tests. Re-ran those two files on base bdfbb3f: 5 failed / 95 passed — identical set, so the PR introduces no new failures.
• Revert check (base runner + new tests): git checkout bdfbb3fc -- backend/scripts/capture_placement_cases.py at the head tree, focused suite → 12 failed, 5 passed. The new tests genuinely bind the new enforcement rather than passing against the burst-7 runner.
• Mutation check, hash bindings: neutralized both input_sha256 mismatch guards (if False and not ... at both binding sites) → test_forging_the_solver_input_hash_fails_the_suite fails. The bindings are load-bearing, not decorative.
• Mutation check, identity strip: see finding 4 — removing it leaves the suite green (coverage gap, not a live detection hole).
• Stack inspection: git merge-base 5f9e2957 <base> = bdfbb3f = base tip = [obvious-fleet] Merge origin/alpha into the capture-API placement-case replay runner (resolve stale early-#271 conflicts) #274's head — a single clean commit on top of the base branch, as the body describes; the runner's aggregate checks (finalizeReplay, crossCapture, full-run positive-solve requirement, empty/unknown --cases SystemExit at :883/:890) are all wired into blockers, and the receipt is ok only when there are none.

What I couldn't verify

• Whether the pinned success-path expectations (spot.span_ft, missing-evidence bands in _expected_a/_expected_b) would survive an independently re-derived solver — they are pinned from the solver's own deterministic output, as the body discloses; the independent oracle is the field-by-field wire-scene comparison, which does not depend on them.
• Nothing else material: the diff is confined to the two backend files, and every claim in the body's Verification section reproduced with matching results.

Nothing pushed, committed, merged, or approved — the only write is this comment.

[chatgpt-codex-connector]

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-16T06:39:10.798552Z	2a1dfd5	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]

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]

Confidence Score: 4/5

[Medium impact] Updates test documentation and validation logic for placement case replay.

The PR appears safe to merge, with a non-blocking gap around the unused same-id precedence check.

Findings

1.  The precedence check never runs ▶

Fix with agent prompt

### Issue 1
backend/scripts/capture_placement_cases.py:524-526
`_assert_merge_precedence` has no callers, although the updated documentation says same-id precedence is checked explicitly. The new success fixtures also inject no object that replaces an existing same-id object. This leaves unused code that looks like an active acceptance check.

Call it when collecting the merged scene and add a fixture with a same-id replacement, or remove the helper and narrow the documentation.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The replay runner now ties solver replies to their exact request bytes and compares converted scenes with hand-authored wire fixtures.

• Placement cases check solver inputs and results against independent evidence.
• A full placement replay only passes when its positive cases and aggregate checks pass.

Acknowledged scope:

• obvious-autobuild[bot] explicitly keeps the committed fixtures blocked by design. Their refusal state stays pinned; temporary fixtures supply the success path.
• obvious-autobuild[bot] explicitly replaces negative-only cross-capture checks with checks on solved positive cases.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Load pinned cases] --> B[Create and finalize captures]
  B --> C[Inject source scenes and objects]
  C --> D[Merge, convert, and solve]
  D --> E[Compare merged scene and wire geometry]
  D --> F[Check saved request hashes and direct reply]
  B --> G[Check served result and finalize replay]
  E --> H[Collect blockers]
  F --> H
  G --> H
  H --> I[Check distinct positive geometry]
  I --> J[Receipt and CLI exit code]

Loading

_{Reviews (1) · Last reviewed commit: "Merge remote-tracking branch 'origin/alp..." · Reviewed by Greptile}

[greptile-apps]

The precedence check never runs

_assert_merge_precedence has no callers, although the updated documentation says same-id precedence is checked explicitly. The new success fixtures also inject no object that replaces an existing same-id object. This leaves unused code that looks like an active acceptance check.

Call it when collecting the merged scene and add a fixture with a same-id replacement, or remove the helper and narrow the documentation.

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/scripts/capture_placement_cases.py
Line: 524-526

Comment:
**The precedence check never runs**

`_assert_merge_precedence` has no callers, although the updated documentation says same-id precedence is checked explicitly. The new success fixtures also inject no object that replaces an existing same-id object. This leaves unused code that looks like an active acceptance check.

Call it when collecting the merged scene and add a fixture with a same-id replacement, or remove the helper and narrow the documentation.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.