Repository navigation
[obvious-fleet] Orchestrator: prove the deterministic local stub path on a committed packet - #253
Merged
Merged
Conversation
…itted packet One committed synthetic packet (tests/data/packets/synthetic-meter.tar) drives run-local — the supported local entry point — with all eleven stage stubs, no Modal, GPU, network or credentials. Asserts the run, eleven done stage records, one terminal verdict event, the API-consumed result artifacts, byte-identical determinism across two runs, and that an exhausted stage lands as recorded failure data (manual result published, failed event emitted) instead of a coordinator crash.
Previous commit f0df5cd (test(orchestrator): prove the deterministic local stub path on a committed packet) is this same change. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.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 |
Author
|
Independent review (Obvious, Autobuild + Max) Verdict: Findings (ranked by severity)
No code findings. Correctness, failure handling, scope, and safety check out (below). What I verified (fresh clone, head
|
SamGu-NRX
changed the base branch from
feat/port-house-scanning-server
to
alpha
October 16, 2026 04:29
SamGu-NRX
marked this pull request as ready for review
October 16, 2026 04:29
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. |
 [Low impact] Adds test data and a test for the local orchestrator stub. The PR appears safe to merge, with two non-blocking gaps in what the new tests enforce.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+52
to
+56
| def _forbidden(*args: Any, **kwargs: Any) -> bool: | ||
| touched.append("modal_available") | ||
| return False | ||
|
|
||
| monkeypatch.setattr("server.orchestrator.cli.modal_available", _forbidden) |
There was a problem hiding this comment.
The no_modal fixture patches cli.modal_available, but run-local reaches Modal through modal_spawn.launch_modal, which imports modal_functions.modal_available directly. The guard cannot detect that path. If a developer's shell sets HSS_ORCH_MODAL_REMOTE=1, these tests can attempt a remote call despite the guard. Patch the function used by launch_modal and raise immediately if called; also pin remote execution off for these local tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/tests/test_local_fixture_proof.py
Line: 52-56
Comment:
**Modal guard misses remote calls**
The `no_modal` fixture patches `cli.modal_available`, but `run-local` reaches Modal through `modal_spawn.launch_modal`, which imports `modal_functions.modal_available` directly. The guard cannot detect that path. If a developer's shell sets `HSS_ORCH_MODAL_REMOTE=1`, these tests can attempt a remote call despite the guard. Patch the function used by `launch_modal` and raise immediately if called; also pin remote execution off for these local tests.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+164
to
+166
| key: hashlib.sha256(_stored(out, stored)).hexdigest() | ||
| for key, stored in run["artifacts"].items() | ||
| if key in {"verdicts/verdict.C.json", "result/result.internal.json", "scene/scene.json"} |
There was a problem hiding this comment.
The digest comparison filters the keys already present in run["artifacts"], so it silently skips missing files. If both runs omit result/result.internal.json or scene/scene.json, the test still passes without comparing those promised outputs. Iterate over the three required keys and index each one directly so a missing file fails the test.
Suggested change
| key: hashlib.sha256(_stored(out, stored)).hexdigest() | |
| for key, stored in run["artifacts"].items() | |
| if key in {"verdicts/verdict.C.json", "result/result.internal.json", "scene/scene.json"} | |
| key: hashlib.sha256(_stored(out, run["artifacts"][key])).hexdigest() | |
| for key in ("verdicts/verdict.C.json", "result/result.internal.json", "scene/scene.json") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/tests/test_local_fixture_proof.py
Line: 164-166
Comment:
**Missing files pass the comparison**
The digest comparison filters the keys already present in `run["artifacts"]`, so it silently skips missing files. If both runs omit `result/result.internal.json` or `scene/scene.json`, the test still passes without comparing those promised outputs. Iterate over the three required keys and index each one directly so a missing file fails the test.
```suggestion
key: hashlib.sha256(_stored(out, run["artifacts"][key])).hexdigest()
for key in ("verdicts/verdict.C.json", "result/result.internal.json", "scene/scene.json")
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fleet-packet: burst5-20261015-bs-orchestrator-fixture
What changed and why
Proves the orchestrator's deterministic local path on one committed capture packet, through its supported local entry point —
python -m server.orchestrator.cli run-local— with stage stubs enabled and no Modal, GPU, network or credentials. Two additions, no production code touched and no defaults altered:backend/server/orchestrator/tests/data/packets/synthetic-meter.tar— a 10 KB committed synthetic packet (deterministic tar, mtime 0): one keyframe, one meter still, meter tap, scale reference. Built following the repo's tracked-fixture precedent (reads.stub.jsonalso lives under the ignoredtests/data/path).backend/server/orchestrator/tests/test_local_fixture_proof.py— three tests:test_committed_packet_runs_to_a_terminal_verdict— the packet produces a run (status=done,outcome_kind=manual_review,failure_code=None,packet_sha256matching the stored packet bytes), stage records for every stage reachingdone, exactly one terminal event (verdict_ready), and the result artifacts the API consumes (run.artifacts["result/result.json"]fetched from storage and validated asResultResponse, plusreview.html,preview.ply,verdicts/verdict.C.json). A spy onrunner.stage_callableasserts the stubbed stages from the actual execution trace.test_same_packet_twice_is_deterministic— two runs of the same committed tar agree on all eleven stage outcomes and on the SHA-256 ofverdicts/verdict.C.json,result/result.internal.jsonandscene/scene.jsonbytes.test_stage_failure_is_recorded_data_not_a_crash— with the supported injection seamHSS_ORCH_TEST_FAIL_STAGE=scale:compute_error:2,run-localstill exits 0, the run recordsfailure_code=stage_exhausted:scale:compute_errorwithoutcome_kind=manual_review, thescalestage record showsfailedafter retries, exactly one terminalfailedevent is emitted, and the manual-result path publishes a schema-validresult/result.json— failure as data, not an uncaught coordinator crash.Every stubbed stage is named: validate, poses, objects, reads, scale, dense, coverage, scene, objects_3d, criteria, result — the eleven of
graph.STAGE_NAMES, each served byserver/orchestrator/stubs/<stage>.py, asserted equal to the traced stub set in the test.How I verified it
Worked in a fresh clone on a shared repo sandbox (
cmp_rISlQZhQ, the agent-routing-kit sandbox; its GitHub token covers SamGu-NRX repos). Baseline before my change:153 passed. After:uv run --frozen pytest server/orchestrator/tests -qHSS_ORCH_STAGE_STUBS=1 uv run --frozen pytest server/orchestrator/tests -qmake lint(ruff check, ruff format --check, lint-imports)The stubs-enabled suite is the honest sandbox proof, compared with attempting the real GPU pipeline:
server/orchestrator/stubs/<stage>.pyfor all eleven stagesserver.<stage>.runreal modulesInlineExecutor)The stub path is the honest proof here because the claim under test is the orchestrator's local path — that a finalized packet yields a run, stage records, a terminal outcome, and API-consumable artifacts, and that stage errors are recorded rather than crashing the coordinator. Those are coordinator properties, and the stubs exercise the real coordinator, real
StateStore/LocalFsStorage, the real events and outcomes table, and the real runner publish path (_publishwrites actual bytes intocaptures/{cap}/runs/{run}/…). Swapping in the real GPU pipeline would not strengthen any of those assertions; it would add Modal credentials, network, GPU cost and nondeterminism, none of which this sandbox can honor.run-modalwas not called (a Modal token absent, b the test patchesmodal_availableto fail the run if anything reaches for Modal);make fixtures(network fetch of.fixtures/tars) was not run — the packet is committed instead.What is not verified
server.validate,server.poses, … real reconstruction, scene merge and criteria on a real capture. Stub verdicts are alwaysmanual_review, so a realclear/retakeoutcome path through the API is unverified here.run-modalagainst a deployed Modal app (modal_functions.remote), prewarm/heartbeat under real Modal latency, and the sweeper's stale-run recovery in a multi-container deployment.make ciend to end (schemas, hygiene, evals) — the orchestrator suite and backend lint were run targeted, per the task; the known pre-existing failures on the base (e.g.server/evalgates per PR fix(backend): address Greptile review findings on the orchestrator #251's notes) were not re-run.Assumptions
cli.run-local(the README's "Run locally" path). The tests callmain([...])in-process, which is the same code path as the module invocation..fixtures/packets/eth3d-electro.taris network-fetched bymake fixtures, so it cannot anchor a hermetic test.HSS_ORCH_TEST_FAIL_STAGE, as existing tests do) rather than monkeypatching coordinator internals — no production defaults changed, env only.packet_uri, confirmation hook, retryentered_at, verdict profile) was read and deliberately not re-fixed or re-tested here; this PR is additive and touches none of its files.tests/data/path follows the tracked-file precedent (reads.stub.json,backend/data/fixtures/**), avoiding a.gitignorechange.Co-authored-by: obvious-autobuild[bot]trailer, since the content commit f0df5cd was already pushed and force-push is not allowed.