Repository navigation
fix(pipeline): preserve reconstruction and verification evidence - #240
Conversation
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
|
@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. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
[Medium impact] The latest changes appear safe to merge and fix the missing Markdown explanations.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
SeenFromTwo.observed rebuilt the frames-by-frames distance matrix on every slab query, and free_space makes thousands. It now builds it once per reconstruction and passes it to two_positions. On a synthetic 20 m clear wall with 150 frames the free-space pass drops from 16.0 s to 10.7 s with identical clearances. The rest is projecting each slab into every frame; batching that means reshaping free_space's march, which #240 rewrites. #242 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@greptileai review this at apex, and go beyond the diff. Read the full files this PR touches, plus the code that calls into them and the code they call, and flag real bugs you find anywhere in that wider area, not only in the changed lines. Include problems that were already there before this PR. Skip style nits. |
Bind ZIP admission to the footer stdlib will parse and refuse ZIP64 overrides before metadata allocation. S2-8 requires execution of all five hostile probes, and geometric fixtures declare the public rule values their expectations depend on. The root verifier target now uses the locked dependency sync from CI. Tiny archive controls, shortened CLI reports, real-case overrides and dispatch failures cover the new review findings.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10f0fb1463
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68cb6859ea
ℹ️ 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".
| scenes = [i for i in infos if i.filename == "scene.json"] | ||
| if len(scenes) != 1: | ||
| raise SystemExit( | ||
| f"{path}: expected exactly one canonical scene.json, found {len(scenes)}" |
There was a problem hiding this comment.
Accept scene.json inside a single top-level folder
When --scene or --app-export points to a valid Finder-style bundle such as capture-2026-09-26/scene.json, this exact-root check exits before the bundle reaches the server. The API explicitly accepts this layout in server/api.py::_scene_folder, and server/tests/test_api.py::test_zip_with_scene_inside_one_top_level_folder protects it, so the verifier should select either the root file or the exact file inside one shared top-level folder while still rejecting ambiguous suffix matches.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67b10e5684
ℹ️ 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".
| valid_widths = [(source, width) for source, width in widths if finite(width) and width > 0] | ||
| if valid_widths and any( | ||
| abs(width - valid_widths[0][1]) > NUMBER_TOL_FT for _, width in valid_widths | ||
| ): | ||
| problems.append("reported battery widths disagree") |
There was a problem hiding this comment.
Compare reported battery width with the active rules
When the server under test uses a stale or hard-coded battery width but reports that same value in both the spot and wall_backing check, this block verifies only internal agreement and never compares it with rules.width_ft loaded from the tested ref. The new rules_assumed.battery_width metadata also compares the fixture with those rules rather than with the response, so a server can pass the verifier while evaluating an undersized battery; include the active rule width in this comparison.
AGENTS.md reference: AGENTS.md:L32-L34
Useful? React with 👍 / 👎.
| # ZIP corruption can name an unsupported compression method. | ||
| NotImplementedError, | ||
| zipfile.BadZipFile, | ||
| zlib.error, | ||
| ): |
There was a problem hiding this comment.
Recompute caches that claim encryption
If corruption flips the encrypted flag in an NPZ central-directory entry, np.load raises RuntimeError (for example, that a password is required), but this repair block does not catch it. The damaged cache therefore remains in place and every subsequent reconstruction fails instead of deleting it and rerunning inference; handle this ZIP read failure alongside the other corruption exceptions.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7b3169fef
ℹ️ 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".
| if values := reported_assumption_values(key, result): | ||
| changes[field] = values[0][1] |
There was a problem hiding this comment.
Verify reported dimensions against matching public rules
When --scene or --app-export supplies no rules_assumed metadata and the response claims the public policy with policy.rules_sha256 matching the tested ref, this unconditional replacement trusts a stale or hard-coded reported width instead of checking it against rules.width_ft. The server can therefore use and report the same incorrect width and still earn contract acceptance. Fresh evidence after a7b3169 is that reported_rules still overrides the active public rules without consulting the policy fingerprint; restrict overrides to private/different policies and compare matching-public responses with the loaded rules.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
Keep loaded dimensions when the response claims the tested public fingerprint, including transformed responses. Reject malformed local case fields without losing the report. Reviews: #240 (comment) #240 (comment)
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Aggregate expectation failures with bounded examples and cap serialized diagnostic text across the report. Use exact string keys instead of adversarial numeric hashes. Make the pre-end sampling count explicit while preserving sampled starts. Reviews: Astra resource addendum; #242 (comment) #242 (comment)
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Prioritize the Markdown-visible problems array before duplicate JSON classifications. Regressions retain the first explanation for 1,000 contract or expectation failures without exceeding the shared diagnostic budget. Review: #240 (comment)
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
What changed
This fixes review comments from already-merged PRs that were never addressed. Reconstruction and verification now refuse the tested invalid evidence, preserve capture inputs and report incomplete runs as incomplete.
#20: recon: handoff to the server team
Reconstructed walls retained old facing and overhead measurements when the meter barely moved. The target wall now loses those entries whenever its baseline is replaced; the rotation regression preserves other walls and the input document. Review 1. Review 2.
Clearance crossed occupied slabs below the 20% threshold or only one voxel thick. Any occupied sampled voxel now stops the facing or overhead search. Tests cover a thin rail and a one-step obstruction. Review 1. Review 2.
Ground samples used a flat datum despite fitting a slope. Sampling now evaluates the fitted plane at each point and uses its normal; synthetic sloped depth supports the surface and rejects the old horizontal samples. Review 1. Review 2.
Coverage could extend beyond the fitted wall. Only complete half-foot cells inside the extent are exported; regressions cover partial endpoints and a wall shorter than one cell. Review 1.
LiDAR without confidence data entered fusion unfiltered. It now fails before reading depth; mixed-capture fixtures supply actual confidence bytes and the missing-map regression checks the refusal. Review 1. Review 2.
Untrusted ZIPs could exhaust disk or allocate unlimited entry metadata. Admission reuses the verifier policy of 2,000 entries and 512 MiB, streams hashes and central records, and publishes extraction only after all members finish. Tiny archives exercise byte/count limits, a false entry count and failed extraction. Review 1. Review 2.
Corrupt caches blocked subsequent runs, and concurrent runs could overwrite them. Cache validation and inference run under one advisory lock; malformed archives trigger recomputation and completed NPZ files publish atomically. Tests exercise truncated ZIPs, invalid deflate data, lock exclusion and an interrupted write that preserves the prior cache. Review 1. Review 2.
EXIF orientation rotated sensor pixels away from their intrinsics and poses. Image loading ignores EXIF rotation; fixtures with orientation 3 and 6 retain raw pixels. Review 1. Review 2.
Failed or no-server reruns left earlier answers beside new outputs. Known outputs now clear at startup, with input/output overlap refused before deletion. Regressions preserve capture inputs and unrelated files. Review 1. Review 2.
The export silently sliced observed entries at 500. It now raises a specific error above the existing schema limit; boundary tests retain all 500 entries and refuse 501. Review 1.
Follow-up review on this PR found another damaged-cache failure: an unsupported ZIP compression method raised NotImplementedError and survived every retry. The cache-repair block now handles that read failure. The regression writes a damaged cache file, verifies fresh inference, then retries and checks that the repaired cache is reused. Cache-retry review.
Deep review found that a later malformed ZIP footer in a comment could bypass the early directory cap. Admission now selects the same footer as Python and refuses malformed tails and ZIP64 locator overrides before constructing
ZipFile. Tiny fixtures assert that metadata allocation is never reached; normal comments still extract. Deep review.Vet follow-ups protect the effective ZIP extraction directory and its ancestors/descendants before startup cleanup. Cached members are verified by name, size and streamed CRC and damaged caches are re-extracted under a lock. Overhead samples include both edges of the existing 0.56 m footprint, catching the missed 1.90 m eave; support for other battery depths still needs the server footprint contract. At df7b75d, twelve new recon cases failed before these fixes; the guarded recon suite passes 125 tests on #240 and 149 on #242.
#13: verification: independent end-to-end checks (S4)
The shared root dispatcher was the one necessary exception to the folder scope. Makefile now invokes verification, and tests/makefile.sh checks its inclusion, standalone target, missing-folder refusal and failure propagation across all 64 optional-suite combinations. Review 1.
Deep-review follow-ups now require all five hostile probes to have actually executed before S2-8 receives full acceptance;
--no-hostileand legacy reports cannot earn that credit. The generator and 43 committed cases declare battery width, with corner allowance on the four relevant fixtures, so changed rules skip stale expectations and cannot establish acceptance. Only assumption metadata changed; geometry and expectations are unchanged. The shared root verification target runsuv sync --lockedbefore its child tests, with dispatch and sync-failure regressions. Hostile evidence review, case assumptions review, locked sync review.Codex found that a server-reported sweep such as
[0, 1e9]could allocate billions of starts. Sweep admission now rejects spans outside the scanned chain before sampling, and the sampler validates eagerly and yields lazily with a 10,000-sample cap including the endpoint. Oversized runs fail with a specific message rather than get truncated. Regressions cover both outcomes, both scan ends, a distant singleton, tiny rule steps and the exact cap boundary. Sweep sampling review.One runtime meter counts the geometry and comparison work that actually runs, including unsure checks, footprint lookups, ground grids, coverage and corner checks. Identical sweep records are deduplicated, and identical geometry checks are memoized. Structural admission rejects nonfinite numbers, overflowing span lengths, invalid dimensions and unknown wall references before geometry. The ground sampler is lazy; ordering comparisons use an interval index. A failed admission or invariant stops property comparisons, and any check exception becomes a named report problem. Exceeding the runtime limit produces
verification work limit exceeded (N samples). Verification ZIP admission still counts directories before metadata allocation, responses remain bounded at 4 MiB, and required simulator states must occur in order.The runtime limit is now 7,834,000 units, twenty times the remeasured valid peak of 391,700 on a saved ETH3D facade response. After caching, 35 valid public-fixture judgments including property variants peak at 31,649 units. Twenty additional measurements cover an 80-foot surveyed wall written as twenty walls, each subdivided 1, 4, 12, 13 or 20 times, original and mirrored, with full-judge resends. Those peak at 88,999 units. Another 468 distinct saved responses are included in the measurement corpus. These are operational work counts, not physical clearance accuracy.
Guarded unit suites pass on this repair: 494 verification tests and 126 recon tests. All checks used
run-guarded.py --min-free-gib 8after the 10 GiB startup threshold was met. Fourteen judge regressions fail on 2d959c7, including oversized integers and unknown wall references. Bounded old-head controls also reproduce repeated unsure geometry, quadratic interval lookup and rejection of the valid direct-coverage result.Null
rules_assumedand null expectation query arrays now producebad inputrecords before counting. A regression saves both failed cases and a subsequent valid case in one report. The matching-public wrong-width and malformed-case controls fail ona7b3169. Public-policy review. Case metadata review.Repeated sweep and check expectation failures now produce one message per expectation, with a failure count, the reason once and at most three examples. A shared report budget retains at most 1,000 diagnostic entries and 262,144 serialized diagnostic characters, including duplicate JSON fields, and emits one omitted-count summary. Contract counts and acceptance are computed before display truncation; raw response payloads remain separate. Markdown-visible explanations receive budget priority over their duplicate JSON classifications. Both 1,000-error regressions retain the first explanation in the Markdown row and stay within the shared limits; both fail on
c32ee97. Numeric cache and interval-map keys use exact rational strings, preserving equal integer/float values while preventing cheap modulus collisions. Regressions cover 1,000 runs with a 4,096-character reason, oversized reasons, JSON escaping and 2,000 colliding integers across four key paths.The sampler count API now explicitly counts starts before the final endpoint;
run_startsyields those starts and then the endpoint. Pinneddb1e4c2controls confirm that the old implementation already checked the last pre-end start, checked a sub-step run's first start and charged each yielded sample. The new count-API assertions fail there; the functional sampling controls already pass. Sampler review 1. Sampler review 2.#22: packet: point to the server's capture packet 0.4 and propose 0.5 additions
#179: Ship beta to main: the app, server and experiments as submitted
The malformed-mesh finding refers to the retired Packet 1.1 parser. Commit
6a12700removed the parser, schema and tests; the current alpha tree contains only the two packet proposal documents. The finding is obsolete.Why
32 repaired findings were already marked resolved while their defects remained on alpha. The full 137-row triage is kept in the assigned external triage file. Existing fixes and deleted packet 1.1 code were checked against current source.
Verification
Scene/rule derivatives, wall lookups and ground plans are cached per judgment; collinear vertices within one wall no longer multiply geometry work. Direct coverage builds no ground fallback strips. Full judgments at 13 and 20 subdivisions fail at the old work limit before this repair and pass now with costs within 20% of the one-segment representation. Distinct evidence IDs and duplicate diagnostics use hash lookups; 12,000-ID and 4,000-width controls fail on the old head after 100,001 comparisons and pass with linear counts.
Metadata admission precedes schema validation, copies and resend comparison. It bounds walls to 50, vertices per wall to 500, runs to 30,000, checks and requests to 16,000 each, numeric leaves to 1,000,000, JSON nodes to 2,000,000, nesting to 32 and string/key characters to 4 MiB per document. Local case rules have 64 queries. Boundary controls verify clean admission and specific over-limit diagnostics.
Reported widths and corner allowances now share the same handling in both PRs. A case mismatch uses the response's reported value. Geometry adopts a validated reported override only for a different policy fingerprint. Matching public fingerprints retain the loaded rules and contract-check all reported battery dimensions, route height and corner allowance, including transformed responses. An unreported value is unverified when policy sources include private rules or the normalized rule fingerprint differs from the tested public rules, even if the assumed numeric value equals the public default. Four private-source/fingerprint controls fail on
30489f8before this repair. Encrypted NPZ flags now trigger locked cache repair; the damaged-file control fails before the repair with a password-required RuntimeError.The public-solver compatibility fixture has an 80-foot wall and 683 candidates. It is accepted with 29,760 actual visits and no ground-grid calls, while
2d959c7rejects it under its predicted budget. Both missing-evidence branches now compare one unsure check with 10,000 duplicates and use 130 units either way; the old branches invoke geometry 10,000 times. The remeasured valid peak is 391,700 units.-m pytest -qthroughrun-guarded.py --min-free-gib 8from the component folder.b5da197before the repair; it passes with the fix.check .andformat --check .in verification and recon;git diff --checkpassed.bash tests/makefile.shpassed all stub-based dispatch cases. It invoked no real build tools.Risk
The reused environment has OpenCV 5.0.0 while recon declares
<5. No dependencies were installed. These unit results do not establish the declared OpenCV range or real dataset acceptance. Model inference, ETH3D regeneration, iOS app builds, simulators and private backend behavior were not exercised. The companion live-dots app builds and all 45 Swift tests pass on #242 commit b6bc4ad. Both commands ran through run-guarded.py --min-free-gib 8 with two build jobs; Metal pixels and native interaction were not inspected.The archive admission limits reuse existing verifier policy, not measured capture maxima. Split archives and ZIP64 end-record metadata are explicitly refused. Cache and device locks are advisory POSIX locks; unrelated manual tools do not participate. Packet edits clarify unapproved proposals and do not change the upload contract.
Checklist
Verification accepts root
scene.jsonor a scene under one shared top-level capture folder, including the public API Finder fixture. Unlike the public API's root precedence, it refuses both forms together so the verifier has one unambiguous input. Every non-metadata entry must lie beneath the shared folder for the wrapped form. All entries still count toward archive limits. macOS metadata is ignored after the archive limits count every entry; duplicate and ambiguous scene paths still fail. The wrapped-capture regression fails at68cb685and passes here. Wrapped bundle review.Merge order
Merge #240 first, then #242. #242 contains #240 at
a7b3169through merge commitdaa36a2. Both include alpha3c33ad7, retain the same runtime work meter and ingestion bounds, and preserve the shared Makefile dispatch repairs. Neither branch was rebased or force-pushed.Companion fixes and remaining decisions
#242 carries the remaining executable repairs from this assignment: two-view free-space evidence (#20), rules-bound facade headroom, HTTP limits/deadlines, ordered simulator flow, monotonicity comparisons and accessibility identity (#13), and the anchor-local ground proposal (#22). It also fixes the follow-up review findings on those repairs. Historical replies remain drafted for the coordinated posting pass.
The genuine decisions remain ground reach, overhead footprint and photo-clearance uncertainty (#20), and per-capture meter yaw uncertainty (#22), as recorded in
DECISIONS.mdand the Opus decisions file. The app's evidence-replay policy (#18) is also still a decision on #242. The packet ground representation remains an unapproved proposal; #242 clarifies its coordinate frame without adopting a wire contract.Summary by CodeRabbit
Bug Fixes
Documentation