diff --git a/crates/omnigraph-cli/src/helpers.rs b/crates/omnigraph-cli/src/helpers.rs index 84c8867..9991486 100644 --- a/crates/omnigraph-cli/src/helpers.rs +++ b/crates/omnigraph-cli/src/helpers.rs @@ -464,8 +464,11 @@ pub(crate) fn resolve_local_graph( let graph = resolve_cli_graph(config, cli_uri, cli_target)?; if graph.is_remote { bail!( - "{} is only supported against local graph URIs in this milestone", - operation + "`{}` is a storage-plane command and needs direct storage access; \ + the resolved target is a remote server ({}). Pass the graph's \ + file:// or s3:// URI.", + operation, + graph.uri ); } Ok(graph) diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index fd67fb3..6c94132 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -49,6 +49,7 @@ mod cli; mod client; mod helpers; mod output; +mod planes; use cli::*; use helpers::*; use output::*; @@ -70,6 +71,10 @@ async fn main() -> Result<()> { Cli::from_arg_matches(&matches)? }; let http_client = build_http_client()?; + // RFC-010 Slice 1: reject data-plane addressing flags (--server/--graph) on + // a verb that doesn't live on the data plane, from one declared table — + // before any per-command dispatch. + planes::guard_addressing(&cli)?; match cli.command { Command::Config { command } => match command { ConfigCommand::Migrate { config, write, json } => { @@ -781,7 +786,7 @@ async fn main() -> Result<()> { json, } => { let config = load_cli_config(config.as_ref())?; - let uri = resolve_uri(&config, uri, target.as_deref())?; + let uri = resolve_local_uri(&config, uri, target.as_deref(), "optimize")?; let db = Omnigraph::open(&uri).await?; let stats = db.optimize().await?; if json { @@ -823,7 +828,7 @@ async fn main() -> Result<()> { json, } => { let config = load_cli_config(config.as_ref())?; - let uri = resolve_uri(&config, uri, target.as_deref())?; + let uri = resolve_local_uri(&config, uri, target.as_deref(), "repair")?; let db = Omnigraph::open(&uri).await?; let stats = db .repair(omnigraph::db::RepairOptions { confirm, force }) @@ -907,7 +912,7 @@ async fn main() -> Result<()> { json, } => { let config = load_cli_config(config.as_ref())?; - let uri = resolve_uri(&config, uri, target.as_deref())?; + let uri = resolve_local_uri(&config, uri, target.as_deref(), "cleanup")?; let older_than_dur = older_than.as_deref().map(parse_duration_arg).transpose()?; diff --git a/crates/omnigraph-cli/src/planes.rs b/crates/omnigraph-cli/src/planes.rs new file mode 100644 index 0000000..81328d3 --- /dev/null +++ b/crates/omnigraph-cli/src/planes.rs @@ -0,0 +1,151 @@ +//! Declared CLI "planes" (RFC-010 Slice 1). +//! +//! Every subcommand belongs to exactly one plane. This classification is the +//! single source of truth the wrong-plane guard consumes — and that later +//! RFC-010 slices (the capability surface, plane-grouped help) will consume +//! too. The `command_plane` match is **exhaustive on purpose**: adding a +//! `Command` variant is a compile error until its plane is declared, so the +//! surface cannot silently drift from the command set. +//! +//! See [docs/dev/rfc-010-cli-planes-restructure.md]. + +use color_eyre::Result; +use color_eyre::eyre::bail; + +use crate::cli::{Cli, Command, QueriesCommand, SchemaCommand}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Plane { + /// Runs against a graph, embedded **or** via `--server` (the `GraphClient` + /// axis). The only plane on which the data-plane addressing flags + /// (`--server`/`--graph`) apply. + Data, + /// Direct storage access; no server. Maintenance + local-only inspection + /// that must work with the server down. + Storage, + /// Operates on a cluster directory, not a graph URI. + Control, + /// Touches no graph at all — session / config / local tooling. + Session, +} + +impl std::fmt::Display for Plane { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Plane::Data => "data", + Plane::Storage => "storage", + Plane::Control => "control", + Plane::Session => "session", + }) + } +} + +/// The plane a subcommand belongs to. Exhaustive — a new `Command` variant +/// will not compile until classified. Descends into the nested enums where +/// the plane differs per subcommand (`schema plan` is storage while `schema +/// show`/`apply` are data; `queries validate` opens the graph while `queries +/// list` only reads config). +pub(crate) fn command_plane(cmd: &Command) -> Plane { + match cmd { + Command::Query { .. } + | Command::Mutate { .. } + | Command::Load { .. } + | Command::Ingest { .. } + | Command::Branch { .. } + | Command::Snapshot { .. } + | Command::Export { .. } + | Command::Commit { .. } + | Command::Graphs { .. } => Plane::Data, + Command::Schema { + command: SchemaCommand::Show { .. } | SchemaCommand::Apply { .. }, + } => Plane::Data, + Command::Schema { + command: SchemaCommand::Plan { .. }, + } => Plane::Storage, + Command::Queries { + command: QueriesCommand::Validate { .. }, + } => Plane::Storage, + Command::Queries { + command: QueriesCommand::List { .. }, + } => Plane::Session, + Command::Init { .. } + | Command::Optimize { .. } + | Command::Repair { .. } + | Command::Cleanup { .. } + | Command::Lint { .. } => Plane::Storage, + Command::Cluster { .. } => Plane::Control, + Command::Policy { .. } + | Command::Embed(_) + | Command::Login { .. } + | Command::Logout { .. } + | Command::Config { .. } + | Command::Version => Plane::Session, + } +} + +/// User-facing label for a subcommand (descends one level for the nested +/// families so messages read `schema plan`, `queries validate`, etc.). +pub(crate) fn command_label(cmd: &Command) -> &'static str { + match cmd { + Command::Version => "version", + Command::Login { .. } => "login", + Command::Logout { .. } => "logout", + Command::Config { .. } => "config", + Command::Embed(_) => "embed", + Command::Init { .. } => "init", + Command::Load { .. } => "load", + Command::Ingest { .. } => "ingest", + Command::Branch { .. } => "branch", + Command::Schema { command } => match command { + SchemaCommand::Plan { .. } => "schema plan", + SchemaCommand::Apply { .. } => "schema apply", + SchemaCommand::Show { .. } => "schema show", + }, + Command::Lint { .. } => "lint", + Command::Queries { command } => match command { + QueriesCommand::Validate { .. } => "queries validate", + QueriesCommand::List { .. } => "queries list", + }, + Command::Snapshot { .. } => "snapshot", + Command::Export { .. } => "export", + Command::Commit { .. } => "commit", + Command::Query { .. } => "query", + Command::Mutate { .. } => "mutate", + Command::Policy { .. } => "policy", + Command::Optimize { .. } => "optimize", + Command::Repair { .. } => "repair", + Command::Cleanup { .. } => "cleanup", + Command::Cluster { .. } => "cluster", + Command::Graphs { .. } => "graphs", + } +} + +/// Reject the data-plane addressing flags (`--server`/`--graph`) on any verb +/// that does not live on the data plane. This replaces the old silent-ignore +/// — e.g. `optimize --server prod` previously dropped `--server` and tried to +/// resolve a default target, failing (if at all) with an unrelated message. +/// Now it fails with one honest, declared error. RFC-010 Slice 1. +pub(crate) fn guard_addressing(cli: &Cli) -> Result<()> { + if cli.server.is_none() && cli.graph.is_none() { + return Ok(()); + } + let plane = command_plane(&cli.command); + if plane == Plane::Data { + return Ok(()); + } + let label = command_label(&cli.command); + let how = match plane { + // `init` is the one storage verb with no `--target` today (it takes a + // required positional URI), so its remediation drops the `--target` half. + Plane::Storage => match cli.command { + Command::Init { .. } => "Pass a storage URI.", + _ => "Use --target or a storage URI.", + }, + Plane::Control => "It operates on a cluster directory (pass --config ).", + Plane::Session => "It does not address a graph.", + Plane::Data => unreachable!("data plane returned early"), + }; + bail!( + "`{label}` is a {plane}-plane command; --server/--graph address the data plane and do not apply. {how}" + ); +} diff --git a/crates/omnigraph-cli/tests/cli_data.rs b/crates/omnigraph-cli/tests/cli_data.rs index 203a7c2..01dbeb3 100644 --- a/crates/omnigraph-cli/tests/cli_data.rs +++ b/crates/omnigraph-cli/tests/cli_data.rs @@ -142,6 +142,47 @@ fn embed_seed_preserves_non_entity_rows() { assert_eq!(embedded[2]["to"], "dec-alpha"); } +#[test] +fn optimize_json_succeeds_on_local_graph() { + // Happy path for the resolve_local_uri swap (RFC-010 Slice 1): a positional + // local path still resolves and runs embedded. + let temp = tempdir().unwrap(); + let graph = graph_path(temp.path()); + init_graph(&graph); + load_fixture(&graph); + + let output = output_success(cli().arg("optimize").arg("--json").arg(&graph)); + let payload: Value = serde_json::from_slice(&output.stdout).unwrap(); + assert!(payload["tables"].as_array().is_some()); +} + +#[test] +fn optimize_with_server_flag_errors_wrong_plane() { + // RFC-010 Slice 1: --server is a data-plane addressing flag; on a + // storage-plane verb the guard rejects it loudly (was: silently ignored). + let output = output_failure(cli().arg("optimize").arg("--server").arg("prod")); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("`optimize` is a storage-plane command") + && stderr.contains("--server/--graph address the data plane and do not apply") + && stderr.contains("Use --target or a storage URI."), + "wrong-plane guard message not found; got: {stderr}" + ); +} + +#[test] +fn optimize_with_remote_target_errors_storage_plane() { + // RFC-010 Slice 1: a maintenance verb pointed at a remote URI fails loudly + // and declaratively (was: whatever Omnigraph::open said about an https URI). + let output = output_failure(cli().arg("optimize").arg("https://graph.example.invalid")); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("`optimize` is a storage-plane command and needs direct storage access") + && stderr.contains("remote server"), + "storage-plane remote-target message not found; got: {stderr}" + ); +} + #[test] fn repair_json_reports_noop_on_clean_graph() { let temp = tempdir().unwrap(); @@ -542,8 +583,12 @@ query list_people() { .arg("http://127.0.0.1:8080"), ); let stderr = String::from_utf8_lossy(&output.stderr); + // RFC-010 Slice 1: the storage-plane verbs now share one declared message + // (was: "query lint is only supported against local graph URIs …"). assert!( - stderr.contains("query lint is only supported against local graph URIs in this milestone") + stderr.contains("`query lint` is a storage-plane command and needs direct storage access") + && stderr.contains("remote server"), + "storage-plane remote-target message not found; got: {stderr}" ); } diff --git a/crates/omnigraph-cli/tests/cli_schema_config.rs b/crates/omnigraph-cli/tests/cli_schema_config.rs index 710c856..c962321 100644 --- a/crates/omnigraph-cli/tests/cli_schema_config.rs +++ b/crates/omnigraph-cli/tests/cli_schema_config.rs @@ -72,6 +72,28 @@ fn schema_plan_json_reports_supported_additive_change() { assert_eq!(payload["steps"][0]["property_name"], "nickname"); } +#[test] +fn schema_plan_with_server_flag_errors_wrong_plane() { + // RFC-010 Slice 1: `schema plan` is storage-plane while `schema show/apply` + // are data-plane — the guard rejects --server on plan with the per-subcommand + // label (proving command_plane/command_label descend into the nested enum). + let output = output_failure( + cli() + .arg("schema") + .arg("plan") + .arg("--schema") + .arg(fixture("test.pg")) + .arg("--server") + .arg("prod"), + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("`schema plan` is a storage-plane command") + && stderr.contains("Use --target or a storage URI."), + "schema plan wrong-plane message not found; got: {stderr}" + ); +} + #[test] fn schema_plan_json_reports_unsupported_type_change() { let temp = tempdir().unwrap();