Repository navigation
feat(result): serve a reviewer page that stays judgeable past render time - #284
obvious-autobuild[bot] wants to merge 10 commits into
Conversation
…time The stored review.html is composed once but served many times, so baked signed URLs expire after HSS_INGEST_RESULT_URL_TTL_S and the three.js import points at a CDN — the page goes dark exactly when a reviewer opens it days later (#255 review finding). Now GET /v1/captures/{id}/review serves the stored page with, at serve time: - photo and raw-file links rewritten to reviewer-gated /review/files/ URLs (photos and verdicts/results only), so an expired URL never reaches the served page and keyframes load from live storage - the three.js import rewritten to /v1/review-static/three.module.js, a vendored pinned module, so the 3D viewer needs no network - a Run status section appended: run id, git SHA, per-stage status and error codes (stand-ins labeled), and the run's degraded codes Review files stay reviewer-only, 404 with result_not_ready when the run has no result yet, and reject ".." path segments. Tests render the real template by file path into volume storage and read the exact HTML back. Co-authored-by: Sam Gu <sgu07966@gmail.com>
The result stage is deterministic CPU work — verdicts, scene and the catalogue are all local — so it is stubs_exempt like placement: the local dev journey now publishes a real reviewer page instead of the 37-byte placeholder. That page is what a reviewer opens, and it discloses which stages were stand-ins, which is exactly what it is for. Also harden the TTL test sleep: tokens mint exp=int(now)+ttl with a strict `<` check, so expiry is only guaranteed once more than a full second has passed past the TTL — 2.5s raced that boundary. Co-authored-by: Sam Gu <sgu07966@gmail.com>
The helper accepted the flag but dropped it before building the StageSpec, so the result stage kept dispatching to its stub on the dev journey. Co-authored-by: Sam Gu <sgu07966@gmail.com>
The /review line in the integration guide now names what reviewers actually receive: rewritten artifact links, the vendored three.js module, and the injected run-status section. Co-authored-by: Sam Gu <sgu07966@gmail.com>
Real compose mixed with stub criteria verdicts changes the device-facing outcome (manual_review where the design says needs_more_photos), reshapes the internal record, and drops preview.ply when the stub scene has no points. The result stage returns to the stub path; real page delivery is proven by result-golden and the review-view tests instead. Co-authored-by: Sam Gu <sgu07966@gmail.com>
|
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 |
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. |
|
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. |
 [Critical impact] Rewrites review pages at serve time with token-gated file access. Do not merge until uploaded files cannot run scripts and normal gallery loads stop exhausting the reviewer’s request budget.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review-page browser subresources (photos, raw files, preview.ply, the
vendored three.module.js) cannot send the reviewer's bearer header, so
the page rewrote them into links only the middleware's bearer gate could
authorize — dead images for the reviewer whose page they belong to. The
routes now accept a short-lived signed review token (?rtok=..., the
upload-token HMAC machinery, bound to one capture and file, expiring
after HSS_INGEST_RESULT_URL_TTL_S) or the reviewer bearer; anonymous and
device requests are still refused, and the middleware exempts only those
route shapes, which enforce the pair themselves.
A deployment whose image lacks the vendored three.module.js no longer
rewrites the import to a same-origin 404: the page keeps its original
CDN import and injects a visible alert naming the missing file.
Stand-in labels in the run-status table come from the run's recorded
stage_stubs flag plus the graph's stubs_exempt stages instead of a guess
from status alone, so real work under stubs never reads as a stand-in.
The integration doc's review routes were corrected: /review/files/{name}
and a capture-free /v1/review-static/three.module.js are the shapes the
app serves. A regression test drives every documented review path past
Starlette's router.
Co-authored-by: Sam Gu <sgu07966@gmail.com>
|
@codex review |
![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: ee8863d259
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…o-secret page Regressions for the two open PR #284 findings, written to fail against the current code: - After an amendment starts a second run, a link minted for run 1 must serve run 1's bytes and a file run 1 lacks must be refused, not served from the later run. - With UPLOAD_URL_HMAC_SECRET unset, the page must keep its stored URLs and name the missing link secret instead of rewriting onto routes a browser cannot authenticate; in the secret-configured mode every subresource the page references must load with no bearer header. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
get_review_file and get_preview resolved the capture's latest run instead of the run named in the signed token, so after an amendment started a second run, an earlier link read the later run's files and an open page could show the wrong run. The artifact now resolves from the token's signed run, and a file that run lacks is refused with 404 result_not_ready instead of served from another run. A reviewer's own bearer fetch still follows the latest run. When UPLOAD_URL_HMAC_SECRET is unset (allowed for GCS/R2) no review token can be minted, but the page still rewrote the three.module.js import and the preview fetch onto the bearer-gated same-origin routes, which a browser module/fetch request cannot authenticate — the 3D preview died silently. The page now keeps its stored URLs, whose signed links still work while they live, and injects an explicit '3D preview unavailable: no link secret configured' alert instead of rewriting onto dead URLs. Regression tests cover both findings: an amendment flow whose run-1 link must keep returning run-1 bytes and refuse a file run 1 lacks, and a no-secret page sweep that fetches every subresource URL the page references with no bearer header in each mode. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
…viewer Dogfooding the page in a real browser showed 'Point cloud preview unavailable: unsupported property type x' for any standard preview.ply: the property-line destructuring read the NAME as the TYPE, so every 'property float x' threw. parseBinaryPly now reads [type, name] from the split line. test_parse_binary_ply_reads_standard_xyz_properties extracts the rendered page's parser and runs it in node against the header shape the scene pipeline writes (float xyz + uchar generated); it fails on the old destructure and passes on the fix. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cadd812bcd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if _token_secret(deps.settings) is None: | ||
| # The Codex finding: with no HMAC secret no review token can be | ||
| # minted, so a rewritten link would demand a bearer header a browser | ||
| # cannot send. Keep the stored page's original URLs — its signed | ||
| # links still work while they live — and say explicitly that the | ||
| # 3D preview will not load here. |
There was a problem hiding this comment.
When HSS_AUTH_MODE is none and UPLOAD_URL_HMAC_SECRET is unset, browser subresource requests are still accepted as the demo principal by authenticate, so the page can safely use the local preview and vendored module routes without a token. This unconditional early return instead leaves the CDN/relative URLs untouched and injects a misleading “3D preview unavailable” warning, causing the demo/local deployment to miss the offline viewer despite having no authentication barrier; gate this fallback on authentication actually requiring a bearer header.
Useful? React with 👍 / 👎.
|
@greptileai review this at apex |
| media_type = mimetypes.guess_type(safe)[0] or "application/octet-stream" | ||
| return Response(content=data, media_type=media_type) |
There was a problem hiding this comment.
The new file route can serve device-uploaded HTML as a page on the API origin. A device can upload evidence.html as other with application/octet-stream; the capture-folder fallback then returns its bytes as text/html. If a reviewer opens that file with reviewer authentication, its script can read any hss-review-token saved by the confirmation form. Force untrusted files to download, or restrict which files may render inline.
How this was verified: Upload rules allow .html names, promotion preserves the uploaded bytes, and this route chooses an inline content type from the filename.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/api/routes/result.py
Line: 553-554
Comment:
**Uploaded HTML can steal tokens**
The new file route can serve device-uploaded HTML as a page on the API origin. A device can upload `evidence.html` as `other` with `application/octet-stream`; the capture-folder fallback then returns its bytes as `text/html`. If a reviewer opens that file with reviewer authentication, its script can read any `hss-review-token` saved by the confirmation form. Force untrusted files to download, or restrict which files may render inline.
**How this was verified:** Upload rules allow `.html` names, promotion preserves the uploaded bytes, and this route chooses an inline content type from the filename.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| payload = _review_token(request, deps.settings, op="review-file", capture_id=capture_id, name=name) | ||
| if payload is not None: | ||
| deps.charge(str(payload.get("for") or "review-token"), "get", 120) |
There was a problem hiding this comment.
Every signed photo request consumes the reviewer’s shared 120-request get budget. A gallery with more than 120 photos, or several pages opened together, can exhaust it while images load. Remaining photos, preview.ply, and ordinary result/status reads then receive 429 responses. The image tags do not retry. Give page resources a separate, suitably sized budget, or pace and retry their requests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/api/routes/result.py
Line: 530-532
Comment:
**Photo loads block other reads**
Every signed photo request consumes the reviewer’s shared 120-request `get` budget. A gallery with more than 120 photos, or several pages opened together, can exhaust it while images load. Remaining photos, `preview.ply`, and ordinary result/status reads then receive 429 responses. The image tags do not retry. Give page resources a separate, suitably sized budget, or pace and retry their requests.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.This branch has not been deployed
2 participants
Summary
The reviewer page is composed once, at result time, and the artifact links baked into it are signed upload URLs with a TTL. After the TTL lapses — typically long before a human opens the page — every link dies and the page stops being judgeable. This PR makes
GET /v1/captures/{capture_id}/reviewserve a page that stays judgeable indefinitely, without weakening the signed-URL model that protects uploads.What changed
backend/server/api/routes/result.py) — when a reviewer fetches the page, the route rewrites every baked/v1/uploads/…signed URL (photos, raw artifacts) into reviewer-gated/review/files/{rel}links, so nothing on the page depends on a token that may have expired. Link targets are validated against the run's recorded artifact list; unknown rels 404, and path traversal (including URL-encoded segments) is rejected.backend/server/api/assets/three.module.js, three.js 0.160.0, MIT license header retained), so the preview loads in an offline/restricted environment./review,/review/files/*, and/review-static/*are all reviewer-token gated, same as before; device tokens do not gain access.backend/docs/INTEGRATION.md) — documents the rewrite behavior and the new routes.Verification
backend/server/api/tests/test_review_view.py, new) — 12 tests: reviewer-only access on all three routes, 404result_not_ready, expired-signed-link handling, photo + raw-file link rewriting, local three.js serving, injected stage/provenance fields, traversal rejection. The full API suite passes 4 consecutive runs (184 tests).lint-imports: 5 kept, 0 broken. Ruff check + format clean.make result-golden— synthetic, photo, and reject fixtures render and validate.~/work/bs-evidence, alpha4bcbedbavs branch head):/v1/uploads/references — every baked link rewritten to a gated/review/files/link — plus a same-origin three.js import and the injected run-status table.Review cap_…, zero broken images. (Honest scope note: the dev journey's scene is a stub, so the page shows no photos and reports the point-cloud preview as unavailable — the link rewriting for photos is proven by unit tests with real stills; the demo page proves the raw-file rewrite, the rewrite counts, and the run-status surface.)make test— 4 failed, 1,433 passed, 19 skipped, 14 errors in 8:27. The 4 failures areserver/eval/tests/test_gates.py(the expired SD-96 eval gates, documented pre-existing on alpha) and the 14 errors are the pre-existingtest_capture_cold_restart.pycollection errors, both identical on the alpha base4bcbedba. No failure or error originates from this PR.burst11-review-evidence.zip(Obvious filefl_kwyoz9EH, https://api.app.obvious.ai/prepare/files/link/fl_kwyoz9EH/burst11-review-evidence.zip). Mobile captures (390×844) are included in the bundle but excluded from the QA evidence upload, which enforces a 720px shorter-edge minimum.Not in scope (deliberate)
needs_more_photospath, internal record shape, unconditionalpreview.ply). A first cut briefly routed stub journeys through the real composer; it changed the device-facing outcome and six orchestrator tests caught it, so that wiring was reverted (b3045efb) and the evidence instead drives the real composer over the journey's artifacts, as the real result stage does.Review-thread fixes, round two (ebed0c8, cadd812)
Both unresolved P1 review threads are fixed on this branch; replies are posted inline and each finding names its line.
get_review_fileandget_previewresolved the capture's latest run instead of the run named in the signed token, so after an amendment started a second run, an earlier page's link read the later run's bytes, and a file that run 1 never produced was served from run 2. The artifact now resolves from the token's signed run; a file that run lacks is refused with404 result_not_readyinstead of cross-run content. A reviewer's own bearer fetch still follows the latest run. Regression tests cover an amendment flow whose run-1 link keeps returning run-1 bytes and refuses a file run 1 lacks.UPLOAD_URL_HMAC_SECRETunset (allowed for GCS/R2), no review token can be minted, but the page still rewrote the three.js import and the preview fetch onto the bearer-gated same-origin routes, which a browser module/fetch request cannot authenticate — the 3D preview died silently. The served page now keeps its stored URLs (whose signed links still work while they live) and injects an explicit "3D preview unavailable: no link secret configured" alert instead of rewriting onto dead URLs.parseBinaryPly's property-line destructuring read the property name as its type, so anyproperty float xthrew "unsupported property type x" and the preview always fell back to the error box. Fixed, with a node-backed regression test (test_parse_binary_ply_reads_standard_xyz_properties) that extracts the rendered page's parser and runs it against the header shape the scene pipeline writes.Local validation: focused suites
test_review_view.py17 passed andtest_review_page.py5 passed (the new amendment-isolation and no-secret tests fail on the pre-fix code); ruff check + format clean on changed files; import contracts 5 kept / 0 broken.Browser evidence (Playwright, headless Chromium; the page navigation carries the reviewer bearer, every same-origin subresource request has the Authorization header stripped — exactly a browser's position):
three.module.js,preview.plyfetch and verdict link all load with 0 console errors and 0 failed requests, and the point cloud draws. Screenshot: Obvious filefl_yHMYY799(https://api.app.obvious.ai/prepare/files/link/fl_yHMYY799/review-page-signed-token-mode.png).gh pr edithas no--attachflag in this gh, so the screenshots ride as project files linked here instead of being committed.fl_XirZUVSY(https://api.app.obvious.ai/prepare/files/link/fl_XirZUVSY/review-page-no-secret-mode.png).Full
make testafter the fixes — 4 failed, 1,523 passed, 19 skipped, 14 errors in 11:17 — the same pre-existing failure set as before (4 expired SD-96 eval gates + 14 cold-restart collection errors), with 90 more passing tests than the previous run from the merged alpha.🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact
Human author: Sam Gu (sgu07966@gmail.com)