Repository navigation
Conversation
An answer that doesn't decode or doesn't name the scene sent (ResultBinding) used to show "We couldn't check this scan" with "Back to review", as if the marks were wrong, and offered no way to send the same scan again. It now has its own upload state: the answer is never shown, the scan is kept, and only the homeowner's "Try again" asks again. From the second such answer the screen says asking again might not help and points to sharing the scan or starting over. A 4xx refusal still goes back to the review. A reply that isn't HTTP counts as an unreachable server, so the words never say a server answered when none did. The transport moves into HouseScanKit (PlacementHTTPClient) unchanged, so the package tests drive it through URLSession against a server on 127.0.0.1. A UI test runs the real engine against the same kind of server: the first answer names another scene, and the test checks that nothing more is sent and nothing reaches a later screen until Try again. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (17)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe upload flow now separates unusable server answers from refusals, allows users to retry unusable answers, and tracks consecutive attempts. Placement submission and failure classification are handled by ChangesUpload Recovery
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ScanEngine
participant HTTPResultClient
participant PlacementHTTPClient
participant PlacementEndpoint
participant ResultBinding
participant UploadingScreen
ScanEngine->>HTTPResultClient: Submit scene
HTTPResultClient->>PlacementHTTPClient: Submit scene
PlacementHTTPClient->>PlacementEndpoint: POST /v1/placements
PlacementEndpoint-->>PlacementHTTPClient: Return HTTP response
PlacementHTTPClient->>ResultBinding: Check answer against scene
PlacementHTTPClient-->>HTTPResultClient: Return answer or error
HTTPResultClient-->>ScanEngine: Return result or upload failure
ScanEngine->>UploadingScreen: Show unusable-answer state
UploadingScreen->>ScanEngine: retryUpload after Try again
ScanEngine->>HTTPResultClient: Resubmit scene
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The reviewed upload and retry changes are mergeable after normal checks; no issue requiring a fix before merge was established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Retries continue to validate that answers belong to the submitted scan, and unusable answers are withheld from results. The main uncertainty is whether the new file-backed testing mode can be activated in distributed installations. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
Both loopback servers retained themselves through their listener's connection handler, so a dropped server never closed its listener. The handlers now hold the server weakly, and the UI test's stub also closes on stop(). The engine test now sends two unusable answers before a usable one: the first names another scene, and the second names the scene sent but has a result schema House Scan doesn't read. That covers the engine's decode failure and its count. The test checks the second state's wording and that no request goes out between taps. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
SamGu-NRX
marked this pull request as ready for review
October 3, 2026 07:42
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. |
 |
On the hosted Simulator (Actions run 37107233020) the app's upload to the UI test's server on 127.0.0.1 failed as offline, so the test never saw an unusable answer. That server lived in the test runner, a background app while the app under test is open; the same listener code answers URLSession on a Mac at 200 B to 3 MB. Under -answersFromGate the app's real PlacementHTTPClient sends through a URLProtocol that writes each request into the -autopilotGate folder and answers HTTP 200 with the test's answer-n.json. The engine, the status check and ResultBinding run as against a server; only the socket is replaced. HouseScanKit's PlacementHTTPClientTests still cover the real loopback socket. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A fresh review found two fault paths in the test hook. The app numbered a request before reading its body, and the test's server waited for numbers in order, so one request the app failed to write left every later one unanswered. The app now numbers a request only once its body is read, and the server finds requests by number in the folder. stop() also returned while its thread could still be writing into the gate folder the test was about to delete. It now waits for the thread and reports any answer it couldn't write from the test's own thread. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The second unusable-answer state used the dark secondary style, which the app keeps for buttons over the camera. On this light screen it drew as a heavy dark pill, narrower than its neighbours and stronger than Share scan, the way out the note points to (CI screenshot uploading-unusableAnswer-2, run 37112184653). It now uses the quiet style the light screens use for a second choice. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
SamGu-NRX
added a commit
that referenced
this pull request
Oct 3, 2026
… packaged #203 wrote its notes before #205's Saved scans existed, so a packaged scan's note still said House Scan can't reopen it after the app closes, with no mention that Saved scans on the first screen can share its file then. Now the first note says so and that it can't try again from there, and the repeated note adds that Saved scans can share it later. Without a packaged scan the notes are unchanged, as is the failed-upload note #205 already aligned. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
SamGu-NRX
added a commit
that referenced
this pull request
Oct 3, 2026
…branch: let the homeowner answer "I can't check this area" on the spot check ResultClient.swift conflicted where #203 replaced UploadError.missingSample with MissingSampleResult and #206 added the answerFile early return; both are kept: the answer file first, then the bundled sample with #203's error. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Base:
codex/public-assembly-20260930. This PR needs nothing from other open PRs, and #192 stays unwired.What changed
An answer House Scan can't use now offers "Try again" instead of sending the homeowner back to their marks. Some answers can't be used: they don't decode as a result, or their
stats.input_sha256doesn't match the scene sent (ResultBinding, #198). The upload screen used to show "We couldn't check this scan" with "Back to review", which offered no way to send the same scan again and implied the measurements were wrong. Those answers now get their own upload state,UploadState.unusableAnswer(attempts:). The screen says the server answered but House Scan couldn't read the answer or match it to the scan. Nothing from that answer reaches the spot check or the result. The scan stays open, and only the homeowner's tap on "Try again" sends it again.A second unusable answer gives a way out. When "Try again" also gets an unusable answer, the title becomes "The answer still couldn't be used". The screen says asking again might not fix it, "Try again" loses its blue fill and takes the same quiet style as Share scan and Start over, and the note points to those two (
UnusableAnswerActions). The engine counts these answers from the last send out of the review or a gap. A usable answer resets the count.A refusal stays separate. A 4xx still means the server refused this scan, and it still goes to "Back to review" with no "Try again".
retryUpload()acts only on.failedand.unusableAnswer.The words never claim a server answered when none did. A response that isn't HTTP is now classified as an unreachable server, not an unreadable answer (
UploadFailureKind.classify). In sample mode no server is asked, so a missing or unreadable bundled sample is reported as a build problem.The placement HTTP client moved into HouseScanKit so tests can drive it over real HTTP.
PlacementHTTPClientsends the same request as the app's oldHTTPResultClient: POST/v1/placements, JSON content type and accept, a 120 s timeout. Progress reporting, the 300-byte error body and Retry-After are also unchanged.HTTPResultClientnow wraps it.Why
The product decision for this case: a bad server answer should keep the scan and let the homeowner try again, without suggesting the measurements were wrong. The app must never show an answer that isn't bound to the scan, and it must never retry in a loop on its own. A genuine refusal stays distinguishable.
UI changes
New state on the upload screen, so there is no before capture. Captures from hosted run 37114791601 on a7458e7, iPhone 17 Simulator. The demo pair comes from
testRepeatedUnusableAnswersPointToAWayOut. The engine pair comes fromtestUnusableAnswersAreHeldBackUntilTryAgain, on the synthetic replay with the test answering as the server.Verification
PlacementHTTPClientTestsrun the client throughURLSessionagainst an HTTP server on 127.0.0.1. The fixture is the synthetic-wall server answer with its input hash set per test. An answer for the scene sent comes back byte for byte. An answer naming another scene, one with no hash, and one that isn't JSON are each refused after exactly one request. 422, 503 and 429 with Retry-After classify as refused, server error and busy(30). Nothing listening classifies as unreachable.UploadRecoveryUITests.testUnusableAnswersAreHeldBackUntilTryAgainruns the real engine on the synthetic replay, with the test playing the server through files. Under the test-only launch flag-answersFromGate, the app's ownPlacementHTTPClientsends through a URLProtocol (GateAnswerProtocol) that writes each request into the autopilot's gate folder and answers HTTP 200 with the file the test writes back. The status check andResultBindingrun as they would against a server; only the socket is replaced. The first answer names another scene. The second names the scene sent but has an unsupportedschema_version, so it fails decoding rather than binding. The third is usable. After each unusable answer the test checks the matching title and that no "Back to review" appears. It then waits six seconds and checks that no new request went out and no spot check or result appeared. After the second "Try again" it expects the result.URLSessionon a Mac at 200 B, 300 KB and 3 MB, so the failure is specific to the runner on the Simulator, which is a background app while the app under test is open. The exact URL error wasn't logged by CI. 4bc4ded replaces that server with the file exchange above.GateServer.stop()returned while its thread could still write into the folder being deleted. Both are fixed in 11d02a3.PlacementHTTPClientTestsamong them, and 59 UI tests with 1 skipped and none failing. The engine test passed in 80 s.PlacementHTTPClientTests, which run the same client on macOS against a server on 127.0.0.1.Risk
-answersFromGateis a launch argument in the shipping app, like-serverURLand-sampleResult. It requires-autopilotGateand-serverURLand stops the app at launch without them, and nothing reads it otherwise. A homeowner can't pass launch arguments..failedrecovery note. This PR leaves those lines alone, and the new state's note will be aligned with that wording once the branch lands.Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit