[ragnorc]

Summary

Policy compared a request's branch with protected_branches exactly as written, while the engine (normalize_branch_name, in two copies), the change-feed handlers, the graph-diff routes and the CLI branch merge trimmed the name before opening it. A padded spelling such as " main" was therefore judged an unprotected branch and then resolved to main. With a policy that allows change, branch_merge and branch_delete only on unprotected branches, the new tests show this on the unfixed code:

• mutate_as, load_as and POST /mutate with " main", "main ", " main ", "\tmain" or "main\u{3000}" wrote to main;
• branch_merge_as("feature", " main") merged into main;
• branch_delete_as(" release") deleted a protected release branch.

Blob writes and the full-text rebuild were not affected (they normalized before the policy check).

The rule

One rule decides every branch name: omnigraph_policy::ensure_canonical_branch_name refuses an empty name, leading or trailing whitespace (str::trim's Unicode set) and control characters. These are the classes the GQ compiler already refused in branch statements. Nothing trims a branch name any more:

• Policy: PolicyEngine::authorize gives no decision on such a name; PolicyConfig::validate refuses such a protected_branches entry.
• Engine: one normalize_branch_name refuses instead of trimming; enforce checks the scope's names before any policy decision, so every _as writer refuses as a typed bad request before effects.
• Server: the shared authorize gate answers 400 before any rule, in every runtime mode; load's fork-if-missing lookup, the change routes (requested_branch) and the /diff sides apply the rule before they look anything up.
• CLI: branch merge applies the rule instead of trimming.

A branch-name newtype through PolicyRequest and ResourceScope was considered; the checks at the decision point (authorize), the engine door (enforce/normalize_branch_name) and the HTTP gate give the same property without touching every struct-literal site. Lance's check_valid_branch refuses whitespace and control characters in ref names, so no existing branch becomes unreachable.

Breaking: clients that sent padded names must send the exact name, and a policy listing a padded protected_branches entry fails validation (changelog.d/branch-names-as-written.breaking.md). Case, zero-width characters and homoglyphs are distinct names, not aliases of a protected branch, and are unchanged.

Tests

• policy_engine_chassis.rs::non_canonical_branch_names_cannot_reach_a_protected_branch (red before the fix: mutate_as(" main") reached a protected branch), plus padded Blob put/clear in blob_put_and_clear_enforce_change_for_the_actor.
• auth_policy.rs::non_canonical_branch_names_are_refused_before_policy replaces the change-route padded test and covers mutate, load, load from, merge, merge dry run, branch create, query, snapshot, commits, changes, baseline, /diff, /branches/diff, /branches/merge-base and a branch statement (red before the fix: POST /mutate with branch " main" reached protected main: 200).
• stored_queries.rs::invoke_stored_query_bad_param_is_400 gains a padded invocation branch.
• Policy unit tests: authorize refuses padded names on both sides and for unknown actors; protected_branches_must_be_canonical.
• CLI cli_data and parity_matrix now expect the refusal on both arms; warm_read_cost pins that a refused name issues no storage request.

Docs

docs/user/branching/index.md (new "Branch names" section), docs/user/operations/policy.md (documents protected_branches), and decision-log amendments to RFC 0055 and the exact-merge-receipts RFC, whose text described trimming.

Test plan

• cargo test --workspace --exclude omnigraph-gqt --exclude omnigraph-gqt-served --exclude omnigraph-dst --locked --no-fail-fast --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints: green except omnigraph-bench catalog_cli::served_preflight_rejects_incompatible_selection_before_acquisition, which fails only when its binary runs in parallel on a loaded machine and passes alone; it touches no branch name.
• cargo test -p omnigraph-gqt --locked --lib --test runner_dispatch (26/26 serially).
• cargo test -p omnigraph-gqt --locked: the only failures were cases exceeding the 10 s wall-time budget at load averages of 20-60 from other builds; each passes alone or times out identically on origin/main at the same load. CI is the clean run.
• cargo clippy --workspace --all-targets --locked -- -D warnings -W clippy::dbg_macro and the GQT clippy graph.
• cargo fmt --all --check, scripts/check-docs.py, scripts/check-agents-md.sh, typos.

[ragnorc]

Recommendation: merge. I found no blocking defect. Reviewed be8140c193db66b96ca1a681f057bb7a1132617c against 32caae4b67ec256d76d31e4aa87a33f1b24423d3.

This PR closes a policy bypass. Policy compared the supplied name with protected_branches, but the engine then trimmed that name. An actor restricted to unprotected branches could therefore write to protected main through " main". The fix refuses names that are empty, have surrounding whitespace, or contain control characters. Authorization and execution now use the same spelling.

The fix addresses the cause at the shared gates. The policy gate validates both sides of a transition before it evaluates a rule. The engine gate does the same, including with no policy or a custom checker. The shared engine normalizer no longer trims. I traced mutation, load/fork, merge, branch create/delete, Blob writes, full-text rebuild, snapshot reads, stored queries, and diff routes. The early HTTP checks cover paths that resolve a name before the general authorization gate. I found no remaining branch-name trimming path in these callers. Unattached-child writes authorize the recorded target and then require an explicit branch to match. Their existing privacy-preserving 404 can precede a malformed-name error when the actor cannot access the commit.

The design reduces long-term liability. It removes separate normalization rules and makes policy, storage lookup, and CLI merge receipts agree on one name. It adds one public validator and a small typed error. The engine and server adapters preserve their existing error contracts. It adds no stored state, migration, branch registry, or publication path. Five more entry points can use the existing gates instead of adding five local repairs. The tradeoff is a deliberate breaking change: clients must send exact names, and padded protected-branch entries now fail policy validation. The user guide and breaking-change note describe that cost. Rollout should follow the existing qualified-build rule: deploy the server/engine fix with the matching client build. The HTTP header alone does not identify this fix.

I read the complete Lance branch/tag and versioning pages and checked the pinned Lance 11 implementation at ab6b5bbe46009ed78746b444df8db59a8bc5d842. Its branch validator already rejects whitespace and controls. OmniGraph resolves native branch incarnations by exact logical-name equality. This refusal therefore does not hide a valid existing branch. Lance 13 retains the relevant validation rules. This is a narrow compatibility check, not a claim that OmniGraph supports all of Lance 13.

I checked the open PR set for overlap. #992's proposed MCP tools reuse the HTTP handlers and should inherit this rule. #951 and #963 do not require another branch identity. Optional integration note: preserve the checks on names as supplied when #968 rebases its diff preparation changes. That PR touches the same classifier and currently contains trimming in its diff-side bookkeeping. I found no reason to add another abstraction here for those future changes.

The existing Rust owners are appropriate. GQT trims branch headers before execution, and its branch statements already refused these names before this fix. It also has no policy/actor setup for this bypass. A GQT case would not prove the defect. The engine test checks protected heads and the live branch list, while the HTTP test checks the stored main head and the 400 response.

Local regression proof used the existing policy chassis test. I first ran it on the clean head: 1 passed. I then applied only the test-file diff to the base. The test file was byte-identical to the head version; production code remained at the base.

Commit	Result for the same regression
Before: 32caae4b67ec256d76d31e4aa87a33f1b24423d3	Failed, exit 101: mutate_as(" main") reached a protected branch. The stored main head advanced. 0 passed, 1 failed, 0 ignored.
After: be8140c193db66b96ca1a681f057bb7a1132617c	Passed, exit 0. 1 passed, 0 failed, 0 ignored.

The base stopped at the first padded mutation, so it proves that concrete bypass. The head completed the full regression matrix. Neither result was a compile failure or a zero-test run. Both runs used this command from the isolated checkout:

env RUSTUP_TOOLCHAIN=1.97.1 \
  CARGO_TARGET_DIR=/tmp/review-990/target \
  RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
  RUST_MIN_STACK=16777216 RUSTC_WRAPPER= \
  cargo test --workspace \
  --exclude omnigraph-gqt --exclude omnigraph-gqt-served \
  --exclude omnigraph-dst --locked \
  --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints \
  -j2 --test policy_engine_chassis \
  non_canonical_branch_names_cannot_reach_a_protected_branch \
  -- --exact --nocapture

The workspace selection keeps dependency features consistent; the command executes only this one test. All temporary edits are restored. The checkout is clean at the reviewed head, and no test change was pushed. Local git diff --check 32caae4 HEAD, cargo fmt --all --check, bash scripts/check-agents-md.sh, and scripts/check-docs.py in the documentation Python environment also passed.

CI evidence is separate from the local comparison. Test Workspace passed 4,410 tests and skipped 33. Its checkout was merge commit 1e9a4d8b51f5de050d1c5348a2bb19edc3407fa7. I fetched it and verified that its tree equals the reviewed head: 828ea8ef66ea5cc3935b5bd6bf90e2d8442120d0. The log includes the engine and HTTP regressions, policy validation, CLI merge checks, and the warm-read test. The listed GQT, S3, Azure, format, docs, and clippy checks also passed.

Limits: local execution uses macOS and file storage. The linker warned that __eh_frame exceeded its compact-unwind limit; both tests still ran. The HTTP and CLI tests are CI evidence, not local runs. I did not repeat the full workspace suite or live cloud tests locally. The name checks scan only the supplied string and add no storage operation. I did not benchmark the extra scans on valid names. CI's warm-read test checks zero storage requests for rejected names, but I did not measure request latency or throughput.

[cursor]

Recommendation: approve. Reviewed be8140c193db66b96ca1a681f057bb7a1132617c against base 32caae4b67ec256d76d31e4aa87a33f1b24423d3.

What and why

Policy compared a request's branch with protected_branches as the caller wrote it, but the engine and several HTTP routes trimmed the name before they opened it. An actor allowed to write only unprotected branches could therefore write to protected main through " main": policy classified that as an unprotected named branch, and the engine trimmed it to main. This is a security-relevant bypass of the Cedar branch-scope gate.

The fix introduces one public validator, omnigraph_policy::ensure_canonical_branch_name, that refuses empty names, names with leading or trailing whitespace (str::trim's Unicode set), and names with control characters. It is applied at three gates:

• Policy (authorize): before any rule evaluation
• Engine (enforce): before the policy checker or no-policy passthrough, so embedded writers without HTTP also refuse
• HTTP (authorize/require_canonical_branches): before authorization on every route that names a branch, including those that resolve a branch before the general gate (authorize_load_scope, requested_branch, required_diff_param)

All trim() calls on branch names are removed. The CLI branch merge uses the same validator instead of trimming. The design gives the same protection with one function at three decision points, instead of five or more local repairs.

Tradeoffs and software liability

The fix reduces long-term liability:

• One validator, one error type, three call sites. Five more entry points inherit the rule through the existing gates instead of needing their own repair.
• No new stored state, no migration, no branch registry, no publication path. The existing error contracts are preserved: the engine wraps it as a manifest/bad-request error, the HTTP layer wraps it as 400, the CLI surfaces either.
• Lance's check_valid_branch already refuses whitespace and control characters in ref names, so no existing branch is made unreachable.

The cost is a deliberate breaking change: clients that sent padded names must send the exact name, and a protected_branches entry with padding now fails validation. The changelog, user docs, and policy docs describe that cost. The RFC amendments (0055, exact-merge-receipts) update the text that previously described trimming.

Validation

CI: all 33 checks pass, including Test Workspace (4,410 tests), GQT (ordinary + DST + served), Lint, Format, Fix Regression Gate, and every S3/Azure/format fence.

Local regression proof on the isolated checkout at be8140c1:

Test	Result
policy_engine_chassis::non_canonical_branch_names_cannot_reach_a_protected_branch	1 passed, 0 failed, 0 ignored
omnigraph-policy unit tests (all 20, including protected_branches_must_be_canonical)	20 passed, 0 failed, 0 ignored

Local command:

CARGO_TARGET_DIR=/tmp/review-990-target \
  RUSTFLAGS='--cfg tokio_unstable' RUST_MIN_STACK=16777216 \
  cargo test --workspace \
    --exclude omnigraph-gqt --exclude omnigraph-gqt-served --exclude omnigraph-dst \
    --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints \
    --test policy_engine_chassis \
    non_canonical_branch_names_cannot_reach_a_protected_branch -- --exact --nocapture

Limits: local execution uses Linux amd64 and file storage. I did not repeat the full workspace suite, live S3/Azure tests, or served tests locally; CI is that evidence. I did not benchmark the additional str::trim and char::is_control scans on valid names; these scan a short string once per entry point and add no storage operation (confirmed by warm_read_cost asserting zero version probes for refused names).

I checked the open PR set for overlap. #992's proposed MCP tools reuse the HTTP handlers and inherit this rule. #951 and #963 do not touch branch identity. #968 touches the diff classifier and currently contains trimming in its diff-side bookkeeping; a rebase should preserve the checks on names as supplied.

Non-blocking observations

1. required_diff_param whitespace guard (crates/omnigraph-server/src/handlers.rs:4046): the guard !value.trim().is_empty() means an all-whitespace value like " " falls through to the _ arm and reports "query parameter 'from' is required". Under the "names as written" principle, " " was provided but is non-canonical, so the SurroundingWhitespace error message would be more precise. The refusal is correct either way — this is only about which message the caller sees. No action needed for this PR; the prior behavior here was also imprecise.

2. Check order in ensure_canonical_branch_name (crates/omnigraph-policy/src/lib.rs:330): the order (control chars, then whitespace, then empty) means "\t" reports ControlCharacter rather than SurroundingWhitespace, which is the more specific diagnosis. An empty string passes the first two checks cleanly (no chars, trim is identity) and hits the explicit empty check. The doc comment's reference to str::trim's Unicode set is useful because it makes the whitespace definition auditable — \u{3000} is not a control character but is whitespace, and the tests cover it. Good as-is.