Skip to content

Add support for null literals in query parameters - #3

Merged
ragnorc merged 2 commits into
mainfrom
claude/debug-optional-param-error-3xQtJ
Apr 13, 2026
Merged

Add support for null literals in query parameters#3
ragnorc merged 2 commits into
mainfrom
claude/debug-optional-param-error-3xQtJ

Conversation

@ragnorc

@ragnorc ragnorc commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds comprehensive support for null literals in query parameters throughout the omnigraph compiler and executor. Previously, null values were rejected or unsupported. Now they are properly handled for nullable parameters while maintaining type safety for non-nullable parameters.

Key Changes

  • Parameter Handling (query_input.rs)

    • Added explicit null checking in json_params_to_param_map() to validate null values against parameter nullability
    • Null values are now rejected for non-nullable parameters with a clear error message
    • Omitted nullable parameters are automatically filled with Literal::Null
    • Inferred parameters can now accept null values (previously rejected)
  • Type System (query/typecheck.rs)

    • Added Literal::Null variant to the Literal enum
    • Updated literal_type() to infer null as a nullable String type (type-agnostic default)
    • Enhanced check_literal_type() to validate null compatibility with expected property types
    • Null is now accepted for any nullable property type
  • SQL Execution (exec/mod.rs)

    • Added SQL NULL conversion in literal_to_sql()
    • Added null array creation in literal_to_array() for constant arrays
    • Updated literal_scalar_type() to handle null literals with String as default type
  • Test Coverage

    • Added 5 comprehensive tests covering:
      • Omitted nullable parameters becoming null
      • Explicit null values for nullable parameters
      • Non-nullable parameters rejecting null with proper error
      • Nullable parameters with actual values working normally
      • Inferred null parameters becoming Literal::Null

Implementation Details

The changes maintain backward compatibility while adding null support. Null handling is consistent across parameter declaration (typed), inference (untyped), and execution layers. The type system defaults null to String when type inference is needed, allowing null to be compatible with any nullable type during type checking.

https://claude.ai/code/session_014oGFKL7EVg1b2cyPgt9Gne


Note

Medium Risk
Changes how query parameters are parsed, typechecked, and executed by introducing null as a first-class literal; mistakes could alter runtime filtering/mutation behavior or surface new edge cases around nullability and SQL/Arrow typing.

Overview
Adds a Literal::Null variant and wires it through the compiler and runtime so null can be provided (or omitted) for nullable query parameters.

json_params_to_param_map now accepts JSON null, rejects it for non-nullable declared params, and auto-fills omitted/absent nullable params with Literal::Null (including when params is null/missing), with new unit tests covering these cases.

Typechecking and execution are updated to treat null as compatible with any nullable property type, emit SQL NULL in pushdown, and build Arrow null arrays when projecting constants or writing mutation values.

Reviewed by Cursor Bugbot for commit c943d97. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 98482881b6

ℹ️ 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 +307 to +310
// Fill in Literal::Null for declared nullable params that were omitted.
for param in query_params {
if param.nullable && !map.contains_key(&param.name) {
map.insert(param.name.clone(), Literal::Null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Default nullable params even when params object is absent

The new nullable-defaulting logic only runs after parsing a JSON object, so requests that pass params = None or params = null still return early with an empty map and never insert Literal::Null for declared nullable parameters. In the CLI/server call paths that omit the params payload entirely, queries with nullable params can still fail later with parameter '<name>' not provided, which contradicts the new omitted-null behavior. Apply the same defaulting in the early-return path as well.

Useful? React with 👍 / 👎.

Comment thread crates/omnigraph/src/exec/mod.rs Outdated
/// Create a constant array from a literal value.
fn literal_to_array(lit: &Literal, num_rows: usize) -> Result<ArrayRef> {
Ok(match lit {
Literal::Null => arrow_array::new_null_array(&DataType::Utf8, num_rows),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null literal creates wrong array type in projections

Medium Severity

literal_to_array hardcodes DataType::Utf8 for Literal::Null, but the parameter's declared type could be anything (e.g., I64?, Bool?, Date?). While the filter path has a cast step that compensates, the projection path in evaluate_projection uses literal_to_array directly to build result columns. This means a null value for, say, an I64? parameter produces a Utf8 column in the result schema, whereas a non-null value produces an Int64 column — an inconsistency that can break clients depending on stable schemas. The sibling function literal_to_typed_array correctly uses the target data_type, but literal_to_array has no type context to do the same.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9848288. Configure here.

Comment thread crates/omnigraph/src/exec/mod.rs Outdated
Parameters declared with `?` (e.g. `$changelogUrl: String?`) now correctly
accept omission or explicit null in JSON input instead of requiring empty
strings as a workaround. Adds `Literal::Null` variant and threads it through
parameter parsing, type-checking, and Arrow array conversion.

https://claude.ai/code/session_014oGFKL7EVg1b2cyPgt9Gne
@ragnorc
ragnorc force-pushed the claude/debug-optional-param-error-3xQtJ branch from 9848288 to 37b7a94 Compare April 13, 2026 08:43
Comment thread crates/omnigraph-compiler/src/query_input.rs

pub(super) fn literal_to_sql(lit: &Literal) -> String {
match lit {
Literal::Null => "NULL".to_string(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQL pushdown generates = NULL instead of IS NULL

Medium Severity

literal_to_sql converts Literal::Null to the string "NULL", which ir_filter_to_sql then uses in comparisons like column = NULL. In SQL, column = NULL always evaluates to NULL (falsy), never matching any rows — the correct form is column IS NULL. Filters using null parameters silently return empty results instead of matching null column values.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 37b7a94. Configure here.

The early return at line 273 for None/Value::Null params was skipping
the null-fill loop, leaving declared nullable params absent from the
map. Downstream code would then error with "parameter not provided".

https://claude.ai/code/session_014oGFKL7EVg1b2cyPgt9Gne

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

There are 4 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c943d97. Configure here.


fn literal_scalar_type(lit: &Literal) -> Result<ScalarType> {
match lit {
Literal::Null => Ok(ScalarType::String),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null in literal arrays breaks non-String type lists

Medium Severity

literal_scalar_type and literal_type both default Literal::Null to ScalarType::String. When a null appears inside a list alongside non-String elements (e.g., [null, 42]), list_scalar_type and the typecheck list validation see mismatched types (String vs I64) and reject the list with a confusing "elements must share a compatible scalar type" error. Null in a list is type-agnostic and needs to be treated as compatible with the surrounding element types rather than hardcoded to String.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c943d97. Configure here.

/// Create a constant array from a literal value.
fn literal_to_array(lit: &Literal, num_rows: usize) -> Result<ArrayRef> {
Ok(match lit {
Literal::Null => arrow_array::new_null_array(&DataType::Utf8, num_rows),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null literal array hardcodes Utf8 ignoring actual type

Medium Severity

literal_to_array creates a DataType::Utf8 null array for Literal::Null regardless of context. In contrast, literal_to_typed_array in mutation.rs correctly uses the target data_type. When a null parameter is projected in a result set, the column will have type Utf8 instead of the expected schema type, potentially causing type mismatches or incorrect schema inference in downstream consumers.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c943d97. Configure here.

@ragnorc
ragnorc merged commit 54252b2 into main Apr 13, 2026
4 checks passed
@aaltshuler aaltshuler mentioned this pull request Apr 14, 2026
ragnorc added a commit that referenced this pull request May 1, 2026
…pe + projection guard + CI

Three new findings from Cubic on commit 3223b51:

* **Pending edge cardinality counted within-input duplicates** (P2):
  count_src_per_edge's pending walk added every row to the count,
  including duplicate rows that finalize will collapse via
  dedupe_merge_batches_by_id. A LoadMode::Merge with the same edge id
  twice would over-count → spurious @card violation. Fix: when
  dedupe_key_column is Some, walk pending in reverse, track seen keys
  via HashSet, count only the kept (last-occurrence) rows. Mirrors
  finalize-time dedupe so cardinality counts what stage_merge_insert
  actually publishes.

* **scan_with_pending silently disabled merge-shadow when projection
  omitted key_column** (P2): if a caller passed Some("id") as
  key_column but their projection didn't include "id", the
  filter_out_rows_where_string_in helper passed batches through
  unchanged — silently degrading to union semantics. Fix: validate
  up front that projection contains key_column when both are Some;
  return a typed Lance error otherwise. Tightened the helper too:
  missing column is now an internal error (was a silent passthrough).

* **Cascade-vs-explicit delete test was too weak** (P2): asserted
  only that edge count decreased after delete. The cascade alone
  could satisfy that even if the explicit second-delete silently
  no-op'd. Strengthened: assert post_knows == 0, which only holds
  when both ops landed (Bob→Diana would survive if op-2 no-op'd).

CI gap: also added test_failpoints_feature job to .github/workflows/ci.yml.
The workspace test runs without --features failpoints (the feature is
behind a Cargo flag), so the failpoints test suite was never exercised
by CI before now. The new job builds + runs
`cargo test -p omnigraph-engine --features failpoints --test failpoints`
on every full CI run, mirroring the test_aws_feature pattern.

New tests on tests/runs.rs:

* load_merge_mode_dedupes_within_pending_for_cardinality_count
  (Cubic P2 #2 — pending-vs-pending dedup, distinct from the
  load_merge_mode_dedupes_edge_for_cardinality_count test which
  covers committed-vs-pending dedup).
* scan_with_pending_rejects_key_column_missing_from_projection
  (Cubic P2 #3 — verifies the up-front validation rejects bad
  callers and that the happy path still works correctly).

Local test results:

* tests/runs.rs: 23/23 passed
* tests/failpoints.rs --features failpoints: 7/7 passed (includes the
  two new finalize→publisher residual tests landed in 3223b51).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aaltshuler added a commit that referenced this pull request May 16, 2026
Wire the dormant DropProperty variant end-to-end for the Soft case.
Per docs/schema-lint-v1-plan.md, commit #3 of the schema-lint chassis
v1 series (MR-694).

Planner (schema_plan.rs):
- plan_properties: emit DropProperty { type_kind, type_name,
  property_name, mode: Soft } instead of UnsupportedChange when a
  property exists in accepted but not in desired. Plan is now
  supported = true for drop-only changes.

Apply (schema_apply.rs):
- Route DropProperty { Soft } through rewritten_tables. The existing
  batch_for_schema_apply_rewrite path already iterates the *target*
  schema fields, so a property absent from desired_catalog is
  naturally projected away. The prior Lance version retains the
  dropped column for time-travel reversibility (until cleanup runs).
- DropType still errors (lands in commit #4 with different mechanics:
  __manifest entry removal instead of column projection).
- DropProperty { Hard } still errors (lands in commit #5 with
  --allow-data-loss CLI flag + immediate compact_files +
  cleanup_old_versions).

Tests:
- Planner unit test plan_emits_soft_drop_for_removed_nullable_property
  asserts the variant emission + supported = true + no UnsupportedChange.
- Integration test apply_schema_drops_a_nullable_property_softly_
  preserves_prior_version (replaces the former
  apply_schema_rejects_dropping_a_property_with_data) asserts:
  (a) plan contains DropProperty { Soft }
  (b) apply succeeds + manifest advances + row count unchanged
  (c) current dataset schema lacks the dropped column
  (d) snapshot_at_version(pre_drop) still has the dropped column
  (e) reopen consistency — drop preserved across engine restart

Recovery: rides on SidecarKind::SchemaApply per MR-847. No new
sidecar kind needed; the entire apply path is already sidecar-wrapped.

Substrate alignment: this commit uses the stage_overwrite full-rewrite
path (full_rewrite cost class) rather than Lance native drop_columns
(catalog_only cost class). MR-948 is the follow-up substrate-alignment
refactor that introduces a LanceColumnOp surface and switches the
metadata-only case onto drop_columns. Functional outcome is identical;
cost-class improvement deferred.

Test results:
- cargo test -p omnigraph-compiler --lib: 238 passed
- cargo test -p omnigraph-engine --test schema_apply: 11 passed

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aaltshuler added a commit that referenced this pull request May 16, 2026
Wire the dormant DropProperty variant end-to-end for the Soft case.
Per docs/schema-lint-v1-plan.md, commit #3 of the schema-lint chassis
v1 series (MR-694).

Planner (schema_plan.rs):
- plan_properties: emit DropProperty { type_kind, type_name,
  property_name, mode: Soft } instead of UnsupportedChange when a
  property exists in accepted but not in desired. Plan is now
  supported = true for drop-only changes.

Apply (schema_apply.rs):
- Route DropProperty { Soft } through rewritten_tables. The existing
  batch_for_schema_apply_rewrite path already iterates the *target*
  schema fields, so a property absent from desired_catalog is
  naturally projected away. The prior Lance version retains the
  dropped column for time-travel reversibility (until cleanup runs).
- DropType still errors (lands in commit #4 with different mechanics:
  __manifest entry removal instead of column projection).
- DropProperty { Hard } still errors (lands in commit #5 with
  --allow-data-loss CLI flag + immediate compact_files +
  cleanup_old_versions).

Tests:
- Planner unit test plan_emits_soft_drop_for_removed_nullable_property
  asserts the variant emission + supported = true + no UnsupportedChange.
- Integration test apply_schema_drops_a_nullable_property_softly_
  preserves_prior_version (replaces the former
  apply_schema_rejects_dropping_a_property_with_data) asserts:
  (a) plan contains DropProperty { Soft }
  (b) apply succeeds + manifest advances + row count unchanged
  (c) current dataset schema lacks the dropped column
  (d) snapshot_at_version(pre_drop) still has the dropped column
  (e) reopen consistency — drop preserved across engine restart

Recovery: rides on SidecarKind::SchemaApply per MR-847. No new
sidecar kind needed; the entire apply path is already sidecar-wrapped.

Substrate alignment: this commit uses the stage_overwrite full-rewrite
path (full_rewrite cost class) rather than Lance native drop_columns
(catalog_only cost class). MR-948 is the follow-up substrate-alignment
refactor that introduces a LanceColumnOp surface and switches the
metadata-only case onto drop_columns. Functional outcome is identical;
cost-class improvement deferred.

Test results:
- cargo test -p omnigraph-compiler --lib: 238 passed
- cargo test -p omnigraph-engine --test schema_apply: 11 passed

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aaltshuler added a commit that referenced this pull request May 16, 2026
… (MR-694) (#90)

* schema-lint chassis v1 (WIP): tier surfacing + plan doc

First commit of the chassis v1 branch. Lands a small, foundational
slice without behavior change, plus a planning doc that lays out the
remaining 7 commits in sequence so the PR can be reviewed
incrementally.

This commit:

- Adds SchemaMigrationStep::diagnostic() returning the full
  &'static DiagnosticCode (family + tier + severity) for
  UnsupportedChange steps with codes. Renderers can now reach the
  tier without re-implementing the code → tier lookup.

- CLI `omnigraph schema plan` output now displays tier alongside
  code:

    unsupported change on node:Person.age [OG-DS-104, destructive]:
        removing property 'Person.age' is not supported in schema
        migration v1

  Operators see at-a-glance the kind of risk each rejection
  represents — not just the rule identifier.

- No behavior change. All 11 existing schema_apply tests still pass.

Planning doc at docs/schema-lint-v1-plan.md tracks the 7 remaining
commits to bring v1 to feature-complete:

  1. (this commit) Tier surfacing in plan output.
  2. Soft / Hard mode enum on drop steps.
  3. Tombstone fields on catalog IR.
  4. Planner emits DropProperty { Soft } by default.
  5. Apply path implements Soft mode.
  6. Convert PR #62 destructive-rejection tests.
  7. --allow-data-loss flag + Hard mode.
  8. (optional) Tombstone unhide / restore command.

Delete the planning doc when v1 lands. Intentionally checked in to
the WIP branch so the scope is reviewable; not intended as a
permanent doc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* schema-lint v1 commit 2: DropMode + dormant Drop* variants

Second commit of the chassis v1 branch. Lands the type-level shape
of soft/hard drops without wiring them up. Variants are reachable
from emitters but the planner doesn't produce them yet; the apply
path returns an explicit not-yet-implemented error if one shows up
via deserialization.

Added:

- `DropMode { Soft, Hard }` — orthogonal to `SafetyTier`. Tier
  classifies the rule's risk class; mode is the operator's intent
  for data treatment.
    - `Soft` → catalog tombstone, data retained. Tier: safe.
    - `Hard` → Lance-level removal. Tier: destructive; will require
      --allow-data-loss to apply (commit 7).

- `SchemaMigrationStep::DropType { type_kind, name, mode }` and
  `SchemaMigrationStep::DropProperty { type_kind, type_name,
  property_name, mode }` variants.

- Re-export `DropMode` from `omnigraph_compiler::DropMode` so
  downstream crates don't reach into the catalog submodule.

- CLI `render_schema_plan_step` arms for both variants, surfacing
  the mode in plan output: `drop property 'Person.age' of node
  'Person' (soft mode)`.

- `apply_schema_with_lock` exhaustive match arm for the two new
  variants that returns `manifest_internal` with a clear
  not-yet-implemented message. If a SchemaIR JSON containing
  Drop{Type,Property} arrives (e.g. from a future tool or hand-
  written), the apply path fails explicitly rather than silently
  misclassifying.

- Two new in-source tests:
    - `drop_steps_round_trip_through_serde` — pins the wire shape
      for all four (variant × mode) combinations.
    - `drop_mode_serde_uses_snake_case` — pins external-tool-
      friendly serialization (`"soft"` / `"hard"`).

Build: clean, only pre-existing warnings.
Tests:
- omnigraph-compiler schema_plan: 6/6 (4 existing + 2 new).
- omnigraph-engine schema_apply: 11/11 (unchanged — planner still
  emits UnsupportedChange for removal paths).

Next commit (commit 3 per docs/schema-lint-v1-plan.md): add the
`tombstoned: bool` fields to NodeIR / EdgeIR / PropertyIR for the
catalog representation of soft-mode tombstones.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* plan doc: reframe v1 around Lance native drop_columns

After a substrate audit of the Lance data-evolution guide on
2026-05-13, the v1 plan was simplified. Two key findings:

1. Lance's `drop_columns()` is already metadata-only and reversible
   via time travel until cleanup. No need for a parallel
   `tombstoned: bool` field in our catalog IR — Lance's version
   graph IS the tombstone.

2. The full schema_apply substrate migration (add_columns,
   drop_columns, alter_columns vs. stage_overwrite across all step
   types) is consolidated in MR-948 as a sibling issue. v1 only
   uses the relevant slice (drop_columns for OG-DS-1XX).

Net plan changes:

- Commit 3 (original): tombstone fields on catalog IR → dropped.
  No catalog IR change needed. The Lance drop_columns commit IS the
  tombstone.

- Commit 5 (original): apply path writes tombstoned: true → replaced
  with: apply path calls Dataset::drop_columns([name]).

- Commit 7 Hard mode: stage_overwrite removing the column → replaced
  with: drop_columns + compact_files + cleanup_old_versions. Same
  APIs omnigraph cleanup already uses.

- Commit 8 (original): omnigraph schema unhide → dropped. Time
  travel is the undo (omnigraph snapshot --at <commit>).

Net result: 8 commits → 5 commits. ~250 LoC less surface. More
substrate-aligned.

The chassis types from commit 2 (DropMode enum, DropType /
DropProperty variants) are kept exactly as designed; only the
implementation strategy changed.

The Lance docs quote is included in the doc so future readers see
the substrate behavior cited verbatim.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* schema-lint v1 commit 3: emit + apply DropProperty { Soft }

Wire the dormant DropProperty variant end-to-end for the Soft case.
Per docs/schema-lint-v1-plan.md, commit #3 of the schema-lint chassis
v1 series (MR-694).

Planner (schema_plan.rs):
- plan_properties: emit DropProperty { type_kind, type_name,
  property_name, mode: Soft } instead of UnsupportedChange when a
  property exists in accepted but not in desired. Plan is now
  supported = true for drop-only changes.

Apply (schema_apply.rs):
- Route DropProperty { Soft } through rewritten_tables. The existing
  batch_for_schema_apply_rewrite path already iterates the *target*
  schema fields, so a property absent from desired_catalog is
  naturally projected away. The prior Lance version retains the
  dropped column for time-travel reversibility (until cleanup runs).
- DropType still errors (lands in commit #4 with different mechanics:
  __manifest entry removal instead of column projection).
- DropProperty { Hard } still errors (lands in commit #5 with
  --allow-data-loss CLI flag + immediate compact_files +
  cleanup_old_versions).

Tests:
- Planner unit test plan_emits_soft_drop_for_removed_nullable_property
  asserts the variant emission + supported = true + no UnsupportedChange.
- Integration test apply_schema_drops_a_nullable_property_softly_
  preserves_prior_version (replaces the former
  apply_schema_rejects_dropping_a_property_with_data) asserts:
  (a) plan contains DropProperty { Soft }
  (b) apply succeeds + manifest advances + row count unchanged
  (c) current dataset schema lacks the dropped column
  (d) snapshot_at_version(pre_drop) still has the dropped column
  (e) reopen consistency — drop preserved across engine restart

Recovery: rides on SidecarKind::SchemaApply per MR-847. No new
sidecar kind needed; the entire apply path is already sidecar-wrapped.

Substrate alignment: this commit uses the stage_overwrite full-rewrite
path (full_rewrite cost class) rather than Lance native drop_columns
(catalog_only cost class). MR-948 is the follow-up substrate-alignment
refactor that introduces a LanceColumnOp surface and switches the
metadata-only case onto drop_columns. Functional outcome is identical;
cost-class improvement deferred.

Test results:
- cargo test -p omnigraph-compiler --lib: 238 passed
- cargo test -p omnigraph-engine --test schema_apply: 11 passed

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs: move schema-lint-v1-plan into docs/dev/ + add to index

Post-rebase fixup for the docs split (#93). The plan doc was added
to docs/ at the top level before main reorganized to docs/{user,dev}/.
This moves it into docs/dev/ and adds an entry to docs/dev/index.md
under a new "Active Implementation Plans" section so the
check-agents-md.sh link check passes.

Per the original commit message (617a77d), the plan doc is intentionally
temporary — it will be deleted when v1 lands.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aaltshuler added a commit that referenced this pull request May 16, 2026
Wire the second half of the dormant Drop* family. Per
docs/dev/schema-lint-v1-plan.md, commit #4 of the schema-lint chassis
v1 series (MR-694). Builds on commit #3 (PR #90, DropProperty Soft).

Planner (schema_plan.rs):
- plan_nodes leftover loop: emit DropType { Node, name, Soft }
  instead of UnsupportedChange (OG-DS-102) for node-type removals.
- plan_edges leftover loop: emit DropType { Edge, name, Soft }
  instead of UnsupportedChange (OG-DS-103) for edge-type removals.

Apply (schema_apply.rs):
- New dropped_tables: BTreeSet<String> accumulator alongside
  added_tables / renamed_tables / rewritten_tables.
- DropType arm in the metadata loop populates dropped_tables for
  Soft mode. Hard mode errors (lands in commit #5 with
  --allow-data-loss).
- New tombstone-emission loop after the rename sidecar build:
  for each dropped table, push to sidecar_tombstones AND populate
  table_tombstones with table_version + 1. The existing manifest
  publish path converts table_tombstones into ManifestChange::Tombstone
  operations — no new manifest plumbing needed.
- Soft DropType has no Phase B per-table write; the tombstone is the
  entire change. Lance dataset files are retained — prior __manifest
  versions still reference them, so time travel + branch-from-snapshot
  can read the dropped table until cleanup_old_versions runs.
- Rides on SidecarKind::SchemaApply per MR-847 (already established
  by commit #3).

Tests:
- Planner unit test plan_emits_soft_drop_for_removed_node_and_edge_types
  asserts both Node and Edge DropType { Soft } emission for the
  Company + WorksAt combined drop, plus no UnsupportedChange.
- Integration test apply_schema_drops_node_and_referencing_edge_softly
  (replaces apply_schema_rejects_dropping_a_node_type): asserts
  plan emission, apply success, current manifest entries absent,
  pre-drop manifest entries present (time-travel reversibility),
  reopen consistency.
- Integration test apply_schema_drops_an_edge_type_softly (replaces
  apply_schema_rejects_dropping_an_edge_type): single edge drop,
  asserts other tables untouched, time-travel reversibility.

Test results:
- cargo test -p omnigraph-compiler --lib: 239 passed (1 new + 238)
- cargo test -p omnigraph-engine --test schema_apply: 11 passed
  (2 converted + 9 unchanged)

Pending for v1 completion:
- Commit #5: --allow-data-loss CLI flag + Hard mode promotion in
  planner + immediate compact_files + cleanup_old_versions for
  both DropProperty and DropType.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
aaltshuler added a commit that referenced this pull request May 17, 2026
…(MR-722) (#102)

PR #2 of the policy chassis series (PR #1 = MR-731, merged in #101).
The structural fix that moves Cedar enforcement from HTTP-only to
engine-wide. apply_schema is the proof-of-concept writer; PR #3 fans
the enforce() call out to the remaining six (mutate_as, load,
ingest_as, branch_create_from, branch_delete, branch_merge).

## What lands

### New crate: omnigraph-policy

The 844-line policy.rs moves from `omnigraph-server` into a new
`omnigraph-policy` workspace crate so both engine and server can
depend on it. Cedar dependency moves with it. The server's policy.rs
becomes a re-export shim (`pub use omnigraph_policy::*`) so existing
`omnigraph_server::PolicyAction` etc. paths keep working — CLI and
test consumers don't have to migrate in one go.

### New trait: PolicyChecker

```rust
pub trait PolicyChecker: Send + Sync {
    fn check(&self, action: PolicyAction, scope: &ResourceScope,
             actor: &str) -> Result<(), PolicyError>;
}
```

`PolicyEngine` (Cedar-backed) implements it. `Omnigraph::with_policy()`
takes `Arc<dyn PolicyChecker>`. Engine tests mock the trait without
spinning up Cedar. MR-725 will extend the trait with `predicate_for()`
for query-layer pushdown — additive, no call-site changes.

### New enum: ResourceScope

Four variants — Graph, Branch, TargetBranch, BranchTransition —
mapping cleanly to today's `(branch, target_branch)` shape on
PolicyRequest via `to_branch_pair()`. Each engine writer picks the
variant that matches the existing HTTP-layer convention so engine
and HTTP evaluate the same Cedar decision.

**Invariant**: ResourceScope stays at branch granularity. Per-type
and per-row scope are MR-725's territory, not engine-layer's.
Adding Type/Row variants here creates two places per-type policy
can be evaluated, which can drift. See chassis design refinements
comment on MR-722 (2026-05-17).

### Omnigraph::with_policy() + enforce()

* New `policy: Option<Arc<dyn PolicyChecker>>` field on Omnigraph,
  None by default (preserves embedded/dev no-enforcement mode).
* `with_policy(self, checker)` setter — builder-style, consumes self.
* `enforce(action, scope, actor)` — the gate. When policy is None,
  no-op. When policy is Some AND actor is None, hard error — silent
  bypass via "I forgot the actor" is exactly the footgun this gate
  is here to prevent.

### apply_schema_as: first writer wired

* New public method `apply_schema_as(source, options, actor)` that
  calls `enforce(SchemaApply, TargetBranch("main"), actor)` before
  acquiring the schema-apply lock or doing any other work.
* Existing `apply_schema(source)` and `apply_schema_with_options(...)`
  delegate to it with actor=None (no-actor variants).
* HTTP handler `server_schema_apply` updated to call apply_schema_as
  with the resolved actor. AppState construction injects the
  PolicyEngine into Omnigraph via `with_policy`. HTTP-layer
  authorize_request still fires first; the engine gate is the
  redundant-but-correct backstop and the only path that protects SDK
  / embedded callers. PR #3 removes the HTTP redundancy.

### OmniError::Policy

New error variant for engine-layer policy denial / evaluation
failure. ApiError::from_omni maps it to 403.

### MR-724 Admin action — Option A reservation

PolicyAction::Admin kept in the enum with a load-bearing doc
comment naming its future consumers (hot reload, audit log query,
approvals list per MR-726 / MR-732 / MR-734). No enforce(Admin, ...)
call site exists yet — the variant is reserved so the action
vocabulary is complete from chassis day one. MR-724 closes when
the first consumer surface ships.

### New SDK-side integration test

`crates/omnigraph/tests/policy_engine_chassis.rs` — four tests
covering:
* Policy denies for unauthorized actor → OmniError::Policy
* Policy permits for authorized actor → apply succeeds
* Policy installed + no actor → hard error (forget-the-actor footgun)
* No policy → no-op (embedded/dev default still works)

These exercise the engine path directly — no HTTP layer involved.

## Test results

- cargo test --workspace --locked --no-fail-fast: 851 passed, 0 failed
  * 45 server tests (existing) pass
  * 14 schema_apply tests (existing) pass
  * 4 new chassis tests pass
  * 60 OpenAPI tests pass (no HTTP API surface changes)
  * No regressions across the workspace

## Architectural decisions baked in

Per MR-722 chassis design refinements comment (2026-05-17):

1. PolicyChecker is a trait, not just a concrete. Engine and server
   consume the trait. MR-725 adds predicate_for() additively.
2. ResourceScope stays at branch granularity. No Type/Row variants.
3. Coarse-vs-fine framing pinned: engine-layer is action gate;
   query-layer (MR-725) is predicate gate. Both backed by same Cedar
   engine; non-overlapping responsibilities.
4. Admin action reserved for policy-management surfaces (MR-724
   Option A).

## Pending follow-ups (PR #3+)

- Fan-out enforce() to mutate_as, load, ingest_as, branch_create_from,
  branch_delete, branch_merge (PR #3).
- Remove HTTP-layer authorize_request redundancy once engine gate
  covers all writers (PR #3).
- CLI policy injection into Omnigraph for non-`policy validate|test|explain`
  subcommands (PR #3 or follow-up).
- MR-723 default-deny 3-state matrix (PR #4).
- MR-736 severity warn/deny (PR #5).
- AGENTS.md scope-of-enforcement rewrite once chassis fully lands.
- Coarse-vs-fine framing in docs/user/policy.md.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
ragnorc added a commit that referenced this pull request May 27, 2026
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>
ragnorc added a commit that referenced this pull request May 28, 2026
* mr-668: add GraphId newtype + Cloud-mode forward identity stubs (PR 1/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>

* mr-668: Omnigraph::init error-path cleanup + three failpoints (PR 2a/10)

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>

* mr-668: add GraphHandle + GraphRegistry data structure (PR 3/10)

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>

* mr-668: router restructure + handler refactor for multi-graph (PR 4a/10)

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>

* mr-668: OpenAPI multi-mode cluster filter (PR 4b/10)

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>

* mr-668: multi-graph startup + mode inference (PR 5/10)

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>

* mr-668: Cedar resource-model refactor (PR 6a/10)

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>

* mr-668: GET /graphs endpoint + per-graph policy wire-up (PR 6b/10)

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>

* mr-668: POST /graphs runtime create endpoint (PR 7/10)

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>

* mr-668: CLI omnigraph graphs list/create (PR 8/10)

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>

* mr-668: composite e2e tests, race fix, v0.7.0 release (PR 9/10)

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>

* mr-668: remove POST /graphs and CLI graphs create (defer runtime graph 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>

* mr-668: comment cleanup and policy format style

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>

* mr-668: drop actor_id from PolicyRequest; pass actor as separate arg

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>

* mr-668: consolidate AppState single-mode constructors; delete with_policy_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>

* mr-668: derive any_per_graph_policy on RegistrySnapshot; simplify dup 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>

* mr-668: fail-fast multi-graph startup with try_collect

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>

* mr-668: drop unused State extractor from 7 read-only handlers

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>

* mr-668: emit info! for graph routing decision

`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>

* mr-668: align GET /graphs 405 body code with HTTP status

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>

* mr-668: drop unused axum::handler::Handler import

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>

* mr-668: drop unused fs2 workspace dependency

`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>

* mr-668: AGENTS.md Cedar row no longer hardcodes action count

The "8 actions" claim drifted as soon as MR-668 added `graph_list`.
Bumping the count would just push the drift one PR forward; the
correct-by-design fix is to defer to the canonical list in
docs/user/policy.md and stop maintaining a duplicate count.

Closes the "doc hardcodes a count that drifts from the enum" class.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* mr-668: cfg(test)-gate GraphRegistry::insert and its mutex

`insert` and the `mutate: Mutex<()>` that serializes it had no
runtime consumer in v0.7.0 — the only insertion path at startup
is `from_handles`, and runtime add/remove is deferred until a
managed cluster catalog ships. Leaving both `pub` and live made
them a "looks like API, isn't" footgun: a future change could
build on `insert` without re-establishing the concurrency contract
with an actual consumer in scope.

Gate both together (`#[cfg(test)]` on the method, the field, and
the `tokio::sync::Mutex` import) so the race-pinning tests still
compile but production cannot reach them. When a real consumer
ships, ungate both — they're a unit. Closes the "public API with
no runtime consumer drifts toward incorrect" class.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* mr-668: drop vestigial PolicyEngine surface

* `validate_request` had zero callsites — pure surface for nothing.
* `deny`'s `_actor_id` and `_request` parameters were both unused
  (the underscore prefix gave it away); the message is built by the
  caller before `deny` ever sees the request. Trim both.

Closes the "public API that the type system can't justify" class
for the policy engine. No behavior change; every existing test
stays green because the deletions never had a runtime effect.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* mr-668: regression test for init re-init footgun (red)

A second `Omnigraph::init` against an existing graph URI today
destroys the existing graph's schema artifacts. `init_storage_phase`
overwrites `_schema.pg` before any preflight, and on the inner
`GraphCoordinator::init` failure that follows,
`best_effort_cleanup_init_artifacts` deletes all three schema files.
The existing Lance datasets and `__manifest/` survive but the
schema metadata is gone — unrecoverable without operator surgery.

This test exercises that path and currently fails with
"_schema.pg must not be deleted by a failed re-init", confirming
the destructive cleanup branch fires. The fix in the next commit
makes the test pass by preflighting with `storage.exists()` and
returning a typed error before any write touches disk.

Per AGENTS.md rule 12, the test commit lands just before the fix
commit so the red → green 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>

* mr-668: close init re-init footgun via InitOptions preflight (green)

`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>

* mr-668: split PolicyEngine::load into kind-typed loaders

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>

* mr-668: introduce GraphRouting, retire single_mode_handle

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>

* mr-668: remove ServerMode, registry field, and the SINGLE_GRAPH sentinel

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>

* mr-668: regression test for nested-route path extraction (red)

`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>

* mr-668: named-field path-param structs for nested cluster routes (green)

`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>

* mr-668: centralize policy-requires-tokens check in the runtime classifier

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>

* mr-668: close coverage gaps surfaced by the test-coverage audit

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…
@aaltshuler
aaltshuler deleted the claude/debug-optional-param-error-3xQtJ branch June 8, 2026 15:43
ragnorc added a commit that referenced this pull request Jun 23, 2026
…e per write (#298)

* docs(rfc-013): step-3b handoff + §4.1 corrections (validated)

Add the RFC-013 write-path handoff doc, and correct §4.1's WriteTxn sketch from the
4-subagent validation against current code:
- HandleCache → handle-threading (forward the commit-return handle; a version-keyed
  cache misses because HEAD walks N→N+1→N+2 across staging + index-build commits).
- "re-resolution unrepresentable" softened to "pinned base for the pre-commit phase +
  named fresh re-reads at the commit/fork boundary" — three reads (commit-time OCC, the
  live-HEAD drift probe, fork authority) are irreducible correctness machinery.
- WriteParams DOES carry a session field; the real constraint is "stage off an open
  Dataset," so attach the Session by opening read-style then staging off it.

* test(engine): RED step-3b capture-once fitness asserts + open_count probe

Two write-path cost gates, RED today, GREEN after the WriteTxn lands:
- write_validates_schema_contract_once: a write must validate the schema contract
  once (3 read_text + 2 exists). Today re-validates at every resolve point —
  measured 12 read_text / 9 exists (~4 validations) via CountingStorageAdapter
  (zero production change; the write twin of the read-path schema-once test).
- keyed_insert_opens_table_at_most_once: a keyed single-table write must open its
  table <=1x. Today measured 10 opens.

Adds an exact open-CALL probe: open_count + record_open() on QueryIoProbes (mirroring
probe_count/record_probe), called at both open chokepoints; surfaced as
IoCounts.open_count. forbidden_apis guarantees every write open routes through them.

* feat(engine): WriteTxn carrier + open_write_txn (3b scaffolding)

The capture-once write transaction (RFC-013 step 3b): WriteTxn{branch, base:
Snapshot, session} + Omnigraph::open_write_txn, which validates the schema contract
once and pins the base snapshot + the shared per-graph Session.

Landed as reviewed scaffolding (gated #[allow(dead_code)]); the next pass threads
Option<&WriteTxn> through open_for_mutation_on_branch / staging on the non-strict
bound-branch path — opening the base once from the pinned entry with the warm session
(a session-aware pinned opener returning a SnapshotHandle) and skipping the per-table
schema re-validation — to turn the two RED cost gates green. Strict ops / fork / the
commit-time OCC re-read keep their fresh reads.

* test(engine): scope write-path open_count to data tables (RFC-013 step 3b)

The keyed_insert_opens_table_at_most_once gate asserted open_count <= 1, but
open_count was a single unclassified counter: record_open() fires in both
open chokepoints, and open_dataset_tracked also opens the internal/system
tables (__manifest via layout.rs, _graph_commits/_graph_commit_actors via
commit_graph.rs). So the count conflated data-table opens with the publisher
CAS + commit-graph append opens — making the gate measure the wrong quantity
and unreachable by threading alone (the manifest publish keeps it >1 regardless).

Scope it by table class, mirroring the read-side counters (which already split
by URI prefix via separate wrappers): record_open(uri) classifies the open's
last path segment and feeds data_open_count vs internal_open_count. IoCounts
exposes both; the gate now asserts data_open_count <= 1.

Re-baselined: a single keyed insert is data_open_count=4 / internal_open_count=6
(sum 10, the old conflated value). The RED target for the WriteTxn threading is
now the real data-table-open count (4 -> 1), with internal opens correctly out
of scope. Pure test-harness/instrumentation; no production behavior change
(classification runs only inside the probe closure, skipped when no probes are
installed).

Also marks #297 (optimize-vs-write race) as landed in the step-3b handoff —
this branch is already stacked on origin/main after it merged.

* feat(engine): validate the schema contract once per write (RFC-013 step 3b)

A single mutate/load re-validated the schema contract ~4 times: at the entry
(ensure_schema_state_valid), per-table in open_for_mutation_on_branch
(resolved_branch_target), at the commit-time OCC re-read (fresh_snapshot_for_branch),
and in the publisher's index-build snapshot (snapshot_for_branch). Each validation
is 3 read_text + 2 exists on the storage adapter — O(touched resolve-points) of
redundant contract I/O on every write.

Thread the already-landed WriteTxn carrier through the write path: capture
`txn = open_write_txn(branch)` once at the mutate/load entry (the single validation),
then source the per-table entry and the commit/publish snapshots from `txn.base`
instead of re-resolving. When `txn` is None (branch merge, schema apply, tests) every
function is byte-identical to before.

- mutate_with_current_actor / load_jsonl_reader capture txn once (replacing the
  entry-point ensure_schema_state_valid) and thread Some(&txn) through
  execute_*/open_table_for_mutation, commit_all, and
  commit_updates_on_branch_with_expected.
- open_for_mutation_on_branch sources (snapshot, branch) from txn.base/txn.branch
  when present — skipping resolved_branch_target's re-validation. The OPEN itself is
  unchanged (still HEAD via open_dataset_head_for_write), and strict ops keep
  ensure_expected_version. Schema-once applies to strict and non-strict alike; the
  data-open collapse is a separate change.
- commit_all uses fresh_snapshot_for_branch_unchecked (the OCC manifest re-read minus
  the schema re-validation) when txn is present; the drift guard is unchanged.
- prepare_updates_for_commit uses txn.base for the publisher index-build snapshot.

fresh_snapshot_for_branch{,_unchecked} now read the manifest directly via
ManifestCoordinator instead of resolve_target. The OCC re-read consumes only the
Snapshot (per-table location + version), which ManifestCoordinator::open().snapshot()
produces identically — but resolve_target additionally opened the commit graph (a
spurious _graph_commits.lance exists probe the OCC read never consults). Dropping that
load is a pure read-cost reduction for every fresh-snapshot caller (commit_all's None
arm, optimize, repair, fork reclaim); the returned Snapshot is unchanged and the read
is a fresher cold manifest re-read, so the OCC freshness guarantee is preserved.

Greens write_validates_schema_contract_once (3 read_text / 2 exists, was 12/9).
keyed_insert_opens_table_at_most_once stays red (data_open_count=4) — the open
collapse lands next. Full engine suite green otherwise.

* feat(engine): open each data table once per write (RFC-013 step 3b)

A single keyed-node mutate opened its data table 4 times: accumulation (to read
.version()), staging (the real write base), the commit-time drift guard (to read
live HEAD), and the publisher's index build (reopen at the just-committed version).
Collapse three of the four — using the WriteTxn carrier threaded for schema-once —
so a write opens each touched data table at most once.

- #1 accumulation: open_for_mutation_on_branch now returns
  (Option<SnapshotHandle>, expected_version, full_path, table_branch). On the txn's
  own branch, a non-strict (Insert/Merge) op needs no open — the only thing the
  caller reads is .version() (the CAS fence), which is exactly the pinned base
  version (entry.table_version). So skip open_dataset_head_for_write and source the
  version from txn.base. The node insert path already discarded that handle; the
  edge path resolves a pinned read only when non-default cardinality needs it.
  STRICT ops and any write that must fork still open live HEAD + ensure_expected_version.
- #3 commit drift guard: commit_all reads live HEAD via
  entry.dataset.dataset().latest_version_id() — a cheap manifest-pointer probe off
  the already-open staging handle (the same primitive ManifestCoordinator::
  probe_latest_version uses) instead of a fresh open_dataset_head_for_write. The
  head<current / head>current drift classification is byte-identical.
- #4 index build: commit_all now returns the per-table post-commit_staged
  SnapshotHandle map; commit_updates_on_branch_with_expected threads it into
  prepare_updates_for_commit, which builds indices on the threaded handle instead of
  reopening at the same just-committed version. Absent a handle (other writers,
  inline/delete tables) the reopen path is byte-identical.

When txn is None (branch merge, schema apply, tests) every function opens and checks
exactly as before. Greens keyed_insert_opens_table_at_most_once (data_open_count 4->1).
Schema-once gate stays 3/2. Full engine suite + failpoints (recovery sidecar lifecycle)
green.

* refactor(engine): name the write-path open/commit returns (RFC-013 step 3b)

The open collapse left two positional returns that are easy to mis-thread and
carry an unwritten contract: open_for_mutation_on_branch's
(Option<SnapshotHandle>, u64, String, Option<String>) and commit_all's 5-tuple
(updates, expected_versions, sidecar_handle, guards, committed_handles). Replace
both with named structs so each field reads at the call site and the Option's
contract is documented, not folklore.

- OpenedForMutation { handle, expected_version, full_path, table_branch } with a
  require_handle(ctx) helper for the callers that must have a handle (strict ops,
  the fork path, every no-txn caller — branch merge, the seed test). The handle is
  None only on the non-strict-txn open-skip path (collapse #1); require_handle
  panics with a named context if that contract is ever broken.
- CommittedMutation { updates, expected_versions, sidecar_handle, guards,
  committed_handles } for commit_all; consumers destructure into the same local
  bindings they already used, so the publish/sidecar/guard-hold logic is unchanged.
- A debug_assert in open_table_for_mutation pins the skip contract: a missing handle
  is legal only on the non-strict txn path, so a future strict arm returning None
  trips in debug builds instead of handing None to a require_handle consumer.

Pure refactor — no behavior change. Both cost gates stay green (schema 3/2,
data_open_count=1), full engine suite + lib (162) green.

* refactor(engine): drop the unearned session field from WriteTxn (RFC-013 step 3b)

The open collapse greens data_open_count<=1 by SKIPPING the accumulation open,
PROBING live HEAD with latest_version_id, and REUSING the commit_staged handle —
none of which consume a session. The captured WriteTxn.session was therefore dead
(`#[allow(dead_code)]`): unearned surface a reviewer rightly flags.

Remove it. The carrier is now {branch, base} — exactly what schema-once + the open
collapse use. Step 5 (PublishPlan unification) makes WriteTxn the non-optional
publish carrier and is the right home for session-aware base opens, where the
warm-session benefit on the single remaining open — an object-store (S3) phenomenon,
invisible on local FS — can be earned by its own cost gate rather than carried dead
through this PR.

No behavior change; both cost gates stay green (schema 3/2, data_open_count=1).

* docs(rfc-013): mark step 3b DONE — schema-once + open-collapse shipped, session deferred to step 5

* docs(rfc-013): capture the write-base-staleness convergence (§1d)

Three findings this cycle share one root — the write base is a stale, un-probed,
un-classified pin (the read path probes; the write path returns the warm
coordinator snapshot):

- #298 edge-@card stale-read regression (cursor High / codex P1, VALID): collapse #1
  made the cardinality scan read txn.base instead of live HEAD, so a concurrent edge
  is uncounted and a max can be exceeded. Fix on #298: restore the live-HEAD read +
  deterministic test + correct the single-writer doc comment.
- The structural liability underneath: no unified write-validation read-set —
  endpoint/cardinality/uniqueness each pick freshness ad hoc (warm/pinned/live),
  the same cardinality check forks mutation-vs-loader, none re-validated at commit.
- The served-strict-write stale-view false-fail (validated on prod + a #[ignore]
  repro): a strict update/delete false-fails ExpectedVersionMismatch after an external
  optimize advance — the write-side mirror of #297/§6.6. The naive blanket probe is
  proven wrong (breaks the cross-process lost-update OCC contract).

All three converge on Design A (step 5): open_txn's warm probe makes the base fresh,
the op-class-aware precondition (derive maintenance vs logical from Lance per-version
transaction metadata — no parallel marker) fast-forwards maintenance and fails logical,
and §7.1's read-set-in-CAS unifies + re-validates the validation read-set. §8 records
the #298 follow-up, the widened §7.1 scope, and the step-5 two-test acceptance contract.

* test(engine): RED — edge @card must scan live HEAD, not stale txn.base (#298)

Regression guard for the cursor-High/codex-P1 finding on #298: 3b's collapse #1
made the non-strict edge-insert cardinality scan read the pinned txn.base instead
of live HEAD (edge_cardinality_read_handle), so a concurrent edge committed after
txn capture is uncounted and a @card max is silently exceeded (invariant 9).

Deterministic two-handle test (no failpoint): handle A commits WorksAt(Alice->Acme)
to the @card(0..1) max; stale handle B (never read since) inserts a second WorksAt
for Alice. B's coordinator is stale by construction (the write path doesn't probe),
so B scans txn.base (Alice has 0) and wrongly commits the 2nd edge. RED: the insert
that must be rejected currently succeeds (panics at unwrap_err). Goes green when the
scan reads live HEAD.

* fix(engine): scan live HEAD for edge @card, not the pinned txn.base (#298)

3b's collapse #1 skips the non-strict edge accumulation open, so edge_cardinality_
read_handle reopened the edge table at the pinned txn.base for the @card scan. Since
cardinality is validated once (never rechecked at commit), a concurrent edge committed
after txn capture was uncounted and a @card max could be silently exceeded (invariant
9) — the cursor-High/codex-P1 regression on #298. Pre-3b the scan read live HEAD (the
mutation's own open_dataset_head_for_write handle).

Restore the live-HEAD read: take the table LOCATION from the pinned entry (stable
across versions) and open the dataset at its current HEAD via open_dataset_head_for_
write. Gate-safe — the data_open_count / merge-insert-only gates are node inserts; the
edge cardinality path (non-default @card only) is untouched by them, and the extra
live-HEAD open is exactly the pre-3b shape. Also drops the dead None-fallback's schema
re-validation (greptile P2, auto-resolved). The residual validate->commit TOCTOU is the
pre-existing §7.1 gap (RFC-013 step 4), recorded in handoff §1d/§8.

Turns cardinality_rejected_for_stale_handle_after_concurrent_edge_commit green;
validators / write_cost / writes / consistency / end_to_end / branching all green.

* docs(dev): link handoff docs from index

* docs(engine): tighten 3b claims to match the code (#298 review)

Review caught several comments/docs overclaiming what the code does (the session
drop + the #298 cardinality fix left stale/too-strong wording). No logic change.

- open_write_txn doc: drop the stale "shared per-graph Session" (WriteTxn no longer
  carries one); scope "once" to the table-touch hot path and note edge/load RI
  validation still re-resolves (→ step 4 §7.1) + the session-aware open is step 5.
- edge cardinality call-site comment: it said the scan uses a "pinned txn.base" — it
  now opens LIVE HEAD (#298); corrected.
- write_cost.rs: "opens the base once (with the shared Session)" → session-aware base
  open is deferred to step 5.
- data_open_count completeness (instrumentation.rs + write_cost.rs): forbidden_apis
  only keeps engine code OUTSIDE the storage layer on the chokepoints; table_store.rs
  is allow-listed and holds direct Dataset::opens for branch-management ops (not the
  keyed-write hot path the gate measures). Narrowed the claim accordingly.
- handoff §4: "schema once / open once" is the node hot path (the two gates); edge
  endpoint + loader RI/cardinality still re-validate and read warm — #298 un-regresses
  cardinality only, it does NOT close write-validation freshness (that's step 4 §1d/§7.1).

build clean; write_cost / validators / forbidden_apis green.
ragnorc added a commit that referenced this pull request Jun 27, 2026
…lible

#5: bound the seed-derived clock_micros offset via splitmix64(seed) %
86_400_000_000 so a large u64 seed can't overflow i64 and mint bogus ULID
times. #3: add a fallible now_micros_result() (deterministic under with_seed;
otherwise the real clock, PROPAGATING a clock-before-epoch error) and route the
commit-graph + recovery-audit helpers through it, so a bad clock surfaces
OmniError::manifest again instead of silently persisting 0 (deny-list: no
silent failures). Verified: prod build clean; --features dst --test dst -> 11
passed (incl. replay_equality_same_seed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants