[obvious-autobuild]

Summary

Addresses four Greptile review findings on PR #247, each with a regression test that fails without the fix:

1. Review page dead links (server/result/compose.py) — review.html is served under /v1/captures/{id}/review, so its relative hrefs to verdicts and result.json 404, and photo paths point at expired uploads/ keys. Now photos resolve to the promoted captures/{capture_id}/... key and get signed URLs when storage is available; raw artifact links are signed under the capture/run prefix, falling back to bare names offline.
2. Packet still paths (server/reads/aggregate.py) — build_reads passed packet-relative paths (s1.jpg) straight to storage, but promotion stores files under captures/{capture_id}/... and uploads expire. Paths are now resolved against the promoted prefix before read_still.
3. C2 join false agreement (server/criteria/c2/veto.py) — with zero matched positions the join reported agreement_with_c1=1.0 (perfect agreement with no evidence). It now reports positions_compared and agreement_with_c1=null when nothing matched, threaded through the criteria report.
4. Orchestrated runs lose reads/scale (server/result/compose.py) — compose.run() only read the short input keys (reads/scale), but the orchestrator hydrates reads/reads.json and scale/scale.json under work_dir. A fallback now resolves the hydrated paths, so meter auto-fill and scale attribution stop being dropped.

Testing

• New regression tests: test_review_page_signs_photos_and_raw_links, test_run_resolves_reads_and_scale_from_work_dir, test_build_reads_resolves_packet_paths_against_promoted_files, test_join_without_matches_reports_no_comparison — all four fail on the pre-fix source and pass with it.
• Touched suites green: 196 passed, 1 skipped (result, reads, criteria).
• Full suite: 5 failures, all pre-existing and identical with and without these fixes (1 SSR overlap hygiene test, 4 eval-gate tests).
• make lint green (ruff check, ruff format, lint-imports).

Handoff

• Draft PR into feat/port-house-scanning-server; branch obv/burst4-20261015-bs-backend-results, commit 6cb76b17, author Sam Gu.
• Not merged; no deploy. CI signals to follow.

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 93c2a737-5b59-494c-a988-a45eaf853100

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[obvious-autobuild]

Review: fixes verified; one real limitation, two nits

All four fixes are mechanically correct and the regression tests are real. Full suite is green apart from the 5 failures the body names, which I reproduced identically on baseline 2d0911f8. Nothing blocking for draft → ready; one limitation below is worth a follow-up ticket.

What I ran

Check	Command	Result
Touched suites	uv run --frozen --extra api pytest server/result/tests server/reads/tests server/criteria/tests -q	240 passed, 1 skipped (96s)
4 new regression tests @ head 6cb76b17	pytest node IDs	4 passed
4 new regression tests, fix reverted to base	git checkout origin/feat/port-house-scanning-server -- <4 source files> then same tests	4 failed — the tests catch the bugs
Lint	ruff check . · ruff format --check . · lint-imports	all green, 5 contracts kept
Full suite @ head	uv run --frozen --extra api pytest -q	5 failed, 1207 passed, 17 skipped (4m58s)
Baseline 2d0911f8	same 5 failing node IDs	identical 5 failures — pre-existing

The 5: tests/test_ssr_overlap_check.py::test_repo_has_no_ssr_shingle_overlap + 4 in server/eval/tests/test_gates.py — matches the body.

Count nit: the body's "196 passed, 1 skipped" doesn't reproduce. My rerun of the three touched suites gives 240 passed, 1 skipped (57 result + 67 reads + 116 criteria + 1 skip) — likely the body counted a C3-off criteria run. Green either way.

Claim checks

• Packet paths: the reads fix reads captures/{capture_id}/{path}, which matches final_key, and server/api/routes/files.py::_path_ok rejects .., //, leading /, and non-normalized paths at registration — the prefix can't traverse, including for attacker-controlled packet filenames.
• Internal result on the review page: the page is reviewer-gated (403 otherwise, per GET /v1/captures/{id}/review), so signing result.internal.json/verdict links there is consistent with the API's reviewer path; the device-facing result route still strips both.
• Dead-link premise: no route serves the old relative targets (/v1/captures/{id}/verdict.C.json etc.) — pre-PR links were genuinely 404.
• CLI path: compose_result(storage=None) keeps the offline bare-name fallback.
• agreement_with_c1: None: no consumers outside the changed code/tests; None serializes to null in criteria_report.json (no schema pins it as number).

Findings

1. Signed URLs baked into the persisted page expire (follow-up worth filing). review.html is written once at compose time with HSS_INGEST_RESULT_URL_TTL_S TTL (900s default); GET /v1/captures/{id}/review serves that stored file unchanged, and the GCS/R2 signed_get_url implementations honor the TTL. So photo/verdict/raw-file links die ~15 minutes after compose — while manual review is precisely the flow that opens the page late. Strictly better than the pre-PR dead links, but the durable shape is serve-time signing (the route already holds storage + settings).
2. Silent signing fallback (low). _signed_url swallows every exception into the bare name; a signing misconfiguration degrades to dead links with no log line. A log_event warning on failure would keep that failure visible.

Both non-blocking. Nice catch on _review_links names/keys staying zip-aligned via the same if name in verdicts filter — subtle and easy to get wrong.

[obvious-autobuild]

Independent review (Obvious, Autobuild + Max)

Verdict: ready (draft → ready for review). I found no blocking defect. Three non-blocking findings below, plus one inaccuracy in the body's test counts. I did not write this PR; I verified every claim below by running the code myself at head 6cb76b17 against base 57f57ff8.

Findings (ranked)

1. Signed URLs baked into the persisted review.html expire — follow-up worth filing. server/result/compose.py:819 (_signed_url) bakes signed_get_url(..., HSS_INGEST_RESULT_URL_TTL_S) URLs into the HTML at compose time (default TTL 900s, server/common/config.py:89), and GET /v1/captures/{id}/review (server/api/routes/result.py:107-130) serves the stored bytes unchanged. So photo/verdict/raw-file links die ~15 minutes after compose, while manual review is exactly the flow that opens the page late. Strictly better than the pre-PR state (all links 404), so not blocking; the durable shape is serve-time signing — the route already holds storage and settings, same as it does for verdictUrl/previewUrl at routes/result.py:87,93. Smallest fix: re-sign the links (or store bare keys and sign at serve time) in get_review.
2. Silent signing fallback (low). compose.py:819-826: _signed_url catches every Exception and returns None, so a signing misconfiguration degrades to bare-name dead links with no log line. Smallest fix: emit log_event("review.sign_failed", key=key, level="warn") in the except branch (the module already logs elsewhere in the codebase; server/common/logging.py::log_event).
3. Hardcoded promoted-prefix duplicates final_key (low). server/reads/aggregate.py:305-309 hardcodes f"captures/{capture_id}/{path}" (comment: server.api pulls in the FastAPI app). I confirmed it matches server/api/keys.py:10-11 today, but nothing ties the two together — if the layout ever changes, reads silently misses every still. Smallest fix: move final_key (and friends) to a leaf module importable without the app, or add one test asserting the literal equals keys.final_key(capture_id, path).

Claim checks (evidence)

• Review-page links: the pre-PR bare names were genuinely dead — no route serves /v1/captures/{id}/verdict.C.json-style paths. The new keys in _review_links (captures/{capture_id}/runs/{run_id}/verdicts/verdict.{A,B,C,S}.json, verdicts/criteria_report.json, result/result.json, result/result.internal.json) match the publisher exactly: server/orchestrator/runner.py:63-65 (prefix = captures/{ctx.capture_id}/runs/{ctx.run_id}) + server/orchestrator/artifacts.py:52-63 (CONTRACT_ARTIFACTS names). Verified end-to-end by test_review_page_signs_photos_and_raw_links.
• Signing result.internal.json on the review page is consistent with access control: the review route is reviewer-gated (routes/result.py:112-113, 403 otherwise); the device-facing /result route still strips internal/verdict references (_strip_hidden, routes/result.py:17-30).
• Packet still paths: server/reads/aggregate.py now prefixes captures/{capture_id}/, which matches final_key, and the only consumer of StillRef.path fetches it as a storage key (server/reads/worker.py:153: storage.get_bytes(still.path)) — the prefix is exactly what that fetch needs. uploads/ keys are already gone post-promotion, so the old code could only fail. The packet loop runs only when a packet is passed (build_reads(..., packet=None) callers unaffected). No regression path found.
• C2 join: join() has a single return path that now always reports positions_compared, and run.py reads it in the same statement — safe. The zero-match case reports agreement_with_c1=None instead of a false 1.0; no consumers outside run.py/tests.
• Hydrated reads/scale: reads/reads.json and scale/scale.json are real contract artifacts (artifacts.py:53,59), hydrated into work_dir, and _stage_input keeps the short-key precedence — no behavior change when the short keys are present.
• No HTML injection via packet-controlled names: review/render.py:13 uses jinja2 select_autoescape(["html"]), and photos/links interpolate via {{ }} (template.html:29,143).

What I ran (all on head 6cb76b17 in a clean worktree; base worktree 57f57ff8)

Check	Command	Result
Touched suites	uv run --frozen --extra api pytest server/result/tests server/reads/tests server/criteria/tests -q	240 passed, 1 skipped (91.9s)
4 new regression tests @ head	pytest node IDs	4 passed (0.83s)
Same tests, fix reverted	git checkout 57f57ff8 -- server/criteria/c2/veto.py server/criteria/run.py server/reads/aggregate.py server/result/compose.py, then same node IDs	5 failed (1.03s) — the 4 new tests plus the modified test_packet_uses_img_and_file_digest all fail on pre-fix source; tree restored clean after
Lint	ruff check . · ruff format --check . · lint-imports	All green; 459 files formatted; 5 contracts kept, 0 broken
Full suite @ head	uv run --frozen --extra api pytest -q	5 failed, 1207 passed, 17 skipped (270.7s)
Full suite @ base 57f57ff8	same	5 failed, 1203 passed, 17 skipped (284.5s) — identical failure set: tests/test_ssr_overlap_check.py::test_repo_has_no_ssr_shingle_overlap + 4 server/eval/tests/test_gates.py nodes. Pre-existing, exactly as the body claims; head is base plus 4 passing regression tests

One body inaccuracy: "Touched suites green: 196 passed, 1 skipped" does not reproduce — the three touched suites give 240 passed, 1 skipped. Green either way; the 196 likely counts a C3-off criteria run.

What I could not verify

• Live GCS/R2 signed-URL behavior — verified through the code path and a signing storage double only; no cloud credentials here.
• Anything the local full-suite runs don't cover (real device packets, Modal workers).
• CI: this PR has no backend test workflow signal (label bot + skipped CodeRabbit only) — the known repo-wide gap that server/ CI doesn't cover backend/.

[codemetrics-ai]

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.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-16T04:33:07.102078Z	6cb76b1	Draft marked ready

ℹ️ 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" or "@codex security review".

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]

Confidence Score: 3/5

[Medium impact] Fixes data structure and file path handling in criteria and results.

Fix the two remaining review-page failures before merging.

Findings

1.  Saved review links expire ▶
2.  Production photos stay broken ▶

Fix with agent prompt

### Issue 1
backend/server/result/compose.py:819-821
The signed URLs expire after the result stage creates `review.html`, not after a reviewer opens it. The default lifetime is 900 seconds, and `get_review` returns the saved HTML unchanged. Reviewers opening the page later get expired photo and raw-file links, even after reloading. Sign the URLs when serving the page, or use stable authenticated download routes.

### Issue 2
backend/server/result/compose.py:860-865
The photo fix skips production keyframes. `_retarget_keyframes` creates them without an `image` field, and the scene stage saves them without adding one. Signing is therefore skipped, and the page still uses the bare `kf["id"]` as its photo URL. Carry the packet image path into the scene, or resolve it from the packet before signing. Add a test using the production keyframe shape.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR fixes promoted still paths, adds hydrated reads and scale inputs, and reports no C2 agreement when nothing was compared. It also adds signed review links and regression tests.

Two review-page problems remain:

• Review pages link to stored photos and run artifacts.
• Result runs load reads and scale files from their work folder.
• Packet stills are read from their promoted capture paths.
• C2 reports no agreement when it compared no positions.

_{Reviews (1) · Last reviewed commit: "fix(backend): address review findings on..." · Reviewed by Greptile}

[greptile-apps]

Saved review links expire

The signed URLs expire after the result stage creates review.html, not after a reviewer opens it. The default lifetime is 900 seconds, and get_review returns the saved HTML unchanged. Reviewers opening the page later get expired photo and raw-file links, even after reloading. Sign the URLs when serving the page, or use stable authenticated download routes.

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/server/result/compose.py
Line: 819-821

Comment:
**Saved review links expire**

The signed URLs expire after the result stage creates `review.html`, not after a reviewer opens it. The default lifetime is 900 seconds, and `get_review` returns the saved HTML unchanged. Reviewers opening the page later get expired photo and raw-file links, even after reloading. Sign the URLs when serving the page, or use stable authenticated download routes.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[greptile-apps]

Production photos stay broken

The photo fix skips production keyframes. _retarget_keyframes creates them without an image field, and the scene stage saves them without adding one. Signing is therefore skipped, and the page still uses the bare kf["id"] as its photo URL. Carry the packet image path into the scene, or resolve it from the packet before signing. Add a test using the production keyframe shape.

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/server/result/compose.py
Line: 860-865

Comment:
**Production photos stay broken**

The photo fix skips production keyframes. `_retarget_keyframes` creates them without an `image` field, and the scene stage saves them without adding one. Signing is therefore skipped, and the page still uses the bare `kf["id"]` as its photo URL. Carry the packet image path into the scene, or resolve it from the packet before signing. Add a test using the production keyframe shape.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.