Repository navigation
[obvious-fleet] Backend: one repeatable no-cloud capture-flow runner with a JSON receipt - #258
Conversation
…N receipt scripts/capture_flow_e2e.py drives create_app() in-process over the committed mini-arkit packet: create, register, signed-URL PUTs, commit, finalize, events. It writes a receipt (request IDs, per-stage HTTP statuses, file counts, final capture status, error codes), exits nonzero on any unexpected result, and supports --skip-upload to reach the awaiting_files path on purpose. A test covers the happy-path receipt, the nonzero failure exit, and the CLI from a clean 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 real defect in the receipt/exit contract; everything else I checked verified clean. It is a draft, and the fix is a one-liner plus a test. Findings1. Medium — a crashed run exits 0 and the receipt says
|
… a stage A crashed run reported ok:true and exited 0: _finish computed success from errorCodes alone, so an exception with no stage code yet (rerunning into the receipt's recorded workdir hits copytree FileExistsError; a missing --packet hits FileNotFoundError) wrote ok:true beside the errorDetail and main exited 0. The except handler now records a runner_exception code and _finish treats any recorded detail as failure (ok = not errors and not detail). Two new tests cover the rerun-into-used-workdir and missing-packet crashes; both failed against the previous behavior.
…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>
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. |
 [Low impact] Adds a test script and test file for the capture flow. Fix the documented expected-missing-upload command before merging; it currently fails even when the API behaves correctly.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if payload.get("missing"): | ||
| errors.append("finalize_missing_files") |
There was a problem hiding this comment.
Running with --skip-upload keyframes/k00001.jpg --expect-status awaiting_files still exits 1. The deliberately skipped file adds both commit_not_found and finalize_missing_files, so matching the expected status cannot make the run pass.
Allow exactly the skipped path in those responses when awaiting_files is expected, keep rejecting other missing files, and add a test for this command.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_flow_e2e.py
Line: 268-269
Comment:
**Expected missing upload still fails**
Running with `--skip-upload keyframes/k00001.jpg --expect-status awaiting_files` still exits 1. The deliberately skipped file adds both `commit_not_found` and `finalize_missing_files`, so matching the expected status cannot make the run pass.
Allow exactly the skipped path in those responses when `awaiting_files` is expected, keep rejecting other missing files, and add a test for this command.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| journey.drain() | ||
|
|
||
| # Final status and the event log the device observes. | ||
| status = journey.client.get(f"/v1/captures/{capture_id}").json() |
There was a problem hiding this comment.
The status and events requests discard their HTTP responses before adding them to the receipt. If either request fails, the receipt loses its HTTP code. A body that is not JSON also prevents that stage from being recorded at all, leaving reviewers without the evidence needed to diagnose the failure.
Keep each response, record its status_code before reading JSON, and add a stage-specific error for a failed request.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_flow_e2e.py
Line: 273
Comment:
**GET requests lose HTTP codes**
The status and events requests discard their HTTP responses before adding them to the receipt. If either request fails, the receipt loses its HTTP code. A body that is not JSON also prevents that stage from being recorded at all, leaving reviewers without the evidence needed to diagnose the failure.
Keep each response, record its `status_code` before reading JSON, and add a stage-specific error for a failed request.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
fleet-packet: burst5-20261015-bs-local-runner
What changed
Reviewers who want to replay the capture journey by hand had two bad options: assemble the environment variables,
uvicorncommand andcurlsequence frombackend/server/api/README.mdthemselves, or read the journey out of a pytest file. This adds one checked-in command:It copies the committed
data/fixtures/packets/mini-arkitpacket into a fresh temporary directory, starts the real app entry pointcreate_app()in-process, and sends the app's own sequence: create the capture, register files, PUT bytes to the signed upload URLs, commit, finalizepacket.json, read status and events. When it finishes it writes a small JSON receipt (<workdir>/receipt.json) and exits 0 only if every stage matched what the app should do — including a crash before any stage, which now fails closed.Shell-only recipe vs the checked-in runner. The README's shell recipe is the same journey, but a reviewer must set six environment variables, pick a volume root, mint two tokens, start a server, then retype roughly six requests with correct idempotency keys and the
X-Packet-Sha256header — every step hand-carried, and the evidence (response bodies) lives only in the reviewer's scrollback. The runner keeps the same app entry point and the same request sequence, but makes the environment deterministic (memory state, volume storage, static auth, test HMAC), copies the committed fixture rather than trusting whateverVOLUME_ROOTpoints at, and leaves a file on disk that a later session can diff against a rerun. The shell recipe still exists and still works; the runner replaces the hand-carried part, not the stack.Why the receipt helps replay. A failed run today leaves nothing behind: a red CI line or a dead curl, with the capture ID only in logs. The receipt records
captureId, the finalizerunId, the idempotency key, per-stage HTTP statuses, file counts (registered / uploaded / committed / expected), the final capture status, and error codes (create_http_503,commit_sha_mismatch,finalize_missing_files,runner_exception,unexpected_final_status, …). Rerunning the command reproduces the journey byte-for-byte against the same committed fixture, and a failed receipt names the exact stage and code that diverged, so the next session starts from evidence instead of from a description. The tests pin the receipt's shape (backend/tests/test_capture_flow_e2e_runner.py), so later fields get added deliberately.What it executes. The HTTP ingest API and the finalize handoff exactly as the iOS device walks them:
POST /v1/captures,POST /v1/captures/{id}/files,PUT /v1/uploads/{token},POST /v1/captures/{id}/files:commit,POST /v1/captures/{id}/finalize(with the finalize background work drained), thenGETstatus and events. What it does not execute. The reconstruction/orchestrator pipeline after ingest (HSS_INGEST_HOOKS=null, so no runs are computed), no Modal or GCP deployment, no cloud storage, no network beyond the in-process ASGI transport, and no secrets — the bearer token and HMAC key are the same static test values the API suite uses. It also does not modify any route behavior;server/apicode is untouched.--skip-upload PATHregisters a file and never uploads it, which drives the capture intoawaiting_fileson purpose — combine with--expect-status awaiting_filesto check that path instead of asserting a failure.Why
The capture-fixture integration test (
backend/tests/test_capture_fixture_flow.py) proves the journey but only inside pytest. A reviewer reproducing it by hand rebuilds the setup from a README snippet and gets no comparable artifact back. The runner makes the same journey a single command whose output can be attached to a bug report and replayed later.Review fixes
Addressing the independent review of head
911efd6d(verdict: changes needed):Finding 1 (medium) — fixed. A crashed run reported success:
_finishcomputedokfromerrorCodesalone, and the catch-all handler finished the run with only anerrorDetailand no code, so a crash before any stage wrote"ok": truebeside the detail andmainexited 0 — contradicting the exit contract above and making the receipt lie in exactly the situations it exists for. The reviewer's two reproducers were real: rerunning into the receipt's own recorded--workdir(the natural replay action) hitscopytreeFileExistsError, and a missing--packethitsFileNotFoundError— both exited 0 withok: true.Two-layer fix in
scripts/capture_flow_e2e.py:runner_exceptioncode before finishing, so a crash is visible inerrorCodes, not just prose inerrorDetail;_finishcomputesok = not errors and not detail, so any path that records a detail without a code fails closed.Both reproducers now exit 1 with
ok: false,errorCodes: ["runner_exception"], and the detail preserved; the happy path is unchanged. Two new tests pin the contract —test_rerun_into_a_used_workdir_fails_closedandtest_missing_packet_dir_fails_closed— and each failed before the fix: on head911efd6dwithout the change,pytest tests/test_capture_flow_e2e_runner.py -qreturned 2 failed, 3 passed (both new tests asserting exit 1 against an exit 0); with the fix, 7 passed.Finding 2 (low, duplicated journey helpers) — no code change, on purpose. The reviewer marks it non-blocking and prescribes sharing a module only if a third copy appears; I agree with that trade and am not restructuring a reviewer tool it calls defensible. Sharing now would also have a concrete cost in either direction: importing the helpers from
tests/test_capture_fixture_flow.pywould pullpytestandserver.api.tests.conftestinto the CLI's runtime (both are imported there,tests/test_capture_fixture_flow.py:21-27), and having that test import from the script couples the base PR's committed test file to this stacked branch until both merge, breaking the base suite in the window between. The copies are identical today —inspect.getsourceequality holds for_content_typeand_sealed_files.How it works
The runner is a linear script over one in-process app instance; each stage appends to the receipt before any check runs, so a failure mid-journey still leaves every earlier stage's evidence behind:
flowchart LR A[copy committed packet] --> B[create_app\(\) in-process] B --> C[create 201] --> D[register 200] --> E[PUT signed URLs] --> F[commit 200] --> G[finalize 202] --> H[status + events] H --> I[receipt.json + exit 0/1]Verification
All commands run from
backend/withuv run --frozen --extra api, at headda0b6380:pytest tests/test_capture_flow_e2e_runner.py tests/test_capture_fixture_flow.py -q— 7 passed (runner happy-path receipt, nonzero failure exit via injected missing upload, CLI subprocess from a clean directory, the two new crash-contract tests, plus the two pre-existing fixture-journey tests).--workdir→ exit 1, receiptok: false,errorCodes: ["runner_exception"],errorDetail: "exception: FileExistsError: …";--packet /nonexistent/pkt→ exit 1, same shape.pytest -q, themake testcommand): 1210 passed / 5 failed / 17 skipped in 4:54. The 5 failures are the same pre-existing set as the base branch —test_ssr_overlap_check.py::test_repo_has_no_ssr_shingle_overlap(needs a git-ignored hygiene file) and 4server/eval/tests/test_gates.pycases — unchanged by this PR; head adds exactly its 5 new tests as passes over the base's 1205/5/17.911efd6dbefore the fix (2 failed / 3 passed).ruff check .— all checks passed.ruff format --check .— all 462 files formatted.lint-imports— 5 contracts kept, 0 broken.Not verified
scripts/e2e_dev.pyremains the live-stack gate.Label PR size(pass), Aikido (skipping) and CodeRabbit (skipped, bot not eligible) — no test or lint job runs onbackend/, so the local runs above are the only test evidence. Not caused by this PR; the branch changes no path CI watches./tmpis a small tmpfs the default temp workdir can fail; pass--workdir(observed here — see Assumptions).Risk
Low: two new files, no production code, no route changes, nothing under
.github/. Open PRs #256 and #257 add separate test files underserver/api/tests/and change no production code, so there is no overlap to reconcile with them.Assumptions
server/api/tests/conftest.py), so reviewers need no secrets; real deployment credentials never apply here.missingfiles, a final capture status other than the expected one (defaultprocessing), events that disagree with the capture status, a missingvalidatestage event, or any exception (recorded asrunner_exception— a crash can never report success).tempfile.mkdtempunder/tmp; on this sandbox/tmpis a small tmpfs, so verification ran withTMPDIRpointed at the larger root filesystem. No other thread's files were touched.Checklist
🔗 Obvious Project · 🧵 Obvious Thread