The schema-lint chassis v1.2 (PR #100) shipped `--allow-data-loss` on
the CLI, but `SchemaApplyRequest` had no equivalent field — Hard-mode
drops were CLI-only. This commit closes that feature gap and adds e2e
test coverage for drop modes across HTTP + CLI, plus data preservation
on additive apply, plus a CLI↔SDK plan-parity assertion.
Feature gap closed:
- `crates/omnigraph-server/src/api.rs` — added `allow_data_loss: bool`
(default false via `#[serde(default)]`) to `SchemaApplyRequest`.
Added `Default` derive so test usages can use `..Default::default()`.
- `crates/omnigraph-server/src/lib.rs` — `server_schema_apply` now
constructs `SchemaApplyOptions { allow_data_loss: request.allow_data_loss }`
and threads through to `apply_schema_as`.
- `crates/omnigraph-cli/src/main.rs` — remote-URI schema-apply path
used to bail with "--allow-data-loss not yet supported on remote";
now forwards the flag into the JSON payload so the CLI behaves
identically against local and remote URIs.
- `openapi.json` — regenerated; only diff is the new field on
`SchemaApplyRequest`.
Tests added (8 new):
* `crates/omnigraph-server/tests/server.rs` (+5):
- `schema_apply_route_soft_drops_property_via_http` — POST schema
removing nullable property, verify catalog reflects the drop AND
`snapshot_at_version(pre)` still has `age` in the field list
(time-travel reachability is the Soft contract).
- `schema_apply_route_soft_drops_node_type_via_http` — POST schema
removing `Company` node + cascading `WorksAt` edge.
- `schema_apply_route_hard_drops_property_with_allow_data_loss` —
POST with `allow_data_loss: true`, verify plan step reports
`mode: hard`.
- `schema_apply_route_keeps_drops_soft_without_flag` — same schema
without flag, verify `mode: soft`. Pins default semantics against
accidental Hard promotion.
- `schema_apply_route_additive_property_preserves_existing_rows` —
load fixture, POST adding nullable property, verify row count
preserved (SDK suite covers data preservation on drops + renames;
additive AddProperty wasn't pinned).
Plus helpers `schema_without_age` and `schema_without_company`.
* `crates/omnigraph-cli/tests/cli.rs` (+3):
- `schema_apply_allow_data_loss_flag_promotes_drops_to_hard` — CLI
`omnigraph schema apply --allow-data-loss --schema X.pg --json`,
verify plan step has `mode: hard`.
- `schema_apply_without_allow_data_loss_keeps_soft_drops` — without
flag, verify Soft.
- `schema_plan_parity_cli_and_sdk` — same `.pg` source through
`Omnigraph::plan_schema` (SDK) and `omnigraph schema plan --json`
(CLI), assert the steps array is byte-identical post-JSON. HTTP
has no `/schema/plan` endpoint; apply-side parity is implicitly
covered by the HTTP drop tests + CLI drop tests using identical
fixtures.
Docs:
- `docs/user/schema-language.md` — new "Destructive drops" section
documenting Soft vs Hard semantics and that `allow_data_loss` is
now honored uniformly across CLI / HTTP / SDK.
Verification: every new test passes; full `cargo test --workspace --locked`
green; `scripts/check-agents-md.sh` passes.
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
* tests: policy chassis e2e gap-fills (MR-722 follow-up)
Audit after PRs #101-105 surfaced real e2e gaps in the policy chassis
that could let regressions ride through silently. Coverage was strong
at the SDK level (18 chassis tests) and reasonable at HTTP (12+ policy
tests), but the CLI×writer matrix was asymmetric (only `change` tested
end-to-end), the `cli.actor` config-only precedence path was untested,
the `OMNIGRAPH_UNAUTHENTICATED` env-var read path was unexercised,
`serve()`'s startup-refusal propagation was structural-review only,
and engine↔HTTP decision parity was a structural property without a
test pinning it. This commit closes those gaps.
Added (15 new tests, all test-only):
* `policy_engine_chassis.rs` (+2): `load_file_as` allow + deny pair —
PR #104 added the actor-aware mirror of `load_file` but it was only
exercised via CLI integration; this is direct-SDK coverage.
* `omnigraph-server/src/lib.rs` mod tests (+2):
- `unauthenticated_env_var_classification` — consolidated single
test (process-global env var; running parallel would race) that
pins truthy values, falsy values, unset, and CLI-flag-overrides-
env behavior of the `OMNIGRAPH_UNAUTHENTICATED` read path inside
`load_server_settings`.
- `serve_refuses_to_start_in_state_1_without_unauthenticated` —
`#[serial]` integration test. Clears all bearer-token env vars,
builds a `ServerConfig` with no policy file and no flag, calls
`serve(config).await`, asserts Err before any side-effecting
work (Lance dataset open, TcpListener::bind). Guards the
classifier→serve propagation path so a future refactor that
drops the call turns red.
* `omnigraph-server/tests/server.rs` (+4): `policy_decision_parity_*`
— four cases (Change×allowed+denied, BranchMerge×allowed+denied).
Each case runs the same Cedar decision via both SDK
(`Omnigraph::with_policy().mutate_as` / `branch_merge_as`) and HTTP
(`POST /change` / `POST /branches/merge`) and asserts both either
Allow or Deny. The structural property (both paths call
`PolicyChecker::check`) is now test-asserted.
* `omnigraph-cli/tests/system_local.rs` (+8): the CLI×writer matrix
fan-out:
- `local_cli_load_enforces_engine_layer_policy`
- `local_cli_ingest_enforces_engine_layer_policy`
- `local_cli_schema_apply_enforces_engine_layer_policy`
- `local_cli_branch_create_enforces_engine_layer_policy`
- `local_cli_branch_delete_enforces_engine_layer_policy`
- `local_cli_branch_merge_enforces_engine_layer_policy`
Each: one denied case (`--as act-bruno` against protected main) +
one allowed case (`--as act-ragnor` via existing/extended admins-*
rules).
Plus:
- `local_cli_actor_from_config_used_when_no_flag` — proves the
config-only precedence path works.
- `local_cli_actor_flag_overrides_config_actor` — proves the
`--as` flag wins over `cli.actor` in the config.
Adds `local_policy_config_with_actor` helper. Extends
`POLICY_E2E_YAML` with `admins-branch-ops` (BranchCreate +
BranchDelete) and `admins-schema-apply` rules so the CLI×writer
matrix has positive-case rule coverage.
Verification: all new tests pass; full `cargo test --workspace
--locked` is green; `scripts/check-agents-md.sh` passes.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* tests: serialize env-touching server lib tests to fix CI flake
CI flake on PR #106's Test Workspace job: two of the new tests
(`serve_refuses_to_start_in_state_1_without_unauthenticated` and
`unauthenticated_env_var_classification`) raced against
`server_bearer_tokens_from_env_reads_legacy_token_and_token_file`,
which sets `OMNIGRAPH_SERVER_BEARER_TOKEN` via `EnvGuard`.
While `serve_refuses` was mid-execution with its EnvGuard cleared,
the bearer-token test's EnvGuard had `OMNIGRAPH_SERVER_BEARER_TOKEN`
set; `resolve_token_source()` saw it and classified the runtime
state as `DefaultDeny` rather than refusing — so the test panicked
with "Dataset at path X not found" instead of the expected refusal
message. The unauthenticated test had the symmetric failure: its
`OMNIGRAPH_UNAUTHENTICATED="anything"` got overwritten by a peer
`EnvGuard` drop.
Fix: mark every test that uses `EnvGuard` with `#[serial]` so they
serialize against each other (default key). Already on
`serve_refuses_to_start_in_state_1_without_unauthenticated`; added
to `unauthenticated_env_var_classification` and
`server_bearer_tokens_from_env_reads_legacy_token_and_token_file`.
The `parse_bearer_tokens_json_*` tests don't touch env vars and
stay parallel.
Locally green (36 tests pass on my workstation); the parallelism
issue is CI-runner-specific (more aggressive thread interleaving)
but the fix is universal.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Closes the "tokens but no policy" trap. Pre-MR-723, an operator who
configured bearer tokens and forgot to set policy.file got a server
that required auth and then permitted every action — the illusion of
protection. After MR-723, that configuration is default-deny: only
`read` actions succeed; every other action returns HTTP 403.
Three startup states, classified deterministically:
- **Open** — no tokens, no policy. Requires explicit
`--unauthenticated` flag or `OMNIGRAPH_UNAUTHENTICATED=1`; otherwise
`serve()` refuses to start. Forces the operator to opt in to
"fully open dev mode" so it can't happen accidentally.
- **DefaultDeny** — tokens configured, no policy. `authorize_request`
rejects every action except `Read` with 403. The warn-log on
startup names the misconfiguration explicitly.
- **PolicyEnabled** — policy file configured. Cedar evaluates every
request, unchanged from pre-MR-723.
What landed:
- `ServerConfig.allow_unauthenticated: bool` + `--unauthenticated` flag
on the `omnigraph-server` bin + `OMNIGRAPH_UNAUTHENTICATED` env var
(`load_server_settings` honors both).
- New `classify_server_runtime_state(has_tokens, has_policy,
allow_unauthenticated) -> Result<ServerRuntimeState>` pure function.
`serve()` calls it before opening the engine and bails with a clear
error when the operator hits the no-tokens-no-policy-no-flag cell.
- `authorize_request` state-2 branch: when `policy_engine()` is None
but the bearer-auth middleware delivered an authenticated actor, any
action other than `Read` returns 403 with a message that names the
misconfiguration.
- `AppState::with_policy_engine(self, engine)` builder method so
integration tests that need a custom workload (`new_with_workload`)
can still install a permit-all policy without a new constructor.
- `app_for_loaded_repo_with_auth(token)` and
`app_for_loaded_repo_with_auth_tokens(tokens)` test helpers now
install a permit-all policy alongside tokens — they previously
represented the "tokens but no policy" state that MR-723 makes
default-deny, and tests that don't care about policy were
inadvertently coupled to the loophole.
Tests:
- `classify_*` unit tests (3) — every cell of the matrix.
- `default_deny_mode_allows_read_for_authenticated_actor` — GET
/snapshot succeeds with bearer token + no policy.
- `default_deny_mode_rejects_change_with_forbidden` — POST /change
rejected with 403 + "default-deny" message.
- `default_deny_mode_rejects_schema_apply_with_forbidden` — POST
/schema/apply rejected with 403 + "default-deny" message.
- New `app_for_repo_with_auth_tokens_only(schema, tokens)` helper
builds the State-2 fixture without policy. The pre-MR-723 helpers
`app_for_loaded_repo_with_auth*` shift semantics to "tokens +
permit-all" so existing tests retain their original intent.
docs/user/policy.md: new "Server runtime states (MR-723)" section
documents the matrix and the explicit `--unauthenticated` opt-in.
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Closes the CLI side of the policy chassis fan-out. Before this commit,
CLI direct-engine writes bypassed Cedar entirely because the CLI never
called `Omnigraph::with_policy(...)` for non-`policy validate|test|explain`
subcommands. After this commit, every CLI direct-engine writer
(change, load, ingest, branch create/delete/merge, schema apply) opens
the engine via a new `open_local_db_with_policy(uri, &config)` helper
that installs the configured `PolicyEngine` when `policy.file` is set,
and threads the resolved actor through to the `_as` writer methods.
Actor identity resolution:
- New top-level `--as <ACTOR>` global flag on the CLI overrides config.
- New `cli.actor` field in `omnigraph.yaml` provides a default actor.
- Precedence: `--as` > `cli.actor` > None.
- When policy is configured and neither is set, the engine-layer
footgun guard fires and the write is denied — silent bypass via
"I forgot the actor" is exactly what the guard prevents.
- Remote HTTP writes ignore both — bearer-token-resolved server-side.
Helpers added in main.rs:
- `open_local_db_with_policy(uri, &config) -> Result<Omnigraph>` —
opens the DB and installs the PolicyEngine when configured. Without
policy this is identical to a bare `Omnigraph::open`.
- `resolve_cli_actor(cli_as, &config) -> Option<&str>` — implements
the flag > config > None precedence.
Engine: added `load_file_as` to the loader as the actor-aware mirror of
`load_file`, so CLI file-path loads flow through the same enforce gate
as in-memory `load_as` calls.
Test rewrite: `local_cli_policy_tooling_is_end_to_end_while_local_writes_stay_unenforced`
was the explicit assertion of the pre-chassis hole. Renamed and split:
- `local_cli_policy_tooling_is_end_to_end` — sanity for the read-only
policy CLI surfaces (validate/test/explain), unchanged behavior.
- `local_cli_change_enforces_engine_layer_policy` — the new assertion:
policy installed + no actor → footgun-guard denial; `--as act-bruno`
on protected main → Cedar denial; `--as act-ragnor` (admins-write
rule) on main → permit, write committed.
POLICY_E2E_YAML gains an `admins-write` rule so the permit case has
a non-trivial actor to exercise.
docs/user/policy.md updated with `cli.actor` + `--as <ACTOR>` usage.
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
PR #102 wired apply_schema_as. This PR completes the chassis-side
coverage so every public mutating engine entry point hits the same
Omnigraph::enforce(action, scope, actor) gate regardless of transport:
- mutate_as → enforce(Change, Branch(branch), actor)
- load_as → enforce(Change, Branch(branch), actor)
- ingest_as → enforce(Change, Branch(branch), actor); also threads
actor through the implicit branch_create_from_as so fresh-branch
ingest correctly hits BranchCreate too
- branch_create_as → enforce(BranchCreate, TargetBranch(name), actor)
- branch_create_from_as → enforce(BranchCreate,
BranchTransition { source, target }, actor)
- branch_delete_as → enforce(BranchDelete, TargetBranch(name), actor)
- branch_merge_as → enforce(BranchMerge,
BranchTransition { source, target }, actor)
Three new _as variants for branch ops (create, create_from, delete)
that had no actor surface before; existing actor-less variants delegate
with actor=None so the no-policy path is a strict no-op.
HTTP handlers updated to thread the resolved actor into the new _as
variants for branch_create and branch_delete (was previously dropped).
14 new SDK chassis tests (one allow + one deny pair per wired writer);
the existing 4 apply_schema_as tests stay. All 18 pass.
docs/user/policy.md updated to describe engine-wide enforcement and the
coarse-vs-fine layer split (engine = action gate, query layer per-row =
MR-725 future). AGENTS.md capability matrix updated to match.
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
PR #2 of the policy chassis series (PR #1 = MR-731, merged in #101).
The structural fix that moves Cedar enforcement from HTTP-only to
engine-wide. apply_schema is the proof-of-concept writer; PR #3 fans
the enforce() call out to the remaining six (mutate_as, load,
ingest_as, branch_create_from, branch_delete, branch_merge).
## What lands
### New crate: omnigraph-policy
The 844-line policy.rs moves from `omnigraph-server` into a new
`omnigraph-policy` workspace crate so both engine and server can
depend on it. Cedar dependency moves with it. The server's policy.rs
becomes a re-export shim (`pub use omnigraph_policy::*`) so existing
`omnigraph_server::PolicyAction` etc. paths keep working — CLI and
test consumers don't have to migrate in one go.
### New trait: PolicyChecker
```rust
pub trait PolicyChecker: Send + Sync {
fn check(&self, action: PolicyAction, scope: &ResourceScope,
actor: &str) -> Result<(), PolicyError>;
}
```
`PolicyEngine` (Cedar-backed) implements it. `Omnigraph::with_policy()`
takes `Arc<dyn PolicyChecker>`. Engine tests mock the trait without
spinning up Cedar. MR-725 will extend the trait with `predicate_for()`
for query-layer pushdown — additive, no call-site changes.
### New enum: ResourceScope
Four variants — Graph, Branch, TargetBranch, BranchTransition —
mapping cleanly to today's `(branch, target_branch)` shape on
PolicyRequest via `to_branch_pair()`. Each engine writer picks the
variant that matches the existing HTTP-layer convention so engine
and HTTP evaluate the same Cedar decision.
**Invariant**: ResourceScope stays at branch granularity. Per-type
and per-row scope are MR-725's territory, not engine-layer's.
Adding Type/Row variants here creates two places per-type policy
can be evaluated, which can drift. See chassis design refinements
comment on MR-722 (2026-05-17).
### Omnigraph::with_policy() + enforce()
* New `policy: Option<Arc<dyn PolicyChecker>>` field on Omnigraph,
None by default (preserves embedded/dev no-enforcement mode).
* `with_policy(self, checker)` setter — builder-style, consumes self.
* `enforce(action, scope, actor)` — the gate. When policy is None,
no-op. When policy is Some AND actor is None, hard error — silent
bypass via "I forgot the actor" is exactly the footgun this gate
is here to prevent.
### apply_schema_as: first writer wired
* New public method `apply_schema_as(source, options, actor)` that
calls `enforce(SchemaApply, TargetBranch("main"), actor)` before
acquiring the schema-apply lock or doing any other work.
* Existing `apply_schema(source)` and `apply_schema_with_options(...)`
delegate to it with actor=None (no-actor variants).
* HTTP handler `server_schema_apply` updated to call apply_schema_as
with the resolved actor. AppState construction injects the
PolicyEngine into Omnigraph via `with_policy`. HTTP-layer
authorize_request still fires first; the engine gate is the
redundant-but-correct backstop and the only path that protects SDK
/ embedded callers. PR #3 removes the HTTP redundancy.
### OmniError::Policy
New error variant for engine-layer policy denial / evaluation
failure. ApiError::from_omni maps it to 403.
### MR-724 Admin action — Option A reservation
PolicyAction::Admin kept in the enum with a load-bearing doc
comment naming its future consumers (hot reload, audit log query,
approvals list per MR-726 / MR-732 / MR-734). No enforce(Admin, ...)
call site exists yet — the variant is reserved so the action
vocabulary is complete from chassis day one. MR-724 closes when
the first consumer surface ships.
### New SDK-side integration test
`crates/omnigraph/tests/policy_engine_chassis.rs` — four tests
covering:
* Policy denies for unauthorized actor → OmniError::Policy
* Policy permits for authorized actor → apply succeeds
* Policy installed + no actor → hard error (forget-the-actor footgun)
* No policy → no-op (embedded/dev default still works)
These exercise the engine path directly — no HTTP layer involved.
## Test results
- cargo test --workspace --locked --no-fail-fast: 851 passed, 0 failed
* 45 server tests (existing) pass
* 14 schema_apply tests (existing) pass
* 4 new chassis tests pass
* 60 OpenAPI tests pass (no HTTP API surface changes)
* No regressions across the workspace
## Architectural decisions baked in
Per MR-722 chassis design refinements comment (2026-05-17):
1. PolicyChecker is a trait, not just a concrete. Engine and server
consume the trait. MR-725 adds predicate_for() additively.
2. ResourceScope stays at branch granularity. No Type/Row variants.
3. Coarse-vs-fine framing pinned: engine-layer is action gate;
query-layer (MR-725) is predicate gate. Both backed by same Cedar
engine; non-overlapping responsibilities.
4. Admin action reserved for policy-management surfaces (MR-724
Option A).
## Pending follow-ups (PR #3+)
- Fan-out enforce() to mutate_as, load, ingest_as, branch_create_from,
branch_delete, branch_merge (PR #3).
- Remove HTTP-layer authorize_request redundancy once engine gate
covers all writers (PR #3).
- CLI policy injection into Omnigraph for non-`policy validate|test|explain`
subcommands (PR #3 or follow-up).
- MR-723 default-deny 3-state matrix (PR #4).
- MR-736 severity warn/deny (PR #5).
- AGENTS.md scope-of-enforcement rewrite once chassis fully lands.
- Coarse-vs-fine framing in docs/user/policy.md.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Warm-up commit for the policy chassis epic (MR-722). PR #1 of the
chassis series — same role as schema-lint v1's commit #1 baseline.
Zero behavioral change; establishes the regression test, the
load-bearing doc comment, and the user-doc paragraph for an
invariant already true in code.
Server auth already resolves `actor_id` from the matched bearer
token at `omnigraph-server/src/lib.rs:692-694`, overwriting whatever
the handler put in the PolicyRequest. The principle is named in
docs/dev/invariants.md Hard Invariant 11 ("clients cannot set actor
identity directly"). What was missing: a regression test, a
load-bearing doc comment at the resolution site, and a user-facing
documentation paragraph. This commit adds all three.
Why first. The actor-identity invariant is the foundation every
other policy decision stands on. If `actor_id` can be spoofed, every
chassis primitive (per-row scope, audit log, two-person rule)
becomes ungated. Pinning the invariant first means PR #2 (the
chassis core) doesn't have to re-prove this assertion.
Changes:
* crates/omnigraph-server/tests/server.rs — new regression test
actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers
with three sub-assertions:
- spoof-up: bearer for denied actor + X-Actor-Id naming allowed
actor → 403 (header doesn't promote)
- spoof-down: bearer for allowed actor + X-Actor-Id naming denied
actor → 200 (header doesn't demote)
- empty-string spoof: empty X-Actor-Id doesn't clear resolved actor
Cross-link to MR-777 (auth boundary cases — actor-id collision +
malformed bearer) noted in the test docstring.
* crates/omnigraph-server/src/lib.rs — expanded doc comment at
the actor-resolution site explaining the SECURITY INVARIANT,
citing Hard Invariant 11, the Supabase RLS history footgun, and
the regression test that pins the contract. Reader thinking "I
should let clients override actor_id for impersonation" hits
this comment first.
* docs/user/policy.md — new "Actor identity (signed-claim-only)"
section near the existing Server enforcement section. Closes the
user-facing doc gap MR-731's "Done when" requires.
Architectural decisions for PR #2+ pinned this session (not
implemented here, recorded so future implementers don't re-litigate):
- PolicyEngine moves to new `omnigraph-policy` workspace crate so
both engine and server can depend on it (Q2).
- `enforce(action, scope, actor)` will take a new `ResourceScope`
enum, leaving room for MR-725's per-type and per-row variants (Q3).
- `PolicyAction::Admin` is kept and wired (Option A) — meta-action
for policy-management surfaces (hot reload, audit log query,
approvals list) as those consumer features land (Q4).
Test results:
- cargo test -p omnigraph-server --test server: 45 pass (44 existing
+ 1 new); no regressions
- scripts/check-agents-md.sh: passes (34 links / 33 docs OK)
Out of scope (PR #2+):
- Omnigraph::with_policy() + enforce() method
- omnigraph-policy crate creation
- ResourceScope enum
- CLI policy injection into Omnigraph
- HTTP-layer redundant-check removal
- MR-724 Admin action wiring (PR #2)
- MR-723 default-deny 3-state (PR #4)
- MR-736 severity warn/deny (PR #5)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
* schema-lint v1 commit 5: --allow-data-loss flag + Hard mode
Final v1 commit. Wires up the --allow-data-loss CLI flag and Hard
mode for both DropProperty and DropType. Per
docs/dev/schema-lint-v1-plan.md, commit #5 of the schema-lint
chassis v1 series (MR-694).
CLI (omnigraph-cli/src/main.rs):
- New --allow-data-loss flag on both `omnigraph schema plan` and
`omnigraph schema apply` subcommands. Off by default (Soft).
- HTTP remote schema apply explicitly rejects the flag for now
(CLI-only; HTTP parity is a separate small follow-up that adds
the field to SchemaApplyRequest + the server handler).
Engine (omnigraph.rs + schema_apply.rs):
- New SchemaApplyOptions { allow_data_loss: bool } public struct
(Default = all false), re-exported via omnigraph::db::SchemaApplyOptions.
- New public methods: plan_schema_with_options and
apply_schema_with_options. Existing plan_schema/apply_schema are
now thin wrappers that pass Default::default().
- promote_drops_to_hard: post-plan walk that promotes every
DropMode::Soft step to DropMode::Hard when the flag is set.
Keeps the compiler's plan_schema_migration signature unchanged
(no breaking change for tests / callers).
- Apply path: both Drop arms accept Hard mode; behavior is
identical to Soft inside the apply loop. The DIFFERENCE is the
new hard_cleanup_targets: Vec<(String, String)> accumulator,
populated for every Hard variant with (table_key, full_dataset_uri).
- Post-publish cleanup: a new loop after the manifest commit
iterates hard_cleanup_targets and calls cleanup_old_versions
(before_timestamp = now) on each dataset URI. Best-effort —
the apply is already durable; cleanup failure is logged via
tracing::warn rather than failing the apply.
- New cleanup_dataset_old_versions helper inlines the Lance
cleanup_old_versions call against a dataset URI.
Behavioral details:
- DropProperty Hard: stage_overwrite produced a new dataset version
without the column. cleanup_old_versions removes the prior version
(and reclaims unique fragments). After Hard apply,
snapshot_at_version(pre_drop).open(table_key) FAILS because the
prior dataset version was reclaimed.
- DropType Hard: no per-table write happens (the change is the
manifest tombstone). cleanup_old_versions on the orphan dataset
is a no-op in the immediate term (no prior versions to clean
since the dataset wasn't modified by this apply). The dataset
directory persists. Full orphan-cleanup is a documented
follow-up — the user-facing contract is "data is unreachable
via omnigraph" (manifest entry tombstoned), which is satisfied.
Tests (tests/schema_apply.rs):
- apply_schema_with_allow_data_loss_promotes_drops_to_hard:
default plan emits Soft; with options.allow_data_loss=true,
plan emits Hard; apply succeeds.
- apply_schema_hard_drops_property_makes_prior_version_unreachable:
Hard drop succeeds, current snapshot lacks the column, and
snapshot_at_version(pre_drop).open("node:Person") FAILS (Lance
prior version reclaimed by cleanup).
- apply_schema_hard_drops_node_and_edge_with_flag_succeeds: both
Node and Edge DropType variants are promoted to Hard with the
flag; apply succeeds; current manifest entries gone. (Orphan
dataset directory cleanup deferred.)
Test results:
- cargo test -p omnigraph-compiler --lib: 239 passed
- cargo test -p omnigraph-engine --test schema_apply: 14 passed
(3 new Hard tests + 11 existing soft/regression tests)
- cargo test -p omnigraph-server --test openapi: 60 passed (no
HTTP API surface changes in this commit; OpenAPI parity follow-up
noted)
v1 status: complete for CLI/embedded use. MR-694 chassis epic +
MR-700 DropType/DropProperty ticket can close after this lands.
Known follow-ups (separate small PRs):
- HTTP parity: extend SchemaApplyRequest with allow_data_loss field,
thread through server handler, regenerate openapi.json.
- Orphan-dataset directory deletion for DropType Hard (currently
the dataset directory persists; cleanup_old_versions doesn't
remove it because the dataset wasn't modified).
- MR-948 substrate alignment: swap DropProperty Soft from
stage_overwrite to Dataset::drop_columns (catalog_only vs
full_rewrite cost class).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fixup: use bail! from color_eyre::eyre instead of anyhow
The remote-rejection branch in SchemaCommand::Apply used
anyhow::anyhow! which isn't in scope; the CLI's Result type is
color_eyre::eyre::Result and bail! is already imported.
Caught by CI Test Workspace job on PR #100.
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Wire the second half of the dormant Drop* family. Per
docs/dev/schema-lint-v1-plan.md, commit #4 of the schema-lint chassis
v1 series (MR-694). Builds on commit #3 (PR #90, DropProperty Soft).
Planner (schema_plan.rs):
- plan_nodes leftover loop: emit DropType { Node, name, Soft }
instead of UnsupportedChange (OG-DS-102) for node-type removals.
- plan_edges leftover loop: emit DropType { Edge, name, Soft }
instead of UnsupportedChange (OG-DS-103) for edge-type removals.
Apply (schema_apply.rs):
- New dropped_tables: BTreeSet<String> accumulator alongside
added_tables / renamed_tables / rewritten_tables.
- DropType arm in the metadata loop populates dropped_tables for
Soft mode. Hard mode errors (lands in commit #5 with
--allow-data-loss).
- New tombstone-emission loop after the rename sidecar build:
for each dropped table, push to sidecar_tombstones AND populate
table_tombstones with table_version + 1. The existing manifest
publish path converts table_tombstones into ManifestChange::Tombstone
operations — no new manifest plumbing needed.
- Soft DropType has no Phase B per-table write; the tombstone is the
entire change. Lance dataset files are retained — prior __manifest
versions still reference them, so time travel + branch-from-snapshot
can read the dropped table until cleanup_old_versions runs.
- Rides on SidecarKind::SchemaApply per MR-847 (already established
by commit #3).
Tests:
- Planner unit test plan_emits_soft_drop_for_removed_node_and_edge_types
asserts both Node and Edge DropType { Soft } emission for the
Company + WorksAt combined drop, plus no UnsupportedChange.
- Integration test apply_schema_drops_node_and_referencing_edge_softly
(replaces apply_schema_rejects_dropping_a_node_type): asserts
plan emission, apply success, current manifest entries absent,
pre-drop manifest entries present (time-travel reversibility),
reopen consistency.
- Integration test apply_schema_drops_an_edge_type_softly (replaces
apply_schema_rejects_dropping_an_edge_type): single edge drop,
asserts other tables untouched, time-travel reversibility.
Test results:
- cargo test -p omnigraph-compiler --lib: 239 passed (1 new + 238)
- cargo test -p omnigraph-engine --test schema_apply: 11 passed
(2 converted + 9 unchanged)
Pending for v1 completion:
- Commit #5: --allow-data-loss CLI flag + Hard mode promotion in
planner + immediate compact_files + cleanup_old_versions for
both DropProperty and DropType.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
* schema-lint chassis v1 (WIP): tier surfacing + plan doc
First commit of the chassis v1 branch. Lands a small, foundational
slice without behavior change, plus a planning doc that lays out the
remaining 7 commits in sequence so the PR can be reviewed
incrementally.
This commit:
- Adds SchemaMigrationStep::diagnostic() returning the full
&'static DiagnosticCode (family + tier + severity) for
UnsupportedChange steps with codes. Renderers can now reach the
tier without re-implementing the code → tier lookup.
- CLI `omnigraph schema plan` output now displays tier alongside
code:
unsupported change on node:Person.age [OG-DS-104, destructive]:
removing property 'Person.age' is not supported in schema
migration v1
Operators see at-a-glance the kind of risk each rejection
represents — not just the rule identifier.
- No behavior change. All 11 existing schema_apply tests still pass.
Planning doc at docs/schema-lint-v1-plan.md tracks the 7 remaining
commits to bring v1 to feature-complete:
1. (this commit) Tier surfacing in plan output.
2. Soft / Hard mode enum on drop steps.
3. Tombstone fields on catalog IR.
4. Planner emits DropProperty { Soft } by default.
5. Apply path implements Soft mode.
6. Convert PR #62 destructive-rejection tests.
7. --allow-data-loss flag + Hard mode.
8. (optional) Tombstone unhide / restore command.
Delete the planning doc when v1 lands. Intentionally checked in to
the WIP branch so the scope is reviewable; not intended as a
permanent doc.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* schema-lint v1 commit 2: DropMode + dormant Drop* variants
Second commit of the chassis v1 branch. Lands the type-level shape
of soft/hard drops without wiring them up. Variants are reachable
from emitters but the planner doesn't produce them yet; the apply
path returns an explicit not-yet-implemented error if one shows up
via deserialization.
Added:
- `DropMode { Soft, Hard }` — orthogonal to `SafetyTier`. Tier
classifies the rule's risk class; mode is the operator's intent
for data treatment.
- `Soft` → catalog tombstone, data retained. Tier: safe.
- `Hard` → Lance-level removal. Tier: destructive; will require
--allow-data-loss to apply (commit 7).
- `SchemaMigrationStep::DropType { type_kind, name, mode }` and
`SchemaMigrationStep::DropProperty { type_kind, type_name,
property_name, mode }` variants.
- Re-export `DropMode` from `omnigraph_compiler::DropMode` so
downstream crates don't reach into the catalog submodule.
- CLI `render_schema_plan_step` arms for both variants, surfacing
the mode in plan output: `drop property 'Person.age' of node
'Person' (soft mode)`.
- `apply_schema_with_lock` exhaustive match arm for the two new
variants that returns `manifest_internal` with a clear
not-yet-implemented message. If a SchemaIR JSON containing
Drop{Type,Property} arrives (e.g. from a future tool or hand-
written), the apply path fails explicitly rather than silently
misclassifying.
- Two new in-source tests:
- `drop_steps_round_trip_through_serde` — pins the wire shape
for all four (variant × mode) combinations.
- `drop_mode_serde_uses_snake_case` — pins external-tool-
friendly serialization (`"soft"` / `"hard"`).
Build: clean, only pre-existing warnings.
Tests:
- omnigraph-compiler schema_plan: 6/6 (4 existing + 2 new).
- omnigraph-engine schema_apply: 11/11 (unchanged — planner still
emits UnsupportedChange for removal paths).
Next commit (commit 3 per docs/schema-lint-v1-plan.md): add the
`tombstoned: bool` fields to NodeIR / EdgeIR / PropertyIR for the
catalog representation of soft-mode tombstones.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* plan doc: reframe v1 around Lance native drop_columns
After a substrate audit of the Lance data-evolution guide on
2026-05-13, the v1 plan was simplified. Two key findings:
1. Lance's `drop_columns()` is already metadata-only and reversible
via time travel until cleanup. No need for a parallel
`tombstoned: bool` field in our catalog IR — Lance's version
graph IS the tombstone.
2. The full schema_apply substrate migration (add_columns,
drop_columns, alter_columns vs. stage_overwrite across all step
types) is consolidated in MR-948 as a sibling issue. v1 only
uses the relevant slice (drop_columns for OG-DS-1XX).
Net plan changes:
- Commit 3 (original): tombstone fields on catalog IR → dropped.
No catalog IR change needed. The Lance drop_columns commit IS the
tombstone.
- Commit 5 (original): apply path writes tombstoned: true → replaced
with: apply path calls Dataset::drop_columns([name]).
- Commit 7 Hard mode: stage_overwrite removing the column → replaced
with: drop_columns + compact_files + cleanup_old_versions. Same
APIs omnigraph cleanup already uses.
- Commit 8 (original): omnigraph schema unhide → dropped. Time
travel is the undo (omnigraph snapshot --at <commit>).
Net result: 8 commits → 5 commits. ~250 LoC less surface. More
substrate-aligned.
The chassis types from commit 2 (DropMode enum, DropType /
DropProperty variants) are kept exactly as designed; only the
implementation strategy changed.
The Lance docs quote is included in the doc so future readers see
the substrate behavior cited verbatim.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* schema-lint v1 commit 3: emit + apply DropProperty { Soft }
Wire the dormant DropProperty variant end-to-end for the Soft case.
Per docs/schema-lint-v1-plan.md, commit #3 of the schema-lint chassis
v1 series (MR-694).
Planner (schema_plan.rs):
- plan_properties: emit DropProperty { type_kind, type_name,
property_name, mode: Soft } instead of UnsupportedChange when a
property exists in accepted but not in desired. Plan is now
supported = true for drop-only changes.
Apply (schema_apply.rs):
- Route DropProperty { Soft } through rewritten_tables. The existing
batch_for_schema_apply_rewrite path already iterates the *target*
schema fields, so a property absent from desired_catalog is
naturally projected away. The prior Lance version retains the
dropped column for time-travel reversibility (until cleanup runs).
- DropType still errors (lands in commit #4 with different mechanics:
__manifest entry removal instead of column projection).
- DropProperty { Hard } still errors (lands in commit #5 with
--allow-data-loss CLI flag + immediate compact_files +
cleanup_old_versions).
Tests:
- Planner unit test plan_emits_soft_drop_for_removed_nullable_property
asserts the variant emission + supported = true + no UnsupportedChange.
- Integration test apply_schema_drops_a_nullable_property_softly_
preserves_prior_version (replaces the former
apply_schema_rejects_dropping_a_property_with_data) asserts:
(a) plan contains DropProperty { Soft }
(b) apply succeeds + manifest advances + row count unchanged
(c) current dataset schema lacks the dropped column
(d) snapshot_at_version(pre_drop) still has the dropped column
(e) reopen consistency — drop preserved across engine restart
Recovery: rides on SidecarKind::SchemaApply per MR-847. No new
sidecar kind needed; the entire apply path is already sidecar-wrapped.
Substrate alignment: this commit uses the stage_overwrite full-rewrite
path (full_rewrite cost class) rather than Lance native drop_columns
(catalog_only cost class). MR-948 is the follow-up substrate-alignment
refactor that introduces a LanceColumnOp surface and switches the
metadata-only case onto drop_columns. Functional outcome is identical;
cost-class improvement deferred.
Test results:
- cargo test -p omnigraph-compiler --lib: 238 passed
- cargo test -p omnigraph-engine --test schema_apply: 11 passed
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs: move schema-lint-v1-plan into docs/dev/ + add to index
Post-rebase fixup for the docs split (#93). The plan doc was added
to docs/ at the top level before main reorganized to docs/{user,dev}/.
This moves it into docs/dev/ and adds an entry to docs/dev/index.md
under a new "Active Implementation Plans" section so the
check-agents-md.sh link check passes.
Per the original commit message (617a77d), the plan doc is intentionally
temporary — it will be deleted when v1 lands.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First slice of the schema-lint chassis. Adds stable `OG-XXX-NNN`
codes to schema-migration rejections so operators can suppress, look
up, and filter on identifiers rather than free-text prose. Atlas-style
chassis adapted to omnigraph's typed-IR substrate (no SQL injection
vector, no per-engine locks, native edge/vector/embedding types).
What's in v0:
- New `omnigraph-compiler/src/lint/` module with:
- `diagnostic.rs` — Family / SafetyTier / Severity enums covering ten
families (DS, MF, CD, BC, NM, OW, NL, VE, ED, LK). Only DS and MF
are populated in this PR.
- `codes.rs` — 8 DiagnosticCode constants (OG-DS-101..105,
OG-MF-103, OG-MF-104, OG-MF-106). Five of the eight are wired to
real emission sites; the other three are reserved.
- Unit tests for catalog invariants: codes unique, prefix matches
family, suffixes are 3-digit, destructive defaults to error,
lookup() works, EMITTED_IN_V0 codes exist in ALL_CODES.
- `SchemaMigrationStep::UnsupportedChange` gains an optional
`code: Option<String>` field. New `unsupported_error_message()`
helper prefixes the message with `[code]` when present.
- 5 of 17 existing rejection paths now carry codes:
- `removing node type` → OG-DS-102
- `removing edge type` → OG-DS-103
- `removing property` → OG-DS-104
- `adding required property without backfill` → OG-MF-103
- `changing property type` → OG-MF-106
Remaining 12 paths carry `code: None` and are tagged as future work.
- `schema_apply` surfaces the formatted error (with `[code]` prefix);
CLI `omnigraph schema plan` renders the code on the
`unsupported change on <entity>` line.
- PR #62 destructive-rejection tests in `tests/schema_apply.rs` now
assert on the stable code (`msg.contains("OG-DS-104")`) instead of
the error-message substring. 11/11 tests pass.
- New `docs/schema-lint.md` documents the v0 catalog + the 10 families
+ Atlas prior art. AGENTS.md index updated.
What's explicitly NOT in v0 (subsequent PRs):
- No severity config in `omnigraph.yaml` (MR-694 §2).
- No `@allow(OG-XXX-NNN, "rationale")` suppression directive (§3).
- No `--allow-data-loss` flag or destructive-tier enforcement.
- No new `SchemaMigrationStep` variants (soft/hard drops, default,
widen/narrow). MR-700, MR-697 land those.
- No pre-migration checks (MR-941).
- No CD / VE / LK / NM family rules (MR-942..945).
- No CI integration (MR-946).
Tests: 235 compiler tests, 11 schema_apply integration tests, 14
lint module tests, 55 CLI tests — all green.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defensive — Lance 4.0.0 preserves the source dataset's flag through
Operation::Overwrite even when WriteParams omits it (pinned by the
prior commit's test), but setting it explicitly matches the public
overwrite_dataset path at line 454 and documents the dependency at
the call site so a future refactor doesn't accidentally drop it.
Setting it on a dataset created without stable row IDs is a no-op
per Lance's row-id-lineage spec, so this stays correct for legacy
datasets.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stage_overwrite is used by schema_apply to rewrite tables when an
additive migration touches data. If Lance Operation::Overwrite ever
stopped preserving the source dataset's enable_stable_row_ids flag,
every schema_apply that triggers a rewrite would silently disable
stable row IDs on the affected tables and downstream readers that
depend on _rowid stability (change-feed validators, index
reconcilers) would observe silent corruption.
Empirically Lance 4.0.0 does preserve the flag through Overwrite
even when WriteParams omits it — but the preservation isn't
documented at the Lance spec level, so pin it here. Any future
behaviour change surfaces as a test failure rather than silent
corruption.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These three files in crates/omnigraph/src/loader/ have no `mod`
declaration anywhere in the workspace and no `#[path = "…"]`
reference. They are not compiled — `touch`-ing them does not trigger
`cargo check` to recompile anything.
Their imports (`crate::catalog::schema_ir`, `crate::error::NanoError`,
`crate::store::manifest::hash_string`, `crate::types::ScalarType`,
`super::super::graph::DatasetAccumulator`) reference modules that no
longer exist in the engine crate, so they could not even be wired in
without further work. They are vestigial code from an earlier
monolithic crate layout. The live functionality is independently
implemented inside crates/omnigraph/src/loader/mod.rs.
These files have been orphaned since the initial public commit.
`cargo check --workspace --all-targets` and
`cargo test --workspace --no-run` both pass with no new warnings.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
Reviewer feedback on PR #62: the original
`cleanup_then_optimize_succeed_in_sequence` only unwrapped both calls
and asserted nothing, so it didn't validate the claimed sequencing
behavior. The concern that motivates the test is that cleanup destroys
version history and optimize on a freshly-cleaned table could trip on
dropped fragment refs or stale manifests.
Rename to `cleanup_then_optimize_preserves_rows_and_table_remains_writable`
and add three concrete postconditions: row counts in both Person and
Company tables survive the sequence; the head remains readable; and a
subsequent merge load still succeeds.
The previous version of `apply_schema_renames_node_type_via_rename_from_and_preserves_rows`
kept the node name as `Person` (`@rename_from("Person")`) and only renamed
a property. The planner only emits a `RenameType` step when the new name
differs from the accepted one, so the test name overstated what it
covered: a regression in `RenameType` step emission or in the
coordinator's table-key remap during type rename could pass while the
test still went green.
Rename the desired node from `Person` to `Human` (with
`@rename_from("Person")`), update the dependent edge endpoints to point
at `Human`, and assert both the `RenameType` step and that the manifest
table key has moved from `node:Person` to `node:Human`.
The audit of test coverage flagged three holes:
- `omnigraph optimize` and `omnigraph cleanup` had no integration tests
(no `maintenance.rs`). Add one covering empty/idempotent edges, the
policy-validation contract on `cleanup`, and head preservation under
aggressive policies.
- `apply_schema` only covered I32 -> I64 type-change rejection. Add the
symmetric narrowing case plus rejections for the other destructive
shapes (drop property with data, drop node type, drop edge type, add
required property without backfill) and assert the manifest version
doesn't advance. Add a positive `@rename_from` case to pin the
stable-type-id contract preserves rows through a rename.
- `docs/testing.md` was missing `validators.rs` and the new
`maintenance.rs` from its file table; bump the count and add rows.
* MR-786: merge-pair truth table with exhaustive op-variant matrix
Add crates/omnigraph/tests/merge_truth_table.rs that enumerates every
(left_op, right_op) cell from the operation vocabulary named in the
ticket — {noop, addNode, removeNode, addEdge, removeEdge, setProperty,
dropProperty, addLabel, removeLabel} — and asserts the deterministic
outcome of Omnigraph::branch_merge against a structured oracle.
The matrix is built in a 9x9 match in build_case, so adding a new
OpVariant is a compile-time, fail-on-omission task. Today's mutation
grammar only exposes insert | update set | delete (see
docs/query-language.md), so the 36 cells over the first six ops are
executable and the 45 cells involving dropProperty/addLabel/removeLabel
are recorded as Expected::Unsupported with a note. Each executable cell
spins up a fresh tempdir, applies one mutation per branch, calls
branch_merge, and asserts either:
* MergeOutcome (AlreadyUpToDate / FastForward / Merged) plus a
GraphAssert on the affected entities, or
* an OmniError::MergeConflicts whose entries match the expected
table_key + MergeConflictKind (row_id is optional because edge
ULIDs are generated at runtime).
branch_merge is directional, so the (L, R) and (R, L) cells live in
separate entries in the matrix and are run independently — the
op-pair symmetry encoded in build_case serves as the commutativity
oracle without doubling the runtime. End-to-end the suite runs in
~10s on a fresh build, well under the 30s budget asserted at the
bottom of the test.
Also adds a row to docs/testing.md so the test-coverage map points
future agents at this file.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* Use one Omnigraph handle for both branches
Self-review caught that the runner was opening two Omnigraph handles
on the same temp dataset (one for main, a second via Omnigraph::open
for feature). tests/branching.rs uses one handle and passes the branch
name to mutate_branch — same pattern works here and avoids any
cache-coherency surprises between the two handles. Also drops the
post-merge reopen, which only existed to give the second handle a
fresh snapshot.
Runtime drops ~10s -> ~9s.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* Assert exact conflict count, not subset inclusion
cubic and Devin Review both flagged that check_outcome's
Expected::Conflicts arm only enforces want ⊆ got, so a regression that
produces a spurious extra conflict (e.g. emitting both OrphanEdge and
a stray DivergentInsert) would silently pass the truth-table cell.
For a deterministic oracle that's the wrong direction — the cell pins
the exact conflict-artifact set, not a lower bound. Add an
assert_eq!(got.len(), want.len()) before the existence loop. All 36
executable cells still pass; runtime unchanged.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* Subsume 4 conflict tests in branching.rs into truth table
The four `branch_merge_reports_*_conflict` tests
(DivergentUpdate / DivergentInsert / DeleteVsUpdate / OrphanEdge)
were redundant with the deterministic-oracle cells in the new
`merge_truth_table.rs` and only added drift risk.
To preserve the post-conflict invariant that lived in
`branch_merge_reports_divergent_update_conflict` (target unchanged
after a failed merge), the truth-table runner now generalizes it:
on every `Conflicts` cell, main's state is asserted against
`state_after_apply_only(right_op)`. That gives strictly more
coverage than the deleted tests carried, since the invariant now
applies to *all* seven conflict cells, not just one.
The `UniqueViolation` and `CardinalityViolation` cases stay in
`branching.rs` — they're combinatorial (require >1 op per side
with a non-default schema) and out of scope for the pair-wise
truth table.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* Fix misleading 'Total edges: 0' comment in (AddEdge, RemoveEdge) cell
Devin Review flagged that the comment said 'Total edges: 0' while the
parenthetical math evaluates to 1 (matching `GraphAssert::base()`).
The assertion is correct; only the leading number in the comment was
wrong. Reworded to 'Net edges: … = 1 (matches base)' so the prose
agrees with both the math and the assertion.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
---------
Co-authored-by: Ragnor <ragnor@modernrelay.com>
Co-authored-by: Ragnor Comerford <ragnor.comerford@gmail.com>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The matrix cell d:merge×change:into-target already exercises this
race: pre-fix it flakes ~20% on shared-CPU hardware (sentinel 409s);
post-fix it passes 100% regardless of which side of the racing pair
returns first. That flake-to-stable transition is the regression
signal.
The replacement test (concurrent_merge_clean_409_does_not_poison_next_
change_on_target) tried to sharpen this by looping until the clean-
409 path fired and then strictly requiring it. On fast CI hardware
the race window never opens in 50 iterations, which made the strict
variant fail in CI despite passing 10/10 locally. The bug genuinely
needs a real concurrent writer to advance on-disk manifest during
the swap window — a deterministic failpoint can't substitute because
forcing the merge body to Err without a real concurrent writer leaves
no cache-vs-disk drift to validate.
Reverting to the matrix cell as the sole regression coverage. Updated
the comment in merge.rs accordingly.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
Switch from match-on-Result to if-let-Err so the refresh outcome and
merge_result outcome are checked independently, making the intent
clearer: 'attempt refresh; on Ok-merge-with-refresh-error propagate;
on Err-merge-with-refresh-error log and surface the original merge
error'. No semantic change — both shapes were valid (wildcard patterns
don't move the scrutinee) — but the if-let form sidesteps a
needs-second-reading question raised in code review.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
merge.rs: best-effort refresh on the Err path so a refresh-time
storage error doesn't replace the merge body's structured error
(typically the manifest_conflict that the HTTP layer maps to a 409
with a structured payload) with a less informative one. Ok-path
behavior is unchanged — there a refresh failure is propagated so the
caller knows the coord's cache is unsynced.
server.rs: bump MAX_ITERATIONS to 50 and assert at the end that the
named clean-409 path actually fired at least once. With ~20% per-iter
rate on shared-CPU CI (per the original MR-923 repro), P(no hit in
50) is < 0.002%. Without this assertion the test silently degraded
to exercising only the 200-merge path — covered already by the
matrix cell.
Both changes per Devin Review + cubic comments on PR #80.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
The previous fix used `self.refresh()` to sync the restored
coordinator's cache after the swap-restore window. `refresh` runs the
`RollForwardOnly` recovery sweep — which, on the merge Err path with a
phase-B failure (sidecar written, per-table HEAD advanced, manifest
publish skipped), would observe the merge's own in-flight sidecar and
close it here.
That violates the contract documented on `Omnigraph::refresh`:
> Engine-internal callers that already hold an in-flight sidecar
> (e.g. `schema_apply` mid-write) MUST use `refresh_coordinator_only`
> to avoid the recovery sweep racing their own sidecar.
The post-restore step's purpose is to sync the coord cache with disk,
not to run recovery, so `refresh_coordinator_only` is the right
primitive on both paths. CI surfaced this via
`branch_merge_phase_b_failure_recovered_on_next_open` in
`crates/omnigraph/tests/failpoints.rs`, which asserts the sidecar
persists after the failpoint fires.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
branch_merge_impl swaps the coordinator for the merge target, runs the
merge body, then restores the original coordinator. A concurrent /change
on the same target during this window publishes against the swapped
coord, advancing on-disk manifest state that the restored coord doesn't
see.
The post-restore refresh was previously gated on merge_result.is_ok(),
so the clean-409 path (merge body's post_queue_snapshot drift check
returning a recoverable conflict) left the restored coord's cached
snapshot stale relative to disk. The next sequential /change seeded its
publisher expected_versions from that stale cache and 409'd with
ExpectedVersionMismatch — a non-retryable conflict surfaced to a caller
with no concurrent writer of their own.
Refresh on both Ok and Err paths so cached state cannot diverge from
the manifest across the swap-restore window.
Add a focused regression test
(concurrent_merge_clean_409_does_not_poison_next_change_on_target) that
loops the cell-d scenario until the clean-409 branch fires and asserts
the follow-up sentinel succeeds in that branch specifically.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
Cursor Bugbot LOW on commit 3ad359d: try_admit_rewrite is defined and
tested but no HTTP handler calls it; the six handler OpenAPI
annotations declared status = 503 (added in 8e1a8e7) but try_admit
(the only path handlers invoke) returns 429 only. 503 was unreachable.
Fix: remove (status = 503, ...) from the six handler OpenAPI
annotations and regenerate openapi.json. Kept as forward-looking
infrastructure: try_admit_rewrite, global rewrite semaphore,
RejectReason::GlobalRewriteExhausted, ApiError::ServiceUnavailable,
the 503 branch in IntoResponse, --global-rewrite-cap, and
OMNIGRAPH_GLOBAL_REWRITE_MAX. When a future commit wires
try_admit_rewrite into a handler, the 503 OpenAPI annotation lands
alongside that wiring.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migrates `ingest_per_actor_admission_cap_returns_429` from env-var
override to direct `WorkloadController::new(1, ...)` construction via
`AppState::new_with_workload`. Removes the `EnvGuard` and the
`#[serial]` annotation that paired with it.
Why correct by design (AGENTS.md rule 9): the previous round's matrix
fix (commit 8bd9a5f) shielded the matrix from this test's env
mutation, but the broader bug class — "test A's process-wide env
mutation can leak into any test B that calls
`AppState::open` / `WorkloadController::from_env()`" — was still
reachable by any future test that didn't think to opt out. Closing
the class at the source: this test no longer mutates global state at
all, so no other test needs to defend against it.
Net effect:
- This test no longer needs `#[serial]` (was the only reason it was
marked) — runs in parallel with the rest of the suite.
- The matrix's defensive `with_defaults()` construction (commit
8bd9a5f) remains correct but is no longer required for correctness;
it's now a "belt and suspenders" guard against any FUTURE
env-mutating test.
Verified locally: both tests pass when run together; full server
suite (44 tests) green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 4 CI failure: Test Workspace and server-aws both red on
`concurrent_branch_ops_morphological_matrix` cell b
("merge × merge: same-target-distinct-sources") — second merge
returned 429 instead of 200. The matrix passes locally.
Root cause: cargo test runs tests in parallel by default. The admission
test `ingest_per_actor_admission_cap_returns_429` is wrapped with
`#[serial]` and an EnvGuard that sets
`OMNIGRAPH_PER_ACTOR_INFLIGHT_MAX=1` for its duration. Process-wide
env vars are visible to concurrently-running tests; the matrix's
`Harness::new()` called `AppState::open()` which delegates to
`WorkloadController::from_env()`, picking up cap=1 if it ran while
the admission test held the EnvGuard. With cap=1 + 2 concurrent
merges in cell b, one merge waits behind merge_exclusive while the
other is admitted; the waiter holds its admission permit, but a
fresh actor permit is needed when admission is per-actor — the
second merge's permit acquisition fails because the first hasn't
released yet, and 429 fires.
Fix (correct by design, AGENTS.md rule 9): the matrix harness builds
the WorkloadController explicitly via
`WorkloadController::with_defaults()` and passes it to
`AppState::new_with_workload`, the constructor added in commit
22d76db. Closes the bug class "tests pick up another concurrent test's
env override at construction time" — the matrix is now insulated from
any env-var manipulation in the rest of the test suite.
Verified locally: with `OMNIGRAPH_PER_ACTOR_INFLIGHT_MAX=1` set in the
environment, the matrix passes (it ignores env entirely now).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit added `concurrent_branch_ops_morphological_matrix`
covering 11 cells with stronger assertions (identity + post-op /change
+ reopen). The three narrow tests it replaces:
- concurrent_branch_create_from_distinct_parents_does_not_corrupt_coordinator
→ matrix cell f, with identity assertions added
- concurrent_branch_merges_distinct_targets_do_not_swap_into_each_other
→ matrix cells a + b + c, with identity assertions that close the
symmetric-swap blind spot cubic flagged on commit 64f2b99
- concurrent_change_during_branch_merge_preserves_writes
→ matrix cell d
The matrix retains the original tests' diagnostic granularity through
named cell labels in every assertion message ("[a:merge×merge:distinct-targets]
merge a"), so a CI failure points to the exact cell + invariant.
Net: 522 lines removed, 0 coverage lost. All other server tests pass
unchanged (44 total).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces three narrow concurrent_branch_* tests (folded in below) with
one parameterized matrix test covering 11 representative
(op_a, op_b, target_overlap) cells, asserting C1-C6 uniformly:
C1 — both complete (no deadlock; tokio::time::timeout(15s))
C2 — status: both 200 or exactly one clean conflict; never 500
C3 — per-target row count
C4 — per-target row identity (named persons present + absent — catches
the symmetric-swap class that count assertions miss; cubic P2 on
commit 64f2b99 flagged this gap on the round-3 merge race test)
C5 — engine state coherent (subsequent /snapshot consistent)
C6 — post-op /change on main succeeds (engine isn't poisoned)
Cells:
a. Merge × Merge, distinct targets — branch_merge_impl race pin
b. Merge × Merge, same target / distinct sources — merge_exclusive serialization
c. Merge × Merge, same source / distinct targets — fanout
d. Merge × Change, into target — per-(table, branch) queue
e. Merge × BranchCreateFrom, target — interaction with refresh path
f. BranchCreateFrom × BranchCreateFrom, distinct parents — round-1 race pin
g. BranchCreateFrom × BranchDelete, unrelated branches — disjoint state
h. BranchDelete × BranchDelete, distinct branches — concurrent refresh
i. BranchDelete × Change, distinct branch — refresh-side vs writer
j. BranchCreateFrom × Change, on source — fork-while-writing
k. Reopen consistency after concurrent pair — disk-vs-cache drift
Each cell:
- spins up its own tempdir + AppState so failures don't cascade,
- aligns the pair at a tokio::sync::Barrier so both reach the engine
close in time,
- wraps in a 15s deadlock timeout,
- asserts identity via a /read with the `get_person` fixture query
(specific names must be present on the right branch and absent from
the wrong one).
Subsumes:
- concurrent_branch_create_from_distinct_parents_does_not_corrupt_coordinator
(now cell f, with identity assertions added)
- concurrent_branch_merges_distinct_targets_do_not_swap_into_each_other
(now cells a + b + c, with identity assertions; the symmetric-swap
blind spot cubic flagged on commit 64f2b99 is closed)
- concurrent_change_during_branch_merge_preserves_writes
(now cell d)
Those three narrow tests are removed in the next commit so this lands
green standalone.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the cubic P2 finding on commit 22d76db: `Semaphore::new(concurrency.max(1))`
silently coerced --heavy-concurrency=0 to 1, so the JSON output reported
0 while execution actually used 1. Reported settings differed from
actual.
Adds an explicit `--heavy-concurrency > 0` check in `main()` (with a
helpful error message pointing to --heavy-batches=0 as the way to
disable heavy traffic) and a defensive `assert!()` inside
`drive_heavy_actor` so future callers can't pass 0 silently.
Verified: `bench_actor_isolation --heavy-concurrency 0` exits with
code 2 and the explanatory error message.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the Cursor Bugbot HIGH on commit 22d76db (round 2 review):
`branch_merge_impl` at `crates/omnigraph/src/exec/merge.rs:1085-1100`
still used the swap_coordinator_for_branch + operate +
restore_coordinator pattern across three separate
`coordinator.write().await` acquisitions. Two concurrent merges with
distinct targets would interleave their swaps, leaving each merge's
body running against the other's swapped coord — A's `feature_a →
target_a` would land its rewrite in target_b instead.
Adds `merge_exclusive: Arc<tokio::sync::Mutex<()>>` to `Omnigraph`,
held across the entire swap → operate → restore window in
`branch_merge_impl`. Concurrent branch merges now serialize relative
to each other; everything else (per-(table, branch) writer queues,
/change, /ingest) is unaffected.
Why the mutex rather than the deeper "operate on local coord"
refactor (the round-1 fix shape applied to `branch_create_from`):
`branch_merge_on_current_target` calls `self.snapshot()` and
`self.ensure_commit_graph_initialized()` internally, which use
`self.coordinator` directly. Threading an explicit target coord
parameter through the merge body would unwind dozens of call sites.
The mutex is a smaller intrusion that fully closes the race.
Documented as a follow-up if telemetry shows merge concurrency
matters.
Pinned by `concurrent_branch_merges_distinct_targets_do_not_swap_into_each_other`
(previous commit). Pre-fix: M=4 iterations of concurrent merges
deterministically corrupted target row counts. Post-fix: all M
iterations land each merge on its declared target. The two adjacent
branch concurrency tests
(`concurrent_change_during_branch_merge_preserves_writes`,
`concurrent_branch_create_from_distinct_parents_does_not_corrupt_coordinator`)
still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per AGENTS.md rule 8, this commit lands the failing regression test
ahead of the fix.
Cursor Bugbot HIGH on commit 22d76db rediscovered the residual flagged
in the round 1 honest-review note: `branch_merge_impl` at
`crates/omnigraph/src/exec/merge.rs:1085-1100` still uses the
swap_coordinator_for_branch + operate + restore_coordinator pattern
across three separate `coordinator.write().await` acquisitions. The
same shape that branch_create_from_impl shed in commit 4ffbf6e.
The test spawns two concurrent /branches/merge calls A (feature-a →
target-a) and B (feature-b → target-b) aligned at a tokio::sync::Barrier
so both reach swap_coordinator_for_branch close in time. M=4
iterations boost race-catching odds.
Currently fails on 22d76db with target-a=5, target-b=4: B's merge
landed on the wrong coord — target-b never got Frank because A's
swap pushed self.coordinator to target-a, B's swap captured target-a
as B's "previous", and B's restore set self.coordinator back to
target-a (not the original main). Subsequent operations using
self.coordinator point at the wrong branch.
Fix lands in the next commit: serialize concurrent branch merges via
`merge_exclusive: Arc<tokio::sync::Mutex<()>>` held across the entire
swap-operate-restore window. Closes the bug class "non-atomic
three-step coordinator manipulation" for branch_merge by serializing
merges relative to each other; per-(table, branch) queue inside the
merge body still lets merges and other writers run concurrently.
A deeper "operate on local coord" refactor (the round-1 fix shape for
branch_create_from) requires unwinding `branch_merge_on_current_target`
and its uses of `self.snapshot()` / `self.ensure_commit_graph_initialized()`;
deferred to a follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two cubic findings on bench_actor_isolation.rs flagged together:
P2 (lib.rs:202): `unsafe { std::env::set_var(...) }` ran inside
`#[tokio::main] async fn main()` AFTER the multi-thread tokio runtime
was up. Rust 2024 made `set_var` unsafe because libc's `setenv` is
not thread-safe; concurrent env reads from logging or runtime
internals can race or read torn state.
Fix (correct by design, AGENTS.md rule 9): add a public
`AppState::new_with_workload(uri, db, bearer_tokens, workload)`
constructor that takes a caller-built `WorkloadController`. Tests and
benches override per-actor caps via the constructor instead of
mutating global env. Closes the bug class "tests need to mutate
global env to override AppState defaults."
P2 (lib.rs:130): heavy actor's `oneshot.await` inside the loop
serialized — heavy in-flight count was always 1, so cap=1 never
tripped on the heavy side. The bench validated isolation (light p99
bounded) but didn't demonstrate the rejection path.
Fix: add a `--heavy-concurrency` arg (default 4) and spawn batches
as concurrent tokio tasks bounded by an internal semaphore. With
heavy_concurrency=4 and inflight_cap=1, the bench now reports
heavy_too_many_requests > 0 and heavy_ok == 1 at peak — proving the
gate fires for the heavy actor.
Sample run on local FS (4 light actors × 30 ops, 20 heavy batches ×
50 rows, heavy_concurrency=4, cap=1):
heavy_ok: 1
heavy_too_many_requests: 19
light_ok: 120
light_too_many_requests: 0
light_p99: 565 ms (target < 2 s)
Heavy saturates its own cap; light actors are completely unaffected.
The isolation property is now empirically proven by the rejection
counts rather than just by the latency tail.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the cubic finding (P2) at lib.rs:1061: the new admission gates
add HTTP 429 / 503 failure paths but the affected endpoint
`#[utoipa::path(... responses(...) ...)]` annotations weren't updated.
Also closes a pre-existing miss on /change (admission-gated since
PR 2 Step F).
Adds (status = 429, ...) and (status = 503, ...) to all six
admission-gated endpoints:
- POST /change (operation_id = "change")
- POST /schema/apply (operation_id = "applySchema")
- POST /ingest (operation_id = "ingest")
- POST /branches (operation_id = "createBranch")
- DELETE /branches/{branch} (operation_id = "deleteBranch")
- POST /branches/merge (operation_id = "mergeBranches")
The descriptions reference the `Retry-After` header, which the
`IntoResponse for ApiError` impl emits on both codes (added in
commit c745dd6).
openapi.json regenerated via OMNIGRAPH_UPDATE_OPENAPI=1; the openapi
sentinel test passes both with the regen flag and in strict-check
mode.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the HIGH-severity deadlock flagged by Cursor Bugbot on PR #75
review of commit b09a097.
Pre-fix: `Omnigraph::refresh()` held `coordinator.write().await` from
omnigraph.rs:468 through function exit, including across the call to
`reload_schema_if_source_changed()` at line 484. That helper's
`self.coordinator.read().await` (only reached when on-disk schema
source differs from in-memory cache) deadlocked against the outer
write guard because tokio's RwLock is non-reentrant. Reachable from
`branch_delete` (omnigraph.rs:910) and `branch_merge` (post-merge
refresh at merge.rs:1100). Cross-handle scenario: handle A calls
apply_schema, handle B's stale cache hits the reload path on its
next refresh.
Why correct by design (AGENTS.md rule 9): the write guard's purpose
is to serialize the recovery sweep's mutation of GraphCoordinator;
the schema reload reads coord.branch_list() and stores into the
ArcSwap'd schema_source / catalog without touching the coord. The
two operations have disjoint lock requirements; coupling them was
over-locking. Scoping the guard matches the natural data-flow:
snapshot recovery state under the write, release, then reload schema
using a fresh read on the same lock.
Pinned by `composite_flow_schema_apply_then_branch_ops_no_deadlock_in_refresh`
(previous commit). Pre-fix: 15s timeout fires. Post-fix: completes
in 0.25s. Both other composite_flow tests still pass:
canonical_lifecycle and multi_branch_sequential_merges.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per AGENTS.md rule 8, this commit lands the failing regression test
ahead of the fix so the red → green pair is visible in git log.
Cursor Bugbot flagged the deadlock at HIGH severity on commit b09a097:
`Omnigraph::refresh()` holds `coordinator.write().await` from
omnigraph.rs:468 through function exit, including across the call to
`reload_schema_if_source_changed()` at line 484. That helper, when the
on-disk schema source differs from the in-memory cache, attempts
`self.coordinator.read().await` at line 496. Tokio's RwLock isn't
reentrant — the read blocks waiting for the write to release, the
write isn't released until refresh() returns. Hard hang.
Reachable from `branch_delete` (omnigraph.rs:910 calls `self.refresh()`)
and `branch_merge_as` (post-merge refresh at merge.rs:1100).
Cross-handle setup is the realistic trigger: handle A applies a
schema, advancing _schema.pg on disk and updating A's ArcSwap cache
in-line; handle B has stale in-memory schema_source. B's next
refresh() (here via branch_delete) hits the read-after-write reload
path because B's cache no longer matches disk. Single-handle is
unreachable since apply_schema updates the local cache atomically.
Test currently fails on b09a097 with the timeout firing at 15s,
proving branch_delete hung. The next commit scopes the write guard
to the recovery section so reload_schema_if_source_changed runs
without the write held — uncontested read acquisition, no deadlock.
The test extends `composite_flow.rs` with a broader sequence
(apply_schema → branch_create → branch_delete → branch_merge → mutate
with new column → reopen) so the post-fix path's correctness is
pinned alongside the deadlock pin per the user's request.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
