Repository navigation
(feat): multi-graph server mode - #119
Merged
Merged
Conversation
…/10)
PR 1 of the MR-668 multi-graph server work. Pure types, no runtime
behavior changes yet.
Ships the validated identity vocabulary that the rest of the implementation
will consume:
- `GraphId(String)` — `^[a-zA-Z0-9-]{1,64}$`, leading underscore rejected
(engine reserves every `_*` filename), reserved route names rejected
(`policies`, `healthz`, `openapi`, `openapi.json`, `graphs`). Validation
lives in `try_from` only; serde `Deserialize` re-runs it so JSON payloads
cannot bypass.
- `TenantId(String)` — same regex shape as GraphId. `None` in Cluster
mode; reserved for Cloud mode (RFC 0003) where it carries the OAuth
`org_id` claim.
- `GraphKey { tenant_id: Option<TenantId>, graph_id }` — the registry
HashMap key. `cluster()` constructor for the Cluster-mode default.
- `Scope` enum with `Full` variant — Cluster mode default; RFC 0004 will
extend with OAuth scopes (`graph:read`/`write`/`admin`/`*`).
- `AuthSource` enum with `Static` variant — Cluster mode default; RFC
0001 step 1 will add `Oidc`.
- `ResolvedActor { actor_id, tenant_id, scopes, source }` — replaces the
upcoming refactor of `AuthenticatedActor(Arc<str>)` in PR 4a.
Per MR-668 design decision 13: ship the Cloud-mode forward type shapes
now (no `TokenVerifier` trait yet — that's RFC 0001 step 1) so handler
signatures stay stable across the Cluster → Cloud trajectory. `Scope`
and `AuthSource` use `#[non_exhaustive]` so future variants don't break
caller matches.
Tests: 26 new (15 graph_id + 11 identity), all passing. No regression
in the existing 36 server library tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 2a of the MR-668 multi-graph server work. Bug fix: a partially-failed
`Omnigraph::init` previously left orphan schema files at the graph URI,
making the URI unusable for a retry (the next `init` would refuse because
`_schema.pg` already exists).
Changes:
1. `init_with_storage` now wraps the I/O phase. On any error from
`init_storage_phase`, calls `best_effort_cleanup_init_artifacts` to
remove the three schema files before returning the original error:
- `_schema.pg`
- `_schema.ir.json`
- `__schema_state.json`
Cleanup is best-effort: a failure to delete is logged via
`tracing::warn` but does NOT mask the init error.
2. Three failpoints added at the init phase boundaries:
- `init.after_schema_pg_written`
- `init.after_schema_contract_written`
- `init.after_coordinator_init`
3. Four new failpoint tests in `tests/failpoints.rs` pin the cleanup
behavior at each boundary plus the "original error wins over cleanup
error" contract. All 23 failpoint tests pass.
Coverage gap (documented in code comments):
Lance per-type datasets and `__manifest/` directory created by
`GraphCoordinator::init` are NOT cleaned up after a coordinator-init-phase
failure. Recursive directory deletion requires `StorageAdapter::delete_prefix`,
which was deferred along with `DELETE /graphs/{id}` (originally PR 2b). When
that primitive lands, the third failpoint test can be tightened to assert
the graph root is fully empty.
Tests: 4 new (init_failpoint_*), all 23 failpoint tests green. No
regression in the 105 engine library tests or 64 end_to_end tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 3 of the MR-668 multi-graph server work. Pure data structure — no
routing changes yet (that's PR 4a).
New file: `crates/omnigraph-server/src/registry.rs`
- `GraphHandle { key: GraphKey, uri: String, engine: Arc<Omnigraph>,
policy: Option<Arc<PolicyEngine>> }` — the per-graph state that the
routing middleware (PR 4a) will inject as a request extension.
- `RegistrySnapshot { graphs: HashMap<GraphKey, Arc<GraphHandle>> }` —
immutable snapshot; replaced atomically via `ArcSwap`.
- `GraphRegistry { snapshot: ArcSwap<_>, mutate: Mutex<()> }` — lock-free
reads, mutex-serialized mutations.
- `RegistryLookup { Ready(Arc<GraphHandle>) | Gone }` — two-valued, no
`Tombstoned` variant since DELETE is deferred in v0.7.0 scope.
- `InsertError { DuplicateKey | DuplicateUri }` — both rejection cases
for create-graph (maps to HTTP 409 in PR 7).
- Methods: `new`, `from_handles` (bulk startup-time init), `get`, `list`,
`len`, `insert`.
Race semantics pinned by three multi-thread tests:
- `concurrent_insert_same_key_exactly_one_succeeds` — N=8 spawned
inserts with the same key; exactly 1 returns Ok, 7 return DuplicateKey.
- `concurrent_insert_distinct_keys_all_succeed` — N=8 spawned inserts
with distinct keys; all succeed.
- `concurrent_reads_during_inserts_see_consistent_snapshots` — reader
loop concurrent with sequential writes; every listed handle's key
resolves via `get()` (no torn state).
Why no tombstones field: `DELETE /graphs/{id}` is deferred to bound
the scope of v0.7.0. Without a delete endpoint, there's no use for
tombstones — every key in the registry is `Ready`, and every key
not in the registry is `Gone`. When DELETE lands later, the
`Tombstoned` variant + `tombstones: HashSet<GraphKey>` slot in
additively without breaking caller signatures (the `Gone` variant
remains the "not currently active" case).
Why `tokio::sync::Mutex`: insert is async because PR 7's flow holds
this mutex across the atomic YAML rewrite step (file I/O). std::Mutex
would footgun across .await.
Dependency additions: `arc-swap = { workspace = true }`,
`thiserror = { workspace = true }` (used by InsertError).
Tests: 12 new (12 passing). 74 server lib tests total green
(62 from PR 1 + 12 new). Clippy clean on server crate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 4a of the MR-668 multi-graph server work. The heaviest single PR —
rewires every handler to extract `Arc<GraphHandle>` from a routing
middleware, replaces `AuthenticatedActor(Arc<str>)` with `ResolvedActor`
everywhere, and adds the `ServerMode` discriminator.
Behavior changes:
- **Single mode** (legacy `omnigraph-server <URI>`): flat routes
(`/snapshot`, `/read`, `/branches`, …) continue to work exactly as
v0.6.0. Internally, the registry holds a single handle keyed by the
sentinel `SINGLE_GRAPH_KEY_ID = "default"`; routing middleware injects
that handle on every request. No HTTP-visible change.
- **Multi mode** (new): routes nest under `/graphs/{graph_id}/...`.
Routing middleware extracts the graph id from the path, looks it up
in the registry, and injects the handle. 404 if not found.
(Multi-mode startup itself lands in PR 5; this PR provides the
router-side wiring.)
AppState refactor:
- `engine: Arc<Omnigraph>` and `policy_engine: Option<Arc<PolicyEngine>>`
fields removed — both now live inside `GraphHandle` in the registry.
- `mode: ServerMode { Single { uri } | Multi { config_path } }` added.
- `registry: Arc<GraphRegistry>` added.
- `server_policy: Option<Arc<PolicyEngine>>` added (placeholder for
management endpoints in PR 6b; unused today).
- Existing constructors (`new`, `new_with_bearer_token{s,_and_policy}`,
`new_with_workload`, `open*`) build a single-mode AppState
internally and remain source-compatible. Tests that constructed
AppState via these constructors continue to work.
- `with_policy_engine` post-construction setter — rebuilds the
single-mode handle with the policy attached. Engine-layer
enforcement is NOT reinstalled (matches the old single-field
semantics; `open_with_bearer_tokens_and_policy` is the path that
installs both layers).
- `new_multi` constructor added for PR 5's startup loop.
- `uri()` now returns `Option<&str>` (Some in single, None in multi).
Routing middleware:
- `resolve_graph_handle` injects `Arc<GraphHandle>` as a request
extension. Mode-aware: single returns the only handle; multi parses
`/graphs/{graph_id}/...` from the URI. Returns 404 in multi mode
when the graph id is unregistered. Records `graph_id` on the
current tracing span.
- `require_bearer_auth` updated to insert `ResolvedActor` (was
`AuthenticatedActor`).
Handler refactor — every protected handler:
- Gains `Extension(handle): Extension<Arc<GraphHandle>>` param.
- Replaces `state.engine` → `handle.engine`.
- Replaces `state.policy_engine()` → `handle.policy.as_deref()`.
- Replaces `state.uri()` → `handle.uri.as_str()` (or `.clone()`
where String is needed).
- Replaces `Arc::clone(&state.engine)` → `Arc::clone(&handle.engine)`
(the spawn-and-clone pattern in `server_export` — proof that a
long-running export survives the registry being mutated later).
authorize_request signature:
- Was: `(state: &AppState, actor: Option<&AuthenticatedActor>, request: PolicyRequest)`.
- Now: `(actor: Option<&ResolvedActor>, policy: Option<&PolicyEngine>, request: PolicyRequest)`.
- Per-graph callers pass `handle.policy.as_deref()`. The (future PR 6b)
management endpoints will pass `state.server_policy.as_deref()`.
MR-731 invariant preserved:
- The single chokepoint `request.actor_id = actor.actor_id.as_ref().to_string()`
inside `authorize_request` still overwrites any client-supplied
actor identity. Regression test
`actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers`
at `tests/server.rs:1114-1216` passes unchanged.
Tests: 0 new (the registry race tests in PR 3 already cover the
data structure; this PR exercises them indirectly via the existing
test suite). 74 lib + 57 server integration + 60 openapi = 191 tests
green. Clippy clean.
LOC: +397 insertions, -153 deletions in `crates/omnigraph-server/src/lib.rs`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 4b of the MR-668 multi-graph server work. In multi mode, the served
`/openapi.json` reports cluster routes (`/graphs/{graph_id}/...`) instead
of the legacy flat protected paths — matching what `build_app` actually
mounts (PR 4a's `Router::nest`). Single mode is unchanged.
Implementation:
- New `server_openapi` branch: when `state.mode()` is `Multi`, call
`nest_paths_under_cluster_prefix(&mut doc)` after `ApiDoc::openapi()`.
- The rewrite consumes `doc.paths.paths`, then for every path-item:
- If the path is in `ALWAYS_FLAT_PATHS` (`/healthz` for now), keep
it flat.
- Otherwise, prefix every operation_id with `cluster_` and reinsert
the item at `/graphs/{graph_id}<original_path>`.
- Single mode hits no extra work — the path map is untouched.
- The static `ApiDoc::openapi()` still emits the flat surface, so
in-process callers (the existing `openapi_json()` helper in tests)
see the unmodified spec.
Why cluster_ prefix on operation IDs: OpenAPI specs require unique
operation_ids across the document. With both flat (single-mode) and
cluster (multi-mode) surfaces ever co-existing in a generated SDK,
the prefix prevents collision. The current served doc only carries
one surface, so the prefix is forward-compat with potential future
dual-surface generation.
Tests: 6 new in `tests/openapi.rs`, all via the `/openapi.json` route
(not the static `ApiDoc::openapi()` helper):
- `multi_mode_openapi_lists_cluster_paths` — every protected path
appears as a cluster variant.
- `multi_mode_openapi_drops_flat_protected_paths` — flat protected
paths are absent.
- `multi_mode_openapi_keeps_healthz_flat` — `/healthz` survives.
- `multi_mode_openapi_prefixes_operation_ids_with_cluster` — every
cluster operation_id starts with `cluster_`.
- `multi_mode_operation_ids_are_unique` — no operation_id collisions.
- `single_mode_openapi_unchanged_by_cluster_filter` — single mode
still emits the legacy flat surface (regression).
New test helper `app_for_multi_mode(graph_ids)` exercises the new
`AppState::new_multi` constructor from PR 4a — first user of multi-mode
construction outside of unit tests.
Result: 66 openapi tests + 57 server integration tests + 74 lib tests
= 197 green. No regression in the existing OpenAPI drift check
(`openapi_spec_is_up_to_date` still validates the static flat surface
matches the committed openapi.json).
LOC: +67 in lib.rs (rewrite logic), +219 in tests/openapi.rs (test
suite + helper).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 5 of the MR-668 multi-graph server work. This is the first PR that
makes multi mode actually usable end-to-end: operators invoking
`omnigraph-server --config omnigraph.yaml` with a non-empty `graphs:`
map and no single-mode selector now get a running multi-graph server.
Mode inference (MR-668 decision 2, four-rule matrix in
`load_server_settings`):
1. CLI `<URI>` positional → Single
2. CLI `--target <name>` → Single (URI from graphs.<name>)
3. `server.graph` in config → Single (URI from graphs.<name>)
4. `--config` + non-empty `graphs:` + no single-mode selector
→ Multi (all entries in `graphs:`)
5. otherwise → error with migration hint
Rule 5's error message names every escape hatch so operators can fix
their invocation without grepping docs.
Config schema extensions:
- `TargetConfig.policy: PolicySettings` (per-graph Cedar policy file).
`#[serde(default)]` so existing single-graph YAMLs keep parsing.
- `ServerDefaults.policy: PolicySettings` (server-level Cedar policy
for management endpoints — loaded in PR 5, wired into `GET /graphs`
in PR 6b).
- `OmnigraphConfig::resolve_target_policy_file(name)` and
`resolve_server_policy_file()` helpers — both resolve relative to
the config file's `base_dir`.
Public types added to `omnigraph-server`:
- `ServerConfigMode { Single { uri, policy_file } | Multi { graphs,
config_path, server_policy_file } }`.
- `GraphStartupConfig { graph_id, uri, policy_file }` — one entry
per graph in multi mode.
`ServerConfig` shape change:
- WAS: `{ uri: String, bind, policy_file, allow_unauthenticated }`.
- NOW: `{ mode: ServerConfigMode, bind, allow_unauthenticated }`.
- Breaking for any code that constructs `ServerConfig` directly.
`main.rs` is unaffected (uses `load_server_settings`).
`serve()` now forks on `ServerConfig.mode`:
- Single: existing flow via `AppState::open_with_bearer_tokens_and_policy`.
- Multi: parallel open via `futures::stream::iter(graphs)
.map(open_single_graph).buffer_unordered(4).collect()`. Bound 4 is
a rule-of-thumb for I/O-bound work — at N≤10 this trades startup
latency for a small amount of concurrent S3/Lance open pressure.
Fail-fast: first open error aborts startup; in-flight opens drop
their engine via Arc (Lance datasets close cleanly).
New helper `open_single_graph(GraphStartupConfig)`:
- Validates `GraphId` per the regex in PR 1.
- `Omnigraph::open(uri).await` with descriptive error context.
- Loads per-graph policy file and re-applies it via
`Omnigraph::with_policy` (engine-layer enforcement, MR-722).
- Returns `Arc<GraphHandle>` ready for the registry.
Routing middleware bug fix:
- `Router::nest("/graphs/{graph_id}", inner)` rewrites
`request.uri().path()` to the inner suffix (e.g. `/snapshot`).
The previous middleware tried to parse `{graph_id}` from
`request.uri().path()` and got 400 instead of 200. Fixed by reading
from `axum::extract::OriginalUri` request extension, which preserves
the pre-rewrite URI.
- Caught by the two new tests
`cluster_routes_dispatch_per_graph_handle` and
`cluster_route_for_unknown_graph_returns_404`.
Tests (14 new, all passing):
- Four-rule matrix: one test per branch + the joint case
`mode_inference_cli_uri_overrides_graphs_map` + the empty-graphs-map
error case.
- Per-graph + server-level policy file path resolution.
- Reserved `GraphId` rejection at startup.
- End-to-end multi-graph routing: two graphs side by side, each
cluster route hits the right engine.
- Unknown graph id under cluster prefix → 404.
- Flat routes 404 in multi mode.
Inline `ServerConfig` test (`serve_refuses_to_start_in_state_1_without_unauthenticated`)
and three `server_settings_*` tests updated to the new `mode` shape.
Result: 211 server tests green (74 lib + 71 integration + 66 openapi),
MR-731 regression test still pinned and passing.
LOC: +45 config.rs, +281 lib.rs (net), +395 tests/server.rs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 6a of the MR-668 multi-graph server work. Policy-crate-only refactor —
no HTTP handler changes, no operator-supplied policy.yaml changes. Sets
up the chassis that PR 6b's `GET /graphs` consumes.
Two new `PolicyAction` variants:
- `GraphCreate` — gates `POST /graphs` (deferred behavioral PR).
- `GraphList` — gates `GET /graphs` (lands in PR 6b).
Note: `GraphDelete` is intentionally NOT added in this PR. `DELETE
/graphs/{id}` is deferred from MR-668's v0.7.0 scope to bound complexity
(no `delete_prefix`, no tombstone, no `RegistryLookup::Tombstoned`).
Adding the Cedar action without a consumer would be the same kind of
"dead vocabulary" trap the `Admin` variant already documents.
New `PolicyResourceKind { Graph, Server }` enum, plus a
`PolicyAction::resource_kind()` method that classifies every action.
Per-graph actions (Read, Change, BranchCreate, …) bind to
`Omnigraph::Graph::"<graph_label>"`; server-scoped actions
(GraphCreate, GraphList) bind to the singleton
`Omnigraph::Server::"root"`. `Admin` stays classified as per-graph for
now — MR-724 will pick the final shape when the first consumer surface
ships.
Cedar schema string additions:
- `entity Server;`
- `action "graph_create" appliesTo { principal: Actor, resource: Server, ... }`
- `action "graph_list" appliesTo { principal: Actor, resource: Server, ... }`
Compiler updates:
- `compile_policy_source` picks the resource literal based on the
action's `resource_kind`. Existing graph-only policies generate
the same Cedar source as before — pinned by
`per_graph_rules_continue_to_work_alongside_server_rules`.
- `compile_entities` includes the `Server::"root"` entity only when
a rule references a server-scoped action. Keeps test assertions
for graph-only policies tight.
- `PolicyEngine::authorize` builds the right resource UID at
request time based on `request.action.resource_kind()`.
Validation rules added to `PolicyConfig::validate`:
- A rule may not mix server-scoped and per-graph actions (different
resource kinds need different `permit` clauses).
- Server-scoped actions cannot have `branch_scope` or
`target_branch_scope` — there's no branch context at the server
level.
Operator impact: zero. The Cedar schema `Omnigraph::Server` entity is
internally referenced by `compile_policy_source`; operator policy.yaml
files only declare actions in `rules[].allow.actions` and never
reference the resource entity directly. Decision 6's "internal rename
only; operator policies unaffected" contract is preserved and pinned
by `per_graph_rules_continue_to_work_alongside_server_rules`.
Tests: 5 new (11 policy tests total, up from 6):
- `graph_list_action_authorizes_against_server_resource`
- `graph_create_action_authorizes_against_server_resource`
- `server_scoped_rule_cannot_use_branch_scope`
- `rule_mixing_server_and_per_graph_actions_is_rejected`
- `per_graph_rules_continue_to_work_alongside_server_rules`
No regression: 145 server tests (74 lib + 71 integration) still green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 6b of the MR-668 multi-graph server work. First management endpoint —
`GET /graphs` lists every graph registered with the server, gated by the
server-level Cedar policy from PR 6a.
New API shapes (in `omnigraph-server::api`):
- `GraphInfo { graph_id, uri }` — one entry per registered graph.
- `GraphListResponse { graphs: Vec<GraphInfo> }` — sorted alphabetically
by `graph_id` for deterministic output.
Handler `server_graphs_list`:
- Mounted at `GET /graphs` in both modes.
- Single mode: returns 405 (resource exists in the API surface, just
not operational without a `graphs:` map). 405 chosen over 404 so
clients see "resource exists, wrong context" rather than "no such
resource".
- Multi mode: requires bearer auth (when configured); Cedar-gated by
`PolicyAction::GraphList` against `Omnigraph::Server::"root"`
(PR 6a's chassis). Returns the sorted registry list.
Cedar gate composition:
- When no `server.policy.file` is configured, the MR-723 default-deny
falls through: `GraphList` is not `Read`, so an authenticated actor
without a server policy gets 403. This is the right default — don't
expose the registry until the operator explicitly authorizes it.
- When a server policy is configured, Cedar evaluates the rule. The
test `get_graphs_with_server_policy_authorizes_per_cedar` pins the
admin-allow / viewer-deny split.
Routing:
- New `management` sub-router holding `/graphs` (auth-required, no
`resolve_graph_handle` middleware — operates on the registry, not
a single graph).
- Single mode merges flat protected routes + management.
- Multi mode merges nested `/graphs/{graph_id}/...` + management.
OpenAPI:
- `server_graphs_list` registered in `ApiDoc::paths(...)`.
- `EXPECTED_PATHS` in `tests/openapi.rs` gains `/graphs`.
- `openapi.json` regenerated (auto-tracked by
`openapi_spec_is_up_to_date` in CI).
Tests: 4 new in `tests/server.rs::multi_graph_startup`:
- `get_graphs_lists_registered_graphs_in_multi_mode`
- `get_graphs_returns_405_in_single_mode`
- `get_graphs_requires_bearer_auth_when_configured`
- `get_graphs_with_server_policy_authorizes_per_cedar`
What's NOT in this PR (deferred):
- Per-graph policy enforcement is wired through `handle.policy`
(PR 4a already did this); PR 6b doesn't add new per-graph
behavior beyond making sure the server policy lookup composes
cleanly alongside it.
- `POST /graphs` (PR 7) and `DELETE /graphs/{id}` (out of scope
for v0.7.0).
- CLI `omnigraph graphs list` (PR 8 will add).
Result: 215 server tests green (74 lib + 66 openapi + 75 integration),
11 policy tests green. MR-731 spoof regression preserved across all
this work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 7 of the MR-668 multi-graph server work. Operators can now add a
graph to a running multi-graph server without restarting:
curl -X POST http://server/graphs \
-H "Content-Type: application/json" \
-d '{
"graph_id": "beta",
"uri": "/data/beta.omni",
"schema": { "source": "node Person { name: String @key }\n" },
"policy": { "file": "./policies/beta.yaml" }
}'
DELETE remains deferred (out of v0.7.0 scope per the trimmed plan —
no `delete_prefix`, no tombstones).
Body shape (decision 7):
- Nested `schema: { source: "..." }` (mirrors the `policy: { file }`
pattern; leaves room for future fields without breakage).
- Optional nested `policy: { file: "..." }` for per-graph Cedar.
- 32 MiB body limit (reuses `INGEST_REQUEST_BODY_LIMIT_BYTES`).
- Asymmetric with `SchemaApplyRequest` which keeps flat
`schema_source: String` — documented in api.rs.
Atomic YAML rewrite + drift detection:
- New `config::rewrite_atomic(path, new_config, expected_hash)`:
flock → re-read + hash check → serialize → write `.tmp` → fsync
→ rename → fsync parent dir. Returns the new hash for the caller
to update its in-memory baseline.
- New `config::hash_config_file(path)` — SHA-256 of the on-disk
bytes, used at startup and after each rewrite.
- New `RewriteAtomicError { Drift | Io | Serialize }` enum.
- `AppState.config_hash: Option<Arc<Mutex<[u8;32]>>>` carries the
in-memory baseline. Updated after every successful rewrite so
subsequent POSTs don't false-trigger drift.
- The mutex is `std::sync::Mutex` (brief critical section, no .await
inside). The flock itself serializes file access process-wide
AND across multiple server instances (defense in depth).
- All sync I/O runs inside `tokio::task::spawn_blocking` — flock
is sync.
Handler ordering (the load-bearing sequence):
1. Mode check: 405 in single mode.
2. Cedar authorize: `GraphCreate` against `Omnigraph::Server::"root"`.
3. Validate body: `GraphId::try_from` (regex + reserved-name), empty
schema/uri checks, per-graph policy file parse.
4. Pre-check registry for duplicate graph_id / duplicate uri (409).
5. `Omnigraph::init` the new engine.
6. Atomic YAML rewrite (drift detection inside).
7. Publish in registry (atomic re-check via `GraphRegistry::insert`).
Failure modes (documented in handler rustdoc):
- Init fails → orphan storage at `req.uri` (PR 2a cleans up schema
files; Lance datasets remain orphans until `delete_prefix` lands).
- YAML rewrite fails (drift, IO) → orphan storage; YAML unchanged.
- Registry insert fails (race) → YAML has entry but registry doesn't;
next restart opens it cleanly.
New dependency: `fs2 = "0.4"` (workspace + omnigraph-server). POSIX-only
file locking. Linux/macOS deployment supported; Windows out of scope.
Tests (10 new in `tests/server.rs::multi_graph_startup`):
- `post_graphs_creates_a_new_graph_end_to_end` — happy path, includes
YAML inspection to confirm the rewrite landed.
- `post_graphs_baseline_hash_updates_between_rewrites` — two POSTs in
a row both succeed (drift baseline updates correctly).
- `post_graphs_duplicate_graph_id_returns_409`
- `post_graphs_duplicate_uri_returns_409`
- `post_graphs_invalid_graph_id_returns_400` (reserved name)
- `post_graphs_empty_schema_source_returns_400`
- `post_graphs_returns_405_in_single_mode`
- `post_graphs_yaml_drift_detection_returns_503` — operator hand-edits
omnigraph.yaml; server refuses to clobber.
- `hash_config_file_is_deterministic_and_detects_changes`
- `rewrite_atomic_refuses_when_hash_drifts`
OpenAPI: `server_graphs_create` registered in `ApiDoc::paths(...)`;
openapi.json regenerated.
Result: 225 server tests green (74 lib + 66 openapi + 85 integration),
all MR-731 regressions still pinned.
LOC: ~580 lib.rs net (handler + helpers), ~120 config.rs (rewrite
machinery), +71 api.rs (request/response shapes), +332 tests/server.rs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 8 of the MR-668 multi-graph server work. CLI parity for the
v0.7.0 management surface: operators can now manage graphs from
the command line against a running multi-graph server.
omnigraph graphs list --target dev --json
omnigraph graphs create \
--target dev \
--graph-id beta \
--graph-uri /data/beta.omni \
--schema schema.pg
DELETE is intentionally absent — server-side DELETE was deferred from
v0.7.0 scope, and shipping a client subcommand for a server endpoint
that doesn't exist would be dead vocabulary. The help output, the
subcommand enum, and the test that pins it (`graphs_subcommand_help_
lists_list_and_create`) all agree.
CLI architecture (modeled on `BranchCommand`):
- New `Command::Graphs { command: GraphsCommand }` top-level variant.
- `GraphsCommand { List, Create }` enum.
- List: GET `<base>/graphs`. Stdout is `<graph_id>\t<uri>` per line,
or JSON via `--json`.
- Create: reads `--schema <path>` from local disk, inlines as
`schema: { source: <file> }` in the POST body (nested per
MR-668 decision 7). Optional `--policy-file <path>` becomes
`policy: { file: <path> }`. Returns 201 → "created graph X at Y"
or JSON via `--json`.
- Both subcommands reject local URI targets with a clear
"remote multi-graph server URL" error.
New API type imports in the CLI: `GraphCreateRequest`,
`GraphCreateResponse`, `GraphListResponse`, `GraphSchemaSpec`,
`GraphPolicySpec` — all from `omnigraph-server::api`.
Tests:
- cli.rs (4 new, non-network):
* `graphs_subcommand_help_lists_list_and_create` — pins the
deferral of `delete` (catches scope creep).
* `graphs_list_against_local_uri_errors_with_remote_only_message`
* `graphs_create_against_local_uri_errors_with_remote_only_message`
* `graphs_create_with_missing_schema_file_errors` — pins the
IO context in the schema-read error path.
- system_remote.rs (1 new, `#[ignore]` like its peers):
* `graphs_list_and_create_against_multi_graph_server` — spawns a
multi-mode server, calls `graphs list` (sees `alpha`),
`graphs create` (adds `beta`), `graphs list` again (sees both),
and confirms the new graph is reachable via its cluster route.
CLI suite: 62 tests green (58 existing + 4 new). The new ignored
end-to-end test runs locally with `cargo test --ignored`.
LOC: +159 main.rs (enum + handlers), +88 cli.rs (unit tests),
+131 system_remote.rs (integration test).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR 9 — the final integration PR for MR-668 multi-graph server work.
Closes the v0.7.0 release.
Composite lifecycle tests (closes gaps flagged in PR 7's coverage
review):
- `multi_graph_lifecycle_post_query_restart_persistence` — POST a
graph, query it via cluster route, reload the config from disk
and confirm `load_server_settings` sees the rewritten YAML.
Validates the "restart resolves orphans" failure-mode story.
- `per_graph_policy_enforced_on_post_created_graph` — POST a graph
with a per-graph policy attached, then send authenticated read
and change requests. Per-graph Cedar enforcement fires correctly
on a POST-created graph (engine-layer policy reinstalled via
`Omnigraph::with_policy` inside the create flow).
- `concurrent_post_graphs_distinct_ids_all_succeed` — 4 concurrent
POSTs with distinct graph_ids all return 201. Caught a real
race in `rewrite_atomic` (see below).
Race fix — `rewrite_atomic_with_modify`:
The first composite test surfaced a real bug. The old
`rewrite_atomic(path, new_config, expected_hash)` captured the
baseline hash OUTSIDE the flock, then called rewrite_atomic which
re-acquired it inside. Under concurrent writers:
- POST A: captures baseline H0, calls rewrite_atomic.
- POST B: captures baseline H0 too (before A's update lands).
- A: acquires flock, on-disk == H0, writes H1, releases.
- A: updates baseline H0 → H1.
- B: tries to acquire flock — waits.
- B: acquires flock. On-disk is now H1. Expected (captured
before A finished) is H0. MISMATCH → spurious Drift error.
Worse: even if the timing happens to align, B's `updated` config
was constructed from BYTES read before the flock. B writes a config
that doesn't include A's new graph — silent data loss.
The fix: new `config::rewrite_atomic_with_modify(path, baseline,
modify)` takes a closure. Inside the flock + baseline mutex:
1. Read on-disk bytes, hash, compare to baseline.
2. Parse on-disk YAML.
3. Call `modify(parsed)` to produce the new config — receives
fresh on-disk state, returns the modification.
4. Serialize + write + fsync + rename + update baseline.
Everything is read-modify-write under the same critical section.
Concurrent writers serialize cleanly. Test confirmed this is no
longer a race.
The old `rewrite_atomic(path, new_config, expected_hash)` API stays
for tests that don't need the read-modify-write shape; the POST
handler switches to the new shape.
Version bump v0.6.0 → v0.7.0:
- All 5 `crates/*/Cargo.toml` (compiler, engine, policy, cli, server)
plus their inter-crate `path` dep version constraints.
- `Cargo.lock` regenerated by `cargo build --workspace`.
- `AGENTS.md` "Version surveyed" line, capability matrix HTTP-server
row updated to mention multi-graph + cluster routes + atomic YAML
rewrite.
- `openapi.json` regenerated.
Docs:
- `docs/releases/v0.7.0.md` (new) — release notes with breaking
changes, new features, deferred items (DELETE, `delete_prefix`,
actor forwarding), and the single→multi migration recipe.
- `docs/user/server.md` — substantial section additions for the
two modes, mode inference, cluster endpoint table, management
endpoints, `omnigraph.yaml` ownership contract, `POST /graphs`
body shape + status codes.
- `docs/user/cli.md` — `omnigraph graphs list/create` section,
deferred-DELETE note.
- `docs/user/policy.md` — server-scoped Cedar actions
(`graph_create`, `graph_list`), per-graph vs server-level policy
composition, example server-level policy.
Workspace test pass: 573 tests green across all crates. Zero
failures. MR-731 spoof regression still pinned and passing across
the entire 10-PR series.
This commit closes MR-668. v0.7.0 is ready for tagging.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11c18fb27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1444
to
+1445
| config::rewrite_atomic_with_modify(config_path, config_hash, move |mut config| { | ||
| config.graphs.insert(graph_id, new_target); |
There was a problem hiding this comment.
rewrite_yaml_with_new_graph unconditionally calls config.graphs.insert(...) under the rewrite lock, while duplicate detection happens later in registry.insert. Under concurrent POST /graphs requests, both writers can commit YAML changes, then one request fails with 409 at registry publish; this can overwrite an existing graph_id mapping or persist duplicate URIs in omnigraph.yaml, leaving on-disk config inconsistent with the live registry and potentially breaking restart behavior.
Useful? React with 👍 / 👎.
Comment on lines
+1362
to
+1363
| let engine = Omnigraph::init(&request.uri, &request.schema.source) | ||
| .await |
There was a problem hiding this comment.
POST /graphs initializes storage using request.uri as-is and later persists that raw value, but multi-graph startup resolves configured URIs relative to the config file base directory. If a caller provides a relative URI and the server CWD differs from the config directory, the graph is created in one location now and reopened from a different location after restart, causing data to appear missing or loading the wrong dataset.
Useful? React with 👍 / 👎.
Comment on lines
+1507
to
+1509
| rename_operation_ids(&mut item, CLUSTER_OPERATION_ID_PREFIX); | ||
| let new_path = format!("{CLUSTER_PATH_PREFIX}{path}"); | ||
| rewritten.insert(new_path, item); |
There was a problem hiding this comment.
The OpenAPI cluster rewrite prefixes every non-flat path with /graphs/{graph_id}, but management routes are mounted flat at /graphs in multi mode. This produces a served spec that advertises paths like /graphs/{graph_id}/graphs instead of the real endpoint, so generated clients will call non-existent routes (404) for graph management operations.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 27 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph-server/tests/server.rs">
<violation number="1" location="crates/omnigraph-server/tests/server.rs:5195">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Test/comment overstates end-to-end multi-graph startup coverage; it only validates config parsing and mode inference.</violation>
</file>
<file name="crates/omnigraph/src/db/omnigraph.rs">
<violation number="1" location="crates/omnigraph/src/db/omnigraph.rs:212">
P1: The new init error cleanup deletes schema files unconditionally, which can remove an existing graph’s schema artifacts when `init` fails on a non-empty URI.</violation>
</file>
<file name="crates/omnigraph-server/src/lib.rs">
<violation number="1" location="crates/omnigraph-server/src/lib.rs:1362">
P1: Normalize relative graph URIs against the config file directory before calling `Omnigraph::init`; otherwise a relative `uri` can point to one location at create time and a different location after restart.</violation>
</file>
<file name="crates/omnigraph-server/src/registry.rs">
<violation number="1" location="crates/omnigraph-server/src/registry.rs:157">
P1: Duplicate-URI detection compares raw URI strings, so equivalent local paths in different forms (relative vs absolute) can bypass the conflict check and register the same dataset twice.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
ragnorc
changed the title
…h mgmt)
The POST /graphs runtime-create endpoint shipped in PR 7/10 has three
unresolved high-severity bugs:
- flock-on-renamed-inode race: the YAML flock is taken on
omnigraph.yaml itself, then a temp file is renamed over it.
Cross-process writers end up locking different inodes — both
believing they hold exclusive access.
- duplicate-check outside the file lock: precheck runs against
the in-memory registry only; the locked closure does
config.graphs.insert(...) unconditionally. Concurrent same-id
POSTs can persist the loser in YAML while the in-memory registry
keeps the winner — they disagree after restart.
- best_effort_cleanup_init_artifacts deletes _schema.pg /
_schema.ir.json / __schema_state.json on any init failure. An
accidental re-init against an existing graph's URI destroys its
schema; subsequent open() fails at read_text(_schema.pg).
The correct fix is a Lance-style cluster catalog (reserve → init →
publish with recovery sidecars), parallel to the engine's existing
__manifest discipline. That work is out of scope for v0.7.0.
For now, disable runtime add/remove from the network and CLI surface.
Operators add graphs by editing omnigraph.yaml and restarting. The
GET /graphs read-only enumeration stays.
Removed:
- POST /graphs handler + router fragment + utoipa registration
- 13 post_graphs_* server tests + 3 composite POST tests +
multi_mode_app_with_real_config / post_graph helpers
- CLI omnigraph graphs create subcommand + its handler + cli.rs tests
- system_remote.rs combined list+create test trimmed to list-only
- YAML rewrite infra: rewrite_atomic[_with_modify], RewriteAtomicError,
staging_path, hash_config_file, AppState::config_hash field +
threading through new_multi and open_multi_graph_state
- fs2 dependency (verified absent from cargo tree)
- sha2/fs2 imports in config.rs (only the rewrite path used them)
- Cedar PolicyAction::GraphCreate variant + "graph_create" match arms
+ action def in Cedar schema + graph_create_action_authorizes_against_server_resource test
- GraphCreateRequest / GraphCreateResponse / GraphSchemaSpec /
GraphPolicySpec API types (only the POST handler / CLI imported them)
Kept:
- GET /graphs (read-only enumeration) and graph_list Cedar action
- omnigraph graphs list CLI subcommand
- All multi-graph startup, mode inference, cluster routes,
per-graph + server-level Cedar policies
- server_settings_drive_multi_graph_startup_end_to_end (the test
that covers operator-authored YAML + restart — the path that
survives)
- best_effort_cleanup_init_artifacts and the three init failpoints
(still reachable from CLI `omnigraph init`; preflight fix deferred
as a follow-up)
- GraphRegistry::insert and its concurrency tests — production
callers gone, but the method is the natural seam for the future
cluster-catalog work
Also fixed (transcript issue 4):
- ALWAYS_FLAT_PATHS now includes /graphs so multi-mode OpenAPI
advertises the management route correctly (was previously rewritten
to /graphs/{graph_id}/graphs)
- multi_mode_openapi_keeps_healthz_flat → renamed to
multi_mode_openapi_keeps_management_paths_flat, asserts both
/healthz and /graphs stay flat
- multi_mode_openapi_prefixes_operation_ids_with_cluster skips
/graphs in addition to /healthz
Doc fixes:
- docs/user/cli.md: graphs list example was --target http://...,
but --target is a config-graph-name lookup; corrected to --uri.
Removed the graphs create example.
- docs/user/server.md: dropped POST /graphs row, "omnigraph.yaml
ownership", and "POST /graphs body shape" sections. Added a
paragraph stating runtime add/remove is not exposed in v0.7.0.
- docs/user/policy.md: dropped graph_create action; reworded the
"Configuration" line to clarify that server-scoped rules (graph_list)
take neither branch_scope nor target_branch_scope.
- docs/releases/v0.7.0.md: rewrote release narrative — multi-graph
mode ships; runtime add/remove deferred.
- AGENTS.md: HTTP server bullet and capability matrix row updated to
reflect read-only GET /graphs and the operator-edit workflow.
- openapi.json regenerated; /graphs has only .get, no .post.
Diff: 17 files, +123 −1525 LOC.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 18 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph-server/tests/server.rs">
<violation number="1" location="crates/omnigraph-server/tests/server.rs:4868">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Deletes the regression suite for POST /graphs and atomic config rewrite without replacement coverage.</violation>
<violation number="2" location="crates/omnigraph-server/tests/server.rs:5195">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Test/comment overstates end-to-end multi-graph startup coverage; it only validates config parsing and mode inference.</violation>
</file>
<file name="crates/omnigraph-server/src/lib.rs">
<violation number="1" location="crates/omnigraph-server/src/lib.rs:921">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
PR description claims POST /graphs management endpoint is shipped in v0.7.0, but the code change removes all supporting code (handler, route, config_hash, rewrite logic) and explicitly comments that runtime graph creation is deferred</violation>
<violation number="2" location="crates/omnigraph-server/src/lib.rs:1362">
P1: Normalize relative graph URIs against the config file directory before calling `Omnigraph::init`; otherwise a relative `uri` can point to one location at create time and a different location after restart.</violation>
</file>
<file name="crates/omnigraph-server/src/registry.rs">
<violation number="1" location="crates/omnigraph-server/src/registry.rs:157">
P1: Duplicate-URI detection compares raw URI strings, so equivalent local paths in different forms (relative vs absolute) can bypass the conflict check and register the same dataset twice.</violation>
</file>
<file name="docs/user/policy.md">
<violation number="1" location="docs/user/policy.md:77">
P3: Per-graph rules do not have to specify a branch scope here; they may omit both scopes. Please change this to “at most one” (or similar) so it matches validation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // returns 405 so clients see "resource exists, wrong context" | ||
| // rather than 404 "no such resource." | ||
| // | ||
| // Runtime add/remove (`POST /graphs`, `DELETE /graphs/{id}`) is not |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
PR description claims POST /graphs management endpoint is shipped in v0.7.0, but the code change removes all supporting code (handler, route, config_hash, rewrite logic) and explicitly comments that runtime graph creation is deferred
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-server/src/lib.rs, line 921:
<comment>PR description claims POST /graphs management endpoint is shipped in v0.7.0, but the code change removes all supporting code (handler, route, config_hash, rewrite logic) and explicitly comments that runtime graph creation is deferred</comment>
<file context>
@@ -929,20 +911,18 @@ pub fn build_app(state: AppState) -> Router {
+ // returns 405 so clients see "resource exists, wrong context"
+ // rather than 404 "no such resource."
+ //
+ // Runtime add/remove (`POST /graphs`, `DELETE /graphs/{id}`) is not
+ // exposed in v0.7.0 — operators add graphs by editing
+ // `omnigraph.yaml` and restarting.
</file context>
Strip "PR Na/Nb" sub-PR references throughout MR-668 surfaces — they were useful during the 10-PR delivery sequence but rot now that the work is in the tree. Keep the MR-668 umbrella references. Also: - Add explicit `when = when` and `resource_literal = resource_literal` named args in `compile_policy_source`'s outer `format!` to match the surrounding crate style (already explicit for `group` and `action`). - Rename the best-effort cleanup tracing target from "omnigraph::init" to "omnigraph::init::cleanup" so operators can filter init-failure cleanup events separately from init's other log lines. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MR-731 "server-authoritative actor identity" invariant was enforced by an in-function chokepoint (`request.actor_id = actor.actor_id...` overwrite inside `authorize_request`). That worked but relied on every caller passing in a `PolicyRequest` and trusting the overwrite — a comment-enforced invariant. Move the invariant into the type system: * `PolicyRequest` no longer carries `actor_id`. The struct now models what a caller wants to do, not who they are. * `PolicyEngine::authorize(actor_id: &str, request: &PolicyRequest)` and `validate_request(actor_id, request)` take identity as a separate argument. The same shape `PolicyChecker::check` already had for the engine layer. * `authorize_request` in the HTTP layer extracts `actor_id` from the bearer-resolved `ResolvedActor` and passes it positionally — no overwrite step that could be skipped. * CLI `omnigraph policy explain` updated (the only other consumer that built a `PolicyRequest`). Public API break for the `omnigraph-policy` crate. Worth it: handlers can no longer accidentally populate `actor_id` from a request body field, and external consumers are forced by the compiler to source actor identity from a trusted path. The MR-731 chokepoint test `actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers` still passes — the bearer-resolved actor is what reaches the engine. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…licy_engine The prior `with_policy_engine` constructor reused the engine `Arc` from the existing handle (`engine: Arc::clone(&existing.engine)`) without re-applying `Omnigraph::with_policy`. Combined with `new_with_workload`, the documented composition pattern was `AppState::new_with_workload(...).with_policy_engine(p)` — which produced an `AppState` whose HTTP layer enforced Cedar but whose underlying engine had no `PolicyChecker` installed. Any caller reaching the engine via `state.registry().list()[i].engine` could bypass policy entirely. The doc comment named this gap; the type system didn't. Make composition impossible to get wrong: * Add `AppState::new_single(uri, db, tokens, Option<PolicyEngine>, WorkloadController)` — canonical single-mode constructor that takes every option together and routes through `build_single_mode` (which applies `db.with_policy(checker)` to the engine itself). * `new`, `new_with_bearer_token`, `new_with_bearer_tokens`, `new_with_bearer_tokens_and_policy`, `new_with_workload` all become thin wrappers around `new_single`. * Delete `with_policy_engine`. There is no post-construction policy install path any more; the single linear construction forces HTTP-layer and engine-layer policy to install together or not at all. Regression test `engine_layer_policy_fires_via_direct_arc_omnigraph_from_new_single` constructs an `AppState::new_single` with a deny-all policy, pulls the `Arc<Omnigraph>` from the registry handle (the same path an embedded SDK consumer would take), and asserts a direct `mutate_as` call returns `OmniError::Policy`. Pre-fix this test would have succeeded the mutation. Test caller in `ingest_per_actor_admission_cap_returns_429` migrates from `.with_policy_engine(...)` to `new_single(..., Some(policy_engine), workload)`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… check
`AppState::requires_bearer_auth` walked the entire registry per
request (cloning Arcs into a `Vec`, then `.iter().any(|h| h.policy
.is_some())`) to decide whether the auth middleware should challenge.
The walk is unnecessary — the answer only changes when the registry
mutates, which is exactly the moment a new snapshot is constructed.
Move the flag onto the snapshot itself:
* `RegistrySnapshot { graphs, any_per_graph_policy: bool }`.
* `RegistrySnapshot::new(graphs)` is the only construction path —
it derives the flag from `graphs.values().any(|h| h.policy
.is_some())` so the cached value can't drift from the source data.
* `Default` delegates to `new(HashMap::new())`.
* `GraphRegistry::from_handles` and `insert` build snapshots via
`RegistrySnapshot::new(...)`.
* `GraphRegistry::snapshot_ref()` exposes the current snapshot
through an `arc_swap::Guard`; callers that need cached derived
state go through this accessor (callers that only want `graphs`
still use `list` / `get`).
`requires_bearer_auth` becomes one `ArcSwap::load` + bool read.
Also (drive-by, same file, same hunk): replace the dead
`if let Some(other) = seen_uris.get(...)` + `let _ = other;` pattern
in `from_handles` with a plain `seen_uris.contains_key(...)`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `open_multi_graph_state` doc comment claims "Fail-fast — the
first open error aborts startup; other in-flight opens are dropped"
but the code did
.buffer_unordered(4)
.collect::<Vec<_>>()
.await
.into_iter()
.collect::<Result<Vec<_>>>()?;
which drains every future in the stream before propagating the first
`Err`. With N S3-backed graphs and graph #2 failing fast, the caller
still waits for #1, #3, #4, … to either succeed or fail before
seeing the error.
Replace the four-line dance with `futures::TryStreamExt::try_collect`,
which short-circuits on the first `Err` and drops the rest. The
doc comment now matches behavior.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the routing-middleware refactor moved the engine into the per-graph `GraphHandle` (extracted via `Extension<Arc<GraphHandle>>`), seven read-only handlers — `server_snapshot`, `server_read`, `server_export`, `server_schema_get`, `server_branch_list`, `server_commit_list`, `server_commit_show` — kept an unused `State(_state): State<AppState>` extractor. Drop it. Each request avoids one `FromRequestParts` clone of `AppState`'s Arcs. Handlers that actually use state (workload admission for write paths, `server_policy` for management endpoints) keep theirs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`tracing::Span::current().record("graph_id", ...)` in the routing
middleware silently no-ops here: no upstream `#[tracing::instrument]`
on the handlers declares a `graph_id` field, and `TraceLayer::new_for_http`
doesn't either. The recorded value never lands anywhere visible.
Replace with an explicit `info!(graph_id = %handle.key.graph_id,
"graph routed")` event so operators can grep logs and correlate
requests with the active graph. In single mode the value is the
sentinel `"default"`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The single-mode `GET /graphs` handler returned an `ApiError` built via struct literal with `status: METHOD_NOT_ALLOWED, code: BadRequest`. The body code disagreed with the HTTP status — clients deserializing on `code` saw `bad_request`, clients deserializing on `status` saw 405. Same bug class as the earlier 503+Conflict mismatch on the removed YAML drift path. Close the class for this one remaining instance: * Add `ErrorCode::MethodNotAllowed` to the API enum. * Add `ApiError::method_not_allowed(msg)` — pairs the 405 status with the matching code. * Replace the struct literal in `server_graphs_list` with the constructor. * Regenerate `openapi.json` (adds `method_not_allowed` to the ErrorCode schema enum). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The import landed in earlier work but no current call site uses it. Emitted an `unused_imports` warning on every server build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`fs2 = "0.4"` lingered in [workspace.dependencies] after the POST /graphs flock-on-rename design was pulled. `cargo tree -i fs2` reports no consumers in the workspace and the dep is not in Cargo.lock. Removing the declaration closes the "phantom dep" class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`Omnigraph::init` is "create a new graph"; existing graphs need
an explicit overwrite. Today's behavior — silently overwrite
schema files, then on inner failure delete them via best-effort
cleanup — is destructive against an existing graph regardless of
which branch fires.
Correct-by-design fix:
* New `InitOptions { force: bool }` struct (default `force: false`).
* New `Omnigraph::init_with_options(uri, schema, options)`. The
old `Omnigraph::init(uri, schema)` is a thin shortcut that
passes `InitOptions::default()`.
* `init_with_storage` runs a `storage.exists()` preflight on the
three schema URIs BEFORE any parse, write, or coordinator call.
Any hit → typed `OmniError::AlreadyInitialized { uri }`. The
destructive code paths (the `write_text` overwrite and the
best-effort cleanup) are now unreachable in strict mode against
an existing graph.
* `force: true` skips the preflight; existing operators who
actually mean to overwrite opt in explicitly.
* CLI: `omnigraph init --force` maps to `InitOptions { force: true }`.
* HTTP: `OmniError::AlreadyInitialized` maps to 409 via
`ApiError::from_omni`. Not currently HTTP-reachable (POST /graphs
was pulled), but the wiring lands here so a future runtime
create endpoint has one canonical translation.
Closes the "init is destructive against existing state" class.
The regression test added in the previous commit
(`init_on_existing_graph_uri_does_not_destroy_existing_schema`)
turns green: the original schema files now survive a second
init attempt byte-for-byte, and the call errors cleanly with
`AlreadyInitialized`. The four existing
`init_failpoint_after_*_cleans_up_*` tests stay green — strict
mode's preflight passes on a fresh tempdir, and cleanup still
runs as before when a failpoint fires mid-write.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-fix, every caller of `PolicyEngine::load(path, graph_id)` passed *some* `graph_id` argument — even when the policy was server-scoped and Cedar's resolution would never touch a Graph entity. The server-level loader at lib.rs passed the meaningless sentinel `"server"`. A graph policy file containing a `graph_list` rule compiled fine; a server policy file containing a `read` rule compiled fine. Both silently no-op'd at request time because the engine kind and the rule's resource kind disagreed. Correct-by-design fix: replace `load` with two kind-typed loaders. * `PolicyEngine::load_graph(path, graph_id)` — for per-graph policy files. Rejects any rule whose action `resource_kind()` is `Server`. * `PolicyEngine::load_server(path)` — for server-level policy files. Takes no `graph_id`: server-scoped actions resolve against the singleton `Omnigraph::Server::"root"` entity, never a Graph. Rejects any rule whose action `resource_kind()` is `Graph`. The old `load` is hard-deleted in the same commit because every in-tree consumer migrates here (no semver promise on the workspace crate, no external pinners). New `PolicyEngineKind` enum types the loader's intent; `validate_kind_alignment` is the load-time check that closes the "wrong action, wrong file, silent no-op" class — operators get a load-time error instead of confused-and- silent behavior at request time. Callsites migrated: * server lib.rs:374 (single-mode per-graph) → load_graph * server lib.rs:1065 (multi-mode server) → load_server * server lib.rs:1103 (multi-mode per-graph) → load_graph * CLI main.rs:732 (resolve_policy_engine) → load_graph * tests/server.rs ×5 (4 graph, 1 server) → load_graph/load_server * policy_engine_chassis.rs → load_graph Four new in-source tests pin the contract: both rejection paths and both positive paths. Closes the "operator puts an action in the wrong file and the rule silently never matches" class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-fix, `AppState` always carried `Arc<GraphRegistry>` even when
serving one graph. Single mode populated the registry with one
handle keyed by the `SINGLE_GRAPH_KEY_ID = "default"` sentinel;
`single_mode_handle` walked the registry, asserted `len == 1`,
and returned the single element with a 500-class "programmer
error" branch on mismatch. Three smells in a row — magic key,
walk-and-assert, programmer-error guard — all because the
single-mode runtime was forced through a multi-mode abstraction.
Correct-by-design fix: type the routing.
* New `pub enum GraphRouting { Single { handle }, Multi { registry,
config_path } }` on `AppState`. The `Single` arm carries the handle
directly — no registry, no key, no walk.
* `resolve_graph_handle` middleware matches on `routing`. Single mode
returns the handle in O(1); multi mode does the same path-extract +
registry lookup as before. The 500-class programmer-error branch
is gone — the type system now makes the violated invariant
("single mode has exactly one handle") unrepresentable.
* `requires_bearer_auth` reads `handle.policy.is_some()` directly
in the Single arm; Multi arm still uses the cached
`any_per_graph_policy` flag.
`ServerMode` and the legacy `registry` field on `AppState` are still
populated for now — C-3 removes both once every reader is migrated.
The `SINGLE_GRAPH_KEY_ID` sentinel and `ServerMode` will also go
away in C-3.
Closes the "single mode forced through a multi-mode abstraction"
class. All 76 server integration tests stay green: handlers still
extract `Extension<Arc<GraphHandle>>` from the request, so the
middleware's internal change is invisible to them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
C-1/C-2 introduced `GraphRouting` and pointed the middleware at it.
This commit removes the legacy shape that's now dead:
* `ServerMode` enum — deleted. Single mode's `uri` lives on
`handle.uri`; multi mode's `config_path` lives on the
`GraphRouting::Multi` arm.
* `AppState.mode: ServerMode` field — deleted.
* `AppState.registry: Arc<GraphRegistry>` field — deleted. Multi
mode's registry is on `GraphRouting::Multi { registry, .. }`;
single mode has no registry at all.
* `AppState::mode()`, `AppState::uri()`, `AppState::registry()`
accessors — deleted. New `AppState::routing() -> &GraphRouting`
is the single public entry point.
* `SINGLE_GRAPH_KEY_ID` constant — deleted. `GraphHandle.key` is
still required by the struct, but in single mode the key is now
only a tracing label (`"default"`, inlined with a comment naming
its sole remaining purpose). Single-mode flat routes never carry
a `{graph_id}` parameter, so the key is never compared against
user input, and there is no registry where it could be a map
key. C-1/C-2 already removed the registry walk that the sentinel
was named for.
Callers migrated:
* `build_app` (lib.rs:944) — matches on `state.routing()` instead
of `state.mode()`.
* `server_graphs_list` (lib.rs:1162) — destructures the Multi arm
to get the registry; Single arm short-circuits to 405.
* `server_openapi` (lib.rs:1217) — matches the Multi arm for the
cluster-prefix rewrite.
* `tests/server.rs:3735` — the B2 footgun regression test now
matches on `state.routing()` to extract the single-mode handle
(the test's earlier `state.registry().list().next()` shape was
the closest pre-fix analog to "embedded consumer reaches the
engine"; the new shape is more direct).
Closes the entire "single mode forced through a multi-mode
abstraction" class. After this commit:
* No magic sentinel as a routing key.
* No `single_mode_handle` walk-and-assert helper.
* No 500-class "programmer error" branch in the middleware.
* No two-field discriminant on `AppState` where one would do.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph-server/tests/server.rs">
<violation number="1" location="crates/omnigraph-server/tests/server.rs:5195">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Test/comment overstates end-to-end multi-graph startup coverage; it only validates config parsing and mode inference.</violation>
</file>
<file name="crates/omnigraph-server/src/lib.rs">
<violation number="1" location="crates/omnigraph-server/src/lib.rs:921">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
PR description claims POST /graphs management endpoint is shipped in v0.7.0, but the code change removes all supporting code (handler, route, config_hash, rewrite logic) and explicitly comments that runtime graph creation is deferred</violation>
<violation number="2" location="crates/omnigraph-server/src/lib.rs:1362">
P1: Normalize relative graph URIs against the config file directory before calling `Omnigraph::init`; otherwise a relative `uri` can point to one location at create time and a different location after restart.</violation>
</file>
<file name="crates/omnigraph-server/src/registry.rs">
<violation number="1" location="crates/omnigraph-server/src/registry.rs:157">
P1: Duplicate-URI detection compares raw URI strings, so equivalent local paths in different forms (relative vs absolute) can bypass the conflict check and register the same dataset twice.</violation>
</file>
<file name="docs/user/policy.md">
<violation number="1" location="docs/user/policy.md:77">
P3: Per-graph rules do not have to specify a branch scope here; they may omit both scopes. Please change this to “at most one” (or similar) so it matches validation.</violation>
</file>
<file name="crates/omnigraph/src/db/omnigraph.rs">
<violation number="1" location="crates/omnigraph/src/db/omnigraph.rs:233">
P1: `init` preflight uses a non-atomic exists-then-write sequence, so concurrent strict inits can both pass and proceed to initialize the same graph root.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
`server_branch_delete` and `server_commit_show` use bare
`Path<String>` extractors. In single-mode flat routes
(`/branches/{branch}`, `/commits/{commit_id}`) this works — one
capture, one value. In multi-graph cluster routes
(`/graphs/{graph_id}/branches/{branch}`,
`/graphs/{graph_id}/commits/{commit_id}`) axum 0.8 propagates the
outer `{graph_id}` capture into the inner handler, so the
extractor sees two captures and 500s with
"Wrong number of path arguments. Expected 1 but got 2."
`cluster_routes_dispatch_per_graph_handle` only exercises
`/snapshot` (no Path extractor), so the regression slipped through.
This test closes that gap structurally: every cluster route with
an inner path param gets exercised here.
Currently fails with the exact symptom above. Fix in the next
commit makes it pass.
Per AGENTS.md rule 12, the red test commit lands just before the
fix so the pair is visible in `git log` and a reviewer can check
out this commit alone to reproduce.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`Path<String>` deserializes one path-param value positionally.
Single-mode flat routes (`/branches/{branch}`,
`/commits/{commit_id}`) have one capture; multi-mode nested routes
(`/graphs/{graph_id}/branches/{branch}`,
`/graphs/{graph_id}/commits/{commit_id}`) have two — axum 0.8
propagates the outer capture into nested handlers. Same handler,
two different shapes; the multi-mode shape 500s with
"Wrong number of path arguments. Expected 1 but got 2."
Symptomatic fix: change to `Path<(String, String)>` and ignore the
first element. Breaks again the moment we add another nest layer
(e.g. tenant in Cloud mode).
Correct-by-design fix: named-field structs deserialized by name
from axum's path-param map. Each handler picks only the fields it
needs. Stable across single / multi / future-cloud nest depths
because deserialization is by field name, not position.
* New `BranchPath { branch: String }` (file-local to lib.rs)
* New `CommitPath { commit_id: String }`
* `server_branch_delete` extractor → `Path<BranchPath>`
* `server_commit_show` extractor → `Path<CommitPath>`
Closes the "handler path-extractor type is positional and breaks
when route nesting changes" class. Red test from the previous
commit turns green. All 77 server tests pass (single-mode branch
delete + commit show, plus new multi-mode coverage).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fier Single-mode `open_with_bearer_tokens_and_policy` bailed at lib.rs:380 when policy was installed and no tokens. Multi-mode `open_multi_graph_state` had no equivalent: the server started, every request 401'd because no token could ever match, and the operator spent time debugging a misconfiguration the single-mode path would have caught at startup. The doc/code contradiction made the gap easy to miss: the `ServerRuntimeState::PolicyEnabled` docstring said tokens-or-not was "unusual but valid — every request fails 401 without a bearer, which is effectively 'locked'." The single-mode bail contradicted that. In practice, silent-401-on-every-request is bug-shaped, not feature-shaped (operators wanting deny-all should configure tokens plus a deny-all Cedar rule to get meaningful 403s with policy-decision logging). Symptomatic fix: add a copy of the bail to multi-mode. Two copies that can drift again the next time a startup path is added. Correct-by-design fix: hoist the check into `classify_server_runtime_state` so both modes get the same enforcement from one source of truth. The classifier becomes the single source of truth for "should we start?" and adding a startup invariant there is now the natural extension point for any future mode. Classifier matrix is now complete: | has_tokens | has_policy | allow_unauthenticated | Result | |---|---|---|---| | F | F | F | bail (existing) | | F | F | T | Open (existing) | | T | F | * | DefaultDeny (existing) | | F | T | * | bail (NEW — closes the gap) | | T | T | * | PolicyEnabled (existing) | Changes: * `classify_server_runtime_state` (lib.rs:870-890) gains the `(false, true, _) => bail!(…)` arm with a clear message naming the failure mode and the two valid resolutions. * `open_with_bearer_tokens_and_policy` (lib.rs:369+) drops its redundant local bail — the classifier rejected the invalid case before construction was reached. * `ServerRuntimeState::PolicyEnabled` docstring is rewritten: drops the "(unusual but valid)" carve-out and states plainly that PolicyEnabled requires tokens. Names the explicit alternative (tokens + deny-all Cedar rule) for operators who want the all-requests-denied behavior. * `classify_policy_enabled_always_wins` test is renamed to `classify_policy_enabled_requires_tokens` and the now-invalid `(false, true, _)` assertion is removed (covered by the new rejection test). * New `classify_policy_without_tokens_is_rejected` test covers the new arm. * New `serve_refuses_to_start_with_policy_but_no_tokens_multi_mode` integration test pins the multi-mode propagation path — symmetric with the existing single-mode `serve_refuses_to_start_in_state_1_without_unauthenticated`. Closes the "single mode and multi mode startup branches can drift on safety invariants" class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The bot-review pass and the subsequent coverage audit surfaced two material gaps in PR #119's test surface — both easy to close, both worth closing before merge. * **Gap 1 — cluster-route sweep.** The Bug-1 path-extractor regression slipped through because `cluster_routes_dispatch_per_graph_handle` only exercised `/snapshot`. The other six protected cluster routes (`/read`, `/change`, `/export`, `/schema`, `/schema/apply`, `/ingest`, `/branches/merge`) were implicitly trusted to work without any multi-mode integration test. Add `all_protected_cluster_routes_resolve_to_their_handler` (`tests/server.rs`) that hits each protected cluster route with a minimal request and asserts the response is consistent with the handler being reached — no 404 (router didn't match), no 500 with "Wrong number of path arguments" (Bug-1 class), no 500 with "missing extension" (routing middleware didn't inject the handle). Status code is a negative assertion because each handler's happy-path inputs differ; what matters is "the request reached the handler," not "the handler returned 200" — that's already pinned by the single-mode tests. * **Gap 2 — `--force` happy path.** The strict re-init regression test (`init_on_existing_graph_uri_does_not_destroy_existing_schema`) pins the error path; nothing pinned the `force: true` escape hatch actually doing what its docstring claims. Add `init_with_force_recovers_from_orphan_schema_files` (`tests/lifecycle.rs`). Writes a bare `_schema.pg` to simulate orphan files from a failed prior init, confirms strict mode bails as expected, then confirms `init_with_options(force: true)` succeeds and produces a functional graph. Note: the test follows the documented semantics — force skips the preflight only, it does NOT purge existing Lance state. An earlier draft of the test (against full overwrite of an existing populated graph) failed because `GraphCoordinator::init` errored on the existing `__manifest`, which is exactly the limitation the `InitOptions::force` docstring already calls out. Recursive purge needs `StorageAdapter::delete_prefix` (tracked separately). Coverage is now fully aligned with the PR's claims. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cursor bot's review at commit 4120448 surfaced that `server_graphs_list` returns 200 in Open mode (`--unauthenticated`, no tokens, no policy), exposing the full graph registry — graph IDs and URIs that may contain S3 bucket paths or internal hostnames — to any unauthenticated caller. Root cause: `authorize_request`'s no-policy fallback only denies when `actor.is_some()`. In Open mode `actor: None`, so the denial branch never fires and the call returns `Ok(())`. The docstring on `server_graphs_list` claims the endpoint is "Cedar-gated" and that we "don't leak the registry until the operator explicitly authorizes it" — but Open mode has no Cedar at all, so the docstring intent and the code disagree. This commit renames the existing `get_graphs_lists_registered_graphs_in_multi_mode` test to `get_graphs_denied_in_open_mode_without_server_policy` and flips the assertion from 200 → 403. Today this fails (server returns 200) — exactly the symptom the bot named. The fix in the next commit tightens the no-policy fallback to deny server-scoped actions unconditionally, regardless of mode. Per AGENTS.md rule 12, the red test commit lands just before the fix so the red → green pair is visible in `git log` and a reviewer can check out this commit alone to reproduce. Sort-order coverage that previously lived in the renamed test moves to `get_graphs_with_server_policy_authorizes_per_cedar` in the next commit, where the admin-200 response is operator- authorized and a non-empty body is asserted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`server_graphs_list` returned 200 in Open mode (`--unauthenticated`, no tokens, no policy) because `authorize_request`'s no-policy fallback only denied when `actor.is_some()` AND action != Read. In Open mode `actor: None`, so the denial branch never fired and the call returned `Ok(())` — leaking the registry (graph IDs + URIs that may contain S3 bucket paths or internal hostnames) to any unauthenticated caller. The docstring on `server_graphs_list` claimed it was "Cedar-gated" and that the server should "not leak the registry until the operator explicitly authorizes it" — docstring intent and code disagreed. Symptomatic fix: special-case GraphList. Breaks the moment another server-scoped action (`graph_create`, `graph_delete`) is added. Correct-by-design fix: tie authorization to the action's `resource_kind()`. Server-scoped actions (`PolicyResourceKind::Server`) always require explicit policy authorization — there is no runtime state where they're served by default. Per-graph actions keep the existing default-deny logic (DefaultDeny denies non-Read for authenticated actors; Open mode allows everything per the operator's `--unauthenticated` opt-in for graph DATA, but not for server topology). The fix uses the existing `PolicyResourceKind` enum that #119 already added — no new abstraction. Future server-scoped actions (runtime `graph_create`/`graph_delete` when the cluster catalog ships) automatically pick up the same enforcement without any per-action handler change. Changes: * `crates/omnigraph-server/src/lib.rs:51` — re-export `PolicyResourceKind` (the kind discriminator was already public on the omnigraph-policy crate; needed in scope here). * `crates/omnigraph-server/src/lib.rs:1457` — `authorize_request`'s no-policy fallback gains a server-scoped-action check that fires before the actor-based default-deny logic. Error message names the failure mode and points at `server.policy.file`. * `crates/omnigraph-server/tests/server.rs:5037` — `get_graphs_with_server_policy_authorizes_per_cedar` extended to register two graphs in non-alphabetical order and assert the admin-200 response is sorted alphabetically. Restores the sort-order coverage that lived in `get_graphs_lists_registered_graphs_in_multi_mode` before the red commit renamed it to assert denial. Also bundles a small adjacent cleanup that the bot-review flagged: * `crates/omnigraph-server/src/graph_id.rs:124` — drop the unreachable `"openapi.json"` entry from `is_reserved`. The regex `^[a-zA-Z0-9-]{1,64}$` rejects every dot-containing name before `is_reserved` can run, so dotted entries in this list were dead code that misled readers into thinking the list needed to cover them. Comment now names the structural exclusion. The `rejects_reserved_route_names` test loses its `openapi.json` row (covered by `rejects_dots` via the regex). Closes the "server-scoped management actions silently leak in Open mode" class. Red test from the previous commit (`get_graphs_denied_in_open_mode_without_server_policy`) turns green; all 78 server integration tests + 76 lib tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The branch had bumped workspace versions to 0.7.0 and added a dedicated `docs/releases/v0.7.0.md` for the multi-graph work. Per scope decision: ship the graph-rename and the multi-graph mode in one v0.6.0 release. Changes: * Workspace versions bumped 0.7.0 → 0.6.0 in every crate manifest (`omnigraph`, `omnigraph-compiler`, `omnigraph-policy`, `omnigraph-server`, `omnigraph-cli`) and their internal `path = ..., version = "..."` dependency constraints. * `docs/releases/v0.7.0.md` content merged into `docs/releases/v0.6.0.md`, retargeted to a single coherent v0.6.0 release note covering both the graph terminology rename and the multi-graph server mode. The original v0.7.0.md is deleted. * All `v0.7.0` / `0.7.0` doc and comment references throughout `crates/`, `docs/`, `AGENTS.md`, and `openapi.json` retargeted to `v0.6.0` / `0.6.0`. `Cargo.lock` regenerated to match. * OpenAPI spec regenerated via `OMNIGRAPH_UPDATE_OPENAPI=1 cargo test -p omnigraph-server --test openapi openapi_spec_is_up_to_date` — `"version": "0.6.0"` now. Verification: * `cargo build --workspace` — clean (6 pre-existing engine warnings only). * `cargo test --workspace --locked` — zero failures across all 39 test result groups. * `bash scripts/check-agents-md.sh` — passes (34 links / 33 docs). * `grep -rn "0\.7\.0\|v0\.7\.0" --include='*.rs' --include='*.md' --include='*.json' --include='*.toml' .` returns no workspace hits. The three remaining `0.7.0` strings in `Cargo.lock` belong to unrelated 3rd-party crates (`pem-rfc7468`, `radium`, `rand_xoshiro`). The git tag and crates.io publish happen later — this commit just consolidates the surface so the eventual release is one coherent v0.6.0 covering all the work since v0.5.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ragnorc
changed the title
There was a problem hiding this comment.
5 issues found across 18 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph-server/tests/server.rs">
<violation number="1" location="crates/omnigraph-server/tests/server.rs:5195">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Test/comment overstates end-to-end multi-graph startup coverage; it only validates config parsing and mode inference.</violation>
</file>
<file name="crates/omnigraph-server/src/lib.rs">
<violation number="1" location="crates/omnigraph-server/src/lib.rs:921">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
PR description claims POST /graphs management endpoint is shipped in v0.7.0, but the code change removes all supporting code (handler, route, config_hash, rewrite logic) and explicitly comments that runtime graph creation is deferred</violation>
</file>
<file name="docs/user/policy.md">
<violation number="1" location="docs/user/policy.md:77">
P3: Per-graph rules do not have to specify a branch scope here; they may omit both scopes. Please change this to “at most one” (or similar) so it matches validation.</violation>
</file>
<file name="crates/omnigraph-cli/src/main.rs">
<violation number="1" location="crates/omnigraph-cli/src/main.rs:276">
P2: Doc comment incorrectly references v0.6.0 for a command that only exists in v0.7.0. Should say v0.7.0 — the multi-graph release where runtime mutation is deferred.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| /// All operations target a remote multi-graph server URL (http:// or | ||
| /// https://). Local-URI invocations return a clear error. To add or | ||
| /// remove graphs, operators edit `omnigraph.yaml` directly and restart | ||
| /// the server — runtime mutation is not exposed in v0.6.0. |
There was a problem hiding this comment.
P2: Doc comment incorrectly references v0.6.0 for a command that only exists in v0.7.0. Should say v0.7.0 — the multi-graph release where runtime mutation is deferred.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-cli/src/main.rs, line 276:
<comment>Doc comment incorrectly references v0.6.0 for a command that only exists in v0.7.0. Should say v0.7.0 — the multi-graph release where runtime mutation is deferred.</comment>
<file context>
@@ -273,7 +273,7 @@ enum Command {
/// https://). Local-URI invocations return a clear error. To add or
/// remove graphs, operators edit `omnigraph.yaml` directly and restart
-/// the server — runtime mutation is not exposed in v0.7.0.
+/// the server — runtime mutation is not exposed in v0.6.0.
#[derive(Debug, Subcommand)]
enum GraphsCommand {
</file context>
cubic-dev-ai P2 comments flagged that the release notes carried
internal Linear ticket and RFC references (MR-668, MR-731,
MR-723, RFC 0003, RFC 0004). Per AGENTS.md maintenance rule 5,
"Release docs are public project history. Describe capabilities,
behavior changes, breaking changes, upgrade notes, and user
impact; do not reference private ticket systems, internal
codenames, or planning shorthand that an outside contributor
cannot inspect." The bot's comments are correct against our own
published contract — they were a docs-quality regression
introduced when I drafted these notes.
Replaced each internal reference with the public-facing concept
it stood for. The substantive content (capabilities, behavior,
guarantees) was already present alongside the refs; sanitization
just trimmed the bracketed ticket labels:
* Line 6: dropped `(MR-668)` from the multi-graph mode summary —
the descriptive name was already self-sufficient.
* Line 24: `MR-731 spoof defense` → `the bearer-derived-actor-
identity guarantee`; `Forward-compat for Cloud mode (RFC 0003)
and OAuth provider (RFC 0004)` → "forward-compat seams for
future multi-tenant and OAuth deployments; they're inert in
this release" — describes what the operator sees instead of
pointing at planning docs.
* Line 26: `MR-731's server-authoritative-actor invariant` →
"the server-authoritative-actor invariant: actor identity is
always sourced from the bearer-token match resolved at the
auth boundary" — the public-facing statement of the guarantee.
* Line 36: `(MR-723 default-deny otherwise rejects …)` →
"without a server policy the default-deny posture rejects …"
— same content, no ticket label.
* Line 121: `MR-731 spoof regression test` → "The bearer-auth-
derived-actor-identity regression test (client-supplied
identity headers are ignored; the server-resolved actor is the
only identity Cedar sees)" — describes what the test guards
instead of naming the originating ticket.
Verified: `grep -E 'MR-\d+|RFC[ -]?\d+' docs/releases/v0.6.0.md`
returns no matches; the rest of `docs/releases/` is also clean.
`scripts/check-agents-md.sh` passes.
Note: cubic-dev-ai also flagged `crates/omnigraph-cli/src/main.rs:276`
("doc comment incorrectly references v0.6.0 for a command that
only exists in v0.7.0"). That comment is based on a stale model
of the release surface — after folding v0.7.0 into v0.6.0 in
the previous commit, the multi-graph CLI surface IS in v0.6.0
and the comment is correct as written. No change needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 37ec737. Configure here.
| graph_id, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused GraphKey::cloud constructor outside test code
Low Severity
GraphKey::cloud is only called from test code in identity.rs — no production code path constructs a cloud-mode key. The constructor is exported as pub and documented as "reserved for RFC 0003," but since the PR ships no Cloud-mode consumer, it's dead code in the production build. Same applies to TenantId, Scope, and AuthSource types, which are only exercised in tests.
Reviewed by Cursor Bugbot for commit 37ec737. Configure here.
There was a problem hiding this comment.
2 issues found across 45 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph-server/src/lib.rs">
<violation number="1" location="crates/omnigraph-server/src/lib.rs:921">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
PR description claims POST /graphs management endpoint is shipped in v0.7.0, but the code change removes all supporting code (handler, route, config_hash, rewrite logic) and explicitly comments that runtime graph creation is deferred</violation>
</file>
<file name="crates/omnigraph-cli/src/main.rs">
<violation number="1" location="crates/omnigraph-cli/src/main.rs:276">
P2: Doc comment incorrectly references v0.6.0 for a command that only exists in v0.7.0. Should say v0.7.0 — the multi-graph release where runtime mutation is deferred.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
ragnorc
added a commit
that referenced
this pull request
May 29, 2026
Resolves the 4 hard conflicts from PR #119 (multi-graph server mode, MR-668) landing on main: * `crates/omnigraph-cli/src/main.rs` imports: drop unused `ChangeRequest`, take main's `GraphListResponse`. * `crates/omnigraph-server/src/api.rs`: keep branch's `ChangeRequest` field rename (`query_source` -> `query` with serde alias, `query_name` -> `name`); accept main's rustfmt. * `crates/omnigraph-server/src/lib.rs`: take both import lists (branch's `QueryRequest` + main's `GraphInfo`/`GraphListResponse`); rewrite the `server_change` signature to combine the branch's `run_mutate` extraction with main's `Extension<Arc<GraphHandle>>` + `ResolvedActor` parameter shape. * `docs/user/server.md`: re-apply the branch's new `/query` and `/mutate` rows plus deprecation notes for `/read` and `/change` on top of main's two-column (single-mode | multi-mode) table layout. Auto-merged but stale callsites repaired alongside the conflict resolutions so the merge commit compiles: * `server_query` handler now takes `Extension(handle): Extension<Arc<GraphHandle>>` and `Option<Extension<ResolvedActor>>`, with policy read from `handle.policy.as_deref()` instead of the removed `state.policy_engine()`. Fold-in for MR-969 (next-step seam): * Extract `run_query` mirroring `run_mutate`: both helpers now take `(state, handle, actor, query: &str, name: Option<&str>, params_json: Option<&Value>, branch, ...)` instead of the `QueryRequest` / `ChangeRequest` body type. The future `/queries/{name}` handler can call these with registry-supplied fields without rebuilding the request shape. * `server_query` / `server_read` now route through `run_query`; `server_mutate` / `server_change` route through `run_mutate`. * D2 mutation rejection on `/query` is preserved via the `reject_mutations` flag; `/read` keeps the legacy permissive behavior for byte-stable back-compat. `cargo test -p omnigraph-server --test server`: 89 passed, 0 failed. `cargo build --workspace --tests --locked`: clean. Refs: MR-656, MR-668, MR-969.
4 tasks done
ragnorc
added a commit
that referenced
this pull request
Jun 13, 2026
…plementation over object_store (#203) * test(engine): pin the long-lived-handle heal contract for sidecar-covered drift A Phase B -> Phase C failure (commit_staged advanced Lance HEAD, manifest publish did not land, recovery sidecar persists) currently wedges every subsequent staged write on the same engine handle: the commit-time drift guard rejects with 'run omnigraph repair', but repair itself refuses while a recovery sidecar is pending, so a long-lived server can only recover by restart. The documented contract (writes.md 'Long-running servers', invariants.md invariant 5) says refresh-time roll-forward closes this residual without restart -- but no write path runs it. Two red tests pin the intended contract at the write entry points: a follow-up load (the POST /ingest shape: shared handle, no reopen) and a follow-up mutation must heal roll-forward-eligible sidecars in-process and then succeed. Currently failing with: table 'node:Company' has Lance HEAD version 2 ahead of manifest version 1; run `omnigraph repair` before writing The fix lands in the next commit. * fix(engine): heal pending recovery sidecars at the staged-write entry points Close the long-lived-process gap in the recovery protocol: a Phase B -> Phase C residual (per-table commit_staged landed, manifest publish did not, sidecar persists) previously recovered only at the next ReadWrite open or via an explicit refresh() that no production write path called, so a long-lived server wedged every subsequent write on the commit-time drift guard until restart. New recovery::heal_pending_sidecars_roll_forward: - one list_dir of __recovery/ at write entry (empty -> immediate return, the steady state), so the per-write cost is one storage list; - per sidecar, acquires the same per-(table_key, table_branch) write queues every sidecar writer holds from before write_sidecar until after delete_sidecar, then re-checks sidecar existence -- this serializes the heal against live writers instead of rolling an in-flight sidecar forward from under its writer (which would fail that writer's publish CAS spuriously). Lock order queues -> coordinator matches every writer's commit->publish path. This is the queue-acquisition design recovery.rs and write_queue.rs already documented for in-process recovery; - processes in RollForwardOnly mode: the common residual rolls forward in-process; rollback-eligible sidecars still defer to the next ReadWrite open (Dataset::restore is unsafe under concurrency). Wire it into load_as and mutate_as (before the inline delete path can advance any HEAD), and rebase Omnigraph::refresh onto the same helper so refresh stops racing live writers' sidecars. The maintenance entry points (apply_schema_as, branch_merge_as, ensure_indices) intentionally keep their strict fail-loud preconditions for now; wiring the same heal there is a follow-up with its own tests. Turns the previous commit's two red tests green. * fix(engine): name the right recovery path in the commit-time drift guard The drift guard's 'run omnigraph repair before writing' advice is a dead end when the drift is covered by a pending recovery sidecar: repair refuses while a sidecar is pending. With the write-entry heal in place, reaching this guard with sidecar-covered drift means the heal deferred it (rollback-eligible), and the actual recovery path is a read-write reopen. Distinguish the two classes on the error path only (one sidecar list, after the conflict is already certain); a listing failure falls back to the uncovered-drift wording rather than masking the conflict. Pinned by extending refresh_defers_rollback_eligible_sidecar_to_next_open with a write attempt against the deferred sidecar. * docs: write-entry in-process sidecar heal — contract and coverage Update the recovery contract docs to match the previous two commits: invariant 5 now states that the staged-write entry points and refresh run in-process roll-forward recovery (long-lived processes converge on the next write, not at restart); writes.md 'Long-running servers' describes the heal's queue-acquisition concurrency contract, the improved drift-guard error, and the entry points that intentionally do not heal yet; testing.md indexes the new failpoint tests; AGENTS.md capability matrix drops the claim that in-process recovery is entirely future work (only the rollback path remains with the background reconciler). * test(engine): pin the entry heal contract for schema apply and branch merge Without the write-entry heal, the two maintenance writers do worse than wedge on sidecar-covered drift -- they proceed and decide its fate implicitly: - schema apply re-plans table rewrites from the manifest pin, orphaning the drifted Phase-B commit (its rows silently vanish from the rewritten table) while the stale sidecar lingers to misclassify against the post-apply pins; - branch merge publishes over the drift, making the failed writer's commit visible as an unattributed side effect (no recovery audit row), and leaves the stale sidecar behind. Two red tests pin the intended contract: both entry points heal the sidecar first (attributed roll-forward), then run on the converged state. Currently failing on the stale-sidecar / dropped-rows assertions; the fix lands in the next commit. * fix(engine): heal pending recovery sidecars at the schema-apply and branch-merge entries Extend the write-entry heal to the remaining two write entry points. Unlike load/mutate (which wedge on the drift guard), these proceeded over sidecar-covered drift and decided its fate implicitly: - schema apply re-planned table rewrites from the manifest pin, orphaning the drifted Phase-B commit -- its rows silently vanished from the rewritten table -- while the stale sidecar lingered to misclassify against the post-apply pins; - branch merge published over the drift, making the failed writer's commit visible without a recovery audit row, and left the stale sidecar behind. Both now run the same queue-serialized roll-forward heal at entry, before their own sidecar exists, so recovery is attributed (audit row) and deterministic. ensure_indices stays heal-free: it runs inside the load / schema-apply flows after their entry heal. Turns the previous commit's two red tests green. Docs updated in the same change (invariant 5, writes.md, testing.md, AGENTS.md). * test(engine): pin Phase A sidecar-write failure semantics Storage fault-injection matrix, row 1: a sidecar PUT failure (S3 PutObject / fs write) in Phase A. New failpoint recovery.sidecar_write at the top of write_sidecar -- the single choke point all five sidecar writers go through -- models the storage error backend-generically. Also adds the other three storage-fault failpoints used by the following commits (recovery.sidecar_delete, recovery.sidecar_list, recovery.record_audit); each is a no-op without the failpoints feature. Pinned contract: every writer writes its sidecar BEFORE its first HEAD-advancing commit, so a put failure aborts with zero drift (no sidecar, Lance HEAD == manifest pin, no rows) and a transient fault never wedges the graph -- the same handle writes/merges normally once it clears. Covered for load (the staging writer) and branch_merge (the multi-table writer, forced onto the RewriteMerged path by diverging both sides). * test(engine): pin Phase D delete, list, and audit-append storage-fault semantics Storage fault-injection matrix, rows 2/3/5, plus the real-backend run: - recovery.sidecar_delete: a Phase D delete failure (S3 DeleteObject) must NOT fail the user's write -- the manifest publish already landed, so the caller's data is durable. The swallowed failure leaves a stale sidecar; the next write's entry heal consumes it via the stale-sidecar audit-recovery path (RolledForward, attributed). - recovery.sidecar_list: a __recovery/ list failure (S3 ListObjectsV2) is loud at every consumer -- the write-entry heal fails the write and the open-time sweep fails the open. Silently skipping recovery over a pending sidecar would be consumer tolerance of drift. Once the fault clears, open recovers the pending sidecar normally. - recovery.record_audit: an audit write failure after the roll-forward's manifest publish aborts that recovery attempt and keeps the sidecar; re-entry detects the already-published manifest, records exactly ONE RolledForward audit row, and converges -- the retry tolerance documented on record_audit, exercised end-to-end. - s3_load_recovers_after_publisher_failure_without_reopen: the same-handle heal scenario on a real bucket (gated on OMNIGRAPH_S3_TEST_BUCKET, skips locally), exercising sidecar put/list/delete through S3StorageAdapter instead of the local-FS adapter. CI wiring lands in a follow-up commit. * test(engine): refuse corrupt recovery sidecars loudly Storage fault-injection matrix, row 4 (no failpoint needed -- the corrupt file is written by hand, sibling to the unknown-schema-version refusal test): a truncated/garbage __recovery/{ulid}.json must be refused loudly by both the write-entry heal (the write fails naming the parse error) and the open-time sweep (ReadWrite open fails naming the file), with the file left on disk for operator inspection. Read-only opens still work -- the sweep is skipped there. * test(engine): run the S3 sidecar-lifecycle coverage in CI + document the fault matrix - ci.yml rustfs_integration: new step running the bucket-gated failpoints tests (name filter s3_) against the RustFS container, so sidecar put/list/delete are exercised through S3StorageAdapter on every storage-affecting PR. - writes.md: sidecar I/O failure semantics -- Phase A put failure aborts with zero drift; Phase D delete failure is swallowed (write already durable) and healed by the next write; list failures are loud at heal and open; corrupt sidecars are refused with the file kept for inspection; audit-append failures are retried to exactly one audit row. - testing.md: index the storage-fault matrix in the failpoints.rs row and the new RustFS CI line. * test(engine): pin read-visibility of acknowledged local if-absent writes The cluster lib test import_missing_state_creates_state_with_graph_- observation flakes at ~50% under full-workspace load ('EOF while parsing a value' reading back the state.json its own import just acknowledged). Root cause is in the engine's local storage adapter: write_text_if_absent writes through a buffered tokio::fs::File and returns when write_all resolves -- which, per tokio's documented File semantics, means the bytes reached tokio's internal buffer, not the file. The actual write completes in a background blocking task after drop, so a caller that acknowledges success and reads the object back can see an empty or partial file. Under load the window widens; the red run fails at iteration 0 with 0 of 8192 bytes on disk. The regression test pins the contract at the adapter boundary: when write_text_if_absent resolves, the full contents are visible to any reader; a losing second claim leaves the winner's object untouched. The fix lands in the next commit. * fix(engine): publish local storage writes with atomic visibility Close the class, not the instance. The local adapter admitted three ways for a reader to observe a write that was acknowledged or visible before its bytes were complete: 1. write_text_if_absent acknowledged success when the buffered tokio::fs::File write_all resolved -- i.e. when the bytes reached tokio's internal buffer, not the file. A caller reading back its own acknowledged write could see an empty object (the ~50% cluster import flake under full-workspace load; the regression test failed at iteration 0 with 0 of 8192 bytes visible). 2. The same call published its CLAIM (create_new) before its CONTENT, so concurrent readers saw an empty claimed file in the window. 3. write_text (plain tokio::fs::write) exposed truncated content mid-replace -- silently falsifying write_sidecar's 'readers either see the complete sidecar or none' contract on local FS (true on S3, where PutObject is atomic). A flush in write_text_if_absent would have fixed only (1). Instead, both local write paths now publish complete temp files atomically: rename for replace (write_text -- the idiom write_text_if_match already used) and hard_link for no-replace (write_text_if_absent -- link fails AlreadyExists, so exactly one of N concurrent claimants wins and the winner's object is fully readable at the instant it becomes visible). The local adapter now honors the same object-level atomic-visibility contract as the S3 adapter, which is what every caller (recovery sidecar protocol, cluster state CAS) was written against. Crash-orphaned *.tmp.* files are inert: the sidecar sweep filters to .json, and cluster state reads address state.json by name. fsync/durability policy is unchanged (no fsync before, none now); this fix is about visibility ordering, not power-loss durability. Pre-existing on main (landed with the multi-graph server mode change, PR #119); surfaced by this branch's heal work only because one extra list_dir per write shifted test timing. Cluster lib suite: 12/25 failures before, 0/25 after. Turns the previous commit's red test green. * refactor(engine): one storage implementation over object_store for every backend Collapse LocalStorageAdapter (hand-rolled tokio::fs) and S3StorageAdapter into a single ObjectStorageAdapter backed by Arc<dyn object_store::ObjectStore> -- LocalFileSystem for local URIs, the existing AmazonS3 build for s3://, plus a pub in_memory() constructor (full contract including TRUE conditional updates; the in-memory test backend testing.md asked for at the adapter level). Why: the acknowledged-before-visible bug showed the two-impl shape has no referee -- one prose contract, two independent answers. Upstream LocalFileSystem::put_opts is byte-for-byte the staged-temp+rename/ hard_link idiom that fix converged on, and Lance's own commit protocol is built on the same primitives (put-if-not-exists / rename-if-not- exists), so the substrate-aligned move is to stop hand-rolling it. The per-backend residue shrinks to a UriCodec (URI <-> object path) and one capability flag. Semantics preserved by construction, with three deliberate deltas: - exists() is now object-store-semantics everywhere (head + non-empty prefix fallback): an EMPTY local directory no longer 'exists'. The only dir-shaped caller (_graph_commits.lance probes) self-heals via ensure_commit_graph_initialized where it previously wedged loudly. - A directory at an object path reads as NotFound, not as an IO error ('only objects exist'). The cluster unreadable-payload test used a same-named directory as a portable non-NotFound trigger; it now uses chmod 000, which still models genuine transient IO. - write_text_if_match keeps content-token semantics on local (PutMode::Update is NotImplemented upstream for LocalFileSystem in 0.12.5 and 0.13.2); the capability flag gates the token SOURCE in read_text_versioned too -- an ETag token with content-compare writes would lose every CAS. delete_prefix keeps a local remove_dir_all branch: directories are a local-FS concept, and list+delete would leave empty skeletons that cluster graph_root_exists (raw Path::exists) reports as still present. LocalStorageAdapter remains as a delegating shim so the pinned contract tests gate this swap textually unchanged; the shim and the test parameterization over local + in-memory land next. Cargo gains the explicit 'fs' feature (already transitively enabled by lance). * test(engine): one executable storage contract, run against every backend Remove the LocalStorageAdapter delegation shim and migrate its construction sites to ObjectStorageAdapter::local(). Replace the per-backend duplicated tests with a single contract_suite asserting the trait's promises (atomic replace, exists incl. the dataset-root prefix probe, one-winner if_absent, versioned CAS with loud CAS-lost, rename, list round-trip with no sibling-prefix bleed, idempotent delete/delete_prefix), run against the local backend and the new in-memory backend -- which implements true conditional updates, so the strong-CAS path is exercised without a bucket. The bucket-gated S3 variant already exists (s3_adapter_conditional_writes_contract). New local-specific pins for the deliberate semantic edges of the collapse: empty directories are not objects (exists=false; the Lance dataset-root probe shape is the non-empty case), file://-anchored and spaces-in-path list output round-trips byte-identically into read_text, dot-segment paths are lexically absolutized (the CLI's ./graph.omni shape), and upstream rename creating missing destination parents. The acknowledged-write visibility regression test stays, now documenting that the cross-API std::fs read-back is the point. * refactor(cluster): drop put_json's per-backend atomicity branch The local temp+rename dance predates the storage adapter guaranteeing atomic visibility; now that write_text publishes via a staged temp + rename on the filesystem (and a single atomic PUT on object stores) by contract, the branch duplicated upstream behavior. One call, both backends. * docs: storage adapter collapse — contract, in-memory backend, local CAS gap - testing.md: the 'no MemStorage backend' note is half-closed — ObjectStorageAdapter::in_memory() covers the text-object layer with the full contract (true conditional updates); Lance datasets bypass the adapter, so the engine substrate ask stays open. - invariants.md: truth-matrix Tests row updated; new Known Gap for local write_text_if_match (upstream PutMode::Update is unimplemented for LocalFileSystem; content-token emulation is safe only under the cluster lock protocol — close before admitting a lock-free caller). - writes.md: backend notes for the unified adapter (name#N staging residue invisible to the sweep, backend-wrapped error text with exists()-probing for missing-vs-error, loud permission failures). * docs: finish renaming the storage adapters in user docs and test comments storage.md's URI-scheme table and the S3 failpoint test's doc comment still named the deleted LocalStorageAdapter/S3StorageAdapter; both now describe the unified ObjectStorageAdapter over object_store, including the relative-path absolutization note for local URIs. * test(engine): pin branch-awareness of the drift guard's recovery advice A pending sidecar on ANOTHER branch does not cover this branch's drift: with a deferred feature-branch sidecar on disk and genuinely uncovered drift on main, the main write's error must still point at omnigraph repair -- a read-write reopen recovers the sidecar but cannot repair main's uncovered drift. Currently red: the guard matches sidecar pins by table_key only, so the feature sidecar flips main's advice to the reopen path. Fix in the next commit. Surfaced by external review of the drift-guard change. * fix(engine): branch-aware sidecar matching in the drift guard's advice The commit-time drift guard's sidecar-covered check matched pins by table_key alone, so a pending sidecar on another branch flipped this branch's uncovered-drift advice from 'run omnigraph repair' to the reopen path -- and a reopen recovers that sidecar but cannot repair this branch's drift. Compare the pin's table_branch too. Turns the previous commit's red test green. Surfaced by external review of the drift-guard change. * test(engine): pin heal non-interference with a live schema apply The write-entry heal's schema-staging reconcile runs before any queue acquisition, so a load on the same handle, overlapping a schema apply parked between its staging write and manifest commit, promotes the apply's staging files (new catalog live against the old manifest), classifies the LIVE apply's sidecar, and publishes its registrations out from under it. The resumed apply then collides with its own stolen commit. Currently red with: Lance("Concurrent modification: table version 3 already exists for node:Tag") The fix (per-sidecar reconcile under the sidecar's write-queue guards, plus a serialization key the schema-apply writer and the heal both acquire) lands in the next commit. Surfaced by external review of the write-entry heal. * fix(engine): serialize the heal's schema-staging reconcile with live schema applies The write-entry heal ran recover_schema_state_files up front, before acquiring any queue guards. Overlapping a live schema apply parked between its staging write and manifest commit, the heal promoted the apply's staging files (new catalog live against the old manifest), classified the LIVE apply's sidecar, and published its registrations — the resumed apply then collided with its own stolen commit. Correct by construction: - New schema-apply serialization queue key, acquired by the schema- apply writer (alongside its per-table keys) from before write_sidecar until after delete_sidecar. Per-table keys alone don't cover a registration-only migration, which pins no existing tables but has a sidecar and staging files on disk. - The heal reconciles schema staging lazily, PER SchemaApply sidecar, after acquiring that sidecar's guards (including the serialization key) and re-confirming the sidecar exists — a sidecar that survives the queue wait belongs to a dead writer, so the reconcile can no longer race a live apply. Recomputing per sidecar also removes the staleness of one up-front result across a multi-sidecar pass. - Omnigraph::refresh drops its up-front reconcile-and-pass-through (same race, and a pre-promoted result would make the heal's guarded reconcile see clean staging and wrongly defer the sidecar): it now reconciles standalone only when NO sidecar exists — which cannot race a live apply, whose sidecar always precedes its staging files — and otherwise defers entirely to the heal. The open-time sweep keeps its precomputed reconcile: open has no concurrent writers. Turns the previous commit's red test green. Surfaced by external review of the write-entry heal. Self-audit addendum folded in: refresh's no-sidecar gate had a TOCTOU (a live apply could write its sidecar + staging between the empty check and the reconcile) — the standalone reconcile now holds the serialization key across the list-then-reconcile pair. The remaining residual is cross-process only (in-process queues cannot serialize against a writer in another process; the open-time sweep has the same pre-existing exposure) and is now an explicit Known Gap in invariants.md rather than an implicit one. * test(engine): pin catalog reload after the heal recovers a schema apply When the write-entry heal rolls a crashed apply's SchemaApply sidecar forward on the same handle, disk and manifest move to the new schema (staging promoted, registrations published) but the handle's in-memory schema_source/catalog do not. Subsequent writes then validate against the stale catalog and reject rows of types the graph already has. Currently red with: record 1: unknown node type 'Tag' refresh() reloads after its heal; the write entry points must too. Fix in the next commit. Surfaced by external review of the write-entry heal. * fix(engine): reload the in-memory catalog after the heal recovers a schema apply heal_pending_recovery_sidecars refreshed the coordinator and invalidated the runtime cache after processing sidecars, but never reloaded schema_source/catalog — so a write whose entry heal rolled a crashed SchemaApply sidecar forward proceeded to validate against the OLD schema while disk and manifest were already on the new one. reload_schema_if_source_changed is the same post-heal step refresh() already runs; it no-ops on the (overwhelmingly common) non-schema heal because the on-disk source is unchanged. Turns the previous commit's red test green. Surfaced by external review of the write-entry heal. * test(engine): pin that a deleted-branch sidecar cannot wedge the graph A rollback-eligible sidecar pinned to a branch is deferred by every roll-forward-only pass; if the branch is then deleted, the sidecar survives, referencing a branch with no manifest tree. The heal (every write entry) and the open-time sweep (every ReadWrite open) both fail opening the dead branch, and repair refuses while a sidecar is pending -- a terminal read-only state with manual sidecar surgery as the only exit. Currently red with: Lance("Not found: .../__manifest/tree/feature/_versions") The branch's tree and forks are already reclaimed, so the pinned drift is unreachable and the sidecar is provably moot; the fix classifies it as an orphaned-branch terminal state (audit + discard) in both passes. Surfaced by review (P1, verified by repro). * fix(engine): classify deleted-branch sidecars as orphaned instead of wedging A deferred (rollback-eligible) sidecar pinned to a branch survives branch_delete; both the write-entry heal and the open-time sweep then failed unconditionally opening the dead branch -- every write and every ReadWrite open errored, and repair refuses while a sidecar pends. Terminal state, manual sidecar surgery the only exit. The branch's tree and per-table forks are already reclaimed at delete, so the drift the sidecar pins is unreachable and the sidecar is provably moot. Both passes now check the sidecar's branch against the manifest's branch list (the authority -- deliberately NOT inferred from a Not-found on open, which could be a transient storage error masking real recovery intent) and discard orphans with an OrphanedBranchDiscarded audit row, commit appended on main since the sidecar's own branch no longer has a commit graph. The open-time half is pre-existing; the write-entry heal made it hot. Turns the previous commit's red test green. Surfaced by review (P1, verified by repro). * chore: harden review nits — vacuous CI filter, root-runner skip, liveness note - ci.yml: the RustFS sidecar-lifecycle step now fails loudly if the 's3_' name filter matches zero tests (cargo passes vacuously on an empty filter; the step exists specifically to prove S3 sidecar I/O coverage). The pre-existing CLI smoke step has the same shape and is left for a follow-up. - cluster unreadable-payload test: cfg(unix) + a skip-with-log when running as root (mode 000 is still readable to root, common in container dev runners), so the test degrades instead of failing. - refresh: document the one-pass-late convergence for legacy staging residue while non-SchemaApply sidecars pend, so nobody 'fixes' it by re-running the reconcile unserialized — the exact race the serialization key closes. * test(engine): pin orphan-discard idempotency across a delete fault discard_orphaned_branch_sidecar writes its audit row and main commit before deleting the sidecar; a Phase D delete fault leaves the sidecar on disk with the audit already durable, and the retry repeated the whole path -- a second OrphanedBranchDiscarded audit row (and commit) for the same operation. Currently red: 2 rows after one fault + retry. The retry must only finish the delete. Fix next. Also promotes the recovery-audit kinds reader into the shared test helpers (it was recovery.rs-local). Surfaced by external review of the orphan-discard fix. * fix(engine): orphan-discard idempotency + heal reports acted-vs-deferred Two review findings on the recovery surface: - discard_orphaned_branch_sidecar now checks the audit table for an existing (operation_id, OrphanedBranchDiscarded) row before appending the commit + audit pair, so a Phase D delete fault retries ONLY the delete instead of duplicating audit rows and commit-graph entries. Cold path: the list scan runs only when an orphaned sidecar exists. Turns the previous commit's red test green (exactly one audit row across fault + retry). - process_sidecar returns whether durable state changed; the heal sets processed_any only for sidecars that were actually rolled forward / rolled back / audit-recovered (orphan discards count). Deferred sidecars (rollback-eligible, invariant-violating, unpromoted SchemaApply) no longer trigger a per-write schema reload + full runtime-cache invalidation while they pend -- the cache is snapshot-keyed so this was waste, not corruption, but it was paid on every write until reopen. Acted-paths' processed=true remains pinned by load_after_schema_apply_phase_b_failure_uses_recovered_catalog (the reload depends on it). Surfaced by external review. * test(engine): pin the orphan-discard audit-append fault leg as documented tolerance The orphan discard's commit append and audit append are two writes; a failure between them leaves a recovery commit with no audit row, and the retry (keyed on the audit row, the operator-facing record) appends a second commit before the audit lands. This is the same not-atomic-pair-write tolerance record_audit documents and the manifest->commit-graph Known Gap covers for every publish: bounded commit-graph noise, audit row exactly-once under clean failures. Keying idempotency on commit rows instead would need an operation_id column on _graph_commits, and audit-before-commit would dangle the graph_commit_id join -- both worse than the documented residual. Make the tolerance explicit instead of implicit: docstring names the window, a failpoint sits inside it, and the new test pins convergence across the fault (sidecar consumed, exactly one audit row), completing the orphan-discard fault matrix alongside the delete-fault leg. Surfaced by external review of the orphan-discard idempotency. * test(engine): pin honest drift-guard advice when sidecar listing fails The guard's unwrap_or(false) conflated 'classified as uncovered' with 'could not classify': a transient list fault on the guard's second list (the entry heal's first list having succeeded) confidently routed the operator to omnigraph repair even when the heal had just deferred a rollback-eligible sidecar -- and repair refuses while a sidecar is pending. Currently red: the error says 'run omnigraph repair' with no mention of the reopen path. The fix names both paths plus the failure cause when classification is impossible. Surfaced by external review of the drift-guard fallback. * fix(engine): admit ambiguity in the drift guard when sidecar listing fails Replace the unwrap_or(false) fallback with a tri-state: covered -> reopen advice; uncovered -> repair advice; listing FAILED -> say the drift could not be classified, name the cause, and give both paths in order ('run repair, or reopen read-write if repair reports a pending sidecar'). The old fallback confidently routed a transient list fault to repair, which refuses while a sidecar is pending -- a self- correcting but pointless detour. The conflict itself is still always raised; only the advice degrades honestly. Turns the previous commit's red test green. Surfaced by external review of the drift-guard fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Multi-graph server mode (MR-668). One
omnigraph-serverprocess serves N graphs concurrently behind cluster routes (/graphs/{graph_id}/...), with per-graph + server-level Cedar policy, read-onlyGET /graphsenumeration, and CLI parity viaomnigraph graphs list. Single-graph deployments (omnigraph-server <URI>) are unchanged.Ships
/graphs/{graph_id}/...cluster routes, OpenAPI cluster filter,cluster_operation-id prefixgraph_listserver-scoped action onOmnigraph::Server::"root"GET /graphsenumeration (Cedar-gated; 405 in single mode)omnigraph graphs listCLIGraphIdnewtype +GraphRegistry+GraphHandleinfrastructureOmnigraph::initfailpoint coverage (PR 2a — kept; reachable from CLIomnigraph init)GraphKey { tenant_id, graph_id },ResolvedActor)Deferred
POST /graphsruntime graph creationDELETE /graphs/{id}omnigraph graphs createCLIopen_read_onlyThese reopen once the managed cluster catalog lands.
Test plan
cargo test --workspace --locked— ~600 tests green across all cratescargo build --workspacecargo clippy -p omnigraph-server -p omnigraph-policy -p omnigraph-cli --no-deps --all-targets— no new warningscargo tree -p omnigraph-server -i fs2—fs2removedOMNIGRAPH_UPDATE_OPENAPI=1 cargo test -p omnigraph-server --test openapi—/graphshas only.get, noGraphCreate*schemas remainscripts/check-agents-md.sh— cross-links resolve🤖 Generated with Claude Code
Note
High Risk
Large routing, auth, and policy refactor across HTTP/CLI; misconfiguration of multi-graph YAML or server vs per-graph policy files could deny or over-expose management APIs.
Overview
Adds multi-graph server mode (MR-668): one process can serve many graphs from
omnigraph.yaml, with cluster routes/graphs/{graph_id}/...for data APIs and a read-onlyGET /graphsregistry list (405 in legacy single-graph mode). Single-graph startup (URI,--target, orserver.graph) keeps flat routes unchanged.Config & ops: Graph membership is config-at-startup only—no runtime
POST/DELETEgraphs; operators editgraphs:and restart. Multi mode rejects top-levelpolicy.filein favor ofgraphs.<id>.policy.fileandserver.policy.file.Policy: Cedar gains
graph_listonOmnigraph::Server::"root", split loadersload_graph/load_server, andauthorize(actor_id, &PolicyRequest)with actor id removed fromPolicyRequest(MR-731). Server-scoped management stays closed without an explicit server policy, including in--unauthenticatedmode.Server internals:
GraphId,GraphRegistry/GraphHandle,resolve_graph_handlemiddleware, parallel bounded opens, OpenAPI path rewrite for multi mode, and unified startup classifier (policy without tokens rejected in both modes).CLI:
omnigraph graphs list(remote only),omnigraph init --forceviaInitOptions, andPolicyEngine::load_graphfor local policy commands.Reviewed by Cursor Bugbot for commit eab99e6. Bugbot is set up for automated code reviews on this repo. Configure here.