server: the meter's error never excuses a working-space overlap (issue #44)

[AidenJohnston]

Fixes #44.

The bug. In device field test run 2 the server's best spot stood with its edge on the meter's centre line, 1 ft 3 in inside the meter's 30 in working space (measured_ft -1.25; #40 is the app reading that as a distance, fixed in #37). Two things let it through. The working-space check added the meter's error (0.792 ft for a tap on an estimated plane) to the wall's error (0.713 ft), so the overlap was within its ±1.505 ft and came back UNSURE instead of FAIL. And _rank_unsure ranked UNSURE spots by cable run without looking at margins.

Already on t3/server. Sam's 9e42970 fixed the ranking: a spot whose best estimate is past a rule now ranks after every spot whose estimates all clear. It left the check itself as it was.

This PR fixes the check. check_meter_space still needs the battery clear of the working space by more than the meter's error plus the wall's error to pass. An overlap by more than the wall's error alone now fails, and the check reports that error, so measured_ft ± plus_minus_ft gives the same FAIL. The spot is placed and shown against the meter's anchor, so the meter's error can hold back a pass but should not excuse an overlap. Without this, a scan whose only spots overlap the working space goes to manual review with one of them as the best spot, rather than failing it. server/README.md states the rule. No clearance value changed, so rules.yaml is untouched.

Tests. Three cases join the #44 section of tests/test_field_run_2.py, on its run-2 scene: the -1.25 ft overlap fails, which failed before the fix, and a spot clear by more than both errors passes. Spots clear by less, and overlaps within the wall's error, stay UNSURE with the combined error. On Windows, three tests in test_api.py, test_hardening.py and test_coderabbit_review.py fail on the base branch too; CI runs on Linux.

Review. server/ becomes Hunter's team's lane once #11 merges, so they should review this. It also changes the rule 9e42970 chose to keep, so Sam should confirm. It targets t3/server, so merging it won't close #44 automatically; close it by hand once it lands.

[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 house-scanning-master to your team's repositories.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f43b2be4-0f68-40ca-b944-7a5b5f26e85f

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

---

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.}

[SamGu-NRX]

For whoever reviews: 9e42970 on t3/server already fixes #44 through the ranking. A spot whose best estimate overlaps the working space now ranks after every spot whose estimates clear, and the strict rule stays as the manager decided. This PR goes further: it judges a working-space overlap against the wall's error alone, so the meter's error no longer counts toward UNSURE there. That is a change to the strict rule for one check, so it needs the rules owner's decision rather than a routine merge.

[AidenJohnston]

Challenger review. The issue's spot (overlap of -1.25 ft, meter error 0.792 ft, wall error 0.713 ft) is now FAIL, and the suite passes locally apart from the two Windows-only tests. Two problems, though.

1. Blocking: the new FAIL can turn a meter-tap error into a false reject. At server/solver.py:597, an overlap larger than the wall's error but within the meter's error becomes FAIL. The README says a reject needs every spot within reach to FAIL, so a scene whose only standable spots sit just inside the working space is now rejected, even though the real meter could be where the battery clears it. Repro (local, not pushed): shared_fixture() with meter plus_minus_ft 1.0, wall plus_minus_ft 0.1, and pads_ground([(-W - 0.45, -0.25)]), so every spot overlaps by 0.8 to 1.0 ft. With this PR the result is reject ("every spot fails ground surface, meter working space, route length"). On t3/server it is manual_review. The reasoning in the comment at solver.py:595 and in README.md:103 doesn't hold physically. The spot is shown relative to the anchor, but the working space surrounds the real meter, and the meter's error measures the gap between the two. Also, reporting plus_minus_ft as the wall's error alone hides the meter's error from the app and from reviewers. Fix: drop the FAIL override and keep the check UNSURE. Ranking already keeps these spots from winning (see item 2). If you want a stronger rule, make it a ranking or manual_review signal that can never count toward reject, and add a regression test that this scene isn't rejected.

2. Blocking at conflict resolution: keep the base branch's ranking order. 9e42970 on t3/server already fixed #44's ranking as (estimate_fails(c), len(c.unsure()), route, s0), which is why this PR conflicts. This PR's solver.py:1206 puts the count of "leans fail" checks after the UNSURE count. That lets a spot overlapping the working space within the wall's error, with one UNSURE check, beat a clear spot with two UNSURE checks (for example, headroom unseen there). That is the #44 symptom again. When resolving, keep the base's tuple, delete _leans_fail (solver.py:1196, a duplicate of estimate_fails), and make the ranking sentence in README.md:127 match. The extra sweep boundaries at the pass line (solver.py:1047) are fine to keep.

No clearance numbers were added, so rules.yaml rightly doesn't change. The response schema is unchanged.

[SamGu-NRX]

Read-only review at 4581d03 found a false-reject case despite green Server CI. Using the public rules and shared fixture with an exact wall, no objects, full coverage, and a sole lawn pad at x=[-3.333,-0.75] ft: the reported meter is x=0 ±0.792 ft. The pad overlaps the reported working space by 0.5 ft. #11 returns manual_review (meter_working_space: -0.5 ±0.792 ft), but this head returns reject (-0.5 ±0 ft). If the physical meter is x=0.7 ft, within the stated original uncertainty, the same pad has 0.2 ft clearance; retaining ±0.792 yields manual_review, and a precise remeasurement yields PASS on all 13 checks. I independently reran the whole-solver case. The anchor removes shared world drift, not uncertainty in where the homeowner tapped the physical meter, so server/solver.py:593-599 cannot treat that error as zero only when failing. Preserve UNSURE for possible placements or explicitly adopt a conservative-rejection policy with a reason that acknowledges a feasible site can be rejected. The current comment and README claim the meter error cannot excuse overlap, which overstates what anchoring proves. No review-clear gate on this head.

[SamGu-NRX]

Not merged. The rule change was approved, but two reviews of 4581d03 found a false reject: a spot whose overlap with the working space exceeds the wall's error but is within the meter's error now FAILs, so a scene whose only standable spot is there gets rejected, although the real meter could be where that spot clears (Aiden's challenger review; the caretaker's reproduction in the comment above). #44 is already fixed on t3/server by the ranking (9e42970), so this can wait for a version that keeps the meter's error from turning into a reject, or for the rules owner to accept conservative rejects explicitly.

[AidenJohnston]

Closing: 9e42970 on t3/server already fixes #44 through the solver's ranking, and two reviews of this PR found it rejects some valid spots. Issue #44 stays open until the next TestFlight build confirms Sam's fix.