Repository navigation
Conversation
Expose scoped provider-signed cursor, prepare and continuation activation through the existing Session owners. Initialize the inherited transcript projection and traverse retained nested prefixes without copying execution authority.
XuPeng-SH
left a comment
There was a problem hiding this comment.
I don't think this is ready to approve yet. The fork coordinator reuse is a good direction, but the complete provider workflow has the following gaps:
-
Cross-workspace continuation can use another workspace's history. Provider identity is derived from provider ID and external subject, while ordinary
/chat/streamresolves the session by user and session ID without checking provider scope. A user with a valid request for workspace B and the child session ID from workspace A can continue that child; a local isolated MatrixOne/HTTP test confirmed that the model request included A's inherited history. This chat behavior predates the PR, but the new fork API relies on it for continuation, so the workspace isolation needs to be enforced at the common session/run admission boundary. -
Re-forking an inherited head fails after deleting the middle ancestor. For A → B → C, with C still at its inherited head, the same fork request succeeds before deleting B and returns
403 session_fork_forbiddenafterward. C's retained history still exists, but authorization follows the prefix to B, whose transcript and run records were deleted. The existing test checks continued chat after deletion, but not forking the retained head. Authorization evidence and retained history need compatible lifetimes without retaining execution authority. -
Interrupted prepare can leave a child and pin indefinitely. Prepare creates an active child session and a prepared manifest pin. Provider-signed callers cannot use the existing abort endpoint, and orphan cleanup requires the child session to be absent. Retrying with the same idempotency key can complete the operation, but cancellation or loss of the pending operation leaves no provider cleanup path. Please define and exercise recovery/cancellation for this lifecycle.
-
Fork creation bypasses the configured session creation quota. The coordinator inserts the child session directly without the normal quota check or creation count. This coordinator behavior also predates the PR, but the new signed endpoint exposes it to provider clients. Unique idempotency keys can create children beyond a configured limit; retries must not be charged twice.
Separately, please assess the cost of loading and validating the full run opening during cursor lookup, prepare, and activation. Each opening can be up to 8 MiB; I have not benchmarked the impact. A bounded common read path would be preferable to a duplicate authorization projection.
Validation: I extended the existing HTTP fixture temporarily against an isolated MatrixOne instance and a mock model. It reproduced findings 1 and 2, including a successful re-fork before ancestor deletion and a 403 afterward. Findings 3 and 4 are based on the creation, authentication, quota and cleanup code paths. The PR's executed CI checks are green; the live regression lane was skipped.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for the four findings in my preceding review, especially the reproduced cross-workspace history exposure and the retained-head re-fork failure after ancestor deletion.
Capability assessment: Unchanged: Agent chat, Context and cache keep their existing Beta profiles. Provider-authenticated access to canonical Session forks adds no execution authority, profile, maturity claim or external-effect replay guarantee.
Summary
MOI needs to branch from an exact completed conversation turn. This change admits its existing request-signed provider credentials to run cursor resolution, fork preparation and activation for continuation. The selected run must belong to the authenticated provider subject and workspace.
The existing Session fork coordinator activates a shared immutable prefix without acquiring a writer lease. The next ordinary conversation request obtains its own execution authority. Existing runtime and Edge user tokens retain their canonical-head prepare behavior: the authentication owner reports the verified mechanism so only request-HMAC credentials receive the new provider-scope prerequisite.
Related issue
Prerequisite for matrixorigin/matrixflow#17540. The MOI UI, attachment ownership and conversation publication are delivered separately in Matrixflow.
Change type
User and compatibility impact
GET /sessions/{session_id}/runs/{run_id}/cursorresolves the exact retained cursor of a committed root conversation run.POST /sessions/{session_id}/forksaccepts request-signed provider credentials while preserving existing user-token behavior.POST /sessions/{session_id}/forks/{fork_id}/activate-for-continuationactivates the prepared prefix and returns its child head without a writer lease. Replays return the current head without rewinding subsequent turns.Existing list/get/abort, writer activation and Work fork routes retain their authentication and execution contracts. Fork creation copies no transcript rows, runs, writer leases, approvals or sandbox authority. Current canonical history and terminal transactions handle first continuation, nested branches and retained ancestry after intermediate Session deletion. No storage schema migration is introduced. Deploy this prerequisite before enabling MOI's branch action.
Architecture and complexity delta
Verification
Reviewed head:
4dee48ea3b3ad8aaffb26b036c20e048cfdfb60c, based on203aed8ac9b4a08398725ade7fd043c0b7657f5b. Independent final source review passed on this exact head, including the feature implementation, final rebase and test-fixture-only delta. No unresolved material source finding remains. The selected local validation below is complete. Broader CI and deployment checks remain separate.cargo test --locked -p astra-runtime --features moi-compat-tests --test moi_fresh_astra -- --include-ignored --nocapture --test-threads=1passed all 10 tests, with zero ignored or filtered tests. This includes the original seven consumers, one complete branching journey and two authentication compatibility tests. The existing always-onTest: MOI fresh-Astra compatibilityjob executes this same target and feeds the online-core aggregate; no extra workflow is added.cargo test --locked -p astra-services --lib session_fork_coordinator::tests -- --ignored --nocapture --test-threads=1passed both database tests, including no-copy and million-token-prefix coverage.cargo test --locked -p astra-runtime --test work_http_db_it work_fork_is_exact_durable_owner_scoped_and_preserves_a_retained_prefix -- --ignored --nocapture --test-threads=1passed.551cf8bcfa3fe70ddf82fce7264acb79e2a5bed1. The subsequent documentation correction and TUI-only upstream rebase leave the feature behavior, dependencies and configuration unchanged. The final commit only warms the existing HTTP client before four pre-existing timing tests; no product code or original budget/assertion changes. Unaffected behavior evidence is reused on these verified deltas.9af0d50f, with unchanged feature files after rebase):cargo clippy --locked -p astra-runtime --features moi-compat-tests --test moi_fresh_astra -- -D warningspassed on9af0d50f.make checkpassed on9af0d50f: workspace Clippy with warnings denied, formatting,cargo check --all-targets, SDK build and Web typecheck. The final rebase adds only upstream CLI TUI changes; the final runtime/CLI static supplement below passed, and unaffected checks are reused.b5319822(unchanged by the final test-fixture delta), both existing canonical terminal database regressions passed:canonical_terminal_success_commits_evidence_usage_and_terminal_togetherandcanonical_terminal_status_conflict_rolls_back_canonical_evidence_and_usage, each run withcargo test --locked -p astra-runtime --lib <case> -- --ignored --nocapture --test-threads=1(one executed case each).make test-contractpassed onb5319822(unchanged by the final test-fixture delta): 11 admin-contract, six HTTP-contract and two core settings-contract tests passed; the six existing ignored admin cases remain ignored.cargo nextest run --manifest-path Cargo.toml -p astra-runtime --profile ci, using each original--lib,--binor--testtarget selector. The 6,171-case list has 5,984 runnable cases with passing evidence (5,676 reused + four corrected + 304 other cases executed on the final head) and 187 originally ignored cases. Twenty-two targets had no runnable cases and are recorded as enumeration only. All exact case identities and per-target results were retained; this is composite coverage, not a fresh single passing full-library invocation.make test-history-work NEXTEST_OFFLINE_PROFILE=cipassed both originally ignored history-work cases on the final head (0.081s).cargo clippy --locked --manifest-path Cargo.toml -p astra-runtime -p astra-cli --all-targets -- -D warningspassed (7m38s).make format-checkpassed (15.01s). Both verified the frozen 2,592-file source set.make test-offlineor all CI passed.Final checklist