Repository navigation
Conversation
The result composer reads every impl/<stage>.json marker the orchestration publishes into the result stage's inputs, refuses a marker without the pinned hss-stage-impl/v1 schema, and records stageImplementations plus a sorted standIns list in the internal record. The reviewer page names the stand-in steps next to the true decision. result.json is unchanged: the iOS decoder pins it. result/'s STAGE_INPUTS gains impl/ so the markers hydrate.
All real, two stand-ins, no impl dir, a marker without the pinned schema refused, and result.json byte-identical with and without impl files. The five fail against the pre-change composer.
|
@codex review |
|
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 |
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. |
![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: 8057732e68
ℹ️ 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".
| "verdicts/", | ||
| "reads/reads.json", | ||
| "scale/scale.json", | ||
| "impl/", |
There was a problem hiding this comment.
Adding impl/ to the result inputs only makes the composer read markers; this commit does not create them. run_stage publishes only the files listed in each stage's returned StageResult.artifacts, and neither the real stages nor the stub stages emit impl/<stage>.json, so normal orchestrated runs hydrate no implementation directory and _stage_implementations always reports every non-result stage as unknown with an empty standIns list. The new result metadata therefore never identifies which stages actually ran as stand-ins.
Useful? React with 👍 / 👎.
| "stageImplementations": stage_implementations, | ||
| "standIns": stand_ins, |
There was a problem hiding this comment.
When HSS_ORCH_STAGE_STUBS is enabled, stage_callable selects server.orchestrator.stubs.result.run for the result stage because result is not stubs_exempt; that stub writes its own result.internal.json and review.html and never calls compose_result. Consequently the newly added stageImplementations, standIns, and reviewer-page warning are absent on the very runs whose stages are stand-ins, even if implementation markers are supplied.
Useful? React with 👍 / 👎.
| implementation = doc.get("implementation") | ||
| if implementation not in {"real", "stand-in"}: | ||
| raise ResultError("impl_invalid", f"{path}: implementation is neither real nor stand-in") | ||
| implementations[str(doc.get("stage") or path.stem)] = implementation |
There was a problem hiding this comment.
For a malformed or stale marker whose stage value is a typo or arbitrary string, this assignment silently adds a new key to stageImplementations instead of rejecting the marker. The resulting internal record and review page can report a bogus stand-in while leaving the actual stage as unknown, undermining the trust guarantee described by this parser's schema validation; the stage should be checked against StageName (and ideally the filename) before insertion.
Useful? React with 👍 / 👎.
|
@greptileai review this at plus |
 [Medium impact] Adds stage implementation tracking to result composition. The PR appears safe to merge, with two non-blocking improvements to marker checks.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| implementation = doc.get("implementation") | ||
| if implementation not in {"real", "stand-in"}: | ||
| raise ResultError("impl_invalid", f"{path}: implementation is neither real nor stand-in") | ||
| implementations[str(doc.get("stage") or path.stem)] = implementation |
There was a problem hiding this comment.
The reader trusts stage without checking that it matches the filename or belongs to StageName. An objects.json marker naming reads as a stand-in can be overwritten by a later reads.json marker naming it real. The internal record and reviewer page then hide that stand-in.
Reject unknown or mismatched stage names with impl_invalid, and add a test for conflicting markers.
| implementations[str(doc.get("stage") or path.stem)] = implementation | |
| stage = doc.get("stage") | |
| if not isinstance(stage, str) or stage not in implementations or stage != path.stem: | |
| raise ResultError("impl_invalid", f"{path}: stage must be a known stage matching the filename") | |
| implementations[stage] = implementation |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/result/compose.py
Line: 374
Comment:
**A stand-in can disappear**
The reader trusts `stage` without checking that it matches the filename or belongs to `StageName`. An `objects.json` marker naming `reads` as a stand-in can be overwritten by a later `reads.json` marker naming it real. The internal record and reviewer page then hide that stand-in.
Reject unknown or mismatched stage names with `impl_invalid`, and add a test for conflicting markers.
```suggestion
stage = doc.get("stage")
if not isinstance(stage, str) or stage not in implementations or stage != path.stem:
raise ResultError("impl_invalid", f"{path}: stage must be a known stage matching the filename")
implementations[stage] = implementation
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| if implementation not in {"real", "stand-in"}: | ||
| raise ResultError("impl_invalid", f"{path}: implementation is neither real nor stand-in") |
There was a problem hiding this comment.
A marker containing "implementation": [] or an object raises TypeError at this set-membership check. It never reaches ResultError("impl_invalid", ...), so the failure loses both the marker filename and the promised error code. This makes the failed run harder to diagnose.
Check that implementation is a string before testing its value, and cover arrays and objects in the tests.
| if implementation not in {"real", "stand-in"}: | |
| raise ResultError("impl_invalid", f"{path}: implementation is neither real nor stand-in") | |
| if not isinstance(implementation, str) or implementation not in {"real", "stand-in"}: | |
| raise ResultError("impl_invalid", f"{path}: implementation is neither real nor stand-in") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/result/compose.py
Line: 372-373
Comment:
**Bad markers lose useful errors**
A marker containing `"implementation": []` or an object raises `TypeError` at this set-membership check. It never reaches `ResultError("impl_invalid", ...)`, so the failure loses both the marker filename and the promised error code. This makes the failed run harder to diagnose.
Check that `implementation` is a string before testing its value, and cover arrays and objects in the tests.
```suggestion
if not isinstance(implementation, str) or implementation not in {"real", "stand-in"}:
raise ResultError("impl_invalid", f"{path}: implementation is neither real nor stand-in")
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.This branch has not been deployed
1 participant
fleet-packet: burst14-bs-result-names-stand-ins
Human author: Sam Gu (sgu07966@gmail.com)
What changed
On a stubbed orchestration (
HSS_ORCH_STAGE_STUBS=1), a run still closes with a verdict, but some of its stages ran stand-in modules underserver.orchestrator.stubs— and nothing downstream said so. The stand-in result stage literally wrote<html><body>stub review</body></html>(server/orchestrator/stubs/result.py:52), so a partly stubbed answer was easy to mistake for a real one.Now the real result composer reads one
impl/<stage>.jsonmarker per run stage from its inputs (impl/joinedSTAGE_INPUTS["result"]inserver/orchestrator/graph.py), each carrying the pinned body{"schema": "hss-stage-impl/v1", "stage": ..., "implementation": "real"|"stand-in", "callable": ...}.result.internal.jsongainsstageImplementations(stage →real|stand-in|unknown;unknownmeans no marker, as for runs composed before markers existed) and a sortedstandInslist. The reviewer page names the stand-in steps next to the true decision.result.jsonis unchanged.Why
Reviewers and downstream consumers need to know which numbers came from real measurement work before they trust a verdict or decide a retake is worthwhile. The markers are written by the orchestration's stage runner (packet
burst14-bs-stage-selection, separate PR); this PR builds the reader side against that contract, so the two land independently.How it works
flowchart LR R[Stage runner] -- "one marker per stage" --> M[(impl/<stage>.json)] M -- "hydrated by 'impl/' input" --> C[Result composer] C --> I["result.internal.json<br/>stageImplementations + standIns"] C --> P["review.html<br/>'Stand-in steps: …'"]One line to notice: a stage with no marker reads as
unknown, so old runs compose unchanged.resultalways reads asreal— the composer runs inside it. A marker without the pinned schema (or with an implementation that is neitherrealnorstand-in) stops the composition with a loudimpl_invaliderror naming the file, because a mis-published marker would otherwise read as a trustworthy name.UI changes
The reviewer page, same screen, state and viewport (1440×900), composed from the committed synthetic fixture:
Before — the header jumps from the true decision straight to the failure code:
After — with
objectsandreadsmarked as stand-ins, the page says so in plain words:No video: this change adds a static text line; the page's point-cloud rotation predates it.
Verification
Tested at head
8057732e(basealpha@969d4e93), cloned fresh, all commands frombackend/:uv run --frozen --extra api pytest server/result/tests/test_stand_ins.py— 5 passed. Each of the five fails against the pre-change composer (verified by checking outHEAD~1ofcompose.py): all real; two stand-ins; no impl dir → every stage unknown; a marker without the pinned schema refused by name;result.jsonbyte-identical with and without markers.pytest server/result/tests— 71 passed.make result-golden— exit 0,ok synthetic photo reject; the regeneratedresult.internal.jsoncarries all eleven stagesunknownplusresult: real,standInsempty.uv run --frozen --extra api ruff checkandruff format --checkon the three changed files — clean.make testonce — 4 failed, 1511 passed, 19 skipped, 14 errors (674.98s, exit 2):Known pre-existing failures on alpha, named separately rather than fixed
The four gate failures are the known SD-96 expired-window cases (Sam decides that window), and the fourteen cold-restart errors are the pre-existing
load_receiptimport break. Both live outside this packet's paths and reproduce on plainalpha; no failure set difference was introduced.Not verified: no end-to-end run against a real orchestrated run publishing markers — the publishing side belongs to
burst14-bs-stage-selectionand is not open yet, so markers were exercised through test fixtures and the CLI--implflag. No Modal, GCS, R2, deploy, or real-capture data was touched, and no iOS build was run (no iOS change). The optional app-facing field below is a proposal only; nothing app-visible changed.What's left: the marker publisher PR (
burst14-bs-stage-selection), the SD-96 window decision, and the PR itself — opened by the maintainer under their account; this branch is pushed and stops there.App-facing field proposal (not implemented)
result.jsonstays byte-identical because the iOS decoder pins its shape (PR #285,test_capture_device_decoder_contract.py). The additive change I would propose for the app: two optional top-level fields,stageImplementations(same object as the internal record) andstandIns(sorted array), both absent-safe so old payloads decode unchanged. Screen copy on the result screen, under the decision banner, as a dim caption: "Parts of this scan ran in reduced mode (objects, reads), so their results are not real measurements." — shown only whenstandInsis non-empty.Alternatives considered
RunRecord.degraded/ stage statuses. Rejected:degradedconflates "never ran" with "ran a stub", and the state store is not part of the result stage's inputs — the composer would guess where it should read.result.json. Rejected for now: that shape is pinned by the iOS decoder contract, and a reviewer-only need does not justify risking the app. Proposed as an additive optional field above instead.unknownfalls out naturally for runs composed before markers existed, and the two packets land independently.Risk
Low.
result.jsonis byte-identical (pinned test), the reviewer line renders only whenstandInsis non-empty, and a run with noimpl/input composes exactly as before. The one new failure path — a malformed marker stopping composition — is deliberate: a run whose provenance cannot be proven should close for manual review, not sail through as a real answer. The 14 cold-restart collection errors and 4 SD-96 gate failures in the full suite pre-date this change.Checklist
🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact
Opened by the maintainer from the Obvious thread's own description, so every review bot reviews it (CodeRabbit skips PRs authored by bots). The work and its commits are by Obvious.
Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>