feat(byok): add durable User Runner inference - #712
Conversation
|
Thanks for the substantial work here. The overall architecture is heading in the right direction: the Server retains the single Agent Loop and durable invocation ledger, while the Runner owns credentials, provider transport, dispatch fencing, and terminal custody. However, I do not think this PR is ready to merge yet. I found three blocking issues: 1. Friendly model names are confused with provider model IDs
After setup, however, the TUI searches the catalog using entry.name.eq_ignore_ascii_case(&model_name)For a normal configuration such as: the model can be saved, probed, and published successfully, but the TUI will fail to find it, time out after 12 seconds, and not select it. Please carry the friendly/display name separately in the binding projection and complete setup using a stable binding or Offering identity—not a name lookup. This is also necessary to avoid selecting the wrong model when two Runners publish the same name. 2. The multi-terminal Runner lifecycle does not satisfy the stated isolation contractEvery interactive CLI invocation starts a new Environment credentials are attached only inside the process that successfully owns the inference host; there is no IPC path for another terminal to attach its own credential to that host. Consequently, two terminals using different values for the same environment-variable name may either:
That violates UX-07 and may cross provider-account or billing boundaries. This needs either:
Simply spawning one child per CLI while sharing the same Runner identity and exclusive journal is not sufficient. 3. The TUI performs a real provider request without an explicit pre-submit choiceThe setup form says: but submission automatically calls Even though the request is small, it may still incur cost or trigger provider-side audit activity. This also conflicts with UX-03 in the design document. The form should explicitly offer:
Additional implementation concerns
The durable execution internals are otherwise thoughtfully designed: exact-attempt authority is persisted before provider I/O, the Runner writes a local fence before dispatch, terminal evidence remains in custody until Server ACK, uncertain delivery is represented explicitly, and the queues/artifacts/journals are bounded. My recommendation is Request changes until the three blocking issues above are fixed. At minimum, please add end-to-end tests covering:
|
|
Follow-up review after Thanks for the two follow-up commits. They address several points from the previous review well:
I re-reviewed the updated implementation beyond those original findings. I still recommend Request changes, primarily for the first two issues below. P0 — the provider credential is inside the general tool-execution environmentThe managed Runner is spawned without Foreground This violates the central boundary that provider credentials never reach Server/model context, and also conflicts with the design gate that an inference-only Runner must not expose shell access. Please put inference credentials in an inference-only process/slot that the general tool executor cannot read, and clear/allowlist the environment of every tool subprocess. Add an end-to-end canary test proving that a Server-issued P1 — a random Runner identity per CLI launch defeats durable recovery and accumulates permanent offline OfferingsEach CLI invocation now creates
Consequences:
This needs a stable recoverable host/journal identity plus attachment-scoped credentials, or an explicit persisted attachment identity with a real retirement/GC protocol. A new random durable identity on every process start should not be the lock/isolation mechanism. Shutdown should also drain/reconcile outstanding inference custody rather than relying only on P1 — direct
|
|
Follow-up pushed on the existing PR branch: 01e1023. This is a verified hardening slice, not a claim that the full release/SaaS acceptance gates are complete.
Local verification: 151 targeted tests passed, including 10 MatrixOne service integration tests and the real ledger-to-Runner-custody required-compaction test. Relevant library/binary clippy with Still open before merge/release: stable shared local-host/journal recovery across CLI restart and attachment lifetime; true live Runner preview/streaming; exact Offering persistence in the causal resume projection; and measured 100/500/1,000-session throughput, tail-latency, memory and noisy-tenant fairness. These limits are recorded explicitly in |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact commit f1058fb5144a50cf89e2925928778d9e7745be14. Recommendation: address the two P2 findings below before merging. Submitting COMMENT because the connected account is the PR author.
This revision materially improves the earlier architecture. The managed host has a persisted installation identity and journal, uses inference-only dispatch, and receives environment credentials through per-terminal IPC leases after owner/installation checks. Startup clears the inherited environment; the inference-only connection rejects tool requests before entering the tool-executor path. Acknowledged environment-binding disables also remove the ephemeral local publication entries. Direct model selection now resolves exact catalog identities and rejects ambiguous names, and Save without test accurately says that neither testing nor selection occurred.
I traced the shared-host attach/detach/reconnect lifecycle, journal publication/fence/custody handling, model selection/setup, causal Offering restoration, and bounded preview/continuation coordinator. The architecture retains one durable execution owner. The new subprocess regression meaningfully covers token rotation in the same child, output before provider completion, crash before terminal ACK, and same-journal custody replay with one provider request. I inspected that test; I did not execute it locally.
Two setup-to-execution mismatches remain:
- P2: an accepted full Chat Completions endpoint is probed correctly but has
/chat/completionsappended again during real Runner dispatch. - P2: the explicit local probe constructs a different token-limit wire contract from admitted inference, rejecting otherwise valid OpenAI o-series configurations.
Both are detailed inline. Please test the actual setup/check-to-dispatch boundary against a strict local provider, including path/query preservation and model-specific request validation. Separate passing tests of the probe and executor do not catch these disagreements.
Validation and limits:
- Locally executed
python3 -m unittest discover -s scripts/schema -p 'test_*.py': 47 passed; the CI-script suite: 60 passed. - Downloaded the current services/turn-core/plan CI job log: 5,931 tests passed, including Runner waiter bounds and the test preventing implicit selection of Runner credentials. These are CI results, not local Rust executions.
- At the latest check, runtime, CLI non-edge, core/bridge and both online lanes were still running; Static Checks was also in progress. No claim that complete CI is green.
- Cargo/Rust and a disposable MatrixOne service are unavailable locally. The findings are source-traced; the o-series restriction was checked against OpenAI's official generated API contract. No paid-provider request, installed CLI/TUI/Server journey, cross-pod deployment, or performance benchmark was run. The PR's documented cross-pod preview, platform and performance release gates remain separate from this review.
| let mut endpoint = reqwest::Url::parse(&model.base_url) | ||
| .map_err(|_| InferenceHostError::InvalidRequest)?; | ||
| endpoint.set_path(&format!( | ||
| "{}/chat/completions", |
There was a problem hiding this comment.
[P2] Use the same endpoint normalization for checking and dispatch
A saved base_url ending in /v1/chat/completions is accepted by LocalModelDefinition::validate. The CLI deliberately supports this form: chat_completions_endpoint leaves it unchanged, and provider_endpoint_is_derived_without_rewriting_query_or_duplicating_path explicitly asserts that behavior (including a query string).
This dispatch path always appends the suffix. For https://provider.example/v1/chat/completions?api-version=1, Test and use probes the correct endpoint, but the first admitted inference sends to /v1/chat/completions/chat/completions?api-version=1. A normal provider returns 404 despite setup having reported success.
Extract one local endpoint-construction owner shared by probe and dispatch, or reject the full-endpoint form consistently before saving. Add a test that saves the full endpoint, runs the real check, then dispatches an admitted request against the same strict fixture; assert identical path/query and exactly one provider call per explicit action.
There was a problem hiding this comment.
Addressed in 5bfd47e. Probe and InferenceHost dispatch now use one endpoint normalizer in astra-inference-adapter; full endpoints are not appended again and query parameters are preserved. The new saved_endpoint_probe_and_granted_dispatch_share_strict_wire_contract test saves through the actual local setup path, runs the real check, and dispatches through the real InferenceHost with a fixture grant to the same strict local HTTP provider. It covers both base and full endpoints, exact encoded query preservation, successful terminal custody, and exactly one HTTP request per explicit action. Additional endpoint cases cover trailing slashes, encoded path segments, and malformed URLs. All 10 local model tests passed. This is a local setup-to-host regression with synthetic grant/provider, not a claim of a full installed Server journey.
| &serde_json::json!({ | ||
| "model": definition.model, | ||
| "messages": [{"role": "user", "content": "Reply with OK."}], | ||
| "max_tokens": definition.max_output_tokens.min(4), |
There was a problem hiding this comment.
[P2] Align the explicit probe with the admitted request's token contract
The probe always sends max_tokens, while prepare_runner_request uses the canonical OpenAI request builder and emits max_completion_tokens. OpenAI's generated Chat Completions API contract explicitly says that max_tokens is incompatible with o-series models. Thus configuring an o-series model with a valid endpoint/key makes astra model check and TUI Test and use fail at request validation, even though the normal inference builder uses the accepted field.
Use a shared declared wire contract for the probe and admitted execution while retaining a small explicit budget and the no-retry behavior. Do not generalize one provider's dialect to every compatible endpoint. The existing stream-probe fixture matches method/path but does not validate these body fields, so it cannot detect this failure. Add a strict o-series case that rejects max_tokens and accepts the correct bounded request, plus invalid-key/model cases. This finding is based on source and the official contract; no live paid-provider request was made.
There was a problem hiding this comment.
Addressed in 5bfd47e. The probe and canonical OpenAI request builder now share the declared max_completion_tokens field setter. The probe remains bounded to min(configured limit, 4), including reasoning, with no retry or dialect fallback. The strict setup/check-to-host fixture rejects max_tokens, an invalid model, an invalid key, and an incorrect budget; error cases verify exactly one request and unchanged saved configuration. A separate regression exercises actual prepare_runner_request for o3 and verifies the same four-token field without max_tokens or temperature. Documentation explicitly scopes this local profile: legacy-only max_tokens endpoints are not certified, and a short stream check does not certify answer quality or tools. Verification: 10 model tests plus 26 Runner-related tests passed; 1 live-MatrixOne test was skipped. Four affected crates passed all-target clippy with -D warnings; format and diff checks passed. No paid-provider calls and no CI wait.
|
Rebased onto latest main a6406dc and updated this same PR branch using an exact force-with-lease against the previously published 5bfd47e. New head: 0060e2f. The feature commits were replayed, with a final integration commit for semantic conflicts. Design decisions:
Local verification passed:
No paid-provider request or CI wait. This does not replace the documented installed-journey, cross-pod live-preview or production performance acceptance gates. |
Black-box / white-box challenge — findings onlyReviewed PR #712 at head P1
P2
These are the high-confidence issues found in this pass. I did not count lower-confidence protocol/design observations as findings, and I did not run a real external-provider load test. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact commit cb2dd30d980bc7afcdd88ff55141b68671248221. COMMENT because the authenticated account is the PR author.
I have not confirmed a new blocking defect in the changes reviewed. This is a substantive improvement over the previously reviewed f1058fb, but it is not a claim that the current full test matrix or production release gates have passed.
Both previous P2 findings are addressed in source. Probe and granted dispatch now use the same Chat Completions endpoint helper, preserving an already-complete path and query parameters. The local probe uses the shared output-limit serializer. The new saved_endpoint_probe_and_granted_dispatch_share_strict_wire_contract test crosses setup, probe and real InferenceHost dispatch against a strict local server for both base/full URLs, and checks invalid credentials/model failures without modifying saved configuration. This directly targets the earlier mismatch rather than merely duplicating helper assertions.
I also reviewed the new integration and recovery boundaries:
- Cloud credential setup remains
astra model add; device-only setup is explicitlyastra model local add. Runtime Runner revalidation now shares the canonical owner-scoped resolver across foreground/background/delegated callers and retains typed errors instead of stale fallback. - The Cloud compatible transport retains its request-owned proxy guard and validated-address routing while using the exact-wire adapter.
- TUI Test and use stages a candidate before applying it; a failed test preserves the previous configuration/secret, and revision checks reject concurrent updates.
- Local model generations advance across deletion/recreation. Revisioned IPC snapshots are staged in bounded chunks and activated together, with stale snapshots rejected.
- Dispatch preparation uses per-attempt ordering, shared configuration/attachment leases and cancellation signalling independent of slow configuration maintenance. The fence handoff is owned by a detached task so dropping a connection does not abandon a filesystem fence operation that is still completing.
- Server delivery capacity retains reservations across claim expiry; definitive no-start/terminal evidence releases capacity. Continuation recovery advances over authenticated consumed prefixes even when their pending bit has cleared.
The new tests meaningfully target these boundaries: cancellation during paused preparation, cancellation during blocked maintenance, worker removal during a blocking fence, forged cancellation, stale chunked snapshots, candidate preservation, and consumed-prefix recovery. I inspected these tests; I did not execute them locally.
Non-blocking testing follow-up: runner_delivery_preserves_host_capacity_across_claim_expiry currently claims requests sequentially. Add a real-database barrier test with competing claims from different sessions, asserting that no more than four acquire slots, and a same-session case asserting at most one. The registry lock is the intended serialization point; this would verify the new SQL aggregate and lock behavior under actual contention rather than only sequential transitions.
Validation:
- Locally executed the current schema suite: 47 tests passed.
- No Cargo/Rust toolchain was available on PATH, and no disposable MatrixOne or paid provider was exercised. Rust/DB/subprocess results recorded in the PR description were not independently rerun here.
- Immediately before submission, the head remained cb2dd30; PR Title passed, while Test Suite (34224603988) and Static Checks (34224604033) remained in progress. I am not carrying forward green results from an older head.
- Installed cross-surface journeys, cross-pod preview, Windows hosting and performance measurements remain outside this review's executed validation. The documented release gates still apply.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact commit 799325413155067941fc8b8fd4dba44894dbfd2e. COMMENT because the connected account, XuPeng-SH, is the PR author. Please address the two P2 findings inline before merging.
This review covered the 12 commits since cb2dd30, including the new local readiness projection, removal of superseded Runner paths, lease handoff/settlement and Linux/Darwin identity authorities. The earlier endpoint and token-field findings remain fixed. Positive changes include rejecting probe success without semantic/terminal stream evidence, fencing stale probe persistence against model replacement, retaining leases through cancellation-sensitive handoff, and adding a logical owner/session identity alongside lexical/canonical paths. The new Linux identity-race tests have passing CI evidence; that does not establish Darwin correctness.
New findings:
- Local provider verification is shared by model definition, while environment credentials are terminal-local. A check using one terminal's credential is incorrectly projected as verification of another terminal's credential; a failure in one terminal also marks the other terminal's model as needing repair.
- The macOS-only HashSet use has an unconditional import, causing the current Linux strict lint gate to fail. This is observed in the current head's CI, not inferred from an earlier run.
Validation:
- Downloaded the current services/turn-core/plan log (job 102232164946): 5,969 passed, 377 skipped. Passing tests include execution_lease_lock_inode_replacement_cannot_create_a_second_executor, session_execution_identity_preserves_symlink_parent_dir_semantics, and session_execution_lease_fails_closed_if_identity_retargets_before_open.
- Downloaded the current Static Checks log (job 102231971563): make lint failed with unused import HashSet at session_journal.rs:19 under -D warnings.
- The readiness finding is source-traced through check_definition, persist_probe_state, credential_availability and local_model_status; no live paid-provider or installed two-terminal reproduction was executed.
- No local Cargo/Rust toolchain was available on PATH. I did not rerun unaffected Python suites, a MatrixOne deployment, macOS device-range/kqueue tests, or performance benchmarks. The broader Test Suite was still running at the latest check. Author-reported verification in the PR body is not presented as independently executed evidence.
- Rechecked the head immediately before submission; it remained 7993254.
| } | ||
| match &definition.probe { | ||
| LocalModelProbeState::NotRun => "ready_for_check", | ||
| LocalModelProbeState::Passed { .. } => "ready", |
There was a problem hiding this comment.
[P2] Bind readiness evidence to the credential actually tested
For an Environment credential, credential_availability only checks that this process has a nonempty variable. The Passed/Failed probe is persisted in the shared owner-scoped model definition and checked only against binding_revision, which does not change when different terminals export different values for the same variable. Thus terminal A can run KEY=valid astra model local check work, then terminal B running with KEY=invalid gets status=ready, provider_probe=stream_verified, and select it with /model without its credential ever being checked. B's failed check also overwrites A's readiness. This contradicts the per-terminal credential boundary and the documented material-bound probe evidence; it is not a provider-I/O credential leak.
Scope verification to the actual terminal/material identity, or treat persisted environment-backed results as historical diagnostics and require an explicit check before presenting the current material as verified. Do not persist raw credential material or silently trigger another billable probe. Add public check/list tests with two processes sharing one configuration and different environment keys, plus rotation without editing the model definition.
| @@ -16,14 +16,17 @@ use serde::{Deserialize, Serialize}; | |||
| use sha2::{Digest, Sha256}; | |||
| use std::borrow::Cow; | |||
| use std::cell::RefCell; | |||
| use std::collections::{HashMap, VecDeque}; | |||
| use std::collections::{HashMap, HashSet, VecDeque}; | |||
There was a problem hiding this comment.
[P2] Gate the Darwin-only HashSet import
HashSet is used by ACTIVE_DARWIN_SESSION_EXECUTION_KEYS behind cfg(target_os = "macos"), but this import is unconditional. The current Linux Static Checks job fails here with unused import: HashSet under -D warnings, and make lint exits 101. Move HashSet into a matching cfg-gated import (or qualify its Darwin-only use) and rerun the strict Linux lint gate.
|
Follow-up at
Local verification passed: 34 credential tests, 21 local-model tests, the TUI projection test, affected clippy with |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact commit d9068d4d9d66ac997cdf3c3533b5cc767f3be81f, covering the four commits since 799325413155067941fc8b8fd4dba44894dbfd2e and their surrounding readiness, credential-store and TUI paths. COMMENT because the connected account, XuPeng-SH, is the PR author.
Both previous P2 findings are addressed. I have not confirmed a new blocking defect in this revision.
- Readiness evidence is now bound to the credential material used by the actual probe. Status recomputes that identity for the current terminal, so another terminal's successful check cannot certify a different environment credential. Legacy authenticated evidence without a fingerprint becomes stale.
- The persisted fingerprint uses a private local HMAC key rather than a plain hash of potentially low-entropy credentials. Key generation is part of the evidence identity; losing or rotating that key invalidates old observations and permits recovery instead of permanently preserving unverifiable results.
- Before persisting a result, the implementation checks the current model binding and credential/key identity. This rejects delayed observations after replacement, removal or key rotation. Configuration revision checks protect the write against concurrent configuration changes.
- A failed check for different credential material preserves another terminal's known observation within the same key generation. The failing terminal remains stale/ready-for-check rather than borrowing the successful state. This is a deliberate single-observation tradeoff: the immediate check reports the error, while the shared listing does not retain a separate failure history for each terminal.
- Linux now conditionally imports the macOS-only HashSet; the current Static Checks workflow passes.
The new unhappy-path tests exercise materially different credentials, an HTTP 401, missing fingerprint-key recovery, and a delayed old-generation response after a newer successful check. I also checked how these persistence outcomes reach CLI/TUI messages and how candidate testing continues to preserve saved configuration on failure.
Validation:
- Downloaded the current CLI non-edge job log: 4,097 passed, 992 skipped in its main filtered run. It explicitly records PASS for environment_probe_evidence_is_scoped_to_current_terminal_material, delayed_probe_from_old_key_cannot_overwrite_new_generation, and the existing binding replacement/removal tests.
- The current Test Suite (34289750593), Static Checks (34289750569), and PR Title (34289750598) workflows report success. These are current-head GitHub CI results, not local Rust executions or a claim that skipped tests ran.
- The terminal-material regression simulates terminal environments within one process; it is not an installed two-process integration test. The delayed old-generation failure traverses the mock HTTP request, while the old-success rejection also uses a direct persistence assertion. A real two-process test with independent environments would strengthen this boundary.
- No local Cargo/Rust execution, paid-provider call, installed CLI/TUI journey, macOS device/lease test, cross-pod deployment or performance benchmark was run in this re-review. The broader release gates remain separate.
- Immediately before submission, the live head remained d9068d4 and no review of this head was present in the retrieved review history.
|
Follow-up at
The existing key-rotation/late-result and TUI error-projection fixes remain unchanged. Local verification now includes 34 credential tests, 23 local-model tests (including the child-process regression), affected clippy, format and diff checks. |
Incremental design and size audit (head a0b2162)I reviewed the complete diff against main with an explicit check for duplicated legacy implementation.
One safe duplication was found and consolidated in this head: TUI and prepare_from_tui now share one canonical local-model definition builder and validator. The follow-up also explicitly cleans a protected secret when validation fails before a candidate can own it. Local verification: 24 local-model tests, 8 TUI setup tests, strict CLI clippy, formatting, and diff checks passed. CI was not awaited. Possible follow-ups, intentionally not mixed into this PR: split very large test and design files; consider a shared private-file primitive only after confirming platform and error semantics; and revisit small CLI/TUI status-projection helpers. I did not find a safe deletion of a production authority or compatibility path in this review. |
Summary
Implements Runner-owned BYOK inference while retaining Astra's single durable Agent Backbone. Server owns admission, policy, request compilation, continuation, usage, and audit; only the explicitly selected Runner performs provider I/O with local credentials.
Post-review hardening
--modelwins at launch; missing legacy identity requires a fresh selection instead of a friendly-name or Server fallback. Runner credentials are never selected merely because they are first in the catalog.main, including the existing Darwin session execution-lease fixes, on this same PR branch without rewriting published history.Local verification
No CI wait was used for this verification.
make format-checkand strict all-target clippy for the seven affected CLI/Edge/runtime/services/types/credentials crates passed.runnermatrix: 111 passed;modelmatrix: 419 passed. These filtered counts overlap other suites below.astra-edgesubprocess integration passed: verifies real WS Authorization, rotates the synthetic profile token and reconnects the same child/journal/boot, observes live output before provider completion, kills the host before terminal ACK, observes failed client liveness, then reopens the same journal and replays custody with exactly one provider request.Explicit limitations / release gates
sleepreceives SIGKILL, and ordinary tool-Runner Bash is rejected becauseworkspace_observation::stable_coordination_rootcurrently supports Linux only. Those source files matchmain; no sandbox or coordination check was weakened to bypass them. Therefore this is not a claim that the complete repository offline gate or macOS tool-execution journey is green.Closes #702