Repository navigation
[obvious-fleet] Backend: make the capture-journey fixture explicit and fail loudly when it disappears - #260
Conversation
…st entries The committed packet check ran with --no-files, so the keyframes and streams under data/fixtures/packets/mini-arkit were never re-verified: a file that left the repository passed make validate and only surfaced as a raw FileNotFoundError inside the capture journey test. The manifest now names the packet and all eleven files its packet.json references, and validate_fixtures runs a guard that fails loudly, naming the missing or drifted input, when the manifest, packet.json and disk disagree. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
|
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 |
|
Full backend suite on this branch (ab2d086): The 5 failures are the pre-existing baseline, unchanged from the base branch: |
|
Independent review (Obvious, Autobuild + Max) Verdict: ready — no blocking findings. Everything in the body that could be checked reproduced exactly; nothing failed to reproduce. What I verifiedAll commands run from
Non-blocking observations (no change requested)
Could not verify
|
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 validation for test fixture data integrity. The PR appears safe to merge, with a non-blocking gap in the new fixture check.
|
| for index, entry in enumerate(packet["files"]): | ||
| packet_files[entry["path"]] = index |
There was a problem hiding this comment.
Stale packet details pass validation
capture_journey_errors checks file bytes against manifest.json, but keeps only paths from packet.json. If a contributor updates a fixture file and its manifest hash without updating packet.json, the guard passes even though the packet declares the old size or hash. The earlier --no-files check cannot catch this either.
Compare each packet entry’s bytes and sha256 with the matching manifest entry, and add a test for this disagreement.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/validate_fixtures.py
Line: 64-65
Comment:
**Stale packet details pass validation**
`capture_journey_errors` checks file bytes against `manifest.json`, but keeps only paths from `packet.json`. If a contributor updates a fixture file and its manifest hash without updating `packet.json`, the guard passes even though the packet declares the old size or hash. The earlier `--no-files` check cannot catch this either.
Compare each packet entry’s `bytes` and `sha256` with the matching manifest entry, and add a test for this disagreement.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
fleet-packet: burst5-20261015-bs-fixture-manifest
What changed and why
The capture journey test (
backend/tests/test_capture_fixture_flow.py, PR #254) copies the committeddata/fixtures/packets/mini-arkitpacket and drives the app's upload sequence over it. Two things made that fixture implicit:mini-arkit, kindpacket_dir) but hashed onlypacket.json— none of the eleven keyframes and streams itsfilesarray references. Directory discovery (the glob in_packet_manifests, plusshutil.copytreein the test) was the only thing tying those files to the packet.server.contracts.cli check-packets --no-filesfor every manifest, so nothing on the validation path ever read those files from disk. A keyframe could leave the repository andmake validatestill passed; the only symptom was a rawFileNotFoundErrorfromcopytree/_sealed_filesinside the journey test.The change makes the fixture explicit and fails loudly when it disappears:
backend/data/fixtures/manifest.jsongains elevenkind: "file"items, one per filemini-arkit/packet.jsonreferences, each with its ownbytesandsha256. The diff is purely additive (121 insertions, 0 deletions); every existing hash, includingmini-arkit's ownpacket.jsonentry (9f59f921…, 13734 bytes), is byte-identical. Each new hash was verified equal to the matchingsha256already recorded insidepacket.json'sfilesarray — the manifest restates what the packet asserts, nothing new is invented.backend/scripts/validate_fixtures.pygainscapture_journey_errors(fixtures_dir), wired into_validate_committedafter the packet checks. It cross-checks three sources and fails with named errors when they disagree: the manifest items under the packet directory,packet.json'sfilesarray (by path and index), and the bytes on disk. It reportsmissing input <repo_path>naming both the manifest item and the packetfilesindex,does not matchfor size/hash drift, andreferences <path>, which the fixture manifest does not namewhen the packet references a file the manifest omits. It also re-verifies thepacket_direntry's hash ofpacket.jsonitself. Nothing was loosened: no threshold moved, and missing laser/GPU assets still read as absent (_score_selfstill exits 2 on a missing PLY; the guard adds a check, it removes none).Why a guard instead of implicit directory discovery
Implicit discovery (globs +
copytree) has no opinion about which files the fixture needs, so it cannot say what went missing — it fails where it happens to touch the gap, with a filesystem error and no name. The explicit manifest guard moves the decision to validation time, before any test runs:make validatenow exits 1 and prints the exact missing input, its manifest item, and itspacket.jsonindex. That protects the app path because the journey test exercises the same sequence the iOS uploader sends —create → register → PUT → commit → finalize → events— so a silently incomplete fixture would fail CI with an opaqueFileNotFoundError(or worse, pass if the test's skip-path happened to cover the missing file) instead of telling a reviewer the fixture is broken. With the guard, the fixture contract is checked where every other golden fixture's contract is checked.How I verified it
All commands run from
backend/in a fresh clone at~/work/burst5-20261015-bs-fixture-manifest(BaseScanning repo sandbox, option a; the sandbox's pre-existing checkout was not touched):uv sync --frozen— clean install, Python 3.12.uv run --frozen python scripts/validate_fixtures.py—validate ok scenes=5 verdicts=16 labels=61 packets=36, with the guard active. Same counts as the inventory PR ([obvious-fleet] Backend inventory after the port into backend/: checks run, one post-move path bug fixed #248) reported.keyframes/k00003.jpgout of the tree, re-ran validate — exit 1 withcapture-journey fixture guard failed: mini-arkit: missing input data/fixtures/packets/mini-arkit/keyframes/k00003.jpg (manifest item mini-arkit-file-keyframes-k00003-jpg, packet.json /files/4); restored the file — exit 0. (Note the pre-existingcheck-packets --no-filesstill printsokfor the packet in that state; the guard is what catches it.)uv run --frozen --extra api pytest -q tests/test_capture_fixture_manifest.py tests/test_capture_fixture_flow.py— 7 passed (5 new guard tests + the 2 journey tests). New tests: committed fixture passes the guard; a temporary copy missing a keyframe, and one missing a stream, each fail naming the input; a packet file absent from the manifest fails naming the path; a tampered file fails naming the drift.uv run --frozen ruff check .— all checks passed.uv run --frozen ruff format --check .— 461 files already formatted (one reformat applied to the new seam code first).uv run --frozen lint-imports— contracts 5 kept, 0 broken.uv run --frozen --extra api pytest -q— result appended in a comment below once the run finished.What is not verified
eth3d-*-laser/eth3d-*-occluders/colmap-*points (committed: false, fetched from the fixture store, which is unreachable here) and the GPU weights for the recon/detection tracks.make validate-full,make criteria-goldenand the breadth tracks were therefore not runnable — the committedmake validatetarget, which this PR changes, was.Assumptions
data/fixtures/packets/mini-arkit— the only packettest_capture_fixture_flow.pyuses, and the onevalidate_fixtures.pyalready named explicitly (SD-43).packet.jsonitself asserts (verified equal); thepacket_direntry's existing hash ofpacket.jsonis preserved untouched.mini-arkit-file-<path>pattern and sit adjacent to the existingmini-arkitentry, matching the manifest's identifier style; the manifest's existing (loose) ordering convention is unchanged.git add -fformanifest.jsonbecausebackend/datais gitignored for real captures whiledata/fixtures/manifest.jsonis tracked.obv/burst5-20261015-bs-capture-fixture.