Repository navigation
ios: confirm before a repeated Can't get there ends the scan; no spot for a wall never walked - #135
Conversation
… spot for a wall never walked A second "Can't get there" on a walk card within 5 s of the one that last ended a side now asks "End the scan here?" (Yes, end here / Keep walking) instead of silently ending the other side (#82, option C). Yes ends every open side where "Can't get there" would and finishes the walk; Keep walking dismisses it. The engine now records which ends "Can't get there" set before the homeowner had walked a battery's width on that side (WalkRefusals). When both ends are still those, the result says "We couldn't measure your wall" with no spot, no route and no "See it on your wall" (#76, option (a); app only, no schema change). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A side now counts as refused only when both the walk and the end that "Can't get there" set stayed within a battery's width of the meter. Once ends land at the phone (#71), run 2's right end sits at the wall's physical end even with few views kept on the way, and its result must keep the spot. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@AidenJohnston Review at 057cfd4 found an acceptance gap in the no-walk exit and one smaller follow-up.
Tests currently inject frozen DemoEngine states, whose answerEndScan manufactures completed ends; they do not test this live transition. Exact-head hosted iOS36305435180 skipped, CodeRabbit skipped the draft, and no Codex verdict is present. The reported Mac mini results belong to earlier6f20468 and include one UI failure; full accessibility and current-head runtime acceptance remain unverified. Source review only here; no extra CI launched. |
Keeps both sides in the walk screen and engine: the end-scan question sits beside #136's next-wall confirmation, and "Can't get there" doesn't ask over it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Conflict repair only. Export keeps beta's `self` capture beside the #76 check. The result's Details keeps beta's comment and hides Start over for a wall not measured, as before; "Scan again" takes beta's new Start over hint, since starting over no longer deletes the scan. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
AidenJohnston
left a comment
There was a problem hiding this comment.
Adversarial review of #135 at 88912e5 (the beta 6bc1df0 merge). The merge didn't change this PR's logic: WalkRefusals, endWalkCannotGoOn, cannotAccessArea and answerEndScan are the same as at 73d931e. Sources I checked against: run3-2055.md, run1-2000.md and run2-2015.md on t3/device-test-4.1-analysis, and issues #82 and #76.
Must fix
-
The #76 rule doesn't catch run 3, the case it was chosen for.
WalkRefusals.swift:43-47counts a side as walked whenmax(walkedFarthest, |end s|) >= 0.79 m.ScanEngine+Ends.swift:123getswalkedFarthestfromCoverageMap.walkedPositions, which holds every kept view in the scan, including the ones kept during the tilt-down task at the meter. The public analysis has run 3's numbers (A41-2055-06):- The farthest kept views are at −6.7 ft (2.04 m), from the yard detour on the tilt card (p00039), and +6.0 ft (1.83 m).
- The phone stood at about +5 ft (1.5 m), about 3 m out, when "Walk slowly to your right" was refused. That is inside
phoneOutLimit(4 m), so under #136 the right end lands at the phone, +1.5 m. - Build 4.1's recorded ends were −4.005 m and +0.525 m.
Every one of these is at or above 0.79 m, so both sides count as walked.
neitherSideWalkedreturns false, and replaying run 3 on this branch still gives "4 ft 7 in left of your meter" and "See it on your wall", with the spot inside the house.The PR description says run 3 is "an end at the phone standing by the meter" and so is still refused. The analysis says otherwise.
WalkRefusalsTests.swift:34("Run 3's shape") uses made-up values (ends −0.3 / +0.6, walked 0.2 / 0), not run 3's.Failure scenario: a user repeats run 3 (a bad estimated-plane tap, wandering during the first tilt task, then "Can't get there" on both walk cards). They still get a spot inside the house, plus the AR view.
To unblock, do one of these:
- (a) Base "walked" on a signal that separates run 3 from run 1. Run 3's walk cards were each answered 0.2–0.5 s after they appeared. Run 1 walked right while a tilt card was up (A41-1, 2:21–2:46), so "moved along the side during its walk task" on its own would hide run 1's spot. One candidate: a side ended by "Yes, end here", or by "Can't get there" on a walk card that had been up for less than about 2 s, counts as refused unless the phone moved at least 0.79 m along that side while that walk card was up. Add a
WalkRefusalsTestscase with run 3's real numbers, and keeptestCantGetThereEndsWhereThePhoneIs(run 1) green. - (b) If there isn't time before the demo, correct the PR description and the test name, and state on #76 that this rule does not cover run 3. The team chose option (a) for run 3 and needs to know it isn't covered.
Should fix
-
An unmeasured wall still runs the server's follow-up views before saying so. At
ScanEngine.swift:1814-1818,automaticGapQueue(result)works from the rawplacement, not the presentation thatwithWallNotMeasuredcleared. Failure scenario (run 3 had this): the homeowner who just quit sees "4 more views to finish" and "Show the ground out to about 14 ft", and only after those gets "We couldn't measure your wall". Skip the automatic gaps whenwalkRefusals.neitherSideWalked(...)is true. -
"Yes, end here" with nothing walked is a loop with no way out.
ScanEngine+Actions.swift:495-503ends the open sides and then callsfinishWalk(), which at:389-399clears both ends as too close and puts the homeowner back on the walk.WallWalkScreenhas no "Start over". Failure scenario (run 3's user, with a good anchor):- They tap "Can't get there" twice, get the question, and tap "Yes".
- The card says the ends were too close.
- They tap "Can't get there" twice again (
keepWalking()has reset the window, so the first tap just ends a side), get the question again, and loop.
Before ending anything, compute whether the ends would be too close. If they would, have the question offer "Start over" instead of "Yes, end here".
-
Correct the PR description's run facts. Run 1 did not end "both sides that way 16 ft out". The analysis says the left end was −1.0 ft and the right end 20 ft 3 in, so under this rule run 1's left side counts as refused and only the right side keeps its spot. Both run 1's and run 3's facts in the description are the record the team decided from.
Notes
- Demo safety (question 1): today's rule fails open, so a normal demo keeps its spot. Only ends set by "Can't get there" are recorded. "Wall ends here", a marked end and the automatic end prompt never are. The spot is hidden only when both ends came from "Can't get there", each side's farthest kept view and end were under 0.79 m (2 ft 7 in), and the ends were still at least 0.79 m apart. That is a narrow window: each side 0.4–0.79 m long. For a practice-meter demo indoors, walk at least 3 ft on one side, or end sides with "Wall ends here".
- 0.79 m: as a battery's width it's a fair floor for "walked". Because it's compared with the maximum over all kept views (at the 2 m stand-off, aim-task views a metre to the side are normal), it really measures "the phone was ever 2 ft 7 in to one side", so it will rarely fire. A meter close to a corner on one side is fine: one refused side keeps the spot (run 1's left). The spot is hidden only when both sides are short, a wall of 0.79–1.58 m between obstacles, which can hardly fit a battery beside the meter anyway.
- Can #82 fire by accident? (question 3): No.
- It asks only on
.walkor.markEnd, within 5 s of a refusal that ended a side (:436). - Refusals on tilt, aim and overhead tasks neither ask nor start the window.
- "Can't get there" is on the card at the top, and the answers are in the bottom controls. Run 3's 7 taps on one spot stop at tap 3, and the later taps land on nothing.
- "Keep walking" only clears the flag and
lastEnded. It sets no end, writes no log entry and leaves the walk task unresolved, as described. go()clears the question on every phase change.- Minor: the two answers have no input lock of their own. That's fine while they sit away from the reply.
- It asks only on
- #136 (question 4):
- The question is suppressed while "Is this the next wall?" is up.
- "Yes" puts each open side's end where
WalkedEnd.choosewould: the phone's place on its own side, or the farthest kept view across the meter. - A corner turn clears the end, so the refused record stops matching.
wasRefusedcompares Floats exactly (WalkRefusals.swift:57). That holds today:setEndstores s as it is,refineGround'supdateWallshifts it by exactly 0, and anchor corrections keep s. If a future change moves ends, the rule fails open.
- Beta's spot check:
presentAnswer(ScanEngine+Confirm.swift:62) readsstate.result.spot, which the presentation sets to nil. So an unmeasured wall skips the spot check and never shows the spot photo. Good.ResultScene3Ddraws no battery either, since bothspotandnearestSpotare nil. - Export:
scene.json, the scan stamp and "Share scan" still carry the server's spot. That's intended until option (b). - Public-repo rules: no private material, prompt text or internal numbers in the diff or the description. 0.79 m is the existing public
WallFrame.minWallLength. - Swift: there's no new concurrency.
systemUptimeis monotonic, and thesince >= 0guard is harmless.endWalkCannotGoOnreadswalkedFarthestbeforesetEnd, which is correct.
Verdict: blocked. Only the #76 claim blocks. The #82 half is ready, and the current #76 rule is demo-safe because it fails open. Must-fix 1 can be cleared by either (a) or (b).
🤖 Generated with Claude Code
…d; say run 3 isn't caught With too little walked, "Yes, end here" put the homeowner back on the walk: finishWalk refused the ends as too close, and the next two taps asked again. When ending the open sides now would leave ends closer than a battery is wide (WalkRefusals.endsTooClose), the question offers "Start over" in place of "Yes, end here". The #76 rule doesn't catch field test run 3: its kept views reached about 2 m on both sides. The test that claimed run 3's shape now uses honest synthetic numbers, and a new test records run 3's numbers as a case the rule does not catch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@AidenJohnston At de5357f, the new Start over alternative addresses the stationary zero-length exit loop in source. It does not establish the earlier advertised no-walk-to-result path; please keep the PR body aligned with the disclosed run-3 limitation. Two bounded follow-ups remain: the stale refusal coordinate survives clear/re-mark, as in5854202504; and ScanEngine.swift1814-1827 suppresses the displayed result but still queues raw server missingEvidence through automaticGapQueue1851-1863. Thus a wall classified not measured can ask for more views before showing that outcome. Bypass those automatic requests for the same no-measurement classification, with a regression that follows the live upload transition rather than only a frozen result screen. These are source findings, not a new claim that the ordinary walked-wall demo is blocked. No runtime acceptance claimed. |
|
Review response at
Mac mini at 🤖 Generated with Claude Code |
AidenJohnston
left a comment
There was a problem hiding this comment.
Follow-up review of #135 at de5357f. I read git diff 88912e5 de5357f (9 files, +82/−11), the updated description, the reply comment and #76. This was a source review only; I ran nothing myself. The Mac mini numbers below are as reported.
1. Must-fix (run 3 claim): resolved, via option (b)
- PR text: it now says plainly "It does not catch field test run 3" and gives run 3's real reach (−6.7 ft / +6.0 ft; ends −4.0 m / +0.5 m). Run 1 is corrected: its left end was at −1.0 ft and counts as refused, its right end counts as walked, and the spot stays. That also clears should-fix 4.
- Tests: the made-up "Run 3's shape" test is renamed
bothSidesEndedNearTheMeterMeansNeitherWasWalkedand marked synthetic. The newrun3IsNotCaughtBecauseItsViewsReachedPastTheMinimumfeeds in run 3's numbers (walked 2.04 / 1.83 m, ends −4.0 / +0.525) and asserts!neitherSideWalked, which is what the code actually does. Using the build 4.1 right end (+0.525) instead of #136's +1.5 m doesn't change the outcome, because the walked distance alone is past 0.79 m. - #76: the scope note is posted. It says the rule does not catch run 3, that run 3's spot came from #69 (guarded by #108), and that (b) is deferred.
2. The "Yes, end here" loop: fixed, and there's no trap
cannotAccessAreanow computesendScanTooShortwith the same inputs thatanswerEndScan→endWalkCannotGoOn→finishWalkwould use:map.leftEnd ?? walkedEnd(.left)and the same for the right.WalkRefusals.endsTooClosematchesCoverageMap.endsTooClose(right − left < minWallLength). A missing end counts as too close, which is correct here: with no end,finishWalk'sbothEndsMarkedguard would return silently.WalkedEnd.choosenever returns nil, so in practice that branch doesn't fire.- No trap: "Keep walking" is always shown next to it. If the homeowner keeps walking and then finishes with ends that are too close,
finishWalkstill clears the ends and shows the too-short card, as before. From there, two "Can't get there" taps within 5 s bring the question back with "Start over". So there's always a way out within two taps. - "Start over" resets cleanly: it calls the same
startOver()→resetAll()that the result, uploading and unsupported screens use. That covers coverage, anchor, recorder,endKinds,walkRefusals = WalkRefusals()andwallTooShort, thengo(.onboarding), which clearsendScanQuestionandendScanTooShort.answerEndScanandgo()also clear the new flag, andDemoEngine's reset clears both flags. This is the first timestartOveris called from a live capture phase.breakWalkedPathis a no-op there because coverage is already nil, andfindMetersets the live mode back to idle, so I found nothing that breaks. The device check in step 2 of "Needs a device" will confirm it.
3. Demo safety: unchanged, a normal walk still gets its spot
The new code only changes which button the question shows, and only when the ends would land less than 0.79 m apart. In that case "Yes, end here" was already refused by finishWalk, so no scan that used to finish now can't. A demo that walks a few feet each way and ends with "Wall ends here" or "Done with this wall" never reaches the question. If it uses "Can't get there" after walking, the ends are ≥ 0.79 m apart, and the question offers "Yes, end here" as before. The #76 rule itself hasn't changed since the last review and still fails open.
Notes (non-blocking)
-
The question's choice is computed once, when it's asked. If the phone moves while the question is up, the button can be wrong in either direction:
- Offered "Yes" but now too close:
finishWalkrefuses, clears the ends and shows the too-short card. That's the old one-time bounce, not a loop, becausekeepWalking()reset the window and the next ask recomputes. - Offered "Start over" when "Yes" would now work: "Keep walking" is right beside it.
Recomputing in
answerEndScan, or on each guidance update, would close this. It's not needed for the demo. - Offered "Yes" but now too close:
-
Stale doc comment:
WalkRefusals.swift(header, around line 11) still says "device run 1 ended both sides that way 16 ft out". That's the run 1 fact the description just corrected, so please change it to match. -
"Start over" is one tap and discards the scan in progress, with no confirm. That's acceptable, because it's offered only when the scan couldn't finish anyway, and it sits in the bottom controls away from the card's "Can't get there". The accessibility hint ("keeps your two most recent completed scans") is accurate.
-
Test coverage of the new path:
endsTooCloseToFinishcovers the helper, and the screen-state audit checks thataction.endScanStartOverandaction.keepWalkingexist. No test taps "Start over" from the walk, and none drives the livecannotAccessArea→endScanTooShorttransition, because the simulator can't. Step 2 of the description's device checklist covers this, so do run it on the phone. -
Still open from Sam's comments and the earlier should-fix 2:
- An unmeasured wall still gets the server's automatic follow-up views before it says "We couldn't measure your wall". This is listed under Risk.
- The stale refusal coordinate after clear/re-mark is still unaddressed. It needs an exact Float match to fire.
Neither affects a walked demo.
-
CI: the Mac mini run reports HouseScanKit at 525 tests passing and this branch's UI tests green. The only UI failures are the 4 inherited from
beta6bc1df0(practice meter, add-something, whole flow, aim ring), whicht3/41-beta-greenis fixing. Rerun once that lands.
Verdict: ready-with-notes
🤖 Generated with Claude Code
|
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 house-scanning-master to your team's repositories. |
Merges beta 5f3f4c0 (#135). Review of #147: - Removes the 1 s tracking debounce: a brief drop just before "Wall ends here" put run 2's end back at the meter (#71). The rule reads tracking at the tap, and WalkedEnd.maxPastEvidence bounds a jumped pose. Test: tapRightAfterABriefTrackingDropEndsAtThePhone. - Choice.pastEvidence flags the 2 m bound apart from the kept-photo cap, and logEnd says which ("2 m past evidence", "cap" or "phone"). - The "you saw" flag is set and cleared wherever the leaves-out value is, in the engine and DemoEngine. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Base is
beta, nott3/ios-mvf: the build 4.1 fixes are already integrated there, including #102's 0.6 s input lock on the card's reply and #109's gap loop and "Show my result". This branch builds on them for the next TestFlight build.Fixes #82. Part of #76: this is option (a), app only. It does not catch field test run 3 (see below). The server part, option (b), comes after the deadline.
What changed
A second "Can't get there" asks before ending the scan (#82, option C)
On the walk, "Can't get there" ends the wall on the side being walked. In field test run 3, a homeowner trying to stop tapped it seven times; each tap silently skipped a task or set an end. Now, when "Can't get there" comes on a walk card within
WalkRefusals.repeatWindow(5 s) of the one that last ended a side, the walk asks "End the scan here?" instead of ending this side too.WalkedEnd, unexplored), then finishes the walk as "Done with this wall" does.WalkRefusals.endsTooClose).finishWalkwould refuse those ends and put the homeowner back on the walk, and two more taps would ask again, a loop with no way out. The card then reads "You haven't walked enough of the wall to check it yet. Start over, or keep walking along the wall." The engine works this out when it asks.While the question is up, the card's reply, "Wall ends here" and "Mark something" are hidden, as they are for the end question. It doesn't ask while #136's "Is this the next wall?" is up. The first "Can't get there" behaves as before. The UI demo engine never asks on its own, so existing UI tests that tap the reply keep their behaviour.
A wall walked on neither side shows no spot (#76, option (a))
endWalkCannotGoOnnow records, per side, whether "Can't get there" ended that side before the homeowner gotWalkRefusals.walkedMinimum(a battery's width,WallFrame.minWallLength, 0.79 m) from the meter. A side counts as refused only when both the farthest kept view on that side and the end itself are closer to the meter than that. The farthest kept view counts every view the scan kept, including views kept during aim and tilt tasks. The record holds the end's s, so it stops counting once that end is cleared or moved (a past-end request that was met, a corner). When both ends are still refused ends, the result card says "We couldn't measure your wall" with a "Scan again" button. The engine drops the spot, the nearest spot, the cable route, the clearances and the checks and requests made at them.showARrefuses, so "See it on your wall" can't appear. The server andscene.jsonare unchanged, so another client still gets a spot.What the rule covers, and what it doesn't. It fires only when both sides were ended by "Can't get there" within 0.79 m (2 ft 7 in) of the meter, on views and on ends. It fails open: "Wall ends here", marked ends and the automatic end prompt are never recorded, so a normal walk keeps its spot.
WalkRefusalsTests.run3IsNotCaughtBecauseItsViewsReachedPastTheMinimumrecords this limit with run 3's numbers.testCantGetThereEndsWhereThePhoneIs(the synthetic replay, both sides walked 5 m) stays green.WalkRefusalsTests.anEndAtThePhoneOutAlongTheWallCountsAsWalkedcovers that shape.The issue's proposal says "record whether the walk was refused"; this reads "refused" as "ended before walking it", so a homeowner who walks to a fence on each side and taps "Can't get there" still gets a result.
Why
The team chose option C for #82 and option (a) for #76 on 2026-09-27. Option C adds no new control to the walk. Option (a) needs no contract change before the deadline.
UI changes
Three new states, all in the screen-state audit (
wallWalk-endScanQuestion,wallWalk-endScanTooShort,result-wallNotMeasured); their screenshots are in the Mac mini run's result bundle. No before capture: all three are new.Verification
Mac mini,
ci-local.sh t3/41-walk-exit walk-exitat the headde5357f(betaat6bc1df0merged): xcodegen drift clean,swift testfor HouseScanKit passed (525 tests in 76 suites), and the device build succeeded. UI tests: FAIL, with 4 failures inherited frombeta:testPracticeMeterFromReplay,testAddSomethingFromTheReviewMarksOnTheCamera,testWholeFlowThroughTheButtonsandtestAimRingShowsProgressAndItsLegend.beta6bc1df0fails those same four, andt3/41-beta-greenis fixing them. This branch's tests passed:testEndScanQuestionKeepsWalkingOrEnds,testWallNotMeasuredShowsNoSpotandtestCantGetThereEndsWhereThePhoneIs. Rerun oncet3/41-beta-greenis onbeta.Earlier runs:
73d931e(betaatc60e6ff, with fix(ios): end a side at the phone; confirm and back out of the next wall (#71, #70) #136, merged): PASS, 503 HouseScanKit tests, 26 UI tests with 1 skipped by design.057cfd4: attempt 4 PASS. Attempt 3 failed onlytestAddSomethingFromTheReviewMarksOnTheCamera, while the Mac's load average was about 178. That test passes onbeta, and this branch doesn't change the review.6f20468: attempt 2 PASS. Attempt 1 failed onlyAuditIssueKeyTests.testAGoneElementIsReadWithoutRecordingAFailure(a "Gravel" tap that didn't register under load).betapassed that test.New tests:
WalkRefusalsTests(HouseScanKit): the 5 s window and its edges, Keep walking, both sides ended near the meter (synthetic) versus one side, a side walked before the refusal, an end at the phone out along the wall (run 2's shape), run 3 not caught, an end that moved, andendsTooClose.ScreenStatesUITests: the three new states pass the accessibility audit and show their words and controls;testEndScanQuestionKeepsWalkingOrEndstaps both answers;testWallNotMeasuredShowsNoSpotchecks that "See it on your wall" and the placement lines are absent.-autopilotCantGetThererun answers the question with "Keep walking" if it comes up, then ends the side as before.Needs a device. The simulator can't drive the real engine's walk:
Risk
automaticGapQueueworks from the server's raw answer, not the cleared presentation. This is the review's should-fix 2, not handled here.t3/41-ends-corner(fix(ios): end a side at the phone; confirm and back out of the next wall (#71, #70) #136: Wall ends still land in the wrong place: at the meter on a short side, or 13 ft from where the phone went #71, Corner flow: no "the wall just ends" answer, the refusals loop, and a surface behind the end post was accepted as the next wall #70) edited the ends and the corner flow in parallel. It is now onbeta, and this branch hasbetaat6bc1df0merged (after fix(ios): end a side at the phone; confirm and back out of the next wall (#71, #70) #136, ios: review follow-ups for #132 (AR double draw, result chevron, aim legend contrast) #137, ios: aim ring fills as its stretch is captured, with a legend and an unambiguous arrow #117 and the practice meter). The conflicts were keep-both. Thebetamerge keptbeta's Start over hint ("keeps your two most recent completed scans"); "Scan again" and the question's "Start over" use it too, since starting over no longer deletes the scan.Files touched:
ios/HouseScanKit/Sources/HouseScanKit/Guidance/WalkRefusals.swift(new) andios/HouseScanKit/Tests/HouseScanKitTests/WalkRefusalsTests.swift(new)ios/HouseScan/Contract/ScanContract.swift,ResultPresentation+Reading.swiftios/HouseScan/Runtime/ScanEngine+Actions.swift,ScanEngine+Ends.swift,ScanEngine+Export.swift,ScanEngine.swift,Autopilot.swiftios/HouseScan/UI/Screens/WallWalkScreen.swift,ResultScreen.swift,ios/HouseScan/UI/Copy/ScanCopy.swift,ios/HouseScan/UI/Preview/DemoEngine.swiftios/HouseScanUITests/ScreenStatesUITests.swiftNo
project.ymlorproject.pbxprojchange.Checklist
🤖 Generated with Claude Code