Repository navigation
feat(orchestrator): dev-only scripted local outcomes for stage stubs - #291
obvious-autobuild[bot] wants to merge 4 commits into
Conversation
HSS_LOCAL_OUTCOME walks a stubbed local run to each state the app renders: manual_review (default, today's bytes), eligible, needs_more_photos, not_eligible (needs SHOW_REJECT_TO_HOMEOWNER=1), failed (validate fails onto outcome row 1). HSS_LOCAL_STAGE_DELAY_S paces stub stages 0-30s. Both are refused loudly outside dev stage stubs; scripted messages and the review page say the outcome is a local test, not a check of a wall.
…state fixtures Walk every HSS_LOCAL_OUTCOME state through the real API front door and commit the served payloads plus event streams as fixtures, validated against the orchestrator outcome table on every run. - config: keep OrchConfig field declarations in place (lost in an edit race) - stubs: restore scripted_outcome helpers in _common; terminal-event selection and fixture regeneration fixes in the test module - fixtures: one directory per rendered state (manual_review, eligible, needs_more_photos, not_eligible, failed) with result.json + events.json - README: correct where not-eligible refusal and setting validation happen
|
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 |
…re API base Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
|
@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. |
![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: 7d7d0dde1e
ℹ️ 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".
| ) | ||
| ctx.storage.put_bytes(key, body, "application/json") | ||
| run = ctx.state.get_run(ctx.run_id) | ||
| run.artifacts[key] = key |
There was a problem hiding this comment.
The failing stub updates a separate RunRecord copy and then raises. The coordinator handles the exception with its already-held run object, so _images_usable(self.s, run) cannot see this newly registered artifact and defaults to True; decide() consequently selects the validate-failed/images-usable row (manual review) instead of the intended row 1 (failed) for HSS_LOCAL_OUTCOME=failed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified against the current code: the row-1 outcome lands deterministically. The validate stage retries (max_attempts: 2), and both the first failure and the retry failure are decided on later scheduler ticks that poll the run record after the stub's put_run registered the artifact — so the capture ends failed with the device envelope manual_review/packet_invalid, which the focused test pins. 437a105 also moved the scripted validation bytes to the run-prefixed key runner._publish uses, so consecutive failed captures each keep their own evidence. The registration-timing robustness fix belongs in coordinator._read_json (re-check run.artifacts after the fresh-run reload); the exact change is noted in the PR body. Leaving this open for that coordinator-touching wave.
The scripted local-outcome stubs drift from the behaviors they stand in for, in four ways the review threads on PR #291/#293 name: - The scripted failed run registered packet/validation.json under the bare logical name, so a later failed capture overwrote the earlier run's evidence. The bytes now land at the run-prefixed key runner._publish uses, resolved through the run record's artifact map. - The stub read packet epochs as strings, but the committed mini-arkit packet stores epoch objects ({"id": "e1"}), so eligible placements never carried arkitEpoch/boxArkitWorld. Now read the ids the way the real composer feeds ar.epoch_id. - The scripted placement hardcoded the profile-C footprint; it now uses the configured profile's row in the real composer's FOOTPRINT table. - HSS_LOCAL_STAGE_DELAY_S=nan/inf now refuse by name as non-finite before the range comparison. Regression tests: run-scoped validation evidence across two consecutive failed captures, per-profile footprints for A/B/C, the finite refusal, and the eligible result carrying epoch e1 with an ARKit-world box. Committed app-state fixtures regenerated from the fixed stubs. Co-authored-by: Obvious <noreply@obvious.ai>
|
Pushed 437a105 fixing all four review findings on this PR plus the validation-artifact namespacing finding on #293:
Each fix has a failing-first regression test; focused suite 19 passed, ruff and lint-imports clean. Full @codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@greptileai review this at plus |
 [Medium impact] Dev-only scripted test outcomes for local orchestrator stubs. This appears safe to merge, with non-blocking fixes recommended for the local test paths.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| kind = "manual_review" if outcome == "manual_review" else outcome | ||
| ctx.state.append_event( | ||
| ctx.capture_id, | ||
| "verdict_ready", | ||
| {"runId": ctx.run_id, "kind": kind}, |
There was a problem hiding this comment.
With HSS_LOCAL_OUTCOME=needs_more_photos, the stub still emits verdict_ready. Both decide() row 13 and the real composer use retake_request, with the requested view IDs. Local runs therefore skip that event's handler, and the new tests and fixture lock in the difference. Emit retake_request for this outcome and compare the terminal event against decide() in the tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/stubs/result.py
Line: 81-85
Comment:
**Retakes use the wrong event**
With `HSS_LOCAL_OUTCOME=needs_more_photos`, the stub still emits `verdict_ready`. Both `decide()` row 13 and the real composer use `retake_request`, with the requested view IDs. Local runs therefore skip that event's handler, and the new tests and fixture lock in the difference. Emit `retake_request` for this outcome and compare the terminal event against `decide()` in the tests.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| """ | ||
| width, height, depth = FOOTPRINT.get(profile, FOOTPRINT["C"]) | ||
| size = [width, height, depth] | ||
| center = [0.6937, height / 2, depth / 2 + _STANDOFF_M] |
There was a problem hiding this comment.
The placement center still uses profile C's hardcoded width. For profile A or B, width is 0.6096, so startSM=0.3 requires a center of 0.6048. The current center, 0.6937, puts the box's near edge at 0.3889 instead. Calculate the center from the same start and width used in the returned placement.
| center = [0.6937, height / 2, depth / 2 + _STANDOFF_M] | |
| center = [0.3 + width / 2, height / 2, depth / 2 + _STANDOFF_M] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/stubs/result.py
Line: 150
Comment:
**Placement fields disagree**
The placement center still uses profile C's hardcoded width. For profile A or B, `width` is `0.6096`, so `startSM=0.3` requires a center of `0.6048`. The current center, `0.6937`, puts the box's near edge at `0.3889` instead. Calculate the center from the same start and width used in the returned placement.
```suggestion
center = [0.3 + width / 2, height / 2, depth / 2 + _STANDOFF_M]
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| def run(ctx: StageContext) -> StageResult: | ||
| if scripted_outcome() == "failed": | ||
| _script_failed(ctx) |
There was a problem hiding this comment.
HSS_LOCAL_OUTCOME=failed bypasses write_files(), which contains the only scripted sleep. Setting HSS_LOCAL_STAGE_DELAY_S therefore has no effect on this outcome: both validation attempts fail immediately. Developers cannot use the delay to see validation in progress before failure. Apply the shared delay before branching into _script_failed(), without sleeping twice on successful validation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/stubs/validate.py
Line: 15-17
Comment:
**Failed runs ignore the delay**
`HSS_LOCAL_OUTCOME=failed` bypasses `write_files()`, which contains the only scripted sleep. Setting `HSS_LOCAL_STAGE_DELAY_S` therefore has no effect on this outcome: both validation attempts fail immediately. Developers cannot use the delay to see validation in progress before failure. Apply the shared delay before branching into `_script_failed()`, without sleeping twice on successful validation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| stages = ", ".join(STAGE_DEPS) | ||
| return ( | ||
| "<html><body><h1>Local stage-stub run</h1>" | ||
| f"<p>Every stage here is a stand-in: stage stubs ran instead of real processing ({stages}).</p>" |
There was a problem hiding this comment.
The new review page says every stage was a stand-in and includes placement in that list. But placement has stubs_exempt=True, so the runner executes the real solver even when stage stubs are enabled. List the stages that were actually stubbed and identify placement as real CPU work, so someone checking what ran gets an accurate answer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/stubs/result.py
Line: 232-235
Comment:
**Review page mislabels real work**
The new review page says every stage was a stand-in and includes `placement` in that list. But `placement` has `stubs_exempt=True`, so the runner executes the real solver even when stage stubs are enabled. List the stages that were actually stubbed and identify `placement` as real CPU work, so someone checking what ran gets an accurate answer.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch has not been deployed
2 participants
Human author: Sam Gu (sgu07966@gmail.com)
What
A dev-only way to walk a local run into each terminal state the app renders, with no Modal or GPU: set
HSS_LOCAL_OUTCOMEto one ofmanual_review(default),eligible,needs_more_photos,not_eligible, orfailed, and the stage stubs script the run to that state.HSS_LOCAL_STAGE_DELAY_S(0–30 s) makes each stub sleep so the app shows processing events.HSS_LOCAL_OUTCOMEmanual_review(default)manual_revieweligiblecompleteneeds_more_photosneeds_viewsnot_eligiblecompleteSHOW_REJECT_TO_HOMEOWNER=1; without it the criteria stub refuses and the run fails loudly, leaving the capture onmanual_review.failedfailedimagesUsable: false).Both variables are refused loudly — with an error naming the variable, at config load — when stage stubs are off, the environment is not dev, the value is unknown, or the number is out of range. Scripted messages and review pages always say it is a local test outcome, not a check of a wall.
How it is pinned
tests/test_local_outcome_states.pydrives every state through the real API front door (create → upload → commit → finalize → events → result) and asserts capture status, served result, outcome kind, and the terminal event kind against the orchestrator outcome table (decide()). The same module regeneratestests/fixtures/app_states/ /{result,events}.json— committed evidence that is re-validated against the outcome table on every run. Refusals are verified at dispatch time (no run created, capture stays non-terminal, hook error logged). Thefailedstate's device envelope keeps outcome kindmanual_reviewwith messagepacket_invalid— capture failed, device sees the degraded review envelope.Review-thread fixes (437a105)
Fixes the four #291 review findings and #293's validate-artifact finding, each with a failing-first regression test:
failedrun registeredpacket/validation.jsonunder the bare logical name, so a later failed capture overwrote an earlier run's evidence. The bytes now land at the run-prefixed storage keyrunner._publishuses (captures/{capture_id}/runs/{run_id}/packet/validation.json), resolved through the run record's artifact map — no coordinator change needed. Regression: two consecutive failed captures each endfailedwith their own validation.json at their own key.{"id": "e1"}), so_packet_epochsreturned[]and eligible placements never carriedarkitEpoch/boxArkitWorld. Now reads the ids the way the real composer feedsar.epoch_id. Regression: the eligible result carriesarkitEpoch == "e1"with a non-null ARKit-world box (committed fixtures regenerated).FOOTPRINTtable (server/result/compose.py). Regression: eligible footprints for profiles A/B/C matchFOOTPRINT[profile].HSS_LOCAL_STAGE_DELAY_S=nan/infpassedfloat()and was refused only by the chained range comparison (with the misleading range message). Now refused by name as non-finite before the range check. Regression pins the finite-specific message.Coordinator hardening note (from the P1 thread, no coordinator edit in this PR): a first-attempt validate failure and a retry failure both decide on a later scheduler tick, so the row-1 decision is deterministic today (verified: attempt 1 fails →
_mark_errortick 1; retry fails → tick 2 polls with the artifact registered in tick 1'sput_run). The robustness fix, if wanted, is inserver/orchestrator/coordinator.py_read_json: in theexcept FileNotFoundErrorbranch after_ensure_run_fresh(run), re-check the run record once more before accepting absent:Verification
tests/test_local_outcome_states.py: 19 passed (was 14; +5 regression tests).test_capture_fixture_flow.py,test_runtime_join_journey.py): 22 passed total with the new suite.tests/: 201 passed, 1 skipped, 14 errors — the errors are the pre-existingload_receiptimport collection errors intest_capture_cold_restart.py, present on the alpha base (stacked-PR base artifact), not touched by this branch.ruff check/ruff format --checkclean on touched files;lint-imports5 kept, 0 broken.Draft — ready for review.
🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact