diff --git a/crates/omnigraph-cli/src/cli.rs b/crates/omnigraph-cli/src/cli.rs index 56bfd3a..3e7f394 100644 --- a/crates/omnigraph-cli/src/cli.rs +++ b/crates/omnigraph-cli/src/cli.rs @@ -66,6 +66,18 @@ pub(crate) struct Cli { #[arg(long, global = true, value_name = "DIR|URI")] pub(crate) cluster: Option, + /// Skip the confirmation prompt for a destructive write (`cleanup`, + /// overwrite `load`, `branch delete`) against a non-local scope (RFC-011 + /// Decision 9). Without it, a non-local destructive write prompts on a TTY + /// and refuses (errors) when there is no TTY or `--json` is set. + #[arg(long, global = true)] + pub(crate) yes: bool, + + /// Suppress the one-line resolved-write-target diagnostic that write + /// commands echo to stderr (RFC-011 Decision 9). + #[arg(long, global = true)] + pub(crate) quiet: bool, + #[command(subcommand)] pub(crate) command: Command, } diff --git a/crates/omnigraph-cli/src/helpers.rs b/crates/omnigraph-cli/src/helpers.rs index d0632d7..58817da 100644 --- a/crates/omnigraph-cli/src/helpers.rs +++ b/crates/omnigraph-cli/src/helpers.rs @@ -2,6 +2,8 @@ //! remote HTTP, env/token handling, scaffolding (moved verbatim from //! main.rs in the modularization). +use std::io::IsTerminal; + use super::*; use crate::operator; @@ -16,6 +18,59 @@ pub(crate) fn is_remote_uri(uri: &str) -> bool { uri.starts_with("http://") || uri.starts_with("https://") } +/// Whether a resolved write target is *local* for the purposes of the RFC-011 +/// Decision 9 destructive-confirm gate: a bare path or a `file://` URI. Anything +/// else carrying a scheme — `http(s)://` (served), `s3://` / `gs://` / … (object +/// store) — is non-local and a destructive write against it requires explicit +/// consent. Generalizes `is_remote_uri` (which only catches http(s)). +pub(crate) fn uri_is_local(uri: &str) -> bool { + !uri.contains("://") || uri.starts_with("file://") +} + +/// Echo the resolved write target + access path to stderr (RFC-011 Decision 9), +/// unless `--quiet`. One line, e.g. `omnigraph load → file://g.omni (direct, +/// local)`. stderr so `--json` consumers reading stdout are unaffected; the line +/// legitimately differs embedded-vs-served (that visibility is the point). +pub(crate) fn echo_write_target(quiet: bool, label: &str, uri: &str, served: bool) { + if quiet { + return; + } + let access = if served { + "served" + } else if uri_is_local(uri) { + "direct, local" + } else { + "direct, remote" + }; + eprintln!("omnigraph {label} → {uri} ({access})"); +} + +/// Gate a destructive write (`cleanup`, overwrite `load`, `branch delete`) +/// against a non-local scope (RFC-011 Decision 9). A local target needs no +/// confirmation; otherwise `--yes` consents, an interactive TTY is prompted, and +/// a non-TTY / `--json` run refuses rather than silently proceeding. +pub(crate) fn confirm_destructive(label: &str, uri: &str, yes: bool, json: bool) -> Result<()> { + if uri_is_local(uri) || yes { + return Ok(()); + } + if json || !std::io::stdin().is_terminal() { + bail!( + "refusing destructive `{label}` against non-local target {uri} without confirmation; \ + pass --yes to confirm (an interactive TTY would be prompted instead)" + ); + } + eprint!( + "About to run a destructive `{label}` against {uri} (not local). Type 'yes' to continue: " + ); + io::stderr().flush()?; + let mut answer = String::new(); + io::stdin().read_line(&mut answer)?; + match answer.trim().to_ascii_lowercase().as_str() { + "yes" | "y" => Ok(()), + _ => bail!("aborted: destructive `{label}` not confirmed"), + } +} + /// THE one way the CLI composes a remote request URL. Every remote call /// routes through here so URL assembly has a single mechanism instead of /// per-callsite string interpolation. @@ -1112,6 +1167,40 @@ pub(crate) fn rewrite_deprecated_argv(args: Vec) -> Vec { mod tests { use super::*; + // RFC-011 Decision 9: locality classifier for the destructive-confirm gate. + #[test] + fn uri_is_local_truth_table() { + // Local: bare path or file://. + assert!(uri_is_local("graph.omni")); + assert!(uri_is_local("/abs/path/graph.omni")); + assert!(uri_is_local("file:///tmp/graph.omni")); + // Non-local: served or object-store schemes. + assert!(!uri_is_local("http://host/graphs/g")); + assert!(!uri_is_local("https://host/graphs/g")); + assert!(!uri_is_local("s3://bucket/graph.omni")); + assert!(!uri_is_local("gs://bucket/graph.omni")); + } + + // RFC-011 Decision 9: a non-local destructive write with `--json` (the CI + // shape — also covers the no-TTY case, since tests run without a terminal) + // refuses rather than proceeding; a local one and an explicit `--yes` pass. + #[test] + fn confirm_destructive_refuses_non_local_without_consent() { + let err = confirm_destructive("cleanup", "s3://b/g.omni", false, true) + .unwrap_err() + .to_string(); + assert!(err.contains("--yes"), "{err}"); + } + + #[test] + fn confirm_destructive_allows_local_and_explicit_yes() { + // Local needs no confirmation, even with --json. + assert!(confirm_destructive("cleanup", "file:///tmp/g.omni", false, true).is_ok()); + assert!(confirm_destructive("branch delete", "graph.omni", false, true).is_ok()); + // --yes consents to a non-local target. + assert!(confirm_destructive("cleanup", "s3://b/g.omni", true, true).is_ok()); + } + // RFC-011 Decision 2: `--server` accepts a literal URL (value with `://`), // bypassing the operator-config registry — so no config / OMNIGRAPH_HOME is // read on this path (hermetic). diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 9734573..7d50c0a 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -188,6 +188,10 @@ async fn main() -> Result<()> { cli.store.as_deref(), )?; let branch = resolve_branch(&config, branch, None, "main"); + if matches!(mode, CliLoadMode::Overwrite) { + confirm_destructive("load --mode overwrite", client.uri(), cli.yes, json)?; + } + echo_write_target(cli.quiet, "load", client.uri(), client.is_remote()); let payload = client .load(&branch, from.as_deref(), &data.to_string_lossy(), mode) .await?; @@ -223,6 +227,7 @@ async fn main() -> Result<()> { )?; let branch = resolve_branch(&config, branch, None, "main"); let from = resolve_branch(&config, from, None, "main"); + echo_write_target(cli.quiet, "ingest", client.uri(), client.is_remote()); let payload = client .ingest(&branch, &from, &data.to_string_lossy(), mode) .await?; @@ -251,6 +256,7 @@ async fn main() -> Result<()> { cli.store.as_deref(), )?; let from = resolve_branch(&config, from, None, "main"); + echo_write_target(cli.quiet, "branch create", client.uri(), client.is_remote()); let payload = client.branch_create_from(&from, &name).await?; if json { print_json(&payload)?; @@ -297,6 +303,8 @@ async fn main() -> Result<()> { cli.profile.as_deref(), cli.store.as_deref(), )?; + confirm_destructive("branch delete", client.uri(), cli.yes, json)?; + echo_write_target(cli.quiet, "branch delete", client.uri(), client.is_remote()); let payload = client.branch_delete(&name).await?; if json { print_json(&payload)?; @@ -322,6 +330,7 @@ async fn main() -> Result<()> { cli.store.as_deref(), )?; let into = resolve_branch(&config, into, None, "main"); + echo_write_target(cli.quiet, "branch merge", client.uri(), client.is_remote()); let payload = client.branch_merge(&source, &into).await?; if json { print_json(&payload)?; @@ -440,6 +449,7 @@ async fn main() -> Result<()> { (!registry.is_empty()).then_some(registry) }; let label = client.selected().unwrap_or(client.uri()).to_string(); + echo_write_target(cli.quiet, "schema apply", client.uri(), client.is_remote()); let output = client .apply_schema(&schema_source, allow_data_loss, |catalog| { if let Some(registry) = registry.as_ref() { @@ -802,6 +812,7 @@ async fn main() -> Result<()> { "optimize", ) .await?; + echo_write_target(cli.quiet, "optimize", &uri, false); let db = Omnigraph::open(&uri).await?; let stats = db.optimize().await?; if json { @@ -852,6 +863,7 @@ async fn main() -> Result<()> { "repair", ) .await?; + echo_write_target(cli.quiet, "repair", &uri, false); let db = Omnigraph::open(&uri).await?; let stats = db .repair(omnigraph::db::RepairOptions { confirm, force }) @@ -967,6 +979,11 @@ async fn main() -> Result<()> { ); return Ok(()); } + // Past the preview gate: a real destructive run. Against a non-local + // scope this additionally requires --yes (or an interactive yes), so + // `cleanup --confirm s3://…` in CI refuses rather than destroying. + confirm_destructive("cleanup", &uri, cli.yes, json)?; + echo_write_target(cli.quiet, "cleanup", &uri, false); let options = omnigraph::db::CleanupPolicyOptions { keep_versions: keep, diff --git a/crates/omnigraph-cli/tests/cli_data.rs b/crates/omnigraph-cli/tests/cli_data.rs index 8896912..7240ad2 100644 --- a/crates/omnigraph-cli/tests/cli_data.rs +++ b/crates/omnigraph-cli/tests/cli_data.rs @@ -1565,6 +1565,160 @@ fn branch_delete_rejects_main() { assert!(stderr.contains("cannot delete branch 'main'")); } +// ── RFC-011 Decision 9: write diagnostics + non-local destructive-confirm ── + +#[test] +fn write_echoes_resolved_target_to_stderr() { + // Every write echoes its resolved target + access path to stderr; --json + // (stdout) is unaffected. A local load → "(direct, local)". + let temp = tempdir().unwrap(); + let graph = graph_path(temp.path()); + init_graph(&graph); + let data = fixture("test.jsonl"); + let output = output_success( + cli() + .arg("load") + .arg("--mode") + .arg("append") + .arg("--data") + .arg(&data) + .arg(&graph) + .arg("--json"), + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + stderr.contains("omnigraph load →") && stderr.contains("(direct, local)"), + "missing write-target echo; stderr: {stderr}" + ); + // stdout still parses as JSON — the echo went to stderr. + let _: Value = serde_json::from_slice(&output.stdout).unwrap(); +} + +#[test] +fn quiet_suppresses_the_write_target_echo() { + let temp = tempdir().unwrap(); + let graph = graph_path(temp.path()); + init_graph(&graph); + let data = fixture("test.jsonl"); + let output = output_success( + cli() + .arg("--quiet") + .arg("load") + .arg("--mode") + .arg("append") + .arg("--data") + .arg(&data) + .arg(&graph), + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + !stderr.contains("omnigraph load →"), + "--quiet should suppress the echo; stderr: {stderr}" + ); +} + +#[test] +fn branch_delete_against_non_local_scope_refuses_without_yes() { + // No bucket needed: the confirm gate fires before the graph is opened. + let output = output_failure( + cli() + .arg("branch") + .arg("delete") + .arg("--store") + .arg("s3://fake-bucket/g.omni") + .arg("feature") + .arg("--json"), + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + stderr.contains("refusing destructive `branch delete`") && stderr.contains("--yes"), + "expected a non-local destructive refusal; stderr: {stderr}" + ); +} + +#[test] +fn branch_delete_against_non_local_scope_passes_gate_with_yes() { + // With --yes the gate is bypassed; the command then fails for an unrelated + // reason (the fake bucket can't be opened), so the refusal must be ABSENT. + let output = output_failure( + cli() + .arg("branch") + .arg("delete") + .arg("--store") + .arg("s3://fake-bucket/g.omni") + .arg("feature") + .arg("--yes") + .arg("--json"), + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + !stderr.contains("refusing destructive"), + "--yes should bypass the confirm gate; stderr: {stderr}" + ); +} + +#[test] +fn overwrite_load_against_non_local_scope_refuses_without_yes() { + let output = output_failure( + cli() + .arg("load") + .arg("--mode") + .arg("overwrite") + .arg("--data") + .arg(fixture("test.jsonl")) + .arg("--store") + .arg("s3://fake-bucket/g.omni") + .arg("--json"), + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + stderr.contains("refusing destructive `load --mode overwrite`"), + "expected a non-local overwrite refusal; stderr: {stderr}" + ); +} + +#[test] +fn cleanup_against_non_local_scope_refuses_without_yes() { + // Past the --confirm preview gate, a non-local cleanup still needs --yes. + let output = output_failure( + cli() + .arg("cleanup") + .arg("--store") + .arg("s3://fake-bucket/g.omni") + .arg("--keep") + .arg("5") + .arg("--confirm") + .arg("--json"), + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + stderr.contains("refusing destructive `cleanup`"), + "expected a non-local cleanup refusal; stderr: {stderr}" + ); +} + +#[test] +fn cleanup_against_local_scope_executes_with_confirm() { + // Local cleanup needs no --yes; --confirm alone executes (and echoes). + let temp = tempdir().unwrap(); + let graph = graph_path(temp.path()); + init_graph(&graph); + load_fixture(&graph); + let output = output_success( + cli() + .arg("cleanup") + .arg("--keep") + .arg("1") + .arg("--confirm") + .arg(&graph) + .arg("--json"), + ); + let payload: Value = serde_json::from_slice(&output.stdout).unwrap(); + assert!(payload["tables"].as_array().is_some(), "{payload}"); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!(stderr.contains("omnigraph cleanup →"), "stderr: {stderr}"); +} + #[test] fn branch_merge_defaults_target_to_main() { let temp = tempdir().unwrap(); diff --git a/crates/omnigraph-cli/tests/parity_matrix.rs b/crates/omnigraph-cli/tests/parity_matrix.rs index 65a584f..984cc71 100644 --- a/crates/omnigraph-cli/tests/parity_matrix.rs +++ b/crates/omnigraph-cli/tests/parity_matrix.rs @@ -142,7 +142,10 @@ fn parity_branch_create_delete() { let (l, r) = p.run(&["branch", "create", "--from", "main", "parity-branch", "--json"], ); assert_parity("branch create", &l, &r); - let (l, r) = p.run(&["branch", "delete", "parity-branch", "--json"], + // `branch delete` is destructive: the served (remote) arm is non-local and + // requires consent (RFC-011 Decision 9), so the row passes `--yes` to test + // the operation itself, not the safety gate. The local arm ignores `--yes`. + let (l, r) = p.run(&["branch", "delete", "parity-branch", "--yes", "--json"], ); assert_parity("branch delete", &l, &r); } diff --git a/crates/omnigraph-cli/tests/system_remote.rs b/crates/omnigraph-cli/tests/system_remote.rs index 95a53e7..cb04735 100644 --- a/crates/omnigraph-cli/tests/system_remote.rs +++ b/crates/omnigraph-cli/tests/system_remote.rs @@ -550,6 +550,8 @@ fn remote_branch_delete_removes_branch() { .arg("--config") .arg(&config) .arg("feature") + // Served target is non-local → destructive-confirm gate (RFC-011 D9). + .arg("--yes") .arg("--json"), )); assert_eq!(deleted["name"], "feature"); diff --git a/docs/user/cli/reference.md b/docs/user/cli/reference.md index 85b3435..1e87e46 100644 --- a/docs/user/cli/reference.md +++ b/docs/user/cli/reference.md @@ -23,7 +23,7 @@ Top-level command families and subcommands. Graph-targeting commands accept a po | `cluster validate \| plan \| apply \| approve \| status \| refresh \| import \| force-unlock` | declarative cluster control plane. `validate` checks a local `cluster.yaml` folder and referenced schema/query/policy files; `plan` diffs it against local JSON state at `__cluster/state.json`, annotates dispositions, and embeds real schema-migration previews; `apply` converges the cluster — stored-query/policy catalog writes (content-addressed under `__cluster/resources/`), graph creates, schema updates (soft drops only; `--as` records the actor), and graph deletes behind a digest-bound approval from `cluster approve --as ` (`apply`/`approve` default the actor from the per-operator `omnigraph.yaml`'s `cli.actor` when `--as` is omitted; nothing else in that file affects cluster commands); what apply converges is what an `omnigraph-server --cluster ` deployment serves on its next restart (omnigraph.yaml deployments are unaffected); `status` reads the state ledger; `refresh`/`import` explicitly update local JSON state from read-only graph observations; `force-unlock ` manually removes a held local state lock by exact id | | `optimize` | non-destructive Lance compaction (skips tables with `Blob` columns or uncovered drift; `--json` reports `skipped`) | | `repair [--confirm] [--force]` | preview or explicitly publish uncovered manifest/head drift. `--confirm` heals verified maintenance drift and exits non-zero if suspicious/unverifiable drift is refused; `--force --confirm` publishes suspicious/unverifiable drift after operator review | -| `cleanup --keep N --older-than 7d --confirm` | destructive version GC | +| `cleanup --keep N --older-than 7d --confirm` | destructive version GC (`--confirm` to execute; also needs `--yes` against a non-local `s3://` target — see *Write diagnostics & destructive confirmation*) | | `embed` | offline JSONL embedding pipeline | | `policy validate \| test \| explain` | Cedar tooling. Selects `cli.graph`, else `server.graph`, else top-level `policy.file` | | `version` / `-v` | print `omnigraph 0.3.x` | @@ -49,6 +49,15 @@ To maintain a server-backed graph, run the `direct` verbs from a host with stora `omnigraph --help` lists commands with a **capability legend** at the bottom (any / served / direct / control / local). +## Write diagnostics & destructive confirmation + +Two global flags make writes self-documenting and guard the dangerous ones (RFC-011 Decision 9): + +- **Every write echoes its resolved target to stderr** — `omnigraph load → s3://acme/brain/graphs/knowledge.omni (direct, remote)` — so you catch a scope that resolved somewhere unexpected (e.g. *prod*) before it lands. Applies to `load`, `ingest`, `mutate`, `branch create|delete|merge`, `schema apply`, `optimize`, `repair`, `cleanup`. The line is stderr, so `--json` consumers reading stdout are unaffected; suppress it with **`--quiet`**. +- **Destructive writes against a non-local scope require confirmation.** `cleanup`, overwrite `load` (`--mode overwrite`), and `branch delete` proceed freely against a local (`file://`) graph, but when the resolved target is **not local** (a served `http(s)://` graph or an `s3://` store/cluster) they require explicit consent: pass **`--yes`** to confirm, an interactive terminal is prompted, and a non-interactive run (no TTY, or `--json`) **refuses with an error** rather than silently destroying. `cleanup` still also requires its existing `--confirm` (preview→execute); `--yes` is the additional non-local consent. + +A "local" target is a bare path or a `file://` URI; `http(s)://`, `s3://`, and other object-store schemes are non-local. + ## Config surfaces Two config surfaces with single owners, plus a zero-config tier: diff --git a/docs/user/clusters/index.md b/docs/user/clusters/index.md index bbaf033..9485833 100644 --- a/docs/user/clusters/index.md +++ b/docs/user/clusters/index.md @@ -271,6 +271,12 @@ not resolvable. Run these from a host with storage access — there are no serve routes for them. Conversely, **`init` refuses** a cluster-managed path: graphs in a cluster are created by `cluster apply`, not by hand. +Against an **`s3://`-backed cluster** the resolved graph storage is non-local, so a +destructive `cleanup` additionally requires **`--yes`** (an interactive prompt +otherwise, refusal without a TTY) on top of `--confirm` — see [cli-reference.md](../cli/reference.md)'s +*Write diagnostics & destructive confirmation*. Every maintenance run also echoes +its resolved target to stderr (suppress with `--quiet`). + ## What the control plane does not do (yet) - **No hot reload** — applied changes serve on the next restart. diff --git a/docs/user/operations/maintenance.md b/docs/user/operations/maintenance.md index 668d41c..87688d6 100644 --- a/docs/user/operations/maintenance.md +++ b/docs/user/operations/maintenance.md @@ -34,6 +34,7 @@ backstop, so it does as much as it can and converges on re-run. The CLI reports any failed tables; rerun `cleanup` to retry them. - CLI guards with `--confirm`; without it, prints a preview line. +- **Non-local consent (RFC-011 D9).** Against a non-local target (an `s3://` store/cluster), `cleanup` additionally requires `--yes` on top of `--confirm`: a TTY is prompted, and a non-interactive run (no TTY, or `--json`) refuses rather than destroying. A local (`file://`) target needs only `--confirm`. The same `--yes` gate applies to overwrite `load` and `branch delete`; every maintenance run echoes its resolved target to stderr (suppress with `--quiet`). - **Recovery floor:** `--keep < 3` may garbage-collect versions that crash recovery needs as a rollback target. Default `--keep 10` is safe. - **Orphaned-branch reconciliation:** before the version GC, cleanup reclaims any per-table or commit-graph branch absent from the manifest branch list. These orphans arise when a `branch_delete` flips the manifest authority but a downstream best-effort reclaim does not complete (see [branches-commits.md](../branching/index.md)). The reconciler is idempotent (it no-ops once nothing is orphaned), runs regardless of the `keep_versions` / `older_than` values (those gate version GC only), and never reclaims `main` or system-branch forks. Reclaimed forks are logged.