Repository navigation
Server hardening: camera-evidence coverage, solver budgets, bounded bundles, provenance - #221
Draft
obvious-autobuild[bot] wants to merge 3 commits into
Draft
obvious-autobuild[bot] wants to merge 3 commits into
obvious-autobuild[bot] wants to merge 3 commits into
Conversation
Thirteen witnesses for the review findings behind #176 and the bundle path, each failing on t3/server at 1d1e7e1, plus five behavior-preservation checks that already pass. Baseline on t3/server (uv run pytest -q): 13 failed, 394 passed, 1 xfailed; uv run ruff check . and ruff format --check . clean. Witnesses, by area: - evidence: a ground span inside a gap claims the passage behind the gap's line with no camera evidence at all; the outdoor reach omits the meter's position error - budgeting: MAX_STARTS and the time budget apply only after every wall's starts are generated and after geometry preprocessing; the zip bundle's central directory is parsed before any entry-count bound applies - reason/provenance: zero meter_working_space rules are accepted; the meter working space cites only its width while depth draws the space; backing keeps the width citation when the height decides; a rejection-forbidden policy yields manual review with no policy reason and serializes no allow_reject; the route length reads as measured while a detour object's height is unknown, and the standoff is credited the battery's wall's error instead of the wall carrying the path Missing-field compatibility policy for every change that follows on this branch (old clients without the new evidence keep working): - scene.json: camera_pos_ft is a new optional field on a coverage view. A scene without it parses exactly as before; a span without camera evidence grants nothing behind a gap's line on its own -- absent evidence means unknown, never granted. Old packets stay valid with no behavior change. - result.json: length_is_lower_bound (route) and allow_reject (policy) are new fields, emitted on every result from the next commit and listed in the schema so that output validates. Readers that ignore unknown fields are unaffected; a result without them is pre-change output, not an error. No existing field changes meaning; the schemas add only.
All thirteen witnesses from the previous commit now pass; the full suite is 407 passed, 1 xfailed; uv run ruff check . and ruff format --check . clean. Command run in server/: uv sync --locked, uv run ruff check ., uv run ruff format --check ., uv run pytest -q. Evidence: - scene.py: a coverage view may carry camera_pos_ft, the plan position of the camera that took it, checked at parse time against the packet's keyframe camera poses (within 1.0 ft, CAMERA_TOLERANCE_FT); with no keyframes in the packet a declared position is accepted as claimed, and without one a ground span inside a gap grants nothing behind the gap's line on its own. The observed tuple gains a fourth slot (camera), threaded through every reader. - scene.py: the outdoor reach adds the meter's position error, so the area modelled out from the walls covers where the battery could actually stand relative to what was measured. Budgeting: - solver.py: MAX_STARTS and the time budget are enforced as each wall's battery positions are generated, not after every wall has contributed; geometry preprocessing (unobserved ground, unclassified outdoor area) is deferred to first use, behind both guards. Solver takes an injectable clock. - api.py: a bundle's zip central directory is bounded before zipfile parses it -- entry count (MAX_ZIP_ENTRIES, 20,000) and directory size (MAX_ZIP_DIRECTORY_KB, 8 MiB), both env-tunable, read from the end record zipfile itself uses; bytes with no end record answer 400 unreadable_zip. Reason/provenance: - solver.py: the route standoff is credited the error of the wall carrying the path nearest the object (at its far end, as _walls_between sums), not the battery's wall's error everywhere; the route carries length_is_lower_bound when a detour object's height was unknown; backing cites battery.height_ft (width kept as a cited key) when the height decides; meter_working_space cites depth alongside width; a rejection-forbidden policy adds a policy_review_before_reject reason and serializes allow_reject; zero meter_working_space rules are refused. - api.py: /health's policy report adds allow_reject, matching a placement's. - schemas: scene.schema.json adds the optional camera_pos_ft on a coverage view; result.schema.json adds allow_reject (policy) and length_is_lower_bound (route). Additions only; scenes without the new field parse unchanged (missing evidence means unknown, never granted), and results without the new fields are pre-change output, not errors. Two existing tests updated for the behavior change itself, not to turn a failing rule green: test_a_view_into_the_passage_settles_it now declares the camera position its comment already claimed (with a vouching keyframe), and test_health_reports_the_loaded_policy expects the added allow_reject field. Local /v1/placements smoke on owned synthetic request files (no real captures): bare JSON 200 pass, zip bundle 200 pass with the same decision, bytes with no zip end record 400 unreadable_zip; both valid results carry length_is_lower_bound and allow_reject. Exact invocations in smoke/ along with the request and receipt files.
A second batch of five witnesses, none sharing a mechanism with the first thirteen: a gap mid-wall with an outside corner 6 ft past it, where the passage behind the gap's line is no wall's behind-strip (a camera position in the opening settles it; without one the pool-clearance asks for a view); a split gap span with only the camera-backed half claimed, the other half still unobserved ground; the route reporting its length as a lower bound while the path check stays unsure over an unknown detour's height; and schema compatibility checked both ways -- scenes with and without camera_pos_ft validate against scene.schema.json, and a result with the two new fields stripped still validates against result.schema.json, so pre-change output is not an error. Final counts, server/: uv run ruff check . and ruff format --check . clean; uv run pytest -q reports 412 passed, 1 xfailed (up from 394 passed at the clean-base baseline; the 13 witnesses of the first batch are the same failures the baseline recorded, plus this batch). /v1/placements smoke receipts from the previous commit stand unchanged.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
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 |
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
1 participant
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.
Review-hardening pass over the placement server, driven by a failing-then-passing witness suite.
Witnesses first (3fb7303). 18 regression tests, each failing on the unfixed tree (13 failed / 394 passed baseline recorded in the commit). Adds the compatibility policy: a coverage view may carry optional camera_pos_ft (missing means unknown, never granted), and new result fields are additive.
Repairs (a8182aa).
Independent witnesses + schema compat (468cb86). Five more witnesses (outside-corner gap, split-span claims, lower-bound reporting) and both-way schema checks: old fieldless results still validate.
Full suite: 412 passed, 1 xfailed; ruff check and format clean. Local /v1/placements smoke on synthetic requests: bare JSON 200 pass, zip bundle 200 pass, malformed zip 400 unreadable_zip (invocations and receipts in smoke/). No real captures, private rules, or meter numbers included. Draft: not merged, not deployed.