Skip to content

[obvious-fleet] test(backend): prove the finalize-to-orchestrator handoff over the API - #259

Merged
SamGu-NRX merged 1 commit into
alphafrom
obv/burst5-20261015-bs-hooks-integration
Oct 16, 2026
Merged

SamGu-NRX merged 1 commit into
alphafrom
obv/burst5-20261015-bs-hooks-integration

Conversation

@obvious-autobuild

Copy link
Copy Markdown

fleet-packet: burst5-20261015-bs-hooks-integration

What changed and why

One new test file, backend/server/api/tests/test_hooks_orchestrator.py; no production code changed. It proves the handoff from API finalize to the configured local orchestrator end to end, at the HTTP boundary.

Synchronous finalize vs the existing handoff. accept_packet (server/api/finalize.py) does only the fast checks inline — schema, ids, manifest-vs-committed-files — then schedule_finish hands the rest to a daemon thread (_spawn) so the 202 does not wait. On that thread, finish_ready promotes the files and, once per run id, calls safe_hook("on_finalized", ...). Which hook that is, is decided by resolve_hooks (server/api/hooks.py): HSS_INGEST_HOOKS=null returns NullHooks, so the thread promotes and validates but no run ever exists; HSS_INGEST_HOOKS=auto — the documented dev setting (SD-73: dev deploy runs it with HSS_ORCH_STAGE_STUBS=1) — returns OrchestratorHooks, whose _bind installs the API's state and storage as orchestrator services (services.bind) and calls the real server.orchestrator.start_run. start_run claims the run, writes the run/stage records into the API's state store, and drives the eleven stages through the coordinator inline (stubbed stages here), landing the outcome status and events on the capture the API serves.

Existing coverage and the gap. Every API test builds its client with HSS_INGEST_HOOKS="null"; api/tests/test_seams.py proves only the bind contract with a monkeypatched start_run; orchestrator/tests/test_local_fixture_proof.py proves the orchestrator CLI entry point, not the API seam. No test ran the real resolve_hooks → OrchestratorHooks → start_run → coordinator chain behind a finalize request. This closes that gap. Open PRs #251 (orchestrator review fixes) and #252 (retryable finalize handoff) touch production code in neighboring areas; this change duplicates neither.

How you verified it

All commands run in backend/ with uv run --frozen --extra api, memory/local backends, stage stubs, VISION_EXTERNAL_CALLS=off — no Modal, GPU, network or credentials:

  • pytest server/api/tests/test_hooks_orchestrator.py -q → 2 passed
  • pytest server/api/tests -q → 125 passed (no existing test regressed)
  • pytest server/orchestrator/tests -q → 156 passed
  • ruff check server/api/tests/test_hooks_orchestrator.py → clean; ruff format --check → clean
  • lint-imports → Contracts: 5 kept, 0 broken

The tests drive the committed data/fixtures/packets/mini-arkit/packet.json through the real FastAPI app with the documented hook mode: create capture, register/PUT/commit all 11 files, POST /finalize, then poll /events and /result. Nothing about the hook is mocked — the production seam (resolve_hooks, _bind, start_run, coordinator, outcome table) is the code under test.

Observable success state (HSS_INGEST_HOOKS=auto, stub verdict manual_review, outcome table row 14): finalize returns 202 processing with the deterministic runId (run_ + 16 hex of sha256(capture_id + NUL + packet bytes)); the capture lands on manual_review — equal to the run's outcome_kind, not stuck on processing; the result stage is done; the event feed ends with exactly one terminal verdict_ready; GET /result returns 200 with outcome.kind == "manual_review" and the same run id.

Observable failure state (the orchestrator's supported test knob HSS_ORCH_TEST_FAIL_STAGE=scale:compute_error:2, the same knob the orchestrator fixture proof uses): finalize still returns a 202, never a 500 — the failure lands off the request path as recorded data. Row 3 of the outcome table applies: exactly one failed event with code=stage_exhausted:scale:compute_error and next=manual_review, run status failed with that failure code, capture status manual_review, and GET /result still serves the manual result.

What is not verified

  • No Swift/iOS code touched or built (none exists in this change).
  • The real GPU stages (stubs ran instead), Modal remote spawn, GCS storage and modal_dict state backends: outside the stub path this task asked for.
  • The replay/sweeper/heartbeat paths and the sweeper's claim-recovery loop are untouched and untested here.
  • Nothing was deployed; this is a draft PR against the fixture branch.

Assumptions

  • The committed fixture is mini-arkit (formatVersion 0.4, API-valid, manifest-clean) rather than the orchestrator proof's synthetic-meter.tar: that tar's packet.json predates the API wire contract (no formatVersion, no session block) and finalize would reject it with a 422 before any handoff could be exercised.
  • The failure test uses the stage-exhaustion knob rather than making the hook itself raise, because safe_hook deliberately swallows hook exceptions (that is its own contract); stage exhaustion exercises the documented failure landing the task asked for.
  • The branch name and base are exactly as the task specifies: obv/burst5-20261015-bs-hooks-integration into obv/burst5-20261015-bs-orchestrator-fixture.
  • Process note: the single commit on this branch is authored as Sam Gu but is missing the workspace's Co-authored-by: obvious-autobuild[bot] trailer; adding it after the push would require a force-push, which this task forbids, so the commit is left as pushed.

HSS_INGEST_HOOKS=auto (the documented dev hook mode) drives two API-level
tests over the committed mini-arkit packet fixture with stage stubs: finalize
lands the run's manual_review outcome on the capture, and a controlled scale
stage exhaustion lands as a failed event plus recorded failure data instead of
a 500.
@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

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: 75357dc9-7305-47b8-b7a9-4e93be399785

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

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

@obvious-autobuild

Copy link
Copy Markdown
Author

Independent review (Obvious, Autobuild + Max)

Verdict: ready. I reproduced every claim in the body and could not break the tests with three targeted mutations of the code they guard. No blocking findings; two low, non-blocking notes below.

Findings (ranked; none blocking)

1. Low — head commit is missing the workspace's Co-authored-by trailer (disclosed in the body).
git log -1 --format='%(trailers)' 7085fe4d is empty; the single commit is authored as Sam Gu with no Co-authored-by: obvious-autobuild[bot] trailer, which the workspace convention requires. The PR body discloses this exactly and correctly explains that fixing it needs a force-push (forbidden for this task) and that a later commit cannot amend an earlier commit's trailer. No fix possible inside this PR's constraints; recording it so the process gap is visible, not as a change request.

2. Low (pre-existing, surfaced by this test) — intermittent un-awaited-coroutine warning in the prewarm path.
RuntimeWarning: coroutine '_Function.spawn' was never awaited appeared in 2 of my 3 runs that include this file (and in 0 of 2 runs of server/api/tests without it, which show exactly one warning — the pre-existing Starlette TestClient deprecation). It is GC-timing dependent and traces to the prewarm/modal path (server/orchestrator/prewarm.py, _spawn_call/_warm_spawns), which this PR's fixture is the first to drive through real on_capture_created hooks. Production code is unchanged by this PR, so the smallest correct fix belongs in a follow-up: make _spawn_call uniformly create_task the .aio variant (or dispose the sync coroutine object) so nothing is created-and-dropped.

What I verified

Environment: fresh clone of the repo at head 7085fe4d, backend/, uv 0.13.1, Python 3.12, all commands uv run --frozen --extra api ... — memory/local backends, no Modal, GPU, network or credentials, exactly as the body describes. The tree was verified clean (git status) after every mutation below.

Commands and results:

  • pytest server/api/tests/test_hooks_orchestrator.py -q → 2 passed (body: 2 passed ✓)
  • pytest server/api/tests -q → 125 passed (body: 125 ✓)
  • pytest server/orchestrator/tests -q → 156 passed (body: 156 ✓)
  • ruff check server/api/tests/test_hooks_orchestrator.py → all checks passed; ruff format --check → clean (body ✓)
  • lint-imports → Contracts: 5 kept, 0 broken (body ✓)
  • git diff --stat <base>..head → exactly one new file, backend/server/api/tests/test_hooks_orchestrator.py, +243; no production code, no workflow changes. Scope is right for the claim.

The tests exercise the real path — mutation evidence (each mutation applied locally, suite run, tree restored):

  • resolve_hooks forced to return NullHooks unconditionally (severs the handoff) → both new tests fail with finalize did not hand off to the orchestrator: no run record.
  • run_id_for_capture (server/contracts/jobs.py:248) hash input changed (NUL separator dropped) → test 1 fails on the deterministic runId assertion.
  • Outcome row 3's capture_status mutated manual_review → failed (server/orchestrator/outcomes.py:53) → the failure test fails on record.status == "manual_review".

Body claims spot-checked against source:

  • run_id_for_capture (server/contracts/jobs.py:240-248) implements exactly the derivation the test asserts (run_ + 16 hex of sha256(capture_id + NUL + packet bytes)).
  • finish_ready (server/api/finalize.py:347-354) starts the run once per run id; schedule_finish/_spawn (finalize.py:426-489) run the slow work on a tracked daemon thread, so the 202 genuinely does not wait.
  • resolve_hooks (server/api/hooks.py): "null" → NullHooks, otherwise OrchestratorHooks (the orchestrator.STUB guard is never assigned outside tests — grep clean), and _bind installs the API's state/storage via services.bind — the mechanism the body describes.
  • Outcome rows match the assertions: row 3 = stage_exhausted:{stage}:{code}, run failed, capture manual_review; row 14 = manual_review with verdict_ready.
  • HSS_ORCH_TEST_FAIL_STAGE is the same knob server/orchestrator/tests/test_local_fixture_proof.py:179 uses (body's "same knob" claim holds), and SD-73 (HSS_INGEST_HOOKS=auto + HSS_ORCH_STAGE_STUBS=1 in dev) is documented at backend/docs/components/deploy.md:59 and backend/docs/build/decisions.md:143.
  • The coverage-gap claim holds: every existing API client builds with HSS_INGEST_HOOKS="null" (conftest.py:29; the Settings default is also "null", server/common/config.py:94), and test_seams.py:132-148 proves only the bind contract with a monkeypatched start_run — nothing ran the real resolve_hooks → OrchestratorHooks → start_run → coordinator chain before this PR.
  • Fixture: backend/data/fixtures/packets/mini-arkit is committed (13 tracked files) and its manifest lists exactly 11 files, formatVersion 0.4, tier arkit — matches "register/PUT/commit all 11 files".
  • The mini-arkit-over-synthetic-meter assumption is sound: server/orchestrator/tests/data/packets/synthetic-meter.tar's packet.json has no formatVersion and no session, so finalize would 422 it before any handoff.
  • Flakiness check: ApiClient.request() drains finalize futures after every request (conftest.py:42-55, deps.drain_finalize) and the fixture sets app_state, so the post-finalize state and event assertions are deterministic, not racy.

What I couldn't verify

  • Real GPU stages, Modal remote spawn, GCS storage and modal_dict state backends — outside the stub path, as the body already states.
  • No backend test suite runs in CI on this PR (the checks page shows only the size label plus skipped bots), so "no regression" rests on the local runs above at head 7085fe4d — which I executed and which pass, including the full API and orchestrator suites.
  • Nothing was deployed; this remains a draft against the fixture branch, consistent with the body.

@SamGu-NRX
SamGu-NRX changed the base branch from obv/burst5-20261015-bs-orchestrator-fixture to alpha October 16, 2026 04:31
@SamGu-NRX
SamGu-NRX marked this pull request as ready for review October 16, 2026 04:31
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

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:05.670134Z 7085fe4 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.

@SamGu-NRX
SamGu-NRX merged commit 3aaa5ef into alpha Oct 16, 2026
6 checks passed
@codemetrics-ai

Copy link
Copy Markdown

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.

@greptile-apps

greptile-apps Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge.

Summary

Adds API integration tests for the real finalize-to-orchestrator handoff with local storage and stubbed stages.

  • Finalize tests now follow the API handoff through both outcomes.

Diagram

sequenceDiagram
    participant T as Test
    participant C as ApiClient
    participant A as API
    participant F as Finalize thread
    participant O as Orchestrator
    T->>C: Upload and commit fixture files
    C->>A: POST finalize
    A->>F: Schedule finish
    A-->>C: 202 processing
    F->>O: Bind API stores and start_run
    O-->>F: Record outcome and events
    C->>F: Wait for finalize job
    C-->>T: Return response
    T->>A: GET events and result
    A-->>T: Recorded outcome
Loading

Reviews (1) · Last reviewed commit: "test(backend): prove the finalize-to-orc..." · Reviewed by Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 effective changed lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant