Skip to content

schema-lint chassis v1.2: --allow-data-loss flag + Hard mode (MR-694) — completes v1 - #100

Merged
aaltshuler merged 2 commits into
mainfrom
andrew/mr-694-chassis-v1-commit5
May 16, 2026
Merged

schema-lint chassis v1.2: --allow-data-loss flag + Hard mode (MR-694) — completes v1#100
aaltshuler merged 2 commits into
mainfrom
andrew/mr-694-chassis-v1-commit5

Conversation

@aaltshuler

@aaltshuler aaltshuler commented May 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Final chassis v1 commit. Wires up the --allow-data-loss CLI flag and Hard mode for both DropProperty and DropType. After this lands, the chassis v1 series (MR-694) is complete and the chassis epic + MR-700 (DropType/DropProperty) ticket can close.

What lands

CLI (omnigraph-cli):

  • New --allow-data-loss flag on both omnigraph schema plan and omnigraph schema apply. Off by default → Soft drops (today's behavior).
  • HTTP remote schema apply explicitly rejects the flag for now with a clear error. HTTP parity is a separate small follow-up.

Engine (omnigraph + omnigraph::db):

  • New pub struct SchemaApplyOptions { pub allow_data_loss: bool }, re-exported as omnigraph::db::SchemaApplyOptions.
  • New public methods plan_schema_with_options / apply_schema_with_options. Existing plan_schema / apply_schema are thin wrappers using Default::default(). No breaking changes for existing callers.
  • promote_drops_to_hard: post-plan walk that promotes every DropMode::Soft to DropMode::Hard when the flag is set. Keeps the compiler's plan_schema_migration signature unchanged — promotion lives in the engine layer where operator intent belongs.
  • New hard_cleanup_targets: Vec<(String, String)> accumulator. Apply path for both DropProperty and DropType populates it when the variant is Hard.
  • Post-publish cleanup: new loop iterates hard_cleanup_targets and calls cleanup_old_versions (before_timestamp = now) on each dataset URI. Best-effort — the apply is already durable; cleanup failure is tracing::warn, not a hard failure.

Tests (tests/schema_apply.rs):

  • apply_schema_with_allow_data_loss_promotes_drops_to_hard — default plan emits Soft; with options.allow_data_loss=true, plan emits Hard; apply succeeds. Negative: no Soft drops remain in the promoted plan.
  • apply_schema_hard_drops_property_makes_prior_version_unreachable — the key Hard-mode behavioral test. After Hard drop + cleanup, snapshot_at_version(pre_drop).open("node:Person") FAILS because Lance reclaimed the prior dataset version. This is the user-facing Hard contract.
  • apply_schema_hard_drops_node_and_edge_with_flag_succeeds — both Node and Edge DropType variants promote to Hard with the flag; apply succeeds; current manifest entries gone.

Behavioral details

DropProperty Hard: stage_overwrite produced a new dataset version without the column. cleanup_old_versions removes the prior version + reclaims unique fragments. Post-apply, time-travel back to pre-drop fails (Lance prior version is gone).

DropType Hard: no per-table write happens — the change is the manifest tombstone. cleanup_old_versions on the orphan dataset is a no-op in the immediate term (no prior versions to clean since the dataset wasn't modified by this apply). The dataset directory persists. User-facing contract: data is unreachable via omnigraph (manifest entry tombstoned). Full orphan-cleanup is a documented follow-up.

Test plan

  • cargo test -p omnigraph-compiler --lib — 239 pass
  • cargo test -p omnigraph-engine --test schema_apply — 14 pass (3 new + 11 existing)
  • cargo test -p omnigraph-server --test openapi — 60 pass (no HTTP changes; OpenAPI unchanged)

What's deliberately not in this PR (known follow-ups)

  • HTTP parity — extend SchemaApplyRequest with allow_data_loss: bool field, thread through server handler, regenerate openapi.json. Small focused PR.
  • Orphan-dataset directory deletion for DropType { Hard } — currently the dataset directory persists after Hard type drop because cleanup_old_versions doesn't remove it (no prior versions to clean). Needs object_store recursive delete. Separate ticket.
  • MR-948 substrate alignment — swap DropProperty Soft from stage_overwrite (O(rows)) to Lance native Dataset::drop_columns (O(catalog)). Cost-class optimization; functional behavior unchanged.

v1 status

After this lands, chassis v1 is complete for CLI/embedded use:

Commit What Where
#1+2 chassis v0 + DropMode dormant merged in #90
#3 DropProperty { Soft } end-to-end merged in #90
#4 DropType { Soft } end-to-end merged in #99
#5 --allow-data-loss flag + Hard mode this PR

MR-694 chassis epic + MR-700 DropType/DropProperty ticket can close after merge.

🤖 Generated with Claude Code


Open in Devin Review

Final v1 commit. Wires up the --allow-data-loss CLI flag and Hard
mode for both DropProperty and DropType. Per
docs/dev/schema-lint-v1-plan.md, commit #5 of the schema-lint
chassis v1 series (MR-694).

CLI (omnigraph-cli/src/main.rs):
- New --allow-data-loss flag on both `omnigraph schema plan` and
  `omnigraph schema apply` subcommands. Off by default (Soft).
- HTTP remote schema apply explicitly rejects the flag for now
  (CLI-only; HTTP parity is a separate small follow-up that adds
  the field to SchemaApplyRequest + the server handler).

Engine (omnigraph.rs + schema_apply.rs):
- New SchemaApplyOptions { allow_data_loss: bool } public struct
  (Default = all false), re-exported via omnigraph::db::SchemaApplyOptions.
- New public methods: plan_schema_with_options and
  apply_schema_with_options. Existing plan_schema/apply_schema are
  now thin wrappers that pass Default::default().
- promote_drops_to_hard: post-plan walk that promotes every
  DropMode::Soft step to DropMode::Hard when the flag is set.
  Keeps the compiler's plan_schema_migration signature unchanged
  (no breaking change for tests / callers).
- Apply path: both Drop arms accept Hard mode; behavior is
  identical to Soft inside the apply loop. The DIFFERENCE is the
  new hard_cleanup_targets: Vec<(String, String)> accumulator,
  populated for every Hard variant with (table_key, full_dataset_uri).
- Post-publish cleanup: a new loop after the manifest commit
  iterates hard_cleanup_targets and calls cleanup_old_versions
  (before_timestamp = now) on each dataset URI. Best-effort —
  the apply is already durable; cleanup failure is logged via
  tracing::warn rather than failing the apply.
- New cleanup_dataset_old_versions helper inlines the Lance
  cleanup_old_versions call against a dataset URI.

Behavioral details:
- DropProperty Hard: stage_overwrite produced a new dataset version
  without the column. cleanup_old_versions removes the prior version
  (and reclaims unique fragments). After Hard apply,
  snapshot_at_version(pre_drop).open(table_key) FAILS because the
  prior dataset version was reclaimed.
- DropType Hard: no per-table write happens (the change is the
  manifest tombstone). cleanup_old_versions on the orphan dataset
  is a no-op in the immediate term (no prior versions to clean
  since the dataset wasn't modified by this apply). The dataset
  directory persists. Full orphan-cleanup is a documented
  follow-up — the user-facing contract is "data is unreachable
  via omnigraph" (manifest entry tombstoned), which is satisfied.

Tests (tests/schema_apply.rs):
- apply_schema_with_allow_data_loss_promotes_drops_to_hard:
  default plan emits Soft; with options.allow_data_loss=true,
  plan emits Hard; apply succeeds.
- apply_schema_hard_drops_property_makes_prior_version_unreachable:
  Hard drop succeeds, current snapshot lacks the column, and
  snapshot_at_version(pre_drop).open("node:Person") FAILS (Lance
  prior version reclaimed by cleanup).
- apply_schema_hard_drops_node_and_edge_with_flag_succeeds: both
  Node and Edge DropType variants are promoted to Hard with the
  flag; apply succeeds; current manifest entries gone. (Orphan
  dataset directory cleanup deferred.)

Test results:
- cargo test -p omnigraph-compiler --lib: 239 passed
- cargo test -p omnigraph-engine --test schema_apply: 14 passed
  (3 new Hard tests + 11 existing soft/regression tests)
- cargo test -p omnigraph-server --test openapi: 60 passed (no
  HTTP API surface changes in this commit; OpenAPI parity follow-up
  noted)

v1 status: complete for CLI/embedded use. MR-694 chassis epic +
MR-700 DropType/DropProperty ticket can close after this lands.

Known follow-ups (separate small PRs):
- HTTP parity: extend SchemaApplyRequest with allow_data_loss field,
  thread through server handler, regenerate openapi.json.
- Orphan-dataset directory deletion for DropType Hard (currently
  the dataset directory persists; cleanup_old_versions doesn't
  remove it because the dataset wasn't modified).
- MR-948 substrate alignment: swap DropProperty Soft from
  stage_overwrite to Dataset::drop_columns (catalog_only vs
  full_rewrite cost class).

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

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

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment on lines +322 to +323
#[arg(long, default_value_t = false)]
allow_data_loss: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 AGENTS.md Rule 1 violation: new --allow-data-loss CLI flag ships without user-facing doc updates

AGENTS.md Rule 1 mandates: "New endpoint, query function, CLI flag, env var, constant, schema construct, or invariant: update both the source code and the doc in the same change. Never split documentation drift into a follow-up." This PR adds --allow-data-loss to schema plan and schema apply but none of the user-facing docs are updated: docs/user/cli-reference.md doesn't mention the flag, docs/user/schema-language.md doesn't document DropProperty/DropType steps or the soft/hard mode distinction, and docs/user/maintenance.md doesn't describe the hard-drop cleanup behavior. The SchemaApplyOptions public API type and the plan_schema_with_options/apply_schema_with_options public methods are also undocumented.

Prompt for agents
AGENTS.md Rule 1 requires that new CLI flags, API methods, and behavioral changes are documented in the same PR. This PR adds --allow-data-loss to schema plan and schema apply commands but does not update any user-facing docs.

Files to update:
1. docs/user/cli-reference.md — add --allow-data-loss to the schema plan and schema apply entries, explaining that it promotes soft drops to hard drops (cleanup_old_versions runs post-apply).
2. docs/user/schema-language.md — add DropProperty and DropType to the migration step type list (they were added in a prior PR but never documented). Document the Soft vs Hard mode distinction and the --allow-data-loss flag.
3. docs/user/maintenance.md — mention that hard-drop schema apply runs cleanup_old_versions inline on affected datasets, and note the DropType Hard limitation (dataset directory persists until orphan-cleanup pass).

Also consider updating the quick-reference flows in AGENTS.md to show --allow-data-loss usage.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@cubic-dev-ai cubic-dev-ai 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.

cubic analysis

1 issue found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/omnigraph/src/db/omnigraph/schema_apply.rs">

<violation number="1" location="crates/omnigraph/src/db/omnigraph/schema_apply.rs:214">
P1: Hard DropProperty resolves cleanup target from the renamed key, which can fail valid rename+drop migrations with `--allow-data-loss`.</violation>
</file>

Linked issue analysis

Linked issue: MR-694: Schema-lint chassis: classification tuple + per-rule severity + suppression + pre-migration checks

Status Acceptance criteria Notes
Add --allow-data-loss flag to `omnigraph schema plan` and `omnigraph schema apply`; remote (HTTP) apply explicitly rejects the flag CLI argv parsers in crates/omnigraph-cli/src/main.rs add `allow_data_loss` for both plan and apply and return an error when a remote HTTP apply is attempted with the flag.
Introduce `SchemaApplyOptions` type and re-export it from `db` module; provide `plan_schema_with_options` / `apply_schema_with_options` while keeping `plan_schema` / `apply_schema` wrappers for compatibility New struct `SchemaApplyOptions` added and re-exported; Omnigraph gains `plan_schema_with_options` and `apply_schema_with_options` and the old methods delegate to the new ones with defaults, preserving API shape.
Planner promotes `DropMode::Soft` -> `DropMode::Hard` when `allow_data_loss` is set (plan-time promotion) A `promote_drops_to_hard` function is defined and invoked during planning (and before apply) to mutate the plan when options.allow_data_loss is true; tests assert the promotion.
Apply path records hard-drop cleanup targets for Hard-mode DropProperty and DropType and runs cleanup_old_versions after publish (best-effort, warn on failure) The apply path builds `hard_cleanup_targets` for Hard drops and runs `cleanup_dataset_old_versions` after manifest publish; errors are logged as warnings rather than failing the apply.
Hard-mode semantics enforced: DropProperty Hard causes prior dataset version to be reclaimed so time-travel pre-drop snapshot open fails Tests exercise a Hard DropProperty apply and assert that opening the pre-drop snapshot fails (the code calls Lance cleanup_old_versions with before_timestamp=now to remove prior versions).
Hard-mode DropType handling: tombstone manifest entry and run cleanup_old_versions on orphan dataset (data becomes unreachable via manifest), with note that directory deletion is a follow-up Apply path tombstones the manifest entry and, for Hard, schedules cleanup_old_versions on the dataset; PR documents that directory deletion is a separate follow-up. Tests assert manifest entries gone and that apply succeeds.
Tests added/updated to cover promotion, apply success with flag, and Hard-mode behavior; CI test targets indicated as passing New tests in crates/omnigraph/tests/schema_apply.rs cover plan promotion, apply with allow_data_loss, and the Hard-mode contracts. PR test-plan summary lists test runs and passing counts; code changes include the tests themselves.

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic

Comment on lines +214 to +221
let entry = snapshot.entry(&table_key).ok_or_else(|| {
OmniError::manifest(format!(
"missing table '{}' for hard property drop",
table_key
))
})?;
let full_uri = format!("{}/{}", db.root_uri, entry.table_path);
hard_cleanup_targets.push((table_key.clone(), full_uri));

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: Hard DropProperty resolves cleanup target from the renamed key, which can fail valid rename+drop migrations with --allow-data-loss.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph/src/db/omnigraph/schema_apply.rs, line 214:

<comment>Hard DropProperty resolves cleanup target from the renamed key, which can fail valid rename+drop migrations with `--allow-data-loss`.</comment>

<file context>
@@ -145,51 +191,71 @@ pub(super) async fn apply_schema_with_lock(
                     changed_edge_tables = true;
                 }
+                if matches!(mode, DropMode::Hard) {
+                    let entry = snapshot.entry(&table_key).ok_or_else(|| {
+                        OmniError::manifest(format!(
+                            "missing table '{}' for hard property drop",
</file context>
Suggested change
let entry = snapshot.entry(&table_key).ok_or_else(|| {
OmniError::manifest(format!(
"missing table '{}' for hard property drop",
table_key
))
})?;
let full_uri = format!("{}/{}", db.root_uri, entry.table_path);
hard_cleanup_targets.push((table_key.clone(), full_uri));
let snapshot_key = renamed_tables.get(&table_key).unwrap_or(&table_key);
let entry = snapshot.entry(snapshot_key).ok_or_else(|| {
OmniError::manifest(format!(
"missing table '{}' for hard property drop",
snapshot_key
))
})?;
let full_uri = db.table_store.dataset_uri(&entry.table_path);
hard_cleanup_targets.push((table_key.clone(), full_uri));

The remote-rejection branch in SchemaCommand::Apply used
anyhow::anyhow! which isn't in scope; the CLI's Result type is
color_eyre::eyre::Result and bail! is already imported.

Caught by CI Test Workspace job on PR #100.
@aaltshuler
aaltshuler merged commit a6e0375 into main May 16, 2026
6 of 7 checks passed
@aaltshuler
aaltshuler deleted the andrew/mr-694-chassis-v1-commit5 branch May 16, 2026 19:12
aaltshuler added a commit that referenced this pull request May 18, 2026
…low-up)

The schema-lint chassis v1.2 (PR #100) shipped `--allow-data-loss` on
the CLI, but `SchemaApplyRequest` had no equivalent field — Hard-mode
drops were CLI-only. This commit closes that feature gap and adds e2e
test coverage for drop modes across HTTP + CLI, plus data preservation
on additive apply, plus a CLI↔SDK plan-parity assertion.

Feature gap closed:

- `crates/omnigraph-server/src/api.rs` — added `allow_data_loss: bool`
  (default false via `#[serde(default)]`) to `SchemaApplyRequest`.
  Added `Default` derive so test usages can use `..Default::default()`.
- `crates/omnigraph-server/src/lib.rs` — `server_schema_apply` now
  constructs `SchemaApplyOptions { allow_data_loss: request.allow_data_loss }`
  and threads through to `apply_schema_as`.
- `crates/omnigraph-cli/src/main.rs` — remote-URI schema-apply path
  used to bail with "--allow-data-loss not yet supported on remote";
  now forwards the flag into the JSON payload so the CLI behaves
  identically against local and remote URIs.
- `openapi.json` — regenerated; only diff is the new field on
  `SchemaApplyRequest`.

Tests added (8 new):

* `crates/omnigraph-server/tests/server.rs` (+5):
  - `schema_apply_route_soft_drops_property_via_http` — POST schema
    removing nullable property, verify catalog reflects the drop AND
    `snapshot_at_version(pre)` still has `age` in the field list
    (time-travel reachability is the Soft contract).
  - `schema_apply_route_soft_drops_node_type_via_http` — POST schema
    removing `Company` node + cascading `WorksAt` edge.
  - `schema_apply_route_hard_drops_property_with_allow_data_loss` —
    POST with `allow_data_loss: true`, verify plan step reports
    `mode: hard`.
  - `schema_apply_route_keeps_drops_soft_without_flag` — same schema
    without flag, verify `mode: soft`. Pins default semantics against
    accidental Hard promotion.
  - `schema_apply_route_additive_property_preserves_existing_rows` —
    load fixture, POST adding nullable property, verify row count
    preserved (SDK suite covers data preservation on drops + renames;
    additive AddProperty wasn't pinned).
  Plus helpers `schema_without_age` and `schema_without_company`.

* `crates/omnigraph-cli/tests/cli.rs` (+3):
  - `schema_apply_allow_data_loss_flag_promotes_drops_to_hard` — CLI
    `omnigraph schema apply --allow-data-loss --schema X.pg --json`,
    verify plan step has `mode: hard`.
  - `schema_apply_without_allow_data_loss_keeps_soft_drops` — without
    flag, verify Soft.
  - `schema_plan_parity_cli_and_sdk` — same `.pg` source through
    `Omnigraph::plan_schema` (SDK) and `omnigraph schema plan --json`
    (CLI), assert the steps array is byte-identical post-JSON. HTTP
    has no `/schema/plan` endpoint; apply-side parity is implicitly
    covered by the HTTP drop tests + CLI drop tests using identical
    fixtures.

Docs:

- `docs/user/schema-language.md` — new "Destructive drops" section
  documenting Soft vs Hard semantics and that `allow_data_loss` is
  now honored uniformly across CLI / HTTP / SDK.

Verification: every new test passes; full `cargo test --workspace --locked`
green; `scripts/check-agents-md.sh` passes.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aaltshuler added a commit that referenced this pull request May 18, 2026
…low-up) (#107)

The schema-lint chassis v1.2 (PR #100) shipped `--allow-data-loss` on
the CLI, but `SchemaApplyRequest` had no equivalent field — Hard-mode
drops were CLI-only. This commit closes that feature gap and adds e2e
test coverage for drop modes across HTTP + CLI, plus data preservation
on additive apply, plus a CLI↔SDK plan-parity assertion.

Feature gap closed:

- `crates/omnigraph-server/src/api.rs` — added `allow_data_loss: bool`
  (default false via `#[serde(default)]`) to `SchemaApplyRequest`.
  Added `Default` derive so test usages can use `..Default::default()`.
- `crates/omnigraph-server/src/lib.rs` — `server_schema_apply` now
  constructs `SchemaApplyOptions { allow_data_loss: request.allow_data_loss }`
  and threads through to `apply_schema_as`.
- `crates/omnigraph-cli/src/main.rs` — remote-URI schema-apply path
  used to bail with "--allow-data-loss not yet supported on remote";
  now forwards the flag into the JSON payload so the CLI behaves
  identically against local and remote URIs.
- `openapi.json` — regenerated; only diff is the new field on
  `SchemaApplyRequest`.

Tests added (8 new):

* `crates/omnigraph-server/tests/server.rs` (+5):
  - `schema_apply_route_soft_drops_property_via_http` — POST schema
    removing nullable property, verify catalog reflects the drop AND
    `snapshot_at_version(pre)` still has `age` in the field list
    (time-travel reachability is the Soft contract).
  - `schema_apply_route_soft_drops_node_type_via_http` — POST schema
    removing `Company` node + cascading `WorksAt` edge.
  - `schema_apply_route_hard_drops_property_with_allow_data_loss` —
    POST with `allow_data_loss: true`, verify plan step reports
    `mode: hard`.
  - `schema_apply_route_keeps_drops_soft_without_flag` — same schema
    without flag, verify `mode: soft`. Pins default semantics against
    accidental Hard promotion.
  - `schema_apply_route_additive_property_preserves_existing_rows` —
    load fixture, POST adding nullable property, verify row count
    preserved (SDK suite covers data preservation on drops + renames;
    additive AddProperty wasn't pinned).
  Plus helpers `schema_without_age` and `schema_without_company`.

* `crates/omnigraph-cli/tests/cli.rs` (+3):
  - `schema_apply_allow_data_loss_flag_promotes_drops_to_hard` — CLI
    `omnigraph schema apply --allow-data-loss --schema X.pg --json`,
    verify plan step has `mode: hard`.
  - `schema_apply_without_allow_data_loss_keeps_soft_drops` — without
    flag, verify Soft.
  - `schema_plan_parity_cli_and_sdk` — same `.pg` source through
    `Omnigraph::plan_schema` (SDK) and `omnigraph schema plan --json`
    (CLI), assert the steps array is byte-identical post-JSON. HTTP
    has no `/schema/plan` endpoint; apply-side parity is implicitly
    covered by the HTTP drop tests + CLI drop tests using identical
    fixtures.

Docs:

- `docs/user/schema-language.md` — new "Destructive drops" section
  documenting Soft vs Hard semantics and that `allow_data_loss` is
  now honored uniformly across CLI / HTTP / SDK.

Verification: every new test passes; full `cargo test --workspace --locked`
green; `scripts/check-agents-md.sh` passes.

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
@aaltshuler aaltshuler mentioned this pull request May 23, 2026
6 tasks
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.

1 participant