Repository navigation
[obvious-fleet] Merge origin/alpha into the capture-API placement-case replay runner (resolve stale early-#271 conflicts) - #274
Conversation
…ent runner branch
backend/scripts/capture_placement_cases.py replays every hash-pinned case in server/placement/tests/data/cases/manifest.json through the real capture front door (create, register, signed upload, commit, finalize, events, result) over the documented dev settings (SD-73), in-process under HSS_ORCH_STAGE_STUBS=1. Each case's synthetic source scene is injected only at the upstream scene stage boundary and an explicit object list at the objects_3d boundary, stamped with the actual capture/run/packet identity the scene contract requires. The runner collects a schema-versioned receipt (hss-capture-placement-receipt/v0): scene-merge equivalence against the case's stamped source merged once with the injected objects, refusal type/pointer against the manifest declaration, the conservative device outcome with exactly one terminal event, the tap-hazard merge count, cross-capture scene distinctness, a finalize-replay idempotency probe, and token redaction. Blockers print and the CLI exits 1; the receipt carries no secrets. Pinned fixture state (2026-10-15), recorded as blockers rather than papered over: both positive sources refuse conversion at /objects/0/geometry/standoff_m (a stand-off meter anchor the bridge does not synthesise) with no expected error declared, and generated-hazard-unsupported refuses at /generated_regions instead of its declared /objects/1/evidence pointer. The suite therefore exits nonzero until the fixtures or the bridge converge. backend/tests/test_capture_placement_cases.py pins the journey evidence, proves the boundary checks bite (reference-scene substitution and removal of the real placement stage both fail the suite), and exercises the one-command entry from a temporary working directory.
|
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: changes needed — one gate-completeness fix requested; everything else verified and non-blocking. The pinned blocked state is real and honestly reported (nothing papered over), the boundary checks genuinely bite, and the body's numbers all reproduce. Findings (ranked)1. [medium] A failed finalize-replay probe (or cross-capture collision) never affects the exit code. Evidence (local mutation, reverted after): with Smallest fix: in 2. [low] The cross-capture "distinct scenes" check is vacuous by construction. 3. [low] The solved path has zero coverage on the committed fixtures. 4. [informational] Verified (commands and results)
Couldn't verify
Worktrees used for this review were removed afterwards; no code was changed, pushed, or merged, and this comment is the only write. |
Review of PR #274 (changes needed) found that blockers were built only from per-case data: finalizeReplay.ok and crossCapture.distinctScenes were recorded but never consulted, so a replay-idempotency regression or a cross-capture scene collision left receipt ok:true and the CLI exiting 0 — the exit code is the only automation-facing gate while backend/ has no CI workflow. Both checks now append blockers, and a mutation-style test forces a failing replay and asserts exit 1. The cross-capture distinctness check also compared verbatim scene bytes, whose scene_id embeds the case, packet, capture and run — it could only ever fail when an artifact was missing entirely, presenting identity-stamp difference as evidence of different scenes. It now hashes identity-normalized documents (scene_id and provenance.inputs stripped), so identical geometry fails the check; a two-case same-geometry test manifest proves the twins' raw bytes differ while their geometry hashes are equal and the suite blocks. The dev-env flags are now applied reversibly: _apply_dev_env returns a restore callable and run_cases restores the prior environment in a finally block, so in-process runs no longer leak STATE_BACKEND/STORAGE_BACKEND/VOLUME_ROOT/ UPLOAD_URL_HMAC_SECRET into the rest of the pytest process; a test pins the full environ equality across a run. The solved-path evidence (directComparison, wireSemanticComparison, convertedSha256, resultSha256) still has zero coverage on the committed fixtures (none solves); per the review, its first real exercise belongs to the fixture-convergence PR that will make a case solve.
…into the pytest process Scoped port of the #274 review fix at c255a23 (the one piece the burst8 integrated copy lacked): _apply_dev_env returns a restore callable and run_cases restores the prior environment in a finally block, so in-process runs no longer leak STATE_BACKEND/STORAGE_BACKEND/VOLUME_ROOT/UPLOAD_URL_HMAC_SECRET into the rest of the pytest process; a test pins the full environ equality across a run. The rest of c255a23 (replay and cross-capture blockers, identity-normalized geometry hashing, the mutation tests) is already present in this branch's stronger #275-integrated form — distinctScenes AND distinctNormalizedGeometry AND distinct captureIds, applicable-aware replay blocking, full-run positive solve requirements — and is retained unchanged; the supported default manifest is not weakened. Evidence: the new test fails against the pre-port script (env leaked) and passes after; the placement-cases/pairs/suite suites pass (60 passed). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
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. |
 [Medium impact] Adds a test harness for the placement case replay pipeline. The PR needs fixes before merging because two earlier gaps still let broken behavior pass the runner.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case["deviceOutcome"] = { | ||
| "captureStatus": capture.get("status"), | ||
| "servedResultHttpStatus": served.status_code, | ||
| "servedResultKind": served.json().get("outcome", {}).get("kind") if served.status_code == 200 else None, | ||
| } |
There was a problem hiding this comment.
_collect_case records the served result's HTTP status and outcome but never checks them. If /result returns 503 or a wrong outcome while the capture remains manual_review, an otherwise clean case still passes. Add mismatches unless the served response is 200 with outcome manual_review, and test that these failures reach the CLI exit code.
Knowledge Base Used: Verification execution
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_placement_cases.py
Line: 625-629
Comment:
**Broken results still pass**
`_collect_case` records the served result's HTTP status and outcome but never checks them. If `/result` returns 503 or a wrong outcome while the capture remains `manual_review`, an otherwise clean case still passes. Add mismatches unless the served response is 200 with outcome `manual_review`, and test that these failures reach the CLI exit code.
**Knowledge Base Used:** [Verification execution](https://app.greptile.com/s-gu/-/custom-context/knowledge-base/samgu-nrx/basescanning/-/docs/verification-execution.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| if stage_status == "done" and case["hasPlacementArtifact"]: | ||
| case["status"] = "solved" | ||
| placement = json.loads(deps.storage.get_bytes(placement_artifact_key)) |
There was a problem hiding this comment.
The solved branch never checks record.expected_error. If a bridge change wrongly accepts a negative case, matching the direct solver response is enough to pass it. These cases have no wire_path, so their expected refusal is never checked. Add a mismatch whenever a case declaring expected_error solves.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_placement_cases.py
Line: 556-558
Comment:
**Expected refusals can pass**
The solved branch never checks `record.expected_error`. If a bridge change wrongly accepts a negative case, matching the direct solver response is enough to pass it. These cases have no `wire_path`, so their expected refusal is never checked. Add a mismatch whenever a case declaring `expected_error` solves.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| capture_id = journey.get("captureId") | ||
| if capture_id is None: | ||
| return None |
There was a problem hiding this comment.
_replay_finalize skips only journeys without captureId, but registration, upload, commit, and finalize failures leave a capture ID without a runId. When the first case hits one of these failures, replay accesses journey['runId'] and crashes before the receipt is written. Skip replay for failed journeys or those without a run ID, preserving the original error in the receipt.
| capture_id = journey.get("captureId") | |
| if capture_id is None: | |
| return None | |
| capture_id = journey.get("captureId") | |
| if capture_id is None or journey.get("runId") is None or journey.get("errors"): | |
| return None |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_placement_cases.py
Line: 437-439
Comment:
**Failed runs lose receipts**
`_replay_finalize` skips only journeys without `captureId`, but registration, upload, commit, and finalize failures leave a capture ID without a `runId`. When the first case hits one of these failures, replay accesses `journey['runId']` and crashes before the receipt is written. Skip replay for failed journeys or those without a run ID, preserving the original error in the receipt.
```suggestion
capture_id = journey.get("captureId")
if capture_id is None or journey.get("runId") is None or journey.get("errors"):
return None
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
…e-placement-runner
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c255a23529
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import server.orchestrator.stubs.objects_3d as objects_3d_stub | ||
| import server.orchestrator.stubs.scene as scene_stub |
There was a problem hiding this comment.
The runner patches server.orchestrator.stubs.objects_3d.run, but STAGES["objects_3d"] is configured with module objects and entry run_3d, so runner.stage_callable imports server.orchestrator.stubs.objects and looks up run_3d instead. The injected objects boundary is therefore never used (and the current stub module has no run_3d), causing the stage to fail before the intended case output is produced and making the tap-hazard/merge assertions invalid.
Useful? React with 👍 / 👎.
| merged = [case.get("mergedGeometrySha256") for case in cases[:2]] | ||
| cross_capture = { | ||
| "captureIds": [case.get("captureId") for case in cases[:2]], | ||
| "distinctScenes": len(merged) == 2 and merged[0] != merged[1], |
There was a problem hiding this comment.
The cross-capture gate only compares cases[:2], even though the runner processes every manifest case in the same app. If a later capture reuses an earlier case's geometry (or otherwise receives the wrong scene), the receipt still passes as long as the first two differ, so the advertised cross-capture isolation check does not cover the rest of the suite.
Useful? React with 👍 / 👎.
github-actions
Bot
added
size:XL
obvious-autobuild
Bot
changed the title
|
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. |
2 participants
fleet-packet: burst10-land-bs-placement-runner
What changed and why
This branch (obv/burst7-bs-capture-placement-runner, pre-merge head c255a23) carries the capture-API replay runner
backend/scripts/capture_placement_cases.pyand focused tests for placement cases (manifestserver/placement/tests/data/cases/manifest.jsonwalked through the real capture front door — create → register → signed upload → commit → finalize → events → result — with placement real under SD-73 dev settings andHSS_ORCH_STAGE_STUBS=1). GitHub reported conflicts againstalphainbackend/server/placement/case_suite.pyandbackend/tests/test_placement_case_suite.pybecause the branch had merged an early commit of #271 (ffd476d) while #271 landed in alpha with later edits to those files.This task merges
origin/alphainto the branch so the PR is mergeable. Nothing else changed: the branch's own files (backend/scripts/capture_placement_cases.py,backend/tests/test_capture_placement_cases.py,backend/tests/test_capture_placement_frontdoor.py) are byte-identical to pre-merge, and the two conflicted files take alpha's landed #271 versions in full.How you verified it, with commands and results
Merged alpha
72062663d47601cc82920474d6106d1e8a8ebcb8→ merge commit627533fa(normal merge; no rebase, no force-push). GitHub now reports PR [obvious-fleet] Merge origin/alpha into the capture-API placement-case replay runner (resolve stale early-#271 conflicts) #274MERGEABLE / CLEANat head627533fa.backend/server/placement/case_suite.py— merge result is byte-identical to alpha's blob (525fd768). The branch side's only edit to this file was the stale early-[obvious-fleet] test(backend): placement case gate rejects invalid manifests and forged input hashes #271 merge (fbcea110), so alpha's landed version is kept in full; the branch had no independent additions here to preserve.backend/tests/test_placement_case_suite.py— same: byte-identical to alpha's blob (e9e3ec33).origin/alphaand by grepping for conflict markers (none).uv run pytest tests/test_capture_placement_cases.py tests/test_capture_placement_frontdoor.py tests/test_placement_case_suite.py -q→ 37 passed (the PR's named suite; count grew from 21 because alpha's landed [obvious-fleet] test(backend): placement case gate rejects invalid manifests and forged input hashes #271 added tests totest_placement_case_suite.py).uv run ruff check→ all checks passed;uv run ruff format --check→ 490 files already formatted;uv run lint-imports→ Contracts: 5 kept, 0 broken.uv run pytest -q(full backend suite) → 5 failed, 1371 passed, 18 skipped, 14 errors in ~270s:server/eval/tests/test_gates.pyeval gates) — unchanged from this PR's own baseline runs.backend/tests/test_capture_cold_restart.py, a file that only exists on alpha: it importsload_receiptfromscripts.replay_capture, which alpha'sreplay_capture.pyno longer defines (alpha replaced it withreceipt_problem). Proven pre-existing on pure alpha: in a worktree at72062663the same import raises the sameImportError, and the three involved blobs in this merge are byte-identical to alpha's. Not a merge defect; left unfixed per scope.What is not verified
Assumptions
~/work/burst10-land-bs-placement-runner) on the existing BaseScanning repo sandbox, not in any other checkout.load_receiptor update the import) before those tests run green anywhere.