[ragnorc]

What & why

OmniGraph Studio's History wants to start a branch at a commit in a branch's history, to try a fix or a revert there. branch create only forked a branch's current head.

branch create now starts at a published graph commit on every door: from_commit on POST /branches, --from-commit on omnigraph branch create, from commit "<id>" in the GQ branch create statement (language version 2.3), and from_commit on the MCP branch tool. The new branch holds the commit's rows and history; merging it back uses the commit as the merge base.

Why a typed from_commit, not a commit id in from

• A branch and a commit are different kinds of start, so each gets its own field. A request naming both is refused (400). Nothing is classified from the shape of a string, so no name is ambiguous and branch-from-branch is unchanged.
• CLI, server and integrations upgrade together (docs/dev/versioning.md: no old-client fallback or negotiation), so the field needs no compatibility shim. The new field and the receipt's from_commit_id appear in openapi.json.
• GQ gets from commit "<id>". A bare from commit still names a branch called commit.
• Every receipt gains a required from_commit_id (HTTP, statement outcome, CLI --json and human output): the exact commit the new branch starts at. Its from names the branch the new branch was forked off; for a commit start, that is the branch that wrote the commit.

Engine

• The fork clones the __manifest version that published the commit, on the tree of the branch incarnation that wrote it. Lance create_branch at that version goes through the existing recoverable two-phase create. The first manifest therefore names the commit's table pins, lineage and schema contract in one native create (invariants 2 and 4).
• Under the existing branch-create gates the engine refuses, before any effect, with BranchStartRefused:
  • a pinned table version that no longer opens, or that is missing an object its manifest references (not_retained, 410);
  • a schema contract other than the accepted one (schema_changed, 409);
  • a version only the legacy history serves (earlier_storage_format, 409);
  • a commit whose branch was deleted (branch_deleted, 409, with the branch in branch). The capture of the commit's branch decides this once: a name that resolves to nothing and a name created again since are the same fact and get the same answer on every door.
• It also refuses, as integrity errors, a version above the captured head and a version whose head row is another commit.
• The object probe matters because the collector deletes a version's files before its manifest. After an interrupted cleanup, a manifest can open even though objects it names are deleted. Pins the source head also holds are roots and skip the probe. The probe runs under the exclusive schema permit, so it opens the pins and probes their objects with bounded concurrency (8) and stops at the first missing object; it sends one request per object of the probed pins.
• Unpublished proposals and unattached snapshots are refused (unpublished_commit, unattached_snapshot, 409). Neither is branch history; both are detached __manifest versions.
• Policy: a commit start is checked as branch_create from the branch that wrote the commit. A denial answers the 404 of an unknown id, so the id reveals nothing.
• Retention: the collector already keeps a named branch's base version and its pins, so the commit stays retained while the branch lives.

RFC #677 (draft RFC 0060) covers forking at a retained snapshot under a new format. This is the current-format slice and follows that RFC's rules where they overlap.

Tests

• Engine integration/branching.rs: a main commit (pins, history, writes, a three-way merge back), a named-branch commit, and the refusals, including a deleted writer branch and a name created again.
• Engine db/upgrade/tests.rs: after a real storage upgrade, starts at pre-upgrade commits refuse as earlier_storage_format before any effect, and a start at a post-upgrade commit succeeds.
• Engine integration/branch_control_cost.rs: a slope gate of one probe request per added pinned fragment.
• Engine integration/maintenance.rs: cleanup keeps the started commit, and a reclaimed commit is refused naming the type.
• Engine failpoints.rs: a pre-native failure and a lost acknowledgement for a commit start. A commit an interrupted cleanup emptied is refused; after the cleanup retry (which converges since fix(cleanup): finish a table whose interrupted sweep removed its index directory #1011) it stays refused because its version no longer opens, and the retained head still starts a branch.
• Engine policy_engine_chassis.rs: allow, and deny as not-found.
• Compiler parser: from commit "<id>", a bare commit branch, and malformed ids.
• Server:
  • route and statement receipts;
  • both fields (400), unknown commit (404), and the 409 refusals;
  • policy denial as 404;
  • the 409/410 error mapping;
  • the OpenAPI contract.
• CLI: an embedded/served parity row for --from-commit, and the human receipt line.
• GQT: branch_create_at_a_commit.gqt (start, merge base, merge) and branch_create_at_a_commit_refusals.gqt (proposal, snapshot, deleted and recreated writer, unknown id, on the engine, DST and server environments), with from commit "${name}" support in the runner.

[cursor]

Review: feat(branch): start a branch at a graph commit

Head reviewed: 118081c8e8ed97ca518c505832c12bfa65044e66
Recommendation: request changes. 1 blocking finding, 2 optional findings, 1 merge-housekeeping item.
(This account cannot set a formal review state, so I post the review as a comment.)

Summary

The design is sound. A commit start clones the __manifest version that published the commit. The first manifest of the new branch holds the commit's table pins, lineage and schema contract in one native create. There is no second publication. This keeps invariants 2, 3 and 4. Each refusal comes before any effect, and the tests prove it.

One public outcome is not consistent. When the branch that wrote the commit is gone, the answer changes with the door and with name reuse. The user guide promises a different answer. Fix this in the engine before merge.

Blocking

B1. One fact gets three different answers, and the guide promises another.

Why: the fact is "the branch incarnation that wrote the commit no longer exists". A client must get one answer for one fact, on every door. Now Studio and the CLI cannot handle this case reliably.

What I observed on 118081c8:

Case	Engine / CLI direct	HTTP and served GQ
Writer branch deleted	branch 'gone' not found (BranchNotFound). The caller did not name this branch.	404 not_found, commit '<id>' not found (handler rewrite)
Writer branch deleted, then its name reused	400 bad_request: commit '<id>' has no persisted native-branch incarnation witness: …	the same 400 bad_request

docs/user/branching/index.md:146-147 says: "An id no commit has answers 404, as does a commit of a branch that was deleted." The PR body calls the second row an integrity error. It is neither an integrity error nor a 404: it is a BadRequest, and it is a normal user state.

Where:

• crates/omnigraph/src/db/omnigraph.rs:3639: branch_create_from_impl captures the source by the record's branch name. A deleted name fails inside capture_branch_control_source with BranchNotFound.
• crates/omnigraph/src/db/omnigraph.rs:3724-3729: a reused name passes the capture and fails here as OmniError::manifest (400).
• crates/omnigraph-server/src/handlers.rs:2870-2874: the server rewrites only the first case, and only for HTTP. No test covers this rewrite.

How to fix it at the contract owner (the engine):

1. Decide the case once, in the engine, after the policy check and before the capture by name. Compare the commit's native_branch with the live incarnation of its graph_branch. "No live incarnation" and "another incarnation" are the same fact.
2. Return one typed outcome for both. I recommend a new BranchStartRefusal reason (for example branch_deleted). commit show <id> still shows this commit, so "not found" misleads a user who selects it in History. BranchStartRefusalReason::Unknown already gives older clients a safe fallback. A 404 on every door is also acceptable. One answer is the requirement.
3. Remove the rewrite in branch_create_body.
4. Make the guide match. Add both cases to branch_create_at_a_commit_refusals.gqt with the engine and server environments (the steps are in Validation below).

Impact if not fixed: the embedded CLI and the server disagree, which the parity matrix exists to prevent. A client must parse message text to tell the cases apart. The RFC 0060 rule change (see Future direction) would then need a change to wire semantics, not only a new reason.

Optional

O1. The retention probe sends one object request per file, in sequence, under the process-wide exclusive schema permit.

Why: commit_start_record calls CollectorPathSnapshot::missing_paths (collector.rs:414). That function awaits one exists() per object in a loop. It runs under acquire_schema_exclusive (omnigraph.rs:3651). Mutations, loads, merges and cleanup take the shared side of this gate, so every write in the process waits for the probe.

Evidence: a temporary counter shows 14 sequential exists() calls for a start at a commit with 4 earlier inserts, and 42 calls with 32 inserts. The count is linear in the fragment count of each changed table. On local disk this takes less than 1 ms. On S3, at 10 to 30 ms per HEAD (an estimate, not measured here), 1,000 fragments stall writes for 10 to 30 s per table lane.

How (now, small): put the objects of all probed pins into one bounded concurrent stream (reuse BRANCH_START_RETENTION_OPENS or maint_concurrency()). Stop at the first missing object. Add a deterministic request count to an existing cost owner, so a regression is visible.

How (root cause, follow-up): docs/dev/writes.md:94 already says that the probe stands in for a collector property: "a manifest that is present names only objects that are present". Please open an issue to make that a collector guarantee, for example: remove a version's manifest before its files, and let orphan-age reclamation finish an interrupted run. Then this probe becomes one open per pin, and you can delete it. Tradeoff: the current order gives faster progress after a crash. The other order gives a cheaper and simpler proof on the read side.

O2. earlier_storage_format has no behavior test.

Why: two guards produce this reason: legacy_version in ManifestCoordinator::create_branch_at, and the missing-contract check in commit_start_record. Only the HTTP status-mapping unit test and the OpenAPI snapshot name it. No test shows which guard fires, or that nothing is created.

How: add one case to the storage-upgrade owner (db/upgrade/tests.rs, which can build a legacy commit with LegacyHistory), or to the stamp-13 genuine journey. Assert the reason, BeforeEffect, and an unchanged branch list. If one guard can never fire first, remove it.

Before merge (housekeeping, not a design finding)

#1011 landed on main as 9c2b3679. The PR now conflicts with main only in changelog.d/cleanup-interrupted-sweep-converges.fixed.md (add/add) and docs/user/operations/maintenance.md; the code merges cleanly. Take main's text for both files, and remove "Stacked on #1011" from the PR body.

What I verified as correct

• Lance 11 resolves a bare version number on the checked-out branch. Dataset::create_branch(name, u64) maps Ref::VersionNumber to self.manifest.branch (lance-11.0.0/src/dataset.rs:3412-3414), and checkout_version(u64) does the same (dataset.rs:500-502). So a named-branch source clones its own version, not main's. CloneContext::capture is pinned to that version (lance_clone.rs:38-45). This agrees with the upstream branch/tag spec: a branch is a shallow clone at parentVersion, and it can start at any version or tag.
• Atomicity against cleanup. The probe and the native create run under the exclusive schema permit. cleanup_all_datasets holds the shared permit for its whole run (optimize.rs:764), so no cleanup in the same process can interleave. Cross-process cleanup stays inside the documented single-writer-process boundary.
• Retention. retained_versions(keep_base = true) keeps a named branch's oldest __manifest version (collector.rs:675-706). For a commit start, that is the cloned version. Nothing deletes __history run files outside tests. The maintenance test proves the retention.
• Policy. A denial answers the same 404 as an unknown id. Every refusal comes after the policy check, so no refusal discloses that a commit exists.
• Crash windows. A commit start reuses create_branch_recoverably. The failpoint tests cover the pre-native failure and the lost acknowledgement for a commit start.

Future direction and other open work

• RFC 0060 (rfc: branch operations v2 #677) plans "fork current/retained snapshot", and says "a recreated name gets a new identity. Shared snapshots survive." Under that RFC, a start at a deleted branch's commit can become legal. With a typed refusal reason, that change ships as one removed refusal, not as a change to wire semantics. This is a second reason for the B1 recommendation.
• RFC rfc: schema apply with live branches #963 (schema apply with live branches) lets branches hold different contracts. Here, SchemaChanged compares against the contract at the source branch's head (commit_start_record). Under rfc: schema apply with live branches #963 that rule must use the schema sequence. Please record this in rfc: schema apply with live branches #963.
• feat(server,cli,mcp): report each branch's head, start and times in the branch listing #1012 reads fork from the head at BranchContents.parent_version. For a commit start, that is from_commit_id, so the two PRs agree. If possible, use the same field name there.
• test(cleanup): interrupt the sweep between manifests and before dead-staging manifests #1027 / test(cleanup): run the sweep crash windows on S3 and Azure #1033 add more sweep crash windows. The probe uses the collector's own inventory (capture_dataset_paths), so it covers those windows with no extra code. This is good reuse.
• Lance after 11: upstream format docs now define FLAG_FRAGMENT_TREE (Manifest.fragments is empty). Lance 11 does not have it (lance-table-11.0.0/src/feature_flags.rs stops at bit 7). The probe and the collector both walk manifest().fragments. At the bump that enables fragment trees, update capture_dataset_paths, or the probe passes with nothing to check. Add this to that bump's checklist.

Software-liability assessment

• The core mechanism adds no new source of truth, no new type of durable effect and no new seam. It reuses the native create, the collector inventory and the policy door. Liability is low.
• B1 adds liability if it ships: three shapes for one fact, one of them in a transport only, with no test. Each later branch feature would copy one of the three.
• O1 adds operational liability that grows with table size, until the collector gives the property that the probe stands in for.

Validation

Commit 118081c8e8ed97ca518c505832c12bfa65044e66, isolated worktree, Rust 1.97.1, protoc 3.21.12, Linux x86_64.

PR tests on the exact head (all pass):

• cargo test -p omnigraph-gqt --locked --test gq_logic_tests -- branch_create_at_a_commit: 2 passed (engine on local files, DST seeds 0 and 42).
• cargo test -p omnigraph-engine --locked --features failpoints --test integration -- branch_create_at_a_ cleanup_retains_the_commit_a_branch_starts_at: 4 passed. With -- branching:: maintenance::: 110 passed.
• cargo test -p omnigraph-engine --locked --features failpoints --test failpoints -- --test-threads=1 branch_start_refuses_a_commit_an_interrupted_cleanup_emptied collector_retry_treats_an_index_directory branch_create native_branch_controls_reclassify_lost_acknowledgements: 4 passed.
• CI on this head: every required check is green.

Temporary proofs for B1 and O1. These were local only and never pushed; I restored the tree, and git status was clean afterwards.

• GQT case on the engine environment, which expects the guide's 404 text after the writer branch is deleted: branch create gone from main, an insert on gone, --- bind commit $gil, branch delete gone, branch create b4 from commit "${gil}", --- expect error: commit '. FAIL: branch 'gone' not found.
• The same case with branch create gone from main again before the start (--- expect error: not found). FAIL: commit '<id>' has no persisted native-branch incarnation witness: branch 'gone' was deleted and recreated after the commit was written.
• Server test with the same steps over POST /branches: deleted writer gives 404 not_found; reused name gives 400 bad_request. GET /branches after both: ["gone","main"] (nothing created, as expected).
• Counter in missing_paths: 4 inserts give 14 sequential exists() calls; 32 inserts give 42.

Limits:

• The served GQT runner needs a green in-process verdict first, so it cannot show a door that fails in-process. I proved the HTTP door with a Rust server test instead.
• No S3 or Azure run. The S3 stall numbers are estimates from per-request latency.
• I did not run the full workspace gate locally. CI covers it on this head.
• This is a feature PR. The before/after regression rule applies to the fix(cleanup): finish a table whose interrupted sweep removed its index directory #1011 fix, which is already on main with its own regression test.

[ragnorc]

Thanks for the careful review. Addressed in f91513a:

B1. The engine now decides "the branch incarnation that wrote the commit is gone" in one place, capture_commit_start_source. It runs under the control gates and captures the commit's branch by name. A name that resolves to nothing, and a name created again since (the captured incarnation differs from the record's native_branch), both return the same new typed refusal, BranchStartRefusal::BranchDeleted. On the wire that is 409, branch_start_refusal.reason = "branch_deleted", with the branch in a new optional branch field. The refusal comes after the policy check and before any effect. I removed the HTTP-only rewrite in branch_create_body, so every door returns the engine's answer. The guide's sentence is gone, and the refusal table has a branch_deleted row.

Tests:

• branching.rs: both shapes, through the embedded door.
• branch_create_at_a_commit_refusals.gqt: delete gone, start at its commit, recreate gone, start again; both expect the same error. The case runs on the engine, DST and server environments; the served conformance test covers the HTTP door.
• The server's status-mapping unit test has the new row.

O1. The probe now opens the pins and probes the objects of all of them in one bounded concurrent stream (8, the existing bound), and stops at the first missing object (CollectorPathSnapshot::first_missing_location). branch_control_cost.rs::branch_start_at_a_commit_probes_once_per_pinned_object is a slope gate: one data read per added pinned fragment, plus slack, and one open per probed pin whatever its fragment count. The collector-ordering root cause is now #1094. It compares manifest-first deletion with moving the manifest out of the version listing before deleting its files.

O2. Only one of the two guards can fire first. Every record carries the contract its commit was written under: the storage upgrade gives each converted commit one (legacy/census.rs::lineage_row sets schema_contract: Some(..)). So the engine's "no contract" arm is unreachable. It is now an integrity error (manifest_internal), not a refusal. The catalog's legacy_version check in create_branch_at is the guard that fires. db/upgrade/tests.rs::branch_start_refuses_a_commit_written_before_the_upgrade builds a real pre-upgrade graph and upgrades it. It then asserts earlier_storage_format with BeforeEffect, an unchanged branch list and an unchanged main for every pre-upgrade commit, and that a start at a post-upgrade commit succeeds with that commit's rows.

Housekeeping. Main is merged twice, the second time through 9e2425d2: I took main's text for the #1011 changelog note and the maintenance guide, kept both sides in the branching guide, and moved the CLI changes onto main's new commands/history layout. The "Stacked on #1011" note is gone from the PR body. On the merged head, the full workspace suite passes, 4,745 tests. The two branch-start GQT cases now run one DST seed, as most cases do. They had reached the 10 s case budget on a loaded host, and now take about 7.4 s and 3.8 s alone.

Future direction. Agreed on RFC 0060: with a typed reason, that change becomes one removed refusal. I posted alignment notes on the open PRs this one meets:

• rfc: schema apply with live branches #963: what schema_changed compares against, and the rule once contracts carry sequences.
• feat(server,cli,mcp): report each branch's head, start and times in the branch listing #1012: the creator record must also cover create_branch_at, and the fork/from_commit_id naming.
• feat(server)!: say on every error body whether the request ran #1076: its typed_refusal list needs BranchStartRefusal.
• feat(cli)!: exit with what a failed command's outcome says #1078: rebase onto the changed embedded call.
• build(storage): upgrade lance to 13.0.0 #1072: the two Lance behaviors this PR now relies on.

[cursor]

Review

Head reviewed: 53d53516216e31b0dec8dfcdc70181be3027af98
Recommendation: merge.

0 blocking findings. 0 optional findings.

Why, what, how, impact

A caller starts a branch at a published graph commit. The new branch holds that commit's rows and history. The start is one publication: Lance clones the __manifest version that published the commit.

The previous review found one fact with three answers. The fact is: the branch incarnation that wrote the commit no longer exists. This head answers that fact once.

capture_commit_start_source returns BranchStartRefusal::BranchDeleted in both shapes. A deleted name is one shape. A name created again since is the other. The wire form is HTTP 409. branch_start_refusal.reason is branch_deleted. The optional branch field names the branch that wrote the commit. The message is: branch 'gone', which wrote it, was deleted. The HTTP-only rewrite in branch_create_body is gone.

A denied actor still receives the not-found answer of an unknown commit id. The refusal is BeforeEffect. Native branch creation runs after the proof.

Doors

Door	How it gets the answer
Embedded engine	branch_create_from_as
CLI direct	the same engine call
CLI remote	POST /branches, then ErrorOutput
HTTP POST /branches	branch_create_body, then ApiError::from_omni
Served GQ branch create	the same branch_create_body
MCP branches_create	server_branch_create

The engine owns the decision. The other doors copy it.

Earlier findings

B1 is resolved. A deleted name and a reused name are one typed refusal, decided under the control gates, before prove_commit_start and before create_branch_at. See capture_commit_start_source.

O1 is resolved. first_missing_location probes at most 8 objects at a time and returns at the first missing object. The bound is BRANCH_START_RETENTION_PROBES. Pin opens use the same bound, then the object probe runs. A successful start still checks every object of a pin the captured head does not hold. The slope gate passed. The probe stands in for the collector property in issue #1094. writes.md says that.

O2 is resolved. A record with no schema contract is manifest_internal. The storage upgrade writes a contract on each converted commit. The user refusal for a pre-upgrade commit is legacy_version inside create_branch_at, reason earlier_storage_format. The upgrade test covers that path.

Contract and merge

BranchStartRefusalReason in openapi.json lists branch_deleted. BranchStartRefusalOutput.branch is the optional field. ErrorCode stays the closed value conflict. That matches the existing additive-detail pattern. The server unit test checks status 409, code conflict, the engine message, reason branch_deleted, and branch == "gone".

The user guide table matches: a commit of a deleted branch, even when the name is reused, is 409 branch_deleted with the branch in branch. An unknown id stays 404.

53d53516 merges main 9e2425d2. The branch-start text is present in the changelog, the branching guide, writes.md, and the maintenance guide. The CLI flag lives on main's commands/history layout. No conflict markers remain.

Liability

This removes a transport rewrite and two engine messages for one fact. It adds one reason and one optional field on a detail that already exists. A later similar refusal is one more enum arm and one more guide row.

Draft RFC 0060 (PR #677) makes a fork select a retained root. When that lands, this refusal can go away as one deleted arm. Until then, a start still requires the writing incarnation.

The alignment notes on #963, #1012, #1076, #1078, and #1072 name the right seams. #1072 must keep Lance create_branch(name, version) resolving a bare version on the checked-out branch. Lance 11.0.0 does that: Dataset::checkout_version of a version number uses self.manifest.branch (dataset.rs around lines 500 and 3412). FLAG_FRAGMENT_TREE is absent in lance-table 11. The probe walks manifest fragments.

Validation

Head 53d53516216e31b0dec8dfcdc70181be3027af98, checkout /tmp/og-1016, CARGO_TARGET_DIR=/workspace/target:

• cargo test -p omnigraph-gqt --locked --test gq_logic_tests branch_create_at_a_commit_refusals — ok, 1 passed, 1.26s. Engine and DST seed 0. This harness does not run the server environment.
• cargo test -p omnigraph-gqt-served --locked --test gqt_served_conformance branch_create_at_a_commit_refusals -- --test-threads=1 --nocapture — ok, 0.19s, served=ok in-process=ok.
• cargo test -p omnigraph-engine --locked --test integration at_a_commit — ok, 2 passed, 0.19s. The refusal test and the probe slope gate.
• cargo test -p omnigraph-engine --locked --lib branch_start_refuses_a_commit_written_before_the_upgrade — ok, 0.48s.
• cargo test -p omnigraph-server --locked --lib branch_start_refusal_is_typed_409_or_410 — ok, 0.01s.

Before 118081c8e8ed97ca518c505832c12bfa65044e66, checkout /tmp/og-before, CARGO_TARGET_DIR=/tmp/og-target-before. The head case file was copied in, because the old case has no deleted-branch steps. The file was restored afterward.

• Same gq_logic_tests filter — failed, exit 101. Step 8 does not contain branch 'gone', which wrote it, was deleted. The actual text is branch 'gone' not found, on the engine and on DST seed 0, replay 0 and replay 1.
• Same served conformance, with a larger stack (ulimit -s unlimited, RUST_MIN_STACK=67108864) — failed, 0.31s. In-process step 8: branch 'gone' not found. Served step 8: commit '<id>' not found. That is the old HTTP rewrite.
• To reach the reused-name step, the deleted-name expect was temporarily not found, which both old answers contain. Both doors then failed at step 10 with: commit '<id>' has no persisted native-branch incarnation witness: branch 'gone' was deleted and recreated after the commit was written. The case file was restored to the 118081c8 original.

The new branching.rs assertions cannot compile on 118081c8, because BranchDeleted does not exist there. The GQT failure above is the behavioral proof. On the head, that Rust test passes and checks BeforeEffect, BranchDeleted { branch: "gone" } for both shapes, an unchanged branch list, and an unchanged main version.

Limitations

• I did not run the CLI binary or an MCP client. CLI remote and MCP use the server body above. CLI direct uses the engine call above.
• Policy tests in the tree deny a start at a live commit. I did not run a denied actor against a commit of a deleted branch. In code, HTTP authorize returns 404 commit '<id>' not found before the engine call. The engine returns commit_not_found when actor is set and policy denies, before capture_commit_start_source.
• The first served run on 118081c8 aborted with a stack overflow at the default 8 MiB stack. That abort is not the proof. The rerun with a larger stack produced the messages above.
• I did not re-run the full workspace suite. The author's count of 4,745 tests is outside this review.