Repository navigation
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. (cherry picked from commit 3fb7303)
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. (cherry picked from commit a8182aa)
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. (cherry picked from commit 468cb86)
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
Owner
Author
|
@codex review |
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. |
Owner
Author
|
@codex review |
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.
Zephyrus task Z8. This ports #221's three server-hardening commits onto Z7's port of #11 (#319, branch
gpt-burst/1022z-bs-port-11). It is stacked on #319 and only makes sense after that lands.What changed
The three commits are cherry-picked with
-x, in their original order, and keep Sam Gu as author. After the picks,server/andsmoke/are identical to #221's head468cb86b.3fb7303f: addsserver/tests/test_review_hardening.py, 13 witness tests and 5 checks that existing behavior still holds.a8182aa3: the repairs.camera_pos_ft, checked against the packet's keyframe poses (server/scene.py). A ground span inside a wall gap no longer counts the passage behind the gap as seen unless a camera position backs it.MAX_STARTSand the time budget are now enforced while battery positions are generated (server/solver.py).zipfileparses it (server/api.py).length_is_lower_bound, cited rule keys, and theallow_rejectpolicy reason. Schema changes add fields and change none.468cb86b: adds 5 more witnesses: an outside-corner gap, a split gap span, the lower-bound route, and schema compatibility in both directions.Why
#221 targets
t3/server, whichalphanever took. Z7 portst3/servertoalpha, and this PR brings #221 along with it. #221 has no unresolved review threads.Verification
All runs were in WSL, in
server/, on 16 cores under a load average of 15 to 26. pytest-xdist is not installed, so the runs were serial.Failing first, on Z7's tip
1bbf38ff:3fb7303f,.venv/bin/python -m pytest -q tests/test_review_hardening.pygave 13 failed, 5 passed, the same result Server hardening: camera-evidence coverage, solver budgets, bounded bundles, provenance #221 recorded ont3/server. Each test failed on the missing behavior: the schema rejectedcamera_pos_ft,Solverhad noclockargument,length_is_lower_boundwas missing, an oversized bundle did not raise, a wrong rule key was cited, and so on. None failed on an import error.468cb86b, as a temporary copy that I deleted afterwards, against Z7's code: 18 failed, 5 passed. All 5 witnesses from the second batch fail there.After the port, at
18bf2185:tests/test_review_hardening.py: 23 passed.server/suite: 2 failed, 410 passed, 1 xfailed. On Z7's tip it was 2 failed, 387 passed, 1 xfailed. The same two timing tests fail both times:test_api.py::test_solving_the_example_takes_under_a_second(2.0 s against a 1 s limit) andtest_hardening.py::test_crowded_scene_solves_quickly(SceneTooComplex, projected 9 to 10 s against an 8 s budget). They still fail when run alone on this loaded machine, so I could not confirm them on an idle one.ruff check .andruff format --check .are clean.server/solver, run withcd backend && .venv/bin/python -m pytest -qonserver/orchestrator/tests/test_placement_stage.py,server/placement/tests/test_scene_bridge.pyandtests/test_backend_w6_gate,test_capture_case_pairs,test_capture_placement_cases,test_capture_placement_frontdoor,test_placement_case_suite,test_runtime_join_journey,test_capture_replay_cli_acceptanceandtest_capture_real_downstream:hazard-right-ground-gap,hazard-left-ground-gapandhazard-left-wall-gapreport "decision semantics differ from the independently authored wire case". The 2 that failed only on Z7's tip weredrain_finalizetimeouts under load.Risk
alpha.verification/orrecon.backend/eval/vendor/hs11_serverandbackend/server/eval/vendor/hs11_serverare unchanged, so they do not have these repairs.Checklist
🤖 Generated with Claude Code
Rerun with #334's pins (Oct 23)
I merged PR #334's head
5384d3b57ab24406909a48f46a8dc9694f472822into this PR in a scratch worktree. I did not commit or push that merge.server/:env -u NODE_ENV OMP_NUM_THREADS=1 .venv/bin/python -m pytest -q-> 410 passed, 2 failed, 1 xfailed. The two failures are the existing load-sensitive timing tests,tests/test_api.py::test_solving_the_example_takes_under_a_second(1.46s, limit 1.0s) andtests/test_hardening.py::test_crowded_scene_solves_quickly(SceneTooComplex, projected 10s against the 8s budget). They are unrelated to the pins and remain unverified on an idle machine.backend/:env -u NODE_ENV OMP_NUM_THREADS=1 .venv/bin/python -m pytest -q tests/test_placement_criteria_agreement.py tests/test_capture_real_downstream.py tests/test_placement_case_suite.py tests/test_capture_case_pairs.py tests/test_backend_w6_gate.py tests/test_capture_replay_cli_acceptance.py tests/test_capture_placement_frontdoor.py tests/test_capture_placement_cases.py server/placement/tests server/orchestrator/tests/test_placement_stage.py-> 127 passed.decision semantics differ from the independently authored wire case.