Repository navigation
Conversation
…branch listing `GET /branches?detail=true` and `omnigraph branch list --detail` add a `details` array beside the names: for each branch the caller may read, its head commit, the branch (`from`) and commit (`fork`) its current lifetime was created from, the actor that created it, and when it was created and last moved. Every field derives from the branch refs and each branch's own `__manifest` (the latest version, and the creation version once the branch has written a commit); no lineage is read, so the cost per branch is flat in history. Branch creation now records its actor on the new native ref (`omnigraph.created_by_actor_id`) right after the native create. A failure there leaves the branch without an actor and reports uncertain completion.
Review of #1012 at
|
| Command (suffix) | Result |
|---|---|
--test integration --test failpoints --test forbidden_apis --no-run |
built |
--test integration -- branching:: data_routes::remote_branch_list_create_merge_flow_works auth_policy::branch_statements_take_their_routes_policy_decision auth_policy::branch_list_is_denied_by_a_branch_scoped_read_rule_as_the_route_is openapi::openapi_spec_is_up_to_date parity_matrix::parity_branch_list cli_data::branch_list_outputs_sorted_branches cli_data::catalogue_scope_reads_the_binary_registries_without_a_store |
engine 55 passed (54 at head, plus my temporary probe); server 4 passed; CLI 3 passed |
--test integration -- branch_control_cost::branch_list_detailed_reads_bounded_per_branch_and_flat_in_history --exact |
passed. Reads 8 (2 branches) and 20 (6 branches); scans 5 and 13; 20 reads after 40 more commits |
…_s3 twin |
skipped: OMNIGRAPH_S3_TEST_BUCKET was not set. Libtest reports ok, but the body did not run |
--test failpoints -- branch_create_reports_an_unrecorded_creator_as_uncertain native_branch_controls_reclassify_lost_acknowledgements |
2 passed |
--test forbidden_apis |
35 passed |
| Temporary probe: fast-forward into a branch that has not written | passed: FastForward; fork stayed at the load commit; updated_at equals the fast-forward commit time |
| Temporary mutation: one extra scan per branch | cost gate still passed; reads unchanged; scans 7 and 19 (O1) |
I reverted all temporary edits and pushed nothing. The worktree is clean at the reviewed head.
CI at the exact head. All required checks are green: Test Workspace, Lint, Format, GQT (4 cells), DST, RustFS (2 shards), Azurite, Storage Upgrade Compatibility, the V5↔V10 fence and the server aws feature.
Not run locally. I did not run the full workspace suite, Clippy, rustfmt, the GQT corpus, DST, or S3/Azure suites. For those, I rely on CI.
Regression proof (standard 2). This is a feature, not a bug fix, so no before/after regression proof applies. The new tests call new APIs (branch_list_detailed, create_branch_as), so they cannot compile at 864495a8; I do not count that as proof. GQT cannot express this contract: the GQ branch list statement stays names-only, and the creator window needs a failpoint seam. The Rust owners the PR extends (branching.rs, failpoints.rs, server data_routes.rs and auth_policy.rs, CLI parity_matrix.rs) are the cheapest existing owners.
Unknowns. The S3 request counts are the author's moto numbers; I did not reproduce them. I did not test the legacy fork path (O4) or listing latency.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| .await | ||
| .map_err(OmniError::storage)?; | ||
| let head = read_manifest_rows_projected(&branch).await?.head; | ||
| if head.native_branch.as_deref() != Some(native) { |
There was a problem hiding this comment.
O3 (optional). state::exact_head(head, Some(native)) (state.rs:1035) already answers "did this lifetime write the head". Use it here and at line 125, so the rule has one definition.
There was a problem hiding this comment.
Done in a82600d. Both sites call state::exact_head.
| assert_eq!( | ||
| details["feature"].fork.as_ref(), | ||
| Some(&loaded), | ||
| "a merge does not move the fork" |
There was a problem hiding this comment.
O6 (optional). This merge is a true merge, because both sides wrote. The fast-forward case also depends on the head_is_own decision. I ran it as a temporary probe: create feature, write on main, then branch_merge("main", "feature"). Result: FastForward. head was the new commit, fork stayed at the load commit, and updated_at was the new commit's time. The probe passed at this head. Add these three assertions to this test so the case stays covered.
There was a problem hiding this comment.
Done in a82600d. The test now fast-forwards main into an unwritten branch ff. It asserts that the merge is FastForward, that the head and updated_at follow the new commit, and that the fork stays at the creation commit.
| .checkout_version(contents.parent_version) | ||
| .await | ||
| .map_err(OmniError::storage)?; | ||
| let fork = match crate::legacy::at_version(root, &created.session(), legacy, &created).await? { |
There was a problem hiding this comment.
O4 (optional). No test reaches this Some(at) arm. All new tests use graphs created at the current stamp. Also, LegacyAt::PreGenesis makes served fail, and that error fails the whole listing for the graph. Add one assertion to the storage-upgrade owner (the stamp-13 / 0.11 journey that builds branches, a merge, a deleted and recreated branch, and a fork of a deleted branch). After the upgrade and one write on a forked branch, branch list --detail --json must report fork equal to the commit the branch was created at.
There was a problem hiding this comment.
Added in a82600d to db::upgrade::tests::assert_converted, which runs on every converted route (stamps 13, 9 and 8). After conversion, the detailed listing reports:
feature, which wrote its own head, with its fork at main's commit 2. That fork is read from the legacy history at the pre-upgrade creation version, theSome(at)arm.fresh, forked fromfeatureat commit 5.child, forked from the retiredgoneat commit 3.
On PreGenesis: it means the creation version lies below main's first commit, so the branch was created before the graph had any commit. I have no fixture that produces that history. For now it fails loudly, like any other failed read, rather than inventing a fork. If such graphs exist, fork: null would be the honest answer, and that belongs in a change that comes with the fixture.
| /// created from, each with one scan of the rows. Measured at exactly 8 requests | ||
| /// on an S3 emulator (5 for a branch that has written none); a local store | ||
| /// counts 3, because Lance reads local files without the counting wrapper. | ||
| const DETAILS_PER_BRANCH_BUDGET: u64 = 8; |
There was a problem hiding this comment.
O1 (recommended). On a local store this gate cannot see an extra __manifest scan per branch. I tested it. I added one more read_manifest_rows_projected(&branch) in read_branch, and the test still passed:
| head | with extra scan | |
|---|---|---|
manifest_reads (2 / 6 written branches) |
8 / 20 | 8 / 20 |
manifest_scans |
5 / 13 | 7 / 19 |
manifest_reads is the only gated counter. Locally it moves 3 per written branch against a budget of 8. The S3 twin is the sensitive gate, but CI sets no bucket for it.
Suggestion:
- Gate the local slope on
manifest_scan_count: exactly 2 per written branch at this head. - Keep a separate read budget per backend: 3 local, 8 on S3.
DELETE_PER_BRANCH_BUDGETin this file already pins the exact local value. - Add
manifest_scan_countto the history-flatnessassert_flat.
There was a problem hiding this comment.
Done in a82600d. The local gate and its S3 twin now share one body, assert_branch_list_detailed_cost.
- Scans are gated exactly on every backend: +1 for a branch that has written nothing, +2 for a branch that has written. The scan count must not move across
DEEP_HISTORY_DEPTHcommits (assert_flatwith slack 0). - Each backend pins its own measured read budget: local 1 and 3, S3 5 and 8.
Your extra-scan mutation now fails the exact scan assertion.
| let root = root_uri.trim_end_matches('/'); | ||
| let main = open_manifest_dataset_with_session(root, None, control_session).await?; | ||
| let mut named = Vec::new(); | ||
| for (native, contents) in list_live_manifest_branch_contents(&main).await? { |
There was a problem hiding this comment.
O2 (optional). This loop repeats ManifestCoordinator::list_graph_branches (lib.rs:2472). It has the same skip of main, the same logical-name mapping and the same "more than one live native incarnation" error. The create-namespace check at branch_control.rs:173 repeats that error over a filtered subset. Extract one helper that returns (logical, native, BranchContents), sorted, and use it in both listings. Then branches and details cannot disagree after a later change to what counts as a live branch.
There was a problem hiding this comment.
Done in a82600d. live_named_branches in branch_detail.rs is now the only enumeration, and both list_graph_branches and list_graph_branches_detailed call it. I left ensure_manifest_branch_create_namespace alone. By design it reads only the refs that overlap the target, so that another branch's malformed retirement record cannot fail a create (lance.md, "Branch ref reads"). That makes it a different read rather than a copy.
| let Some(actor) = actor else { | ||
| return Ok(()); | ||
| }; | ||
| crate::branch_control::record_branch_creator(&ds, &native, actor) |
There was a problem hiding this comment.
O5 (coordination). #1016 changes this function. It moves the native create into create_branch_from(ds, name, version) and adds create_branch_at. Whichever PR lands second must put record_branch_creator in that shared door. If not, branches started at a commit list actor_id: null.
Separately, on the Lance side: Lance 11 Branches::create is pub(crate) and always writes empty metadata, so this second write is required on 11. At the bump that accepts metadata at create, fold the actor into the create, and delete BRANCH_CREATE_PRE_CREATOR and the Uncertain path.
There was a problem hiding this comment.
Agreed. #1016 has the same author. Whichever PR lands second moves record_branch_creator into the shared create_branch_from, so a branch started at a commit also records its actor. At the Lance bump that accepts metadata at create, the actor moves into the create, and BRANCH_CREATE_PRE_CREATOR and the uncertain path go away.
There was a problem hiding this comment.
Superseded: the creator is no longer recorded (81f5943), so there is no write to move into create_branch_from.
- One live-branch enumeration (`live_named_branches`) serves both `list_graph_branches` and `list_graph_branches_detailed`, so names and details agree on what is live; `head_is_own` uses `state::exact_head`. - The cost gate pins scans exactly on every backend (one per branch that has written nothing, two per branch that has, flat in history) and a measured read budget per backend (local 1/3, S3 5/8). - The storage-upgrade owner asserts forks served from the legacy history; the branching test pins a fast-forward into an unwritten branch. - The branching guide says the listing is not one cut across branches, that a racing create can list without an actor, and how to confirm an uncertain create.
…nch-list-details # Conflicts: # crates/omnigraph-cli/src/cli.rs # crates/omnigraph-cli/src/main.rs # crates/omnigraph-cli/src/output.rs # crates/omnigraph/src/db/mod.rs # crates/omnigraph/src/db/omnigraph.rs # docs/user/operations/server.md
|
Review addressed in a82600d, and the branch now merges
The merge needed one adaptation to #1047: Locally, with
|
Review of #1012 at
|
| Case | __manifest reads |
scans |
|---|---|---|
| One added unwritten branch | +1 | +1 |
| 2 written branches, plus main | 8 | 5 |
| 6 written branches, plus main | 20 | 13 |
After 40 more commits on b0 and main |
20 | 13 |
The step from 2 written branches to 6 is 3 reads and 2 scans per added branch. Those are the local budgets, so <= is tight here.
I then added one extra read_manifest_rows_projected in read_branch. The gate failed. Scans were +2 and the assertion expects +1. Object reads stayed +1, because a local store does not count the file read. The scan assertion is the check that sees the extra scan. I removed the extra call.
O2. Fixed. live_named_branches in crates/omnigraph-catalog/src/branch_detail.rs is the only enumeration. list_graph_branches and list_graph_branches_detailed both call it. Names and details agree on which refs are live.
ensure_manifest_branch_create_namespace stays a separate read. It checks the source and the names that share the target path. A bad retirement record on some other branch cannot fail a create. That is the branch-ref fence in docs/dev/lance.md.
O3. Fixed. Both head_is_own sites call state::exact_head. For a named branch this is the same test as before: the head row's native_branch equals that ref. For main, exact_head(&head, None) is true when the head row names no native branch. Main has no created_at, so updated_at still uses the head time. The public main row does not change.
O4. Fixed. assert_converted in crates/omnigraph/src/db/upgrade/tests.rs checks the detailed listing on every converted route (stamps 13, 9 and 8). I ran those three tests. feature wrote its own head, so its fork is read at parent version 2 through the legacy arm. Each run printed FORK_ARM ... version=2 arm=legacy for feature. The forks match the fixture: feature at commit 2, fresh at commit 5, child at commit 3 from retired gone. child and fresh have written nothing, so the fork is a copy of the head and they do not call the legacy arm. The test still asserts those fork ids.
LegacyAt::PreGenesis still fails the whole listing. The author has no fixture whose creation version sits below main's first commit. The refusal is the fail-closed outcome in invariant 8. A fork: null answer needs a fixture that produces that history.
O5. Still open, as coordination. #1016 at 118081c8 still moves the native create into create_branch_from and adds create_branch_at. That shared door does not call record_branch_creator. Whichever pull request lands second puts the creator write in that door. The author agreed on the thread. This head has one create door, and that door records the actor.
O6. Fixed. branch_list_detailed_reports_head_fork_creator_and_lifetime now fast-forwards main into the unwritten branch ff. I ran the test. It passed. The merge is FastForward. The head and updated_at follow the new commit. The fork stays at the creation commit.
O7. Fixed. docs/user/branching/index.md now states three facts. Each branch is read on its own, so the details are not one cut across branches. A create that is still recording its actor lists actor_id: null for that moment. After an uncertain create, a retry answers 409 (already exists). List the branches to confirm the branch exists.
Merge with main (ff9519fe)
The merge conflicts were in the CLI entry points, crates/omnigraph/src/db/mod.rs, crates/omnigraph/src/db/omnigraph.rs, and docs/user/operations/server.md. The resolution keeps both sides.
- The engine exports
BranchDetailandBranchListingnext to the commit-page types from feat(commits): page a branch's commit list #1013. ReadCheckdocuments the diff continuation and the detailed listing.- The server route table names
GET /branches?detail=trueand the paged commit list. --detailis a flag onBranchCommand::Listincrates/omnigraph-cli/src/commands/history.write_branch_list_humanwrites the text. Both CLI listing paths refuseog_catalog, including--detail.- MCP
branches_listcallsserver_branch_listwith default query parameters. It stays names-only.
#1013 pages GET /commits. A page returns the newest commits. The fork is the source head at creation, and a later write does not move it. One page does not answer that question. The listing still removes the N lineage reads.
#1059 refuses --as on reads, including branch list. --detail does not change that class. Branch create still records an actor.
Tradeoffs and software liability
The delta removes one drift risk. The names listing and the detailed listing now share live_named_branches. A later change to "what is live" cannot update one and leave the other. The cost gate now fails when a listing adds a scan per branch. That is invariant 13: the instrument detects the regression it claims to prevent.
The liabilities from the first review are unchanged.
- One durable key on Lance refs. Retirement inserts its key into the existing metadata map and writes that map back (
retire_branch_recoverably), so the creator key survives. Older binaries read only the retirement key. - One new create outcome. The branch exists while the call reports
Uncertain. - About four more object-store requests per create that names an actor.
- A second writer of ref objects. Create writes empty metadata.
replace_metadatathen replaces the whole map (lance-11.0.0Branches::createandreplace_metadata). Lance 11Branches::createispub(crate)and cannot take metadata. At the bump that accepts metadata at create, fold the actor into the create and delete the seam and the uncertain path.
Five similar provenance fields should share that one post-create write. Do not add one ref rewrite per field. #1016's start commit is the next field of this kind, and O5 is that shared door.
One branch's read error still fails the whole listing. That matches invariant 8. Cost stays linear in branch count and flat in history. A fork is immutable for its native ref, so a version-pinned cache is allowed later if listing frequency needs one. I did not measure latency.
Validation
Reviewed head: b50afff53ebc1e8ecc99de367bcb5be2d8dbf2cf. It merges a82600d7 with main ff9519fe. The previous review head was 6778c115. Isolated worktree. Rust 1.97.1. RUST_MIN_STACK=16777216. CARGO_TARGET_DIR=/workspace/target. Commands used --locked -p omnigraph-engine --features failpoints.
| Command | Result |
|---|---|
--test integration branch_list_detailed |
3 tests. 2 passed. The S3 twin skipped because OMNIGRAPH_S3_TEST_BUCKET was unset. Libtest reports ok, and the body did not run. Local counts are in the O1 table |
Extra read_manifest_rows_projected in read_branch, then the local cost test |
Failed: scans +2, expected +1. Reads stayed +1. Reverted |
--test failpoints branch_create_reports_an_unrecorded_creator_as_uncertain --exact |
1 passed |
--lib source_root_is_converted_once |
3 passed (stamps 13, 9 and 8). Each printed FORK_ARM ... version=2 arm=legacy for feature |
I removed the temporary log and the extra scan. I pushed nothing. The worktree is clean at the reviewed head.
CI at this exact head. Run 39103149837 completed with success. That includes Test Workspace, Lint, Format, GQT (4 cells), DST, RustFS (2 shards), Azurite, Storage Upgrade Compatibility, the V5 to V10 fence, and the server aws feature.
Not run locally. The full workspace suite, Clippy, rustfmt, the GQT corpus, DST, and the S3 and Azure suites. For those I use the CI run above. I did not reproduce the S3 request counts 5 and 8. I did not measure listing latency.
Regression proof. This delta does not fix a bug that a .gqt case can express. GQ branch list stays names-only. The creator window needs a failpoint seam. The new assertions extend the existing owners: branching.rs, branch_control_cost.rs, and db/upgrade/tests.rs. I did not run a before-and-after GQT case. A test that cannot compile on the parent is not proof. The extra-scan run above is the proof that the O1 gate now fails for the predicted reason.
|
Alignment note from #1016 (branch create at a commit). #1016 adds a second native create path. Creator. This PR records the creator in
|
Merges main at b8e82ab, including ModernRelay#1016 (start a branch at a graph commit), and reshapes the detailed branch listing: - The creating actor is no longer recorded. Its second write of the new ref added a durable key, a crash seam and an uncertain create outcome, and a late landing after a delete could bring the deleted ref back as live. The listing is now purely derived and writes nothing. - The start is reported as `from` and `from_commit_id`, the names the create receipt uses since ModernRelay#1016, instead of a `fork` commit object. A start at a past commit clones that commit's version, so the same read answers it. - Following the draft HTTP API conventions: the query parameter is `detailed`, and absent optional fields are omitted instead of null.
ragnorc
changed the title
|
Reworked in 81f5943 and 87bc0a8, which also merge
|
Review of #1012 at
|
| Command | Result |
|---|---|
--test integration branch_list_detailed (engine, failpoints) |
3 tests reported. 2 passed. The S3 twin printed skipping: OMNIGRAPH_S3_TEST_BUCKET is not set and did not run its body |
| Local cost counts from that run | Unwritten branch: +1 read, +1 scan. 2 written branches plus main: 8 reads, 5 scans. 6 written plus main: 20 reads, 13 scans. After 40 more commits on b0 and main: 20 reads, 13 scans. The step from 2 to 6 written branches is 3 reads and 2 scans per branch. Those are the local budgets |
| Same lifetime test after the temporary wrong-start edit | Failed, as above. Restored. Then 1 passed |
--lib converted_once_and_is_then_current (failpoints) |
3 passed (stamps 13, 9, and 8) |
server --test integration remote_branch_list_create_merge_flow_works |
1 passed |
server --test integration branch_list_is_denied_by_a_branch_scoped_read_rule |
1 passed. GET /branches?detailed=true is forbidden with the names route |
CLI --test integration branch_list_outputs_sorted_branches |
1 passed |
cargo clippy -p omnigraph-catalog -p omnigraph-api-types -p omnigraph-engine -p omnigraph-server -p omnigraph-cli --locked -- -D warnings -W clippy::dbg_macro |
Exit 0. This is the library target of those packages |
python3 scripts/check-docs.py |
Documentation OK (347 Markdown files checked) |
CI at this exact head. Run 39385165789 (CI) completed with success, including Test Workspace, Lint, Format, RustFS (2 shards), Azurite, Storage Upgrade Compatibility, the V5 to V10 fence, and the server aws feature. GQT run 39385165404 and DST run 39385165448 also completed with success.
Not run locally. The full workspace suite, clippy --all-targets, rustfmt, the GQT corpus, DST, and the S3 and Azure suites. For those I use the CI runs above. I did not reproduce the S3 request budgets 5 and 8. I did not measure listing latency. Local clippy did not include integration-test targets. CI Lint did.
# Conflicts: # crates/omnigraph-cli/src/client.rs # crates/omnigraph-cli/src/commands/history/args.rs # crates/omnigraph-cli/src/commands/history/branch.rs
Review of #1012 at
|
| Command | Commit | Result |
|---|---|---|
cargo test -p omnigraph-cli --test integration cli_data::catalogue_scope_reads_the_binary_registries_without_a_store -- --exact |
771a269d |
Failed. 1 test ran. stderr: target_incompatible: branch list is not available for this target kind and scope |
| same command | 16b832f5 |
1 passed. The test checks the plain command and --detailed |
cargo test -p omnigraph-cli --test integration cli_data::branch_list_outputs_sorted_branches -- --exact |
16b832f5 |
1 passed |
cargo test -p omnigraph-server --test integration data_routes::remote_branch_list_create_merge_flow_works -- --exact |
16b832f5 |
1 passed |
cargo test -p omnigraph-server --test integration auth_policy::branch_list_is_denied_by_a_branch_scoped_read_rule_as_the_route_is -- --exact |
16b832f5 |
1 passed |
A first filter without the cli_data:: prefix reported 0 tests. That run is not evidence. The rows above use the module name.
CI at this head. Run 39531431099 completed with success, including Test Workspace, Lint, Format, RustFS, Azurite, and Storage Upgrade Compatibility. GQ Logic Tests and DST also completed with success.
Not run locally. The full workspace suite, clippy, rustfmt, GQT, DST, S3, and Azure. The engine lifetime and cost tests were not rerun. branch_detail.rs, branching.rs, and branch_control_cost.rs are unchanged since 87bc0a81, and Test Workspace on this head passed. I did not measure latency.
GQT cannot express this contract. The refusal is a CLI target check, before a query. The owner is cli_data.rs. The before and after runs are that test.
I pushed nothing.
The graph access RFC gives each operation one JSON contract on the CLI, HTTP and MCP, and ModernRelay#1139 closed the MCP rows. `branch list` now has a detailed form on the route and the CLI, so the tool takes the same `detailed` argument, calls the route's handler with it, and declares `details` in its output schema. The branches resource stays names-only.
ragnorc
changed the title
Review of #1012 at
|
| Surface | Absent or false | True |
|---|---|---|
| HTTP | GET /branches |
GET /branches?detailed=true |
| CLI | branch list --json |
branch list --detailed --json |
| MCP | branches_list with graph |
branches_list with detailed: true |
All three serialize BranchListOutput. details is omitted when the flag is false. On main, from, from_commit_id, and created_at are omitted. A detail row always has name, head, and updated_at. That is the OpenAPI required list for BranchDetailOutput.
The MCP schema types the optional fields as string or integer, and it does not mark them null. The wire omits them. OpenAPI also allows null, because the fields are Option. The running code uses skip_serializing_if, so the body has no nulls. The parity row locks that body.
head in the MCP schema is { "type": "object" }. It does not copy CommitOutput. commits_get declares no output schema. A second copy of the commit fields would drift from the API type. The body still contains the commit. The parity row compares that body with the route.
The resource calls the same handler with BranchListQuery::default(), so detailed is false (mcp.rs). Its description says it returns branch names. That is the tool's default body. A resource has no argument slot for detailed.
The operation inventory still binds one operation, branch list, to branches_list and branch list. detailed is an argument of that operation. It is not a second operation. Tool names at this head match main da1c4636. The merge of #1139 did not drop a tool, and it did not add a second branch-list tool.
At d286a4c5, after both merges and before d549bcaa, the tool still parsed GraphArguments (graph only; an unknown field is rejected) and the handler passed ApiQuery(Default::default()). The route already accepted detailed. That was the gap this commit closes.
The call site is mcp.rs. The argument struct is arguments.rs. The output fields are catalog.rs.
Sibling pull requests
- rfc: narrow graph access to client contracts #1149 keeps the one-contract rule and narrows the graph access RFC to client contracts. This commit is that client argument. It adds no engine control fact.
- feat(query): a read describes its retrievals and score columns #1130 changes query diagnostics. It does not change the branch-list body.
- feat(cli): build a branch's declared indexes with omnigraph index build #1150 adds
index build. The graph access RFC already places index rebuild with the commands that run where the storage is. It does not redefinedetailed.
Tradeoffs and software liability
The delta closes one split. Before it, an MCP client that needed a head or a start had to leave the tool and call HTTP or the CLI, or walk commits. The tool now returns the same body as those surfaces, through the same handler.
The delta adds one optional field on an existing argument struct and one optional property on an existing output schema. It adds no durable state, no ref write, and no new manifest read. Five more optional flags of this kind should be fields on the route query, passed into the existing handler, with one parity row each. They should not grow a second listing function.
One branch read error still fails the whole listing. That is the behavior reviewed at 87bc0a81. Cost stays linear in the branch count and flat in history. I did not measure latency.
Validation
Reviewed head: d549bcaab124e41fa43fa3a62911703e77ea939b. Parent of the new commit: d286a4c5. Merges since 16b832f5: 18272d17 (main d5bf9fe4) and d286a4c5 (main da1c4636). Isolated worktrees. Rust 1.97.1. RUST_MIN_STACK=16777216. Commands used --locked.
| Command | Commit | Result |
|---|---|---|
cargo test -p omnigraph-server --test integration mcp::mcp_read_parity_tools_equal_their_routes --locked -- --exact --test-threads=1 |
d549bcaa |
1 passed. CARGO_TARGET_DIR=/workspace/target |
same command, with the detailed: true parity row added temporarily |
d286a4c5 |
Failed. The tool result was isError: true, code bad_request, message invalid MCP tool arguments. CARGO_TARGET_DIR=/tmp/pr1012-parent-target |
cargo test -p omnigraph-mcp --lib tests::every_tool_parses_its_required_arguments_and_writes_need_the_write_gate --locked -- --exact |
d549bcaa |
1 passed |
A filter without the mcp:: prefix reported 0 tests. That run is not evidence.
The parent failure is the gap: the tool rejected detailed, so it did not return the route body. At the head, the same row passes, so the tool body equals GET /branches?detailed=true on a graph that has main and a feature branch with its own commits. I removed the temporary row. I pushed nothing. Both worktrees are clean.
GQT cannot express an MCP argument. The owner is crates/omnigraph-server/tests/integration/mcp.rs. The case is already in d549bcaa.
CI at this head. Run 40275397060 succeeded for Test Workspace, Lint, Format, RustFS, Azurite, Storage Upgrade Compatibility, the V5 to V10 fence, and the server aws feature. GQ Logic Tests failed. The failed job is GQT (dst): case::load_csv_integer_endpoint_memory.gqt ended with replay_mismatch. The gate job then failed because that qualification failed. The diff of crates/omnigraph-gqt, crates/omnigraph/src/loader, crates/omnigraph/src/engine, and crates/omnigraph-dst against main da1c4636 is empty. Main's GQ Logic Tests run 40253020886 at da1c4636 succeeded. I do not treat that dst mismatch as a defect in this listing.
Not run locally. The engine lifetime and cost tests, clippy, rustfmt, the GQT corpus, DST, S3, and Azure. branch_detail.rs is unchanged since 16b832f5, and Test Workspace on this head passed. I did not measure latency. I did not remeasure the S3 read budgets. Those reads are unchanged.
What & why
GET /graphs/{graph_id}/branchesreturns names only. A client that shows each branch's head and start, such as OmniGraph Studio's branch menu and review, has to readGET /commits?branch=for every branch, which is a full lineage read each.GET /branches?detailed=true, the MCPbranches_listtool withdetailed, andomnigraph branch list --detailedadd adetailsarray beside the unchangedbranches. For each branch the caller mayread:head(CommitOutput)__manifestversion.fromBranchContents.parent_branch, as a logical name.from_commit_idBranchContents.parent_version. While the branch has written nothing, that is its head.created_atBranchContents.create_at, recorded to the second and reported in µs.updated_atcreated_at.fromandfrom_commit_idare the values thePOST /branchesreceipt (BranchCreateOutput) names. That holds for a start at a past commit (#1016), because that start clones the commit's own version. Onmainthe optional fields are absent.The listing writes nothing and stores nothing. Every field comes from the branch refs and each branch's own
__manifest, and no lineage is read.Backing issue / RFC
No accepted issue or RFC backs this yet. The change is row E9 of OmniGraph Studio's engine-change plan (
specs/012-omnigraph-engine-changes), which has no filed issue. It needs an issue or maintainer acceptance before merge.Draft RFC 0060 (#677, branch operations v2) only says "list/inspect branches: read live logical refs from one accepted manifest snapshot; expose graph identities, not storage-owner names". It defines no listing fields. This listing exposes logical names only, never native ref names.
The names follow the accepted HTTP API conventions RFC (#1089,
docs/rfcs/2026-10-19-http-api-conventions.md):detailed), the example that RFC itself names;*_id;*_atin µs;null.That RFC keeps the branch list unpaged until a separate page-token design, and says that design "also owns the branch-list
detailsthat #1012 adds, so the branch page is designed once". This PR does not page the listing. When that design lands,detailspages withbranches.The graph access RFC's "one set of operations" rule, which #1139 implemented for MCP, puts the option on all three surfaces: the HTTP query parameter, the MCP tool argument and the CLI flag.
Checklist
openapi.jsonreadon that branch (invariant 10).Local verification
All runs at head d549bca, which merges
mainat da1c463 (#1076, #1089, #1131 to #1133, #1139 and the rest since 7fd9cc9), withRUST_MIN_STACK=16777216and the failpoint featuresomnigraph-engine/failpoints,omnigraph-cluster/failpoints.target/was cold, so these are focused runs on the touched packages (omnigraph-engine,-server,-cli,-api-types,-catalog,-mcp), not the whole workspace graph.OMNIGRAPH_UPDATE_OPENAPI=1 … openapi::openapi_spec_is_up_to_date: regeneratedopenapi.json, then everyopenapi::test passes, includingspec_follows_the_api_conventions.branch_list,openapi::,mcp::,cli_mcp,catalogue_scopeandparity_branch_list: engine 4, server 121 and CLI 11 pass.--libfor all six packages andforbidden_apis: all pass except one server test,api_error_tests::an_uncertain_engine_failure_logs_the_status_its_response_sends. It failed once in the full run and passes 3 of 3 alone. It checks tracing log capture, which its own comment says a concurrent test can disturb, and this PR does not touch that file.cargo clippy --locked -p … --all-targets -- -D warnings -W clippy::dbg_macroover those packages: clean.cargo fmt --all --check,python3 scripts/check-docs.py,bash scripts/check-agents-md.sh,typos: clean.bound_storage,branch_control_cost::…_s3; the listing's reads did not change since 16b832f, where both passed at 5/8 per branch, flat at 52), the GQT corpus and the DST suite.Notes for reviewers
What "created" means. A branch name can have several lifetimes (
{logical}.{ULID}native refs). The listing describes only the live lifetime. After a delete and recreate, it reports the second creation and its start.fromnames the source as it was named then; that branch may since have been deleted or recreated.Start, not merge base.
from_commit_idis the creation point and stays fixed for the lifetime. A later merge moves the merge base (GET /branches/merge-base) but not the start; the engine test shows both. The merge-base lineage logic answers a different question, costs two lineage captures per branch, and has nothing to answer once the source branch is deleted.No creator. Branch creation records no actor; the actor only gates policy. An earlier revision of this PR recorded one with a second write of the new ref. That revision was dropped because of the liability it added:
Cost. One open of main's
__manifestand one ref listing. Then, per readable branch, a checkout and row scan of its latest version, plus the same for its creation version once the branch has written a commit. Branches are read 8 at a time.branch_control_cost.rspins the scans exactly: one per branch that has written nothing, two per branch that has, and none added by history. It also pins a measured read budget per backend. On an S3 emulator (moto) the listing costs:A local store counts 1 and 3 per branch, because Lance reads local files without going through the counting wrapper.
Policy. The listing keeps
GET /branches's scope-freereadcheck. Each branch's details are also checked againstreadon that branch, asGET /commits?branch=is, and a branch the caller may not read is listed inbranchesalone. With today's YAML policies, a scope-freereadgrant impliesreadon every branch, so this is defense in depth; the engine test covers the filter.Names.
Omnigraph::branch_list_detailed(&mut ReadCheck) -> BranchListing { branches, details: Vec<BranchDetail> }list_graph_branches_detailed;live_named_branches(the one live-branch enumeration, whichlist_graph_branchesnow also uses)BranchListQuery.detailed,BranchListOutput.details: [BranchDetailOutput]branch_list_output,branch_detail_outputbranches_listwithdetailed(BranchesListArguments); its output schema declaresdetailsbranch list --detailed; text keys mirror the JSON (head=,from=,from_commit_id=,created_at=,updated_at=)The
omnigraph://graphs/{graph}/branchesresource stays names-only.Tests.
branching::branch_list_detailed_reports_head_start_and_lifetimechecks every start against its create receipt. It covers:cleanup --keep 1;db::upgrade::tests::assert_convertedchecks starts served from the legacy history after a storage upgrade.branch_control_cost::branch_list_detailed_reads_bounded_per_branch_and_flat_in_history(+_s3).data_routes::remote_branch_list_create_merge_flow_works: details equalGET /commitsheads and the create receipt, and the absent fields are omitted onmain;auth_policy::branch_list_is_denied_by_a_branch_scoped_read_rule_as_the_route_is:detailed=trueis refused like the names;mcp::mcp_read_parity_tools_equal_their_routes:branches_listwithdetailedequalsGET /branches?detailed=true;openapi::: thedetailedparameter, its 400 andBranchDetailOutputpassspec_follows_the_api_conventions.parity_matrix::parity_branch_list(embedded vs served),cli_data::branch_list_outputs_sorted_branches(text).forbidden_apis:branch_list_detailedis registered as read-only.Merged since:
mainat da1c463:detailedwithApiQuery, so a malformed value answers the standard error body, and it declares that 400 (feat(server)!: say on every error body whether the request ran #1076, docs(api): enforce OpenAPI conventions and response coverage #1131);branchList, refactor(api): bind HTTP MCP and CLI operation inventories #1132, feat(api)!: standardize the 0.14 HTTP contract #1133);detailed(feat(mcp): expose every non-streaming graph operation as a tool #1139);og_catalogCLI case moved to main'somnigraph cataloguetest.--target) is reconciled.branch list --detailedresolves throughGraphClient::from_target, and the tests address graphs with--target.__manifeston the handle'scontrol_session(), and every checkout derives from that dataset. The detailed listing is part of thebound_storage.rsworkload, so the decoy-endpoint guard covers it.