Repository navigation
[obvious-fleet] Backend burst8 integration: one command replays the supported capture packet end to end - #277
Conversation
… input hashes One evaluator contract change, from the P1 (Codex r4287473089) and P2 (Greptile r4287478936/r4287478951) findings on #271: - load_cases requires a nonempty cases list, unique nonempty ids, a recognized kind, the fixture records each kind needs (positive: source plus wire; negative: source), meaningful positive expectations (a pinned decision plus at least one geometric expectation of the right shape) and a negative expected_error with nonempty code and pointer. Malformed types are rejected with the manifest path and field instead of a silent default; fixture hash/path errors are still validated before any record is returned. - _positive_case now requires stats.input_sha256 to equal the sha256 of the exact posted wire bytes; a discrepancy becomes a named mismatch, so it fails the case status, report.ok and the CLI exit. load_cases and the existing four-case manifest stay compatible: no fixture repinning, no new geometry, no solver changes.
The case-integrity fix rejects decision-only positive expectations, so the twin-identity manifest now pins the coverage chain ends as well; the cross-capture refusal still comes from normalized geometry, not the loader.
- Deliver the tap hazard at the objects_3d boundary only when the case's source does not already carry it: the merge contract replaces only detector/VLM twins, so a same-id re-delivery of a tap-sourced hazard duplicates the id and is refused as an inconsistent scene. - Evaluate wire-pinned expectations within the packet's declared 1e-4 ft derived-output tolerance (the shared evaluator stays exact by default). - Record the integrated producer SHAs and a distinct solver decision in the receipt; default the replay CLI to the supported packet manifest.
The real replay CLI runs as a subprocess from a temporary cwd with fresh work directory and state: the packet default solves both positives through real conversion/merge/placement with all receipt cross-checks, deliberately corrupted fixtures and unexpected positive refusals exit nonzero, and the legacy cases manifest stays available explicitly with its own pinned refusal state reported as blockers.
The replay CLI's default manifest is now the supported packet, so the two producer tests pinning the committed legacy refusal state pass that manifest explicitly; their intent (committed legacy fixtures stay bridge-incompatible, nonzero exit) is unchanged. Also reformat test_placement_case_suite.py, unformatted since the #271 integrity-fix import (bad040c), so the full lint battery is clean.
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. |
|
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 |
|
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. |
![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: ba7eba2228
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
 [Medium impact] Adds stage-record recovery and placement validation logic. Fix the skipped result update for older captures before merging.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…play can never report ok Scoped imports of two completed upstream review fixes, with their regression tests, onto the burst8 integration branch: - #258 at da0b638 (capture_flow_e2e.py): a run that crashes before any stage code exists reported ok:true beside its errorDetail and exited 0. The except branch now records runner_exception and _finish treats any recorded detail as failure (ok = not errors and not detail). Tests: rerun into the receipt's recorded workdir and a missing --packet both exit 1 with ok:false. - #262 at e2aa8a2 (replay_capture.py): run_replay's except branch stored errorDetail but appended no error code, and a pre-replay receipt without the replay block passed the fixture check vacuously and died on KeyError with a green report. The replay now validates the replay block up front with a named missing_replay_block error, appends exception:<Type> on any crash, flags receipt paths outside the committed manifest as fixture drift, and drops the dead _content_type. The later receipt metadata this branch already carries (idempotencyKeys/files/packetSha256 emission) is preserved. Evidence: all five imported tests fail against the pre-fix scripts (5 failed) and pass with them (14 passed across both suites). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
github-actions
Bot
added
size:XL
…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>
…servations scene_bridge._coverage appended every coverage.sensitivity[].bands entry to the converted observed list, so a what-if sensitivity variant presented itself as ground-truth coverage the device saw and a sensitivity-only scene showed as a covered wall. Sensitivity bands keep their full validation (span, wall, out conversion, refusal pointers unchanged) but no longer convert into observations; a sensitivity-only scene still converts to ends only — a coverage gap, not a covered wall. The shared base_scene fixture carries a distinct primary band beside the sensitivity variant, pinning that only the primary band converts; new tests pin the sensitivity-only gap semantics and that a malformed sensitivity band still refuses with its own pointer. Evidence: the happy-path and sensitivity-only tests fail against the pre-fix bridge (2 failed) and pass after; the scene bridge and placement-stage suites pass (28 passed). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
…d run cannot hang An unfinished run written by an older build can lack stage records entirely: _status_map treats a missing record as pending, but _spawn_one refuses to start a stage with no record, so every tick spawned nothing and the run hung forever between pending and running. tick now materializes only the missing pending records (same shape the API writes) before spawning: idempotent, existing records keep their attempts/failures/status, and terminal runs never reach it because tick returns before spawning. Evidence: the new test — a run with no stage records plus a pre-existing done validate record — fails against the pre-fix coordinator (run stuck running) and passes after; the handoff/run/placement/round2 suites pass (71 passed). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
…cit objects list A format-correct objects_3d manifest whose objects field is missing, null, or not a list is a malformed manifest, not evidence of a hazard-free scene. The placement stage now refuses such artifacts with placement_objects_3d_invalid before any placement artifact is written; an explicit empty list remains a valid, deliberate "no hazards". Evidence: the journey test publishes a format-correct artifact with a null and a missing objects list through the real capture stage and asserts the refusal code, no placement artifact, and a receipt blocker; it fails against the pre-fix adapter and passes after. The focused placement, orchestrator, and runner suites pass (258 passed). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com> EOF2 fix(backend): supported capture fixtures carry their observed coverage as primary bands The supported sources held their only coverage band under the sensitivity what-if wrapper while the independently authored wire scenes expect that band in coverage.observed. That only passed while the bridge wrongly promoted sensitivity bands to observations; since b9032a7 sensitivity variants are validated but are not observations, so the converted scene lacked /coverage/observed and both supported positives failed the receipt check. Each fixture now publishes the observed band as a primary coverage band (the ground truth the device saw) and keeps the sensitivity variant as the what-if it is. The supported end-to-end and supported-CLI tests fail at the previous head and pass after; the focused suites pass (258 passed). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
…e as primary bands The supported sources held their only coverage band under the sensitivity what-if wrapper while the independently authored wire scenes expect that band in coverage.observed. That only passed while the bridge wrongly promoted sensitivity bands to observations; since b9032a7 sensitivity variants are validated but are not observations, so the converted scene lacked /coverage/observed and both supported positives failed the receipt check. Each fixture now publishes the observed band as a primary coverage band (the ground truth the device saw) and keeps the sensitivity variant as the what-if it is. The supported end-to-end and supported-CLI tests fail at the previous head and pass after; the focused suites pass (258 passed). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
7d786cf to
e2376df
Compare
…ordinator cannot reset a running stage Recovery for an old run's missing stage records was a check-then-upsert: two coordinators can resume the same run without an exclusive claim (the sweeper, _resume_if_stale, and the worker tick), so one could read a missing record, pause, watch the other create it and a worker start it, then overwrite that running record with pending, attempt 1, and no worker id — losing the first worker and launching a second one against the same stage. state gains create_stage_if_absent: MemoryState takes a dedicated stage lock across the deciding read and the insert; ModalDictState reuses put_if_absent, whose Modal Dict skip_if_exists is the atomic cross-process claim, under a per-(run, stage) lock for the plain-dict fallback. The coordinator materializes missing records through it. Also format-only cleanups in files pushed earlier. Evidence: five new tests — coordinator-level (the racing record survives materialize with its attempt and worker id) and backend-level claims on both state backends, including eight threads claiming one record — fail against the pre-fix coordinator and backends, pass after; the focused common, orchestrator, placement, and runner suites pass (296 passed). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
|
Four more units on the head branch, each with a bite check (fails before, passes after): a993208 — recovery cannot hang. An older unfinished run can lack stage records entirely: the status map treats a missing record as pending, but c4632e3 — malformed objects_3d refuses. A format-correct e2376df — supported fixtures align with the sensitivity decision. Both supported sources held their only coverage band under the sensitivity what-if wrapper while the wire scenes expect it in 1327752 — recovery claims are atomic. Two coordinators can resume the same run without an exclusive claim, and the recovery check-then-upsert could reset a record another coordinator had created and a worker started (pending, attempt 1, no worker id — a second worker against a live stage). Verification. Seven-case committed-manifest CLI: 2 solved, 5 refused all with declared codes, blockers empty, exit 0. Repo hygiene clean (3135 tracked files). Full backend suite at head 1327752: 1320 passed / 17 skipped / 5 failed — the identical pre-existing baseline (SSR hygiene + 4 eval gates), reproduced on the base head. |
Co-authored-by: Sam Gu <sgu07966@gmail.com>
…e merged runtime Alpha's _collect_case publishes mergedGeometrySha256 for every case, so the legacy manifest's two refused positives now carry geometry hashes and the cross-capture comparison runs instead of skipping — and finds distinct scenes with distinct normalized geometry. The pinned expectation follows the merged data; the anti-vacuous intent (never pass without a real comparison) still holds, now in its stronger branch. Co-authored-by: Sam Gu <sgu07966@gmail.com>
obvious-autobuild
Bot
changed the title
_materialize_missing_records claimed all twelve stages on every tick. With ModalDictState each claim is a conditional remote write plus a read-back, so a run whose records all existed paid 24 sequential round-trips per tick. The tick has already read the run's rows; claim only the stages absent from that read and keep the atomic insert for those. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_STAGE_LOCKS kept a lock for every (run_id, stage) a worker ever claimed and never dropped them, so long-lived workers grew it without bound. The lock only guards the plain-dict fallback's check-then-set (a real Modal Dict claims atomically with skip_if_exists), and claims now happen only for missing records, so one module lock is enough. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Brings in #275 (replay acceptance) and #278 (runtime join) so #277 can land on alpha. backend/scripts/capture_placement_cases.py: the one conflict is the solver-response block; this branch's version is #275's plus solverDecision, so it is kept. Alpha's removal of _geometry_sha256 also merges in, so mergedGeometrySha256 has one meaning again: the canonical normalized geometry of a converted scene. backend/tests/test_capture_placement_cases.py: this branch's suite (bands on the supported sources, explicit --manifest for the legacy CLI runs, the malformed objects_3d test) plus: - the twin manifest pins _expected_a() for both twins (#275), and the twin test now asserts both twins solve cleanly so crossCapture is the only blocker (#277's intent that the refusal comes from geometry); - the legacy manifest's crossCapture is not_applicable again, because refused positives carry no converted geometry hash once _geometry_sha256 is gone; it now also asserts a reason and no crossCapture blocker; - test_a_failed_finalize_replay_fails_the_cli is restored from alpha; it checks the CLI exit code, which the run_cases variant does not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d manifest capture_cases_v1/hazard-right-ground-gap.source.json already carries obj-tap-vent, identical to the runner's TAP_HAZARD. merge_object_lists keeps a tap-sourced object and appends the incoming list, so injecting the vent there too (as alpha's runner still does) merges two vents and the supported positive stops solving. This branch already injects only when the source lacks the id; that decision moves into _objects_3d_for so a test can call it. The new test runs the real merge on each committed manifest's tap case and requires exactly one vent equal to TAP_HAZARD. Making the injection unconditional fails the capture-cases-v1 case; the legacy case, whose source has no vent, still gets the injected one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ecords After the alpha merge, test_confirmation_updates_the_reads_record failed: #278's _recompose_after_correction flips a done run back to running, and this branch's _materialize_missing_records then creates pending records for every absent stage. On a done run with no stage records, reads re-ran and overwrote the correction just persisted. Neither parent fails alone. _materialize_missing_records assumes terminal runs never reach it. Only a run that has a record for every stage is reopened now; any other run stays closed, and a warning is logged. start_run writes every stage record, so current runs still recompose. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
size:L
|
@codex review |
| present = {rec.stage for rec in services.state.stages(run.run_id)} # type: ignore[attr-defined] | ||
| if not present.issuperset(STAGE_NAMES): | ||
| # A finished run without a record for every stage predates per-stage | ||
| # records. Reopening it would let the coordinator's missing-record | ||
| # recovery re-run its upstream stages, and the re-run reads stage would | ||
| # overwrite the correction just persisted. Keep the run closed instead. |
There was a problem hiding this comment.
For an older finished capture missing any stage record, on_confirmation saves the corrected answer, but this guard skips rebuilding the result. GET /result reads the existing result file, so the user gets a success response while still seeing the previous answer. Preserve the upstream work while rebuilding criteria and result, rather than returning without applying the correction.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/api.py
Line: 171-176
Comment:
**Older results ignore corrections**
For an older finished capture missing any stage record, `on_confirmation` saves the corrected answer, but this guard skips rebuilding the result. `GET /result` reads the existing result file, so the user gets a success response while still seeing the previous answer. Preserve the upstream work while rebuilding `criteria` and `result`, rather than returning without applying the correction.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Confirmed. Landed #277 anyway: on alpha before this PR such a run was left stuck in running, so the guard is strictly better, and the fix (rebuild criteria and result while preserving upstream work, with a failing-first test) is the required first item of burst9-bs-real-downstream-journey, which owns backend/server/orchestrator/api.py. — Apps lead
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
fleet-packet: burst10-land-bs-capture-path
What changed and why
Merged
origin/alphainto this branch so PR #277 can land onalpha(the end-to-end test branch where finished work lands, per Sam's 2026-10-16 decision). Merge commitfac367e6(alpha tipc2d4bb27), plus one follow-upc3e479bfadapting a pinned test expectation to the merged runtime. Nothing else changed; no CI or workflow files touched; no rebase, no force-push, nothing pushed toalpha.The three conflicted files and how each was resolved (alpha's version for changes alpha made, this branch's for its own feature):
backend/server/common/backends/state_memory.py— alpha's_event_lock(atomic event-seq append) and this branch's_stage_lock+ atomiccreate_stage_if_absentare both kept; the two features compose (a dedicated lock per write path).backend/scripts/capture_placement_cases.py— four hunks, all kept on this branch's integrated versions: the case-selection guard +full_runflag, the replay/cross-capture receipt shapes with applicability handling, the blocker gates (replay idempotence, normalized-geometry cross-capture, full-run positive-solve requirement), and the four-SHA producer receipt. Alpha's auto-merged changes all survive: the_apply_dev_envrestore-callback (finally: restore_env()runs), the_geometry_sha256identity-strip helper, andmergedGeometrySha256published for every case in_collect_case. Alpha's gate intent — replay and cross-capture failures must block — is carried by this branch's stronger versions.backend/tests/test_capture_placement_cases.py— this branch's suite kept (docstring,import subprocess, the supported-manifest and fault-injection sections, the malformed-objects_3dtest). Alpha's two conflicting tests pinned alpha's runner shapes (its default manifest, its blocker wording, negative-twin gating); all three of alpha's test intents are already covered by this branch's equivalents:test_a_failed_finalize_replay_blocks_the_suite,test_one_geometry_under_two_identities_fails_cross_capture,test_run_cases_restores_the_process_environment.Semantic conflict beyond the markers (follow-up
c3e479bf): alpha's_collect_casenow publishesmergedGeometrySha256for every case, so the legacy manifest's two refused positives carry geometry hashes and the cross-capture check legitimately evaluates instead of skipping.test_every_case_walks_the_real_front_door_with_case_true_boundariesnow assertsevaluatedwith distinct scenes and distinct normalized geometry — the anti-vacuous intent (never pass without a real comparison) holds in its stronger branch.How I verified it
uv run --frozen --extra api pytest -q tests/test_capture_placement_cases.py tests/test_capture_case_pairs.py tests/test_placement_case_suite.py tests/test_capture_placement_frontdoor.py server/placement/tests/test_scene_bridge.py tests/test_capture_replay_cli_acceptance.py server/common/tests/test_state_memory_events.py server/common/tests/test_stage_record_claim.py server/orchestrator/tests/test_handoff.py→ 103 passed.ruff check→ "All checks passed!";ruff format --check→ 498 files clean;lint-imports→ 5 contracts kept, 0 broken.uv run --frozen --extra api pytest -q, 8:55): 1441 passed, 4 failed, 19 skipped, 14 errors. The 4 failures are exactly the pre-existingserver/eval/tests/test_gates.pygates (reproduced by a targeted run on the merged tree). The SSR hygiene failure that pre-existed on this branch now passes on the merged tree. The 14 errors are alpha's own brokentests/test_capture_cold_restart.py— verified identical on a purec2d4bb27worktree (14 errors there too), so they pre-date this merge.make serverwith the pinned uv 0.11.15 → 312 passed, exit 0, same as the PR's pre-merge run.13277524..fac367e6..c3e479bf, fast-forwards only.What is not verified
Assumptions
_collect_caseenrichment is an alpha change, so the branch's stale runtime pin (cross-capturenot_applicableon the legacy manifest) adapted to the data instead of the enrichment being reverted.alpha(already correct).Human author: Sam Gu (sgu07966@gmail.com)
🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact