[obvious-autobuild]

fleet-packet: burst5-20261015-bs-capture-contract

Review response (2026-10-15)

The independent review (issuecomment-6199465927, verdict: changes needed) ranked two findings. Both accepted and fixed in 870725da; nothing disputed.

1. [medium] ruff format --check failed on the PR's own test file — fixed. The original verification list claimed only ruff check; the repo's real gate (backend/Makefile lint target, backend/CONTRIBUTING.md "Checks") is ruff check + ruff format --check + lint-imports. On head a4f8c5a, before the fix: uv run --frozen ruff format --check server/api/tests/test_capture_contract_swift.py → exit 1, "1 file would be reformatted" (the wrapped client.post(...) call-sites and the inline file dicts, exactly as the review showed). Fix: uv run --frozen ruff format server/api/tests/test_capture_contract_swift.py → "1 file reformatted". After: the check passes repo-wide ("460 files already formatted"). No assertion changed. The format gate is now in the verification list below.

2. [low] eventsUrl presented as a production-decoded key — fixed (comment + docstring; assertion kept). Verified against the sources before changing anything: CaptureAPI.CreateResponse decodes exactly captureId/status/upload/finalizeBy — no eventsUrl; CaptureUploader.pollEvents() builds the events URL itself (base.appending(path: "captures/\(captureID)/events")); only ios/HouseScanKit/Tests/HouseScanKitTests/LoopbackCaptureAPI.swift (~L265) emits/decodes eventsUrl; and the server does return it (backend/server/api/routes/captures.py _render → eventsUrl=f"/v1/captures/{id}/events"). The wrong in-test comment ("eventsUrl feeds the poller") is corrected to state the struct omits it and the poller builds the URL, and the module docstring now carries the one documented exception. The eventsUrl assertion itself is kept — it pins real server behavior the Tests-target loopback double depends on — and the Create bullet below says the same. No test accompanies this finding: it corrects a comment and a docstring, not behavior, so there is nothing for a test to fail on; the evidence is the file:line reads above.

Also acted on (noted by the review, not ranked): the uploader's 404 result_not_ready poll path (CaptureUploader.swift:653) is untested here — added to "What is not verified" below. Out of scope for this PR per the review's own framing (repo-wide context, predating this PR): backend/ has no CI workflow, so local make lint is currently the only thing running the documented gate.

Commands run for this round (from backend/, head 870725da):

• uv run --frozen ruff format --check server/api/tests/test_capture_contract_swift.py → exit 1 before the fix, exit 0 after.
• uv run --frozen ruff format server/api/tests/test_capture_contract_swift.py → 1 file reformatted.
• uv run --frozen ruff check . → All checks passed.
• uv run --frozen ruff format --check . → 460 files already formatted.
• uv run --frozen lint-imports → 5 kept, 0 broken.
• uv run --frozen --extra api pytest server/api/tests/test_capture_contract_swift.py -q → 28 passed.
• uv run --frozen --extra api pytest server/api/tests -q → 151 passed.

What changed and why

The iOS capture uploader (ios/HouseScanKit/Sources/HouseScanKit/Upload/CaptureAPI.swift) decodes specific JSON keys from /v1/captures and below, and this contract had no focused proof: the existing suites test server behavior, not the exact shapes the Swift client reads. This adds backend/server/api/tests/test_capture_contract_swift.py — 28 tests, one file, no production code touched — that assert, for each endpoint the uploader walks (create, register, signed PUT, commit, finalize, events, result polling):

• Create (201): captureId (cap_ prefix), status == "uploading" (not in CaptureAPI.terminalStatuses), upload limits keys maxSinglePutBytes/urlTtlS/maxFiles/maxBatch (Swift Limits; partBytes is extra and ignored, which the optional decode tolerates), finalizeBy — plus eventsUrl, which the server returns but the production CreateResponse struct does not decode (the uploader builds the events URL itself; only the Tests-target loopback double reads it — pinned here as server behavior).
• Error body everywhere: {"errors": [{code, pointer, message}]} — the shape CaptureAPI.errorCodes(_:) decodes — with the exact codes: unauthorized (/headers/authorization, plus the WWW-Authenticate: Bearer realm="capture-api" header), idempotency_key_missing, idempotency_conflict, format_version_unsupported (/formatVersion), path_invalid, bad_md5, content_type_invalid (/files/... pointers), bad_digest, sha256_conflict, packet_id_mismatch, packet_sha_mismatch, capture_expired, invalid_state, schema (/query/after on events).
• Register (200): files[].path/state("pending")/upload with upload.method == "PUT", url under /v1/uploads/, headers, expiresAt (Swift RegisteredFile/UploadTarget).
• Signed PUT: works with no Authorization header — the HMAC token is the only credential — and wrong Content-MD5 returns 400 bad_digest.
• Commit (200): exactly committed/notFound/mismatch (Swift CommitResponse); re-commit is safe; unregistered paths land in notFound; a conflicting sha256 is 409 at /files/0/sha256.
• Finalize (202): status ("processing" or "awaiting_files"), missing, runId, etaS (Swift FinalizeResponse); packet listed with uncommitted files reports them in missing; Idempotency-Key must equal the packet id; the same packet bytes re-finalize with the same runId (the uploader retries finalize); expired/finished captures are rejected with capture_expired/invalid_state.
• Events: status/next/events[] with seq/type/at/data, Cache-Control: no-store, cursor polling returns an empty list and an unmoved cursor, files_committed after commit, and status carries terminal states (failed).
• Result polling: runId, status, viewsNeeded, memberActions, outcome — empty/nil before the result exists, and a failed capture polls terminal with outcome == nil, matching CaptureAPI.statusesWithoutOutcome.
• Create idempotency: same Idempotency-Key + same body replays 200 with the same captureId; different body with the same key is 409.

Open PRs #248–#252 were read first. None of their route deltas change the shapes asserted here (#250's delete-time packet-cache purge and uploads-signature change, #252's finalize background handoff, #248/#249 scripts/review/docs), and no test asserts on behavior those PRs fix, so this PR neither depends on them nor conflicts with their findings.

How I verified it

Environment: fresh clone on the BaseScanning repo sandbox, worked only in ~/work/burst5-20261015-bs-capture-contract on obv/burst5-20261015-bs-capture-contract (from feat/port-house-scanning-server). All commands re-run on head 870725da after the review fixes.

• uv run --frozen --extra api pytest server/api/tests/test_capture_contract_swift.py -q → 28 passed.
• uv run --frozen --extra api pytest server/api/tests -q → 151 passed (the 28 new plus the 123 pre-existing, no regressions).
• uv run --frozen ruff check . (from backend/) → All checks passed.
• uv run --frozen ruff format --check . (from backend/) → 460 files already formatted — the format gate from backend/Makefile lint, which the initial push failed (see Review response).
• uv run --frozen lint-imports (from backend/) → 5 kept, 0 broken.

Two initial test expectations were wrong and the tests now encode the real contract, verified against the routes: a path like ../escape.jpg is rejected by the shared RelPath contract as a schema error before the route's own path_invalid rule runs (so the route-level test uses packet.json, which clears RelPath but fails the route check), and a started run appends stage events after files_committed, so the lifecycle test accepts both.

Route-only unit test vs HTTP app test. Calling the route handlers directly (importing the route module and invoking the handlers with a fabricated Request) would be marginally faster, but it would bypass exactly the layers the wire contract lives in: the _Access middleware that enforces the bearer check on every non-upload path and returns the unauthorized error shape, the ApiFailure exception handler that renders every failure as {"errors": [{code, pointer, message}]}, the RequestValidationError handler that turns shared-contract (RelPath/Sha256Hex) rejections into the same error body with schema/role_unknown codes, and the body-cap logic. That difference is not hypothetical: the ../escape.jpg → schema behavior and the 401/WWW-Authenticate contract only exist at the app boundary, and a route-only test would have passed while the Swift client saw different bytes. The app-level TestClient path was chosen for every test; nothing is tested below the HTTP surface.

What is not verified

• No iOS build or Swift decode round-trip: swift/Xcode are unavailable in this sandbox. The asserted keys, codes and statuses were read from CaptureAPI.swift by hand; the server side of the decode is proven, the client decode side is not executed.
• The uploader's 404 result_not_ready poll path (CaptureUploader.swift:653) is not exercised here; the tests cover the 200 polling path and the terminal-status result paths (flagged by the review, see Review response).
• Terminal statuses needs_views, manual_review, and complete after a real run are exercised only via memory-state mutation (failed, expired, complete pre-run); no orchestrator run was started end to end (hooks are null in tests by design), so the real run's stage/verdict_ready event payloads and a fully materialized result body (outcome, verdictUrl, previewUrl, reviewUrl) remain unverified.
• Long-poll wait > 0 timing behavior (the tests poll with the default) and Retry-After header values on 429s are not asserted.
• Deployment/Modal paths (modal_entry, GCP storage) are untouched and untested here, as intended: memory state and local volume storage only.

Assumptions

• "Register" in the task maps to POST /v1/captures/{id}/files (register_files) and "commit" to POST .../files:commit — the route module names match.
• Result-polling decode keys (runId, status, viewsNeeded, memberActions, outcome) were taken from server/contracts/result.py ResultResponse and the result route; the provided CaptureAPI.swift excerpt stops at EventsResponse, so the result struct is assumed to live in a sibling Swift file decoding the same keys.
• Terminal statuses other than failed/expired were simulated by writing the in-memory capture record directly, which the task's "use memory state" scope allows; this is stated in the test docstrings.
• Working method: cloned fresh on an existing repo sandbox (option (a)); the GitHub token there covered the repository, so no new sandbox was requested.

[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: 1360d0c0-d401-4ce5-b10b-75aec600b2c1

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]

Independent review (Obvious, Autobuild + Max)

Verdict: changes needed — one small, real gate failure; every substantive claim in the body verified clean. This PR is test-only (one file, +499, no production code), so regression risk is nil by construction. The review focused on whether the tests exercise the real app, whether they would actually catch a contract break, and whether the Swift-side claims are true.

Findings

1. [medium] The PR fails the repo's own format gate — ruff format --check rejects this test file.
backend/CONTRIBUTING.md:31 says CI runs ruff check, ruff format --check and pytest; backend/Makefile:11 puts ruff format --check in lint. On head a4f8c5a: uv run --frozen ruff format --check . → "1 file would be reformatted, 459 files already formatted" — the one file is server/api/tests/test_capture_contract_swift.py (ruff format --diff shows several sites, e.g. the _create helper around L29-35 wants the wrapped client.post(...) joined). ruff check alone passes, which is all the body claims — the verification list silently omits the format gate. Smallest fix: uv run --frozen ruff format server/api/tests/test_capture_contract_swift.py, commit, no assertion changes.

Context, not this PR's defect: nothing automated runs these tests today. .github/workflows/server.yml triggers on root server/** (L7/L11), which is the legacy pre-monorepo server — backend/ has no CI workflow, so the PR shows only the label check. That gap is repo-wide and predates this PR, but it means local make lint is currently the only thing enforcing the documented gate, and it fails as pushed.

2. [low] eventsUrl is asserted as a key the Swift client decodes, but the production uploader never reads it.
test_create_response_keys_match_swift_create_response (L98-108, asserts at L101/L108) and the body's "Create (201): … eventsUrl" present it as part of the read contract. In production sources eventsUrl is decoded only by the test double ios/HouseScanKit/Tests/HouseScanKitTests/LoopbackCaptureAPI.swift:265; CaptureAPI.CreateResponse decodes captureId/status/upload/finalizeBy only, and the uploader builds the events URL itself from the capture id (CaptureUploader.swift:606). The server does return the field (routes/captures.py:50), so the assertion pins real behavior — but the file docstring's "every assertion names a field the Swift client decodes" overstates for this one key. Smallest fix: drop eventsUrl from the asserted set or amend the L100 comment to say it feeds only the loopback test double.

Everything else checked out (below). Process note: server/api is the server team's area per AGENTS.md — test-only, but Hunter's eyes on the asserted codes/pointers would be the cheap confirmation.

Verified (fresh clone, PR head a4f8c5a; base tip 57f57ff confirmed contained; PR mergeable CLEAN)

• uv run --frozen --extra api pytest server/api/tests/test_capture_contract_swift.py -q → 28 passed (matches the body).
• uv run --frozen --extra api pytest server/api/tests -q → 151 passed (matches the body's 28 + 123).
• uv run --frozen ruff check . → All checks passed; uv run --frozen lint-imports → 5 kept, 0 broken.
• Contract side read against the actual Swift sources: CaptureAPI.swift's Limits/CreateResponse/UploadTarget/RegisteredFile/CommitResponse/FinalizeResponse/EventsResponse/ErrorItem, errorCodes, terminalStatuses (complete/manual_review/failed/expired/needs_views) and statusesWithoutOutcome (failed/expired) all match the asserted keys, codes and statuses — including partBytes being an ignored extra (Swift's Limits fields are all optional). The result-polling asserts are load-bearing: CaptureResult.Response decodes status, viewsNeeded and memberActions as required keys, so set(body) >= {...} pins exactly what a finished upload needs to decode.
• Sensitivity (do the tests bite?): two mutations applied, run, then restored via git. (a) Renaming the create response's captureId key at the model (server/contracts/api.py:76 + the _render kwarg) → 23/28 fail. (b) Renaming the upload route's "code": "bad_digest" (routes/uploads.py:61) → the signed-PUT test fails. Suite green again after restore. All tests go through create_app() over HTTP — real bearer middleware, error shaping and body caps on the path — so the body's route-only-vs-app rationale holds.
• The body's two disclosed expectation corrections reproduce on head: ../escape.jpg is a schema error before the route's path_invalid rule, and the lifecycle test tolerates stage events after files_committed.
• Prior art check: body's read of PRs [obvious-fleet] Backend inventory after the port into backend/: checks run, one post-move path bug fixed #248-fix(backend): retryable finalize handoff and lease-guarded file-index merge #252 is consistent with this branch's state; nothing here asserts on behavior those PRs fix.

Not verified

• No Swift/Xcode in this environment: the client decode side was read, not executed (the body discloses this).
• No end-to-end orchestrator run: real stage/verdict_ready event payloads and a fully materialized result body (outcome, previewUrl, reviewUrl) remain unverified — matches the body's own disclosure.
• The uploader's 404 result_not_ready poll path (CaptureUploader.swift:653) is untested here; the body discloses timing/Retry-After gaps but not this one.
• Deployment/Modal paths untouched, as the body states.

[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

[Low impact] Test file for the iOS uploader API contract.

The PR appears safe to merge, with a non-blocking gap in the event contract checks.

Findings

1.  Unreadable events pass tests ▶

Fix with agent prompt

### Issue 1
backend/server/api/tests/test_capture_contract_swift.py:429-430
The event checks only verify that `at` and `data` exist. An event with `"at": null` or `"data": []` would pass, but Swift cannot decode it: `Event.at` requires a string and `Event.data` requires an object or null. Check those value types, including the string fields that `EventData` reads. Apply the same checks in the lifecycle test so both tests catch responses the uploader would reject.

---

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

Summary

Adds 28 HTTP tests for the capture API responses and errors used by the iOS uploader. No production code changes.

• Backend tests pin the capture uploader's HTTP contract.

_{Reviews (1) · Last reviewed commit: "test(backend): pass ruff format and corr..." · Reviewed by Greptile}

[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-16T04:32:33.441087Z	870725d	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.

[greptile-apps]

Unreadable events pass tests

The event checks only verify that at and data exist. An event with "at": null or "data": [] would pass, but Swift cannot decode it: Event.at requires a string and Event.data requires an object or null. Check those value types, including the string fields that EventData reads. Apply the same checks in the lifecycle test so both tests catch responses the uploader would reject.

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/server/api/tests/test_capture_contract_swift.py
Line: 429-430

Comment:
**Unreadable events pass tests**

The event checks only verify that `at` and `data` exist. An event with `"at": null` or `"data": []` would pass, but Swift cannot decode it: `Event.at` requires a string and `Event.data` requires an object or null. Check those value types, including the string fields that `EventData` reads. Apply the same checks in the lifecycle test so both tests catch responses the uploader would reject.

---

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