[obvious-autobuild]

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

What changed and why

backend/server/api/tests/test_result_boundary.py is new and nothing else changes. It tests the result boundary the capture client consumes (ios/HouseScanKit/Sources/HouseScanKit/Upload/CaptureAPI.swift + CaptureResult.swift) through the FastAPI app: the route in backend/server/api/routes/result.py reading synthetic stored artifacts whose content derives from committed fixtures. It does not change the iOS contract, does not add a placement endpoint, and does not touch the route or contracts — the open PRs #249 (reviewer page template, store-sync script) and #251 (orchestrator run records, coordinator) touch disjoint files.

Schema-only validation vs. the route and stored-artifact boundary. Validating ResultResponse against a payload in a unit test proves the contract type is self-consistent, but the client consumes neither the contract class nor the stored bytes alone — it consumes what the route returns after it re-reads state, merges stage_artifacts, overwrites status/runId, signs URLs, masks the verdict, strips hidden references, and swallows validation failure. Testing through the app with seeded storage proves the composition of all those steps plus the storage failure paths (malformed bytes, missing blob), which a schema test cannot reach. The trade-off is coupling to route internals (artifact key names, the not-ready envelope's null padding), which is exactly the behavior under test; the suite is one file with shared seed helpers, so that coupling is contained. Both are kept: schema validation still runs inside the route (logged as result.schema_drift), and one test pins that a drifted payload is served as stored rather than rewritten or rejected.

Stored artifacts derive from committed fixtures: the verdict blobs are server/result/tests/data/{synthetic,reject}/verdicts/verdict.C.json copied verbatim, the preview is server/result/tests/data/synthetic/points.ply, and the drift case is the committed schema-negative server/contracts/tests/negatives/result-eligible-without-placement.json. The device result.json maps the committed verdict's site decision onto the wire outcome (pass→eligible with a placement, reject→masked as manual_review), matching what server.result.compose produces. Storage is the test settings' volume backend; state is memory.

How I verified it

Worked in a fresh clone at ~/work/burst5-20261015-bs-result-contract on the BaseScanning repo sandbox (branch obv/burst5-20261015-bs-result-contract off feat/port-house-scanning-server), commit 60d0cf14.

• uv run --frozen --extra api pytest server/api/tests/test_result_boundary.py → 13 passed
• uv run --frozen --extra api pytest server/api/tests → 136 passed (no regressions; the pre-existing result tests in test_seams.py, test_more.py, test_auth_none.py still pass)
• uv run --frozen --extra api pytest ios-kit/tests → 18 passed
• uv run --frozen ruff check . → All checks passed
• uv run --frozen ruff format --check . → 460 files already formatted
• uv run --frozen lint-imports → Contracts: 5 kept, 0 broken

Result states proven:

1. Not-ready polling — before any run the body is exactly the 13-field envelope with runId: null, status: "uploading", empty viewsNeeded/memberActions, null outcome; with a pending run the same body carries the runId and the capture status, stable across repeated polls.
2. Ready result retrieval — the served key set is the stored keys plus verdictUrl/previewUrl/reviewUrl; the route overwrites the stored runId/status markers with the live record's; the eligible outcome and its recommendedPlacement round-trip byte-for-byte.
3. Reviewer-only fields — a reviewer gets a signed verdictUrl (/v1/uploads/…); the owner and every device principal get verdictUrl: null; another device's token gets 403 forbidden.
4. Hidden artifact stripping — any string containing verdicts/ or result.internal.json becomes null at any nesting depth (site reasons, criteria fields, meter evidence); the committed verdict's contents never appear in a device body, and an artifact named result.internal.json is masked even from a reviewer.
5. Preview URL presence — with a result/preview.ply artifact both device and reviewer get a signed URL; without one the key is absent (not null).
6. Malformed stored result — unparseable bytes or a missing blob both return 503 with errors[0].code == "storage_unavailable", pointer: "/" and Retry-After: 5.
7. Failed and expired terminal captures — ready route with null outcome returns status: "failed"/"expired" with outcome: null, empty viewsNeeded and a reviewUrl, the shape CaptureAPI.statusesWithoutOutcome treats as final.
8. Drifted payload — the committed eligible-without-placement negative is served 200 as stored, proving the route logs schema drift instead of rewriting or rejecting.

What is not verified

• The HouseScanKit Swift package tests (swift test): this sandbox has no Swift toolchain (which swift → not found). No iOS code changed, and the envelope assertions above were checked against the decoder's contract in CaptureResult.swift (optional fields, unknown-status tolerance, statusesWithoutOutcome) by reading it, not by executing it. The ios Make target and Xcode builds did not run for the same reason.
• The result route's schema-drift log emission (result.schema_drift) is exercised by the drift test but the log line itself is not asserted; the served body is.

Assumptions

• "Committed result fixtures" means the committed test data under backend/server/result/tests/data/ and backend/server/contracts/tests/negatives/; the synthetic device result.json in each test is built in-code from those fixtures rather than committed as a new binary blob, since the API tests' storage is per-test temporary volume state.
• The reject→manual_review device mapping in _outcome_for mirrors server.result.compose's documented masking; the test asserts the route preserves whatever the composer stored, and the mask itself is covered by the result package's own tests.
• ENVELOPE_KEYS reflects the 13 documented ResultResponse fields; the tests assert the not-ready body carries all 13 (null-padded) and the ready body carries only stored-plus-composed keys, which is the observed and client-compatible behavior (CaptureResult.Response decodes absent optionals as nil).
• States still blocked by missing production data: a real WS-10-composed result.json from an actual capture run (coverage summaries, scale, meter blocks with real reads, ARKit-world placements) — no production capture data exists in this public repository, so the tests exercise synthetic artifacts shaped from committed verdict fixtures instead; likewise real signed-URL reachability (GCS/R2 backends) is unexercised because the test settings use volume storage, and the amendment/re-run overlay of result.result.json across multiple runs for one capture awaits a production-shaped multi-run case.

[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: 2d7c504f-59ea-451f-9f9b-e738c6baf095

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: ready — good to move out of draft. The PR is exactly what it claims: one new test file, nothing else, driving the real route through the real app with seeded storage. Every number in the body reproduced here, and three mutation checks show the tests bite. Two low, non-blocking findings and two notes below.

Findings (ranked by severity)

1. Low — the verdict-leak canary can never fire.
test_result_boundary.py:238 and :299 assert "C1-rules-3d" not in body.text to prove the verdict blob never leaks into the served body. But the seeded verdict blob is the committed fixture copied verbatim, and that fixture's evaluator.path is "server/result/tests" (server/result/tests/data/synthetic/verdicts/verdict.C.json:10) — the token C1-rules-3d occurs nowhere in any byte the test stores or serves, so the tripwire passes even if the route served the verdict blob verbatim. In production the criteria engine stamps verdicts with evaluator.path = "C1-rules-3d" (server/criteria/compose.py:237), which is clearly the leak this canary is meant to catch. Smallest fix: assert on a token actually present in the seeded blob (e.g. "synthetic-flatwall" not in body.text), or keep the canary and write it into the seeded blob. The neighboring assertions ("verdict" not in payload at :237, verdictUrl is None) do cover the leak path, so this is a dead tripwire rather than a hole.

2. Low — test_reviewer_verdict_url_closed_for_internal_result claims a branch it doesn't exercise.
The route selects the verdict artifact only by the fixed name verdicts/verdict.<profile>.json (server/api/routes/result.py:85) and only then applies the result.internal.json guard (:86). The test seeds artifacts as {"result/result.json": …, "result/result.internal.json": …} (test_result_boundary.py:293), so verdict_key is None and the masked-verdict branch never runs — the internal blob is excluded by lookup miss, not by masking. The security property still holds (the internal blob never reaches the body), but the guard branch is untested. Smallest fix: register the internal blob under the artifact name verdicts/verdict.C.json while storing it at a key containing result.internal.json (e.g. captures/<id>/result/result.internal.json), which routes it through the guard.

Minor: ENVELOPE_KEYS (test_result_boundary.py:30) is defined and never used — either assert set(NOT_READY_ENVELOPE) == ENVELOPE_KEYS in the not-ready tests or drop it.

What I verified

All runs at the PR head 60d0cf14 in a clean detached worktree, uv 0.13.1, Python 3.12.15:

• uv run --frozen --extra api pytest server/api/tests/test_result_boundary.py → 13 passed (matches the body)
• uv run --frozen --extra api pytest server/api/tests → 136 passed (matches; includes the pre-existing result tests in test_seams.py, test_more.py, test_auth_none.py)
• uv run --frozen --extra api pytest ios-kit/tests → 18 passed (matches)
• uv run --frozen ruff check . → all passed; ruff format --check . → 460 files already formatted; uv run --frozen lint-imports → Contracts: 5 kept, 0 broken (all match)
• Mutation checks (route mutated in the throwaway worktree, restored after each; worktree clean at the end):
  • removed the _strip_hidden call → test_hidden_artifact_references_stripped_recursively fails (1 failed / 12 passed)
  • removed the runId/status overwrite → the terminal-outcome assertion fails ('processing' != 'failed')
  • dropped principal.reviewer from the verdict-URL gate → 4 fail, including test_verdict_url_is_reviewer_only
• Cross-checked seeds against the real code: ResultResponse has exactly the 13 fields of NOT_READY_ENVELOPE (contracts/result.py:151); RunRecord/StageRecord constructor args match; set_status/put_run/upsert_stage exist on the state protocol (common/state.py); the reject→manual_review empty-reasons mapping in _outcome_for matches result/compose.py's hidden-reject branch (compose.py:146–152); the reject fixture does contain drive_clearance, so that canary is live.
• Body accuracy: gh pr diff --name-only shows exactly the one file; fix(backend): reviewer-page form and point cloud, store-sync paths #249 and fix(backend): address Greptile review findings on the orchestrator #251 are open and touch disjoint files — fix(backend): reviewer-page form and point cloud, store-sync paths #249's routes/result.py change is additive (a reviewer-only preview.ply endpoint) and doesn't alter the envelope these tests pin.

One repo-level observation (not this PR's defect): .github/workflows/server.yml triggers on server/** only, so since 57f57ff moved the server under backend/, backend PRs get no server CI — this PR's checks show only the size label. The local runs above are the only test evidence; adding backend/** to that workflow's paths would close the gap.

What I couldn't verify

• The HouseScanKit Swift package tests (no Swift toolchain in this sandbox) — the body declares the same; the envelope assertions were cross-checked against the route and CaptureResult.swift was not executed.
• Signed-URL reachability against GCS/R2 backends — tests run on the volume backend, as the body states.
• The result.schema_drift log line is exercised but not asserted, 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.

[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:31:44.611621Z	60d0cf1	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]

Confidence Score: 4/5

[Low impact] Adds test coverage for the device result API boundary.

The PR appears safe to merge, with two non-blocking improvements to the new tests.

Findings

1.  Internal-file check goes untested ▶
2.  Placement values go unchecked ▶

Fix with agent prompt

### Issue 1
backend/server/api/tests/test_result_boundary.py:293
This test never reaches the check it names. The route reads `verdicts/verdict.C.json`, but the artifact map only supplies `result/result.internal.json`, so `verdict_key` is absent. The test would pass even if the internal-file check were removed. Map `verdicts/verdict.C.json` to `internal_key` so the assertion tests that check.

```suggestion
        {"result/result.json": result_key, "verdicts/verdict.C.json": internal_key},
```

### Issue 2
backend/server/api/tests/test_result_boundary.py:233-234
The ready-result test checks only `wallId` inside `recommendedPlacement`. Dropping or changing `startSM`, `confidence`, or the stored box dimensions would still pass, even though the test seeds those values to check the stored-result boundary. Compare the full returned `outcome` with `_outcome_for("pass")` to protect the values the route should preserve.

```suggestion
    assert payload["outcome"] == _outcome_for("pass")
```

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

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

Summary

Adds 13 FastAPI tests for the stored-result boundary. They cover polling, ready responses, reviewer access, hidden references, previews, storage errors, schema drift, and terminal captures.

• API tests check result polling and response shapes through FastAPI.
• API tests check result contents against each caller's access.
• API tests cover missing, malformed, and terminal result cases.

_{Reviews (1) · Last reviewed commit: "test(backend): cover the device result b..." · Reviewed by Greptile}

[greptile-apps]

Internal-file check goes untested

This test never reaches the check it names. The route reads verdicts/verdict.C.json, but the artifact map only supplies result/result.internal.json, so verdict_key is absent. The test would pass even if the internal-file check were removed. Map verdicts/verdict.C.json to internal_key so the assertion tests that check.

Suggested change

	{"result/result.json": result_key, "result/result.internal.json": internal_key},
	{"result/result.json": result_key, "verdicts/verdict.C.json": internal_key},

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/server/api/tests/test_result_boundary.py
Line: 293

Comment:
**Internal-file check goes untested**

This test never reaches the check it names. The route reads `verdicts/verdict.C.json`, but the artifact map only supplies `result/result.internal.json`, so `verdict_key` is absent. The test would pass even if the internal-file check were removed. Map `verdicts/verdict.C.json` to `internal_key` so the assertion tests that check.

```suggestion
        {"result/result.json": result_key, "verdicts/verdict.C.json": internal_key},
```

---

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

[greptile-apps]

Placement values go unchecked

The ready-result test checks only wallId inside recommendedPlacement. Dropping or changing startSM, confidence, or the stored box dimensions would still pass, even though the test seeds those values to check the stored-result boundary. Compare the full returned outcome with _outcome_for("pass") to protect the values the route should preserve.

Suggested change

	assert payload["outcome"]["kind"] == "eligible"
	assert payload["outcome"]["recommendedPlacement"]["wallId"] == "w1"
	assert payload["outcome"] == _outcome_for("pass")

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/server/api/tests/test_result_boundary.py
Line: 233-234

Comment:
**Placement values go unchecked**

The ready-result test checks only `wallId` inside `recommendedPlacement`. Dropping or changing `startSM`, `confidence`, or the stored box dimensions would still pass, even though the test seeds those values to check the stored-result boundary. Compare the full returned `outcome` with `_outcome_for("pass")` to protect the values the route should preserve.

```suggestion
    assert payload["outcome"] == _outcome_for("pass")
```

---

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

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!