Repository navigation
Conversation
- Wall equipment: the gap's error now includes the battery's position error (the wall's), so a box 0.2 ft away on a wall known to ±0.5 is UNSURE, not a pass. - Route crossings are judged with the object's error and both route ends': a door that may or may not reach the route makes it UNSURE (it was a false pass on one side and a false reject on the other); a detour that may not be needed widens the run's error. - battery_clearance reads the wall as well as the ground, and is left out only when the gas check already covers both bands to its radius. - Start positions that depend on the battery's own error are placed with the error at each start when it drifts with distance, not the segment's largest error, so a passing interval narrower than the grid (8.005 to 8.030 in the review's scene) is found. - The site plan draws an existing battery (it raised KeyError). - smoke.py's keyed requests never go through a proxy. - A mark whose span starts exactly at the right end is listed as past the right end. No decision changes across the suite's 425 recorded answers or the real scenes. Schemas unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The caretaker's follow-up to 6c7ca23: with the gas points 1 ft in front of the battery the narrow passing interval was missed again, because the re-placement stepped a fixed 0.01 ft around the largest-error estimate and gave up after 0.08 ft. A start's error lies between the segment's least and most, so each boundary lies between where the footprint meets the outline drawn with the least error and the one drawn with the most (by a corner or a side edge). Those positions bracket it, and bisection on the footprint's distance finds it. A property test checks that any start evaluate_start passes is found by the solve, for gas points level with or in front of the battery and intervals down to 0.004 ft; it passed 200 examples. No decision or spot changed across the suite's 425 recorded answers; CPU time on the real scenes is equal or lower. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A wall's declared height_ft is honoured: a wall lower than the battery fails wall_backing (a 2 ft wall passed a 3.29 ft battery). Undeclared heights are unchanged. - Facing and headroom coverage must span every place the battery may sit (its stretch widened by the wall's error), not only the nominal one; requests widen to match. - Two walls meet only when the space between them is within both walls' errors (capped at sweep.wall_join_ft); otherwise it is a real gap that s counts and no cable crosses. Tapped walls (±0.3 each) keep today's 0.6 ft; exact walls keep any real gap. - A spot's wall_id and segment (and each sweep run's) come from the same original wall segment; joined walls reported the first wall's index. - sweep.step_ft must be positive; zero is refused when the rules load instead of dividing by zero in every solve. - Under private rules, policy.notice names the checks still on public placeholder values and is never cleared while any remain. No decision or spot changed across the suite's 451 recorded answers or the real scenes. Schemas unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Wall height: collinear walls joined into one piece kept the lower height along the whole piece, so a spot wholly on a 9 ft wall failed on its 2 ft neighbour. Heights now come from the uploaded walls under the battery: a lower wall surely behind it fails, one within the wall's error of it leaves it UNSURE. - Segments: collinear points were merged before segments were numbered, so the second uploaded segment reported 0. wall_spans records each segment as uploaded (with its wall's declared height); spots and sweep runs report those numbers. - Gaps: the join floor was the 0.01 ft coverage tolerance, which erased a real 0.005 ft gap between exact walls. It is now float noise (EPS): walls with zero declared error meet only where their ends coincide. No decision or spot changed across the suite's 453 recorded answers or the real scenes; one test answer's reported segment changed to the uploaded numbering. Schemas unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- route.confident_reach_ft above route.max_ft is refused when the rules load, and reach_outcome passes a run only when it clears both lines: with confident reach 30 and maximum 20, a 21 ± 2 ft run passed. - A wall view without out_ft reaches headroom height, as the schema says, not infinity: an 8 ft battery's back passed from it while the same view declared at 6.5 went to review. The default counts inclusively, so it still settles every public check. - A private file that sets only some children of a placeholder group (ground.drivable, ground.source) no longer hides the group's remaining placeholder: ground_surface stays in the notice. No decision or spot changed across the suite's 444 recorded answers or the six real answers (public and private). Schemas unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An omitted wall out_ft counts as seen to headroom height inclusively; an explicit one must be strictly higher than the height a check needs. The README says so and why (omitted-view answers stay unchanged; the finite default closes the 8 ft false pass), and a test pins both at exactly the needed height. No code or schema change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- wall_backing: a spot within the wall's error of its straight segment's end is UNSURE, not PASS (the edge of [6.1, 8.683] may be at 9.18 on a wall ending at 9 ± 0.5), and the wall behind every such position must have been seen. Starts clear of the ends by the error are added so the first flush spot is still found. - The route starts at the meter, not its projection onto the wall: the meter's distance from the wall's line counts toward the run and the reach cutoff, and the polyline begins at the meter (a 14 ft run from a meter 2 ft off the wall was a pass; it is 16, past 15 confident). - Scoreboard S2-3 properties: equality is unsure, and moving an obstacle away never makes the check or the spot worse (tests only). No decision or spot changed across the suite's 456 recorded answers or the real scenes; their routes gain the meters' sub-0.001 ft offsets. Schemas unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
S4's verifier found ETH3D's gap loop never closing: each exact capture of the ground request came back 0.005 ft deeper. Three causes: - The requested ground depth was the region's distance from the chain's lines, but coverage is credited per wall strip (and past an unexplored end only in front of scanned walls), which near a corner needs more. The depth is now the smallest that covers the region, found by bisection on the same geometry the checks use; a span ending exactly at a convex corner is widened by the tolerance so it takes the wedge. - Ground behind a scanned wall is the house: at an inside corner one wall's strip reached back across the other's line, and that ground no view could ever show. - A facing or headroom request named only unseen stretches, not those seen too shallow, so capturing it left the check unsettled. Also: GEOS 3.13 returned an empty intersection for large polygons sharing boundaries (0 instead of 172 sq ft), so request overlays are snapped to a 1e-9 ft grid, and seams thinner than the coverage tolerance are removed from unseen ground. A property test over corner scenes (inside and outside, every band) checks that capturing exactly what is requested leaves nothing at the spot unseen; it passed 200 examples. The real scenes (app replay upload, ETH3D, sim; public and private) all settle in one round. No decision or spot changed across 311 recorded answers. Schemas unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Route length: its error now adds the error of every other wall segment the cable runs along (at each one's far end), not only the meter's and the battery's wall's. A 14.5 ft run through two walls known to ±1 was a pass; moving their shared corner within that error made it 15.6 ft. Summing may overstate but never understates. The reach cutoff widens to match. - Route coverage: the wall must be seen over the route widened at each end by that end's error (the meter's, the wall's). Unseen wall within the meter's ±0.3 passed, though a door there would block the cable. - Rules setting openings.exempt_bottom_above_ft above headroom.min_ft, the wall height the opening check requires seen, are refused at load; a higher exemption let an unseen window above the view count. The README row now says so. No decision or spot changed across 493 recorded answers or the real scenes. Schemas unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Scene.house() was the union of strips behind each scanned wall; a wall running back parallel behind another had its strip cross the front facade and hide that facade's yard, so a spot passed with no ground seen at all. The house is now those strips intersected with the house side of every scanned wall's line: when in doubt, yard. Where that leaves ground just behind one wall but in front of the next (an inside corner), a ground request's span is widened over every wall whose strip holds part of the region, so capturing it still settles it. - Seen ground grew 0.005 ft each side and seam removal took another 0.005, closing gaps up to 0.02 ft. The growth is now 0.005 - 1e-4 and seams under 2e-4 ft are removed: together, exactly the 0.01 ft tolerance (0.009 closes; 0.011 and 0.019 stay unseen). The finer seam removal also catches a 9e-10 sq ft speck no request could settle. A property test checks that with no ground observed, no check that needs ground passes, across random facades including a wall running back parallel at any error; the caretaker's three-wall scene is a fixed test. The fixed-point property and all six real-scene capture loops still settle in one round. No decision or spot changed across 392 recorded answers; 83 differ only in their requests. Schemas unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Two property tests on the fixed-point test's corner scenes, per check at a fixed start: moving a ground-standing obstacle straight out from the wall, or adding an observed span to any band, never raises a check's rank (PASS < UNSURE < FAIL) or the start's outcome, and every measured PASS clears its threshold by more than its error. Both pass 200 examples. The existing captured-request property found a new counterexample (the same at 5a4b4f4): a ground request past a limit end comes back after it is captured. It is pinned as a strict expected failure, listed and not fixed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The reach around an unexplored end can extend past the other, limit end. There, ground views were clipped to the scanned walls, so ground seen past the limit end never counted, and the request for it came back after every capture. Views are now clipped only at unexplored ends: the same span coverable() uses, shared as coverable_span(). Ground seen past an unexplored end still counts for nothing. The corner example no longer asks for ground at s [-3.93, -3], which its capture had already seen; drive_clearance now passes there, so the spot moves from s 1.458 to the shorter route at 1.25. The decision is unchanged. CI now runs every property test on fixed examples (a Hypothesis profile with derandomize, no example database), so a check can't pass on one run and fail on the next. Local runs still search at random. A 200-example run of the S2-3 obstacle property found a case where moving a gas meter off the route's start resolves route_length from UNSURE to a correct FAIL. It is the same at 22292a8 and is pinned as a strict expected failure, listed and not fixed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A PASS must survive every perturbation of the inputs within their declared errors. A new property test moves every wall end and corner, the meter, and each object and ground outline within its error (eight directions on its error circle), shifts spans and measured values by their errors, keeps the battery at its offset from the meter (where the AR view anchors it), and requires that no passing check becomes FAIL, with the declared errors or read as exact. It also places near-miss objects just past their rules, and steers the search toward the least slack. It found, and these commits fix: - meter_working_space left out the meter wall's error when the battery is on another wall (the caretaker's witness), and measured an overlap from another wall as 0, a tie that never failed. It now adds the errors of the walls under the working space and measures overlap by its depth. - The battery moves with the meter, so its position error now includes the meter's: a battery 0.04 ft short of a wall's end passed while a 0.05 ft meter error could carry it past. - A battery round a corner slides along the walls when the meter or a corner between them moves (0.3 * sqrt(2) ft for a 0.3 ft meter error at a right angle), so checks against spans in s passed a box that could stand over the battery. The slide is part of its error now. - The route counts the meter's error one-sidedly: e * sqrt(1 + k^2) longer, but shorter only as far as its standoff allows, and the check judges that range (golden test 09 unchanged). - Two GEOS failures (a 500 for a valid scene, and one on a real simulator capture) from unsnapped overlays of the band, the house and the seen ground, now snapped to a 1e-9 ft grid, with only their areas passed on. No recorded decision changed. Spots moved where the larger errors turned a check unsure or shifted the sweep's samples. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
da6d308 added the meter's error to every check, but a span on the meter's own wall moves with the meter exactly as the battery does, so the meter's error says nothing about the gap between them. That turned the private example's garage clearance from a clear FAIL into UNSURE (reject became manual_review). The battery now has two errors (Solver._errors): its wall's plus the meter's against anything placed in plan (outlines, ground patches, wall ends), and its wall's plus the slide along the walls against spans (0 on the meter's wall). Clearances pick per object by whether it has a footprint; wall equipment, facing and headroom use the error along the walls; coverage keeps the larger. The sweep also samples the offsets of the smaller error. The garage example is back to reject. No recorded decision changed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Moving the meter by d moves the route's start and the battery together: the run changes by sigma d.v + |o + d.u| - o (v: the difference of the two walls' directions, sigma the battery's side, o the standoff, u from the wall to the meter). That is the larger of two linear functions of d, so over the error disc it is at most max(e|sigma v + u|, e|sigma v - u| - 2o), derived and reached (sqrt(5) e round a right angle; 72 points on the circle come within 0.0002 ft). e * sqrt(1 + k^2) had treated the two components as independent, and the caretaker's witness passed a route that a 0.3 ft meter move takes past the confident reach. The property is now strict: a check that PASSes must still PASS in every perturbed scene read as exact, its measurement on the pass side of every threshold (the confident reach too), with 16 directions per error circle and near-reach placements by route length. It found that wall_equipment_above looked for unseen wall only over the battery's nominal stretch; it now widens by the battery's error along the walls. No recorded decision or spot changed; one real scene asks for one more wall view. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ings - A side passage behind a gap between walls was neither in front of a wall nor the house, so it never counted as unseen and a spot beside it passed gas, AC and pool. It is now yard: a ground view shows it only when its span lies within the gap (the camera pointed into the passage, both sides of the line, as past a limit end), and a request names exactly the gap's stretch. A property checks that views along the walls never show it; the no-ground property now includes gaps. A subnormal coordinate the property found (GEOS dropped the passage) is read as 0: anything below the 1e-9 ft overlay grid is. - drive_clearance counted seen ground with no recorded surface as no driveway. It is now an unknown attribute, as for ground_surface, and the reason names what settles it. - The fixed-point property failed on a 108 degree corner: a request's depth was accepted with a 9.5e-10 sq ft lens left 1.7e-5 ft inside a clearance circle. Requests now leave at most 1e-12 sq ft. The saved example is an explicit example, so CI runs it. - An 11 KB body of 798 wall segments took 28 s. Scenes over 200 straight segments are refused at parse (0.37 s at 200, 2.7 s at 398, 19 to 29 s at 798). - (Manager, ETH3D) An unseen check whose ground lies past an unexplored end beyond cable reach had no request and no reason. The answer now asks to walk past that end and names the check. Fixture: the pad scene records deck past its ends, where seen ground is otherwise unrecorded. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A gas meter with no outline lies on its wall's line at its span. Against a battery on another wall its clearance left that wall's error out: moving the wall's ends within 0.3 ft moved it 0.17 ft, and 3.037 ft passed where the truth could be 2.866 (the caretaker's case at 3baa338). Such a hazard's error now adds, for each wall between it and the battery other than the battery's own, that wall's error times one plus its turn against the battery's wall. The property now moves a shared corner along an exact wall by the other wall's error, and places near-miss hazards on the other wall's line; with the term removed it finds the class. No recorded decision or spot changed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.
…ter sandbox wipes) First module of the scene-witness reducer: executable predicate contracts with a strict three-outcome split (REPRODUCES / NOT_REPRODUCES / UNKNOWN), argv-only invocation (shell=False), timeouts, and explicit handling of signals, spawn failures, and undeclared exit codes. Contract-relative path resolution keeps a contract working from any working directory. Status: 11/17 contract tests pass; 6 fail after a rebuild forced by sandbox recycling. The rebuilt contract module dropped the executable from argv_for (fixed in this commit); the remaining failures need re-diagnosis. See PR Handoff for the recovery plan and the previously-verified full design. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
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 |
SamGu-NRX
marked this pull request as ready for review
October 11, 2026 17:58
|
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. |
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. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+263
to
+264
| stdout=_decode(proc.stdout), | ||
| stderr=_decode(proc.stderr), |
There was a problem hiding this comment.
evaluate() calls _decode, but the module never defines or imports it. Successful runs, undeclared exit codes, signals, and timeouts therefore raise NameError instead of returning an Evaluation. Restoring the executable in argv_for() does not fix these paths.
Add a helper that handles bytes and absent timeout output.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/scene-witness-reducer/contract.py
Line: 263-264
Comment:
**Predicate runs crash**
`evaluate()` calls `_decode`, but the module never defines or imports it. Successful runs, undeclared exit codes, signals, and timeouts therefore raise `NameError` instead of returning an `Evaluation`. Restoring the executable in `argv_for()` does not fix these paths.
Add a helper that handles bytes and absent timeout output.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+154
to
+160
| outcome = raw.get("outcome") | ||
| _require(isinstance(outcome, dict), f"{path}: outcome must be an object") | ||
| assert isinstance(outcome, dict) | ||
| reproduces = frozenset(_check_exit_codes(outcome.get("reproduces_exit_codes", None), | ||
| "reproduces_exit_codes")) | ||
| not_reproduces = frozenset(_check_exit_codes(outcome.get("not_reproduces_exit_codes", None), | ||
| "not_reproduces_exit_codes")) |
There was a problem hiding this comment.
load_contract() rejects unknown top-level fields but ignores unknown fields inside outcome. For example, reproduces_exit_code: [0] alongside a valid not_reproduces_exit_codes loads successfully, but exit 0 becomes UNKNOWN.
Check nested keys against the supported fields, including the existing unknown field, so a typo fails when the contract loads.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/scene-witness-reducer/contract.py
Line: 154-160
Comment:
**Contract typos change outcomes**
`load_contract()` rejects unknown top-level fields but ignores unknown fields inside `outcome`. For example, `reproduces_exit_code: [0]` alongside a valid `not_reproduces_exit_codes` loads successfully, but exit 0 becomes `UNKNOWN`.
Check nested keys against the supported fields, including the existing `unknown` field, so a typo fails when the contract loads.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+520
to
+522
| a, b, depth = self.scene.view_to_cover(behind, gap.s0, gap.s1) | ||
| out.append(View("ground", a, b, _up(depth))) | ||
| region = shapely.difference(region, passage, grid_size=_OVERLAY_GRID) |
There was a problem hiding this comment.
Requests for ground behind a gap do not tell the client to supply camera_pos_ft, although view_polygon() now requires it. view_to_cover() also checks its proposed capture with camera=None, so it cannot show that the passage request settles the check. A client supplying only the requested band, span, and depth gets the same request again.
Carry the camera requirement through View and the response, use it when checking the proposed capture, and keep passage requests separate when nearby spans are merged.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/solver.py
Line: 520-522
Comment:
**Passage requests repeat**
Requests for ground behind a gap do not tell the client to supply `camera_pos_ft`, although `view_polygon()` now requires it. `view_to_cover()` also checks its proposed capture with `camera=None`, so it cannot show that the passage request settles the check. A client supplying only the requested band, span, and depth gets the same request again.
Carry the camera requirement through `View` and the response, use it when checking the proposed capture, and keep passage requests separate when nearby spans are merged.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+341
to
+342
| entries, directory_bytes = _zip_directory(data) | ||
| if entries > MAX_ZIP_ENTRIES: |
There was a problem hiding this comment.
_zip_directory() trusts the entry count claimed by the uploaded ZIP. Python's ZipFile ignores that count and reads entries until it reaches the declared directory byte size. A ZIP containing over 20,000 short entries can claim zero entries and remain below the 8 MB directory cap, bypassing MAX_ZIP_ENTRIES.
Count the directory records within the byte cap before constructing ZipFile, and add a test with a false claimed count. The directory byte cap still limits the overall size.
How this was verified: The upload supplies the claimed count, while ZipFile reads directory records using the byte size rather than that count.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/api.py
Line: 341-342
Comment:
**ZIP entry cap is bypassed**
`_zip_directory()` trusts the entry count claimed by the uploaded ZIP. Python's `ZipFile` ignores that count and reads entries until it reaches the declared directory byte size. A ZIP containing over 20,000 short entries can claim zero entries and remain below the 8 MB directory cap, bypassing `MAX_ZIP_ENTRIES`.
Count the directory records within the byte cap before constructing `ZipFile`, and add a test with a false claimed count. The directory byte cap still limits the overall size.
**How this was verified:** The upload supplies the claimed count, while `ZipFile` reads directory records using the byte size rather than that count.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+327
to
+331
| try: | ||
| with io.BytesIO(data) as f: | ||
| end = zipfile._EndRecData(f) | ||
| except OSError: | ||
| end = None |
There was a problem hiding this comment.
_EndRecData() can raise BadZipFile when a ZIP64 locator says the archive spans multiple disks. The new call happens before the existing BadZipFile handler, and _zip_directory() catches only OSError. Such an upload now returns 500 internal_error instead of 400 unreadable_zip.
Catch BadZipFile here and preserve the existing refusal.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/api.py
Line: 327-331
Comment:
**Bad ZIP returns server error**
`_EndRecData()` can raise `BadZipFile` when a ZIP64 locator says the archive spans multiple disks. The new call happens before the existing `BadZipFile` handler, and `_zip_directory()` catches only `OSError`. Such an upload now returns 500 `internal_error` instead of 400 `unreadable_zip`.
Catch `BadZipFile` here and preserve the existing refusal.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62bd963fb3
ℹ️ 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".
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".
Comment on lines
+263
to
+264
| stdout=_decode(proc.stdout), | ||
| stderr=_decode(proc.stderr), |
There was a problem hiding this comment.
_decode before processing predicate output
Every predicate that starts successfully reaches one of these _decode(...) calls, but this module defines no _decode function. Consequently, normal exits, undeclared exits, signal deaths, and timeouts all raise NameError instead of returning the promised Evaluation; only spawn failures avoid the broken path.
Useful? React with 👍 / 👎.
Comment on lines
+202
to
+207
| proc = subprocess.run( | ||
| argv, | ||
| capture_output=True, | ||
| timeout=contract.timeout_seconds, | ||
| cwd=str(cwd), | ||
| shell=False, # explicit: the contract is argv, never shell text |
There was a problem hiding this comment.
When a contract specifies a non-default cwd, load_contract validates and stores it as relative to the reducer's working directory, but evaluate always launches the predicate directly in the caller-supplied base cwd. Predicates relying on files or relative imports beneath the declared directory therefore run in the wrong location, typically producing an unrelated UNKNOWN result.
Useful? React with 👍 / 👎.
Comment on lines
+392
to
+398
| if s0 - e < piece.s0 - EPS or s1 + e > piece.s1 + EPS: | ||
| c.outcome, c.unsure_cause = UNSURE, "margin" | ||
| c.reason = ( | ||
| "The footprint ends within the wall's measurement error of its straight segment's " | ||
| "end, so the battery may not sit flush." | ||
| ) | ||
| return c |
There was a problem hiding this comment.
For a candidate within the wall-position error of a segment end, this early return reports wall_backing as UNSURE before examining the newly supported wall height_ft. If the wall is definitively shorter than the battery throughout the candidate's sure overlap, the later height logic would return FAIL, so this ordering can turn an otherwise rejectable scene into manual review and omit the actual height failure.
Useful? React with 👍 / 👎.
Comment on lines
+341
to
+347
| entries, directory_bytes = _zip_directory(data) | ||
| if entries > MAX_ZIP_ENTRIES: | ||
| raise ApiError( | ||
| 413, | ||
| "bundle_too_many_entries", | ||
| f"The bundle lists {entries} entries, over the {MAX_ZIP_ENTRIES} limit.", | ||
| ) |
There was a problem hiding this comment.
A crafted archive can underreport the entry count in its end-of-central-directory record while retaining a much larger central directory. Python's ZipFile iterates entries until it consumes the declared directory byte size rather than stopping at that count, so this precheck can accept entries <= MAX_ZIP_ENTRIES and then allocate metadata for far more than 20,000 entries. Check len(zf.infolist()) after parsing as well, or derive a trustworthy bound from the directory itself.
Useful? React with 👍 / 👎.
Comment on lines
+518
to
+522
| behind = polygonal(_meet(region, passage)) | ||
| if behind.area > 1e-12: | ||
| a, b, depth = self.scene.view_to_cover(behind, gap.s0, gap.s1) | ||
| out.append(View("ground", a, b, _up(depth))) | ||
| region = shapely.difference(region, passage, grid_size=_OVERLAY_GRID) |
There was a problem hiding this comment.
When unseen ground lies behind a gap, this emits an ordinary ground View, but such a view is only credited by Scene.view_polygon when it carries camera_pos_ft from inside the opening. The resulting missing_evidence therefore tells clients only to capture the span and depth; supplying exactly those fields leaves the same passage unseen and causes another review round. Preserve a passage/camera requirement in View and surface it in the request or its message.
Useful? React with 👍 / 👎.
Comment on lines
+334
to
+336
| recorded = unary_union([g.polygon for g in self.scene.ground]) | ||
| outdoor = self.scene.outdoor_band(self.scene.reach_ft) | ||
| self._unclassified_cache = outdoor.difference(recorded).buffer(-1e-6).buffer(1e-6) |
There was a problem hiding this comment.
Scene.outdoor_band explicitly includes regions that still need the house geometry removed, but _unclassified subtracts only recorded ground patches. Around corners where the generated outdoor strips overlap the known house side, that house area is consequently treated as an unknown surface or possible driveway, leaving otherwise fully classified candidates UNSURE even though no outdoor ground is missing there.
Useful? React with 👍 / 👎.
Comment on lines
+2051
to
+2055
| reasons.append( | ||
| { | ||
| "code": "policy_review_before_reject", | ||
| "message": "The rules in use leave rejections to a person, so nothing here " | ||
| "is rejected automatically.", |
There was a problem hiding this comment.
When an approved policy has allow_reject: false and every candidate fails, the solver now emits policy_review_before_reject, but the served result schema's reasons[].code enum does not include that value. The server therefore produces a response that fails validation against its own /v1/schemas/result.json contract for this policy configuration.
Useful? React with 👍 / 👎.
Comment on lines
+327
to
+331
| try: | ||
| with io.BytesIO(data) as f: | ||
| end = zipfile._EndRecData(f) | ||
| except OSError: | ||
| end = None |
There was a problem hiding this comment.
For a ZIP carrying a ZIP64 locator with a corrupt offset or unsupported multi-disk metadata, zipfile._EndRecData raises BadZipFile, not OSError. Because this precheck catches only OSError and runs before the later guarded ZipFile construction, such malformed uploads escape as a generic 500 instead of the documented 400 unreadable_zip response.
Useful? React with 👍 / 👎.
SamGu-NRX
marked this pull request as draft
October 11, 2026 18:17
SamGu-NRX
commented
Oct 11, 2026
SamGu-NRX
left a comment
SamGu-NRX
left a comment
Owner
Author
There was a problem hiding this comment.
Verdict: FIX
Reviewed head 62bd963fb359bc48f2537cc50e5e6b8a3ce9e971 against products-l3-scene-witness-reducer-20261010.
tools/scene-witness-reducer/contract.py:263–264calls_decode, which is never defined or imported in the module. Every normally completed predicate reaches this NameError; timeout and signal branches also call it at lines 216/241. The saved executable-in-argv fix does not make these tests green. Supply the decoder and verify the owned suite at the delivered head.- The entire tool tree contains only
contract.py,conftest.pyandtests/test_contract.py. The required schema-valid removal rules, bounded deterministic reducer, failure fixture/predicate, independent replay, nondeterminism refusal and retained-witness receipts are absent. The task'sreduce.py --scene .../fixtures/failure.json --predicate-file .../fixtures/predicate.json --out .../evidencecannot execute. Finish and prove these criteria before acceptance. - PR base is
main, while the authoritative task requiresobv/products-basescanning-hardening-20261009-r1. This exposes 31 inherited server/smoke paths outside the tool's ownership in the PR diff. Comparing against the requested parent468cb86bisolates exactly the three tool files. Retarget to the authorized parent or obtain an explicit integration disposition for the inherited changes; they are not new reducer-owned work.
Criteria met 1/7: explicit argv/exit-code contract definition. Partial: executable predicate/tests and scope/base alignment. Missing: schema-aware allowed removals, bounded reduction, independent replay/refusal, and the required fixture command/evidence. CI run 38120715875 passes 412 server tests plus one xfail from server/; it does not execute this tool. No local tests were run.
The checkpoint Handoff correctly discloses missing implementation. Its earlier 1,054-to-344-byte reduction and 17/84 counts describe lost work, not evidence committed at this head. Keep that distinction until fresh output and independent replay exist.
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.
Scene-witness reducer (checkpoint: contract module saved, rebuild in progress)
Shrinks a scene JSON that triggers a server decision into the smallest scene that still triggers it, keeping a full evidence trail (accepted steps, refusal records, transcripts, no timings). This PR is a shutdown checkpoint, not a finished feature — see Handoff.
Context
An earlier session had the complete tool implemented and green (M1–M3: contract, schema-aware removal rules, greedy reducer, discriminating pool fixture, acceptance script; a full reduction ran 1,054 → 344 bytes across 17 accepted steps and 84 refusals, the last refusal being the pool itself). The sandbox was recycled twice and that work was never pushed, so it was lost with the scratch disk. This commit re-saves the first module from the session record; the rest is re-authored in the Handoff's prescribed order.
Handoff
What is done (commit 62bd963)
tools/scene-witness-reducer/contract.py— executable predicate contracts: strict JSON validation (unknown fields rejected, no shell text, exactly one scene placeholder, overlapping exit-code sets rejected, timeout/repeat bounds), argv-only execution withshell=False, andevaluate()classifying every run intoREPRODUCES/NOT_REPRODUCES/UNKNOWN(signals, spawn failures, timeouts, undeclared exit codes all land in UNKNOWN and are distinguished in the transcript). Transcripts carry stdout/stderr/exit/signal — never timings, so runs are reproducible on slower machines.tools/scene-witness-reducer/conftest.py— makes the flat tool dir importable by pytest.tools/scene-witness-reducer/tests/test_contract.py— 17 tests: 11 pass, 6 fail (see below).468cb86b(main has since advanced toc813182avia PR chore(review): run Greptile again when PR commits change #194; branch is deliberately on the task's recorded base). Author of every commit is Sam Gu with the obvious-autobuild co-author trailer; draft-only; no workflow/deploy/settings files touched.What is NOT done
argv_fordroppedcontract.executablefrom the spawned argv — the rebuild bug), but the fix landed seconds before the save deadline and the suite was not re-run; the 6 failures may all be stale. Re-run before diagnosing anything else.scene_rules.py(schema-aware removal rules — optional properties, array items, list points; respectminItemsand the conditional requirement that aground-band coverage entry keepsout_ft; candidates ordered whole-property → array-item → point, largest first),reduce.py(greedy loop: accept a candidate only if the scene stays schema-valid AND the predicate still reproduces when evaluatedmax(2, determinism_repeats)times with identical outcome and identical transcript; otherwise record a refusal and keep the witness byte-identical; CLI--scene --contract --out --evidence --max-steps),predicate.py+predicates/rejects_when_pool.json(run the server's realparse_scene→solveand exit 0 iff decision isreject; path args resolve relative to the contract file),fixtures/pool_scene.json(synthetic discriminating fixture: a pool spanning wall w1,span_ft [0, 26],footprint [[-12,0],[14,0],[14,8],[-12,8]], makes the serverrejectbecause every candidate fails pool clearance; removing the pool yields a cleanpass— with/without verified), reducer tests (8, were green),acceptance.sh,README.md.Exact next steps
argv_forfix cleared the 6 failures (command below). If any still fail, diagnose against the documented behavior in the module docstrings.scene_rules.py(load the schema fromserver/schemas/scene.schema.json; exposeload_validator()returning(validator, schema)and a removal-candidate generator with a stable order), then its tests; commit as milestone M1.reduce.py+ reducer tests; commit as M2.predicate.py,predicates/rejects_when_pool.json(args["rejects_when_pool.py", "@SCENE"]— the loader resolves the script path against the contract file's directory),fixtures/pool_scene.json,acceptance.sh,README.md; run acceptance; commit as M3.Commands that verify it
Decisions made
468cb86b, not current main, to keep the checkpoint's history clean; rebasing onto main is left to the reviewer's choice.