feat(cli)!: unify graph selection under --graph; --cluster is a global scope; remove --cluster-graph (#241)

RFC-011: --graph is the single graph selector across server and cluster scopes; --cluster becomes a global scope primitive; --cluster-graph removed. Maintenance dispatch unified through resolve_scope. Wrong-address guard validates each scope flag against the verb it can consume.
This commit is contained in:
Andrew Altshuler 2026-06-15 14:30:58 +03:00 committed by GitHub
parent c3d7639377
commit a09045028f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 398 additions and 205 deletions

View file

@ -37,9 +37,11 @@ pub(crate) struct Cli {
#[arg(long, global = true, value_name = "NAME|URL")]
pub(crate) server: Option<String>,
/// Graph id on a multi-graph `--server` (appends `/graphs/<id>` to
/// the server url). Requires --server.
#[arg(long, global = true, value_name = "GRAPH_ID", requires = "server")]
/// Select a graph within a multi-graph scope: on a `--server` it appends
/// `/graphs/<id>` to the server url; on a `--cluster` it picks which
/// cluster graph to maintain. Rejected on a single-graph address (a
/// positional URI / `--store`).
#[arg(long, global = true, value_name = "GRAPH_ID")]
pub(crate) graph: Option<String>,
/// Select a named scope bundle (RFC-011) from `profiles:` in
@ -56,6 +58,14 @@ pub(crate) struct Cli {
#[arg(long, global = true, value_name = "URI")]
pub(crate) store: Option<String>,
/// Address a cluster-managed graph's storage for maintenance (RFC-011):
/// a cluster directory or storage-root URI — named via `clusters:` in
/// ~/.omnigraph/config.yaml, or a literal `file://`/`s3://` root. Pair
/// with `--graph <id>` to select the graph. Used by optimize / repair /
/// cleanup; exclusive with a positional URI / `--store` / `--server`.
#[arg(long, global = true, value_name = "DIR|URI")]
pub(crate) cluster: Option<String>,
#[command(subcommand)]
pub(crate) command: Command,
}
@ -239,13 +249,6 @@ pub(crate) enum Command {
uri: Option<String>,
#[arg(long)]
config: Option<PathBuf>,
/// Cluster directory or storage-root URI; with --cluster-graph, resolves
/// the graph's storage URI from the served cluster state.
#[arg(long, conflicts_with = "uri", requires = "cluster_graph")]
cluster: Option<String>,
/// Graph id within --cluster.
#[arg(long, requires = "cluster")]
cluster_graph: Option<String>,
#[arg(long)]
json: bool,
},
@ -255,13 +258,6 @@ pub(crate) enum Command {
uri: Option<String>,
#[arg(long)]
config: Option<PathBuf>,
/// Cluster directory or storage-root URI; with --cluster-graph, resolves
/// the graph's storage URI from the served cluster state.
#[arg(long, conflicts_with = "uri", requires = "cluster_graph")]
cluster: Option<String>,
/// Graph id within --cluster.
#[arg(long, requires = "cluster")]
cluster_graph: Option<String>,
/// Publish verified maintenance drift. Without this flag, repair only
/// previews what it would do.
#[arg(long)]
@ -279,13 +275,6 @@ pub(crate) enum Command {
uri: Option<String>,
#[arg(long)]
config: Option<PathBuf>,
/// Cluster directory or storage-root URI; with --cluster-graph, resolves
/// the graph's storage URI from the served cluster state.
#[arg(long, conflicts_with = "uri", requires = "cluster_graph")]
cluster: Option<String>,
/// Graph id within --cluster.
#[arg(long, requires = "cluster")]
cluster_graph: Option<String>,
/// Number of recent versions to keep per table. Either `--keep` or
/// `--older-than` (or both) must be set.
#[arg(long)]

View file

@ -100,7 +100,7 @@ impl GraphClient {
let scope = crate::scope::resolve_scope(
&crate::operator::load_operator_config()?,
crate::planes::Capability::Any,
crate::scope::ScopeFlags { profile, store, server, graph, uri },
crate::scope::ScopeFlags { profile, store, server, cluster: None, graph, uri },
)?;
let (server, graph, uri) = (
scope.server.as_deref(),
@ -147,7 +147,7 @@ impl GraphClient {
let scope = crate::scope::resolve_scope(
&crate::operator::load_operator_config()?,
crate::planes::Capability::Any,
crate::scope::ScopeFlags { profile, store, server, graph, uri },
crate::scope::ScopeFlags { profile, store, server, cluster: None, graph, uri },
)?;
let (server, graph, uri) = (
scope.server.as_deref(),

View file

@ -539,12 +539,49 @@ pub(crate) fn resolve_local_uri(
Ok(resolve_local_graph(config, cli_uri, operation)?.uri)
}
/// Resolve a storage-plane verb's address to a direct storage URI (RFC-010
/// Slice 3). `--cluster <dir|uri> --cluster-graph <id>` resolves the graph's
/// storage URI from the **served cluster state** (the truth a `--cluster`
/// server serves); otherwise the ordinary positional-URI path.
/// clap enforces both-or-neither and exclusion with `uri`, so the mismatched
/// arm is defensive.
/// Resolve a maintenance verb's (optimize/repair/cleanup) address to a direct
/// storage URI through the one RFC-011 scope path. Every primitive funnels
/// here: a positional URI, `--store`, `--cluster <root> --graph <id>`, a
/// `--profile` cluster binding, or operator defaults — all resolved at the
/// `Direct` capability (so a server scope is rejected, a cluster scope is
/// allowed), then mapped to a storage URI by `resolve_storage_uri`.
pub(crate) async fn resolve_maintenance_uri(
config: &OmnigraphConfig,
profile: Option<&str>,
store: Option<&str>,
cluster: Option<&str>,
graph: Option<&str>,
cli_uri: Option<String>,
operation: &str,
) -> Result<String> {
let scope = scope::resolve_scope(
&operator::load_operator_config()?,
planes::Capability::Direct,
scope::ScopeFlags {
profile,
store,
server: None,
cluster,
graph,
uri: cli_uri,
},
)?;
resolve_storage_uri(
config,
scope.uri,
scope.cluster.as_deref(),
scope.cluster_graph.as_deref(),
operation,
)
.await
}
/// Map a resolved direct address to a storage URI: a cluster scope
/// (`--cluster <root> --graph <id>`, or a `--profile` cluster binding)
/// resolves the graph's storage URI from the **served cluster state** (the
/// truth a `--cluster` server serves); otherwise the ordinary positional-URI
/// path. The scope resolver guarantees a cluster scope always carries a graph,
/// so the mismatched arm is defensive.
pub(crate) async fn resolve_storage_uri(
config: &OmnigraphConfig,
cli_uri: Option<String>,
@ -555,7 +592,7 @@ pub(crate) async fn resolve_storage_uri(
match (cluster, cluster_graph) {
(Some(cluster), Some(graph_id)) => resolve_cluster_graph_uri(cluster, graph_id).await,
(None, None) => resolve_local_uri(config, cli_uri, operation),
_ => bail!("--cluster and --cluster-graph must be given together"),
_ => bail!("internal error: a cluster scope was resolved without a graph id"),
}
}

View file

@ -790,46 +790,18 @@ async fn main() -> Result<()> {
print_policy_explain(&decision, &actor, &request);
}
},
Command::Optimize {
uri,
config,
cluster,
cluster_graph,
json,
} => {
Command::Optimize { uri, config, json } => {
let config = load_cli_config(config.as_ref())?;
let uri = if uri.is_some() || cluster.is_some() {
resolve_storage_uri(
&config,
uri,
cluster.as_deref(),
cluster_graph.as_deref(),
"optimize",
)
.await?
} else {
// RFC-011: no explicit per-command address — consult the scope
// (a --profile cluster binding, --store, or operator defaults).
let scope = scope::resolve_scope(
&operator::load_operator_config()?,
planes::Capability::Direct,
scope::ScopeFlags {
profile: cli.profile.as_deref(),
store: cli.store.as_deref(),
server: None,
graph: cli.graph.as_deref(),
uri: None,
},
)?;
resolve_storage_uri(
&config,
scope.uri,
scope.cluster.as_deref(),
scope.cluster_graph.as_deref(),
"optimize",
)
.await?
};
let uri = resolve_maintenance_uri(
&config,
cli.profile.as_deref(),
cli.store.as_deref(),
cli.cluster.as_deref(),
cli.graph.as_deref(),
uri,
"optimize",
)
.await?;
let db = Omnigraph::open(&uri).await?;
let stats = db.optimize().await?;
if json {
@ -865,44 +837,21 @@ async fn main() -> Result<()> {
Command::Repair {
uri,
config,
cluster,
cluster_graph,
confirm,
force,
json,
} => {
let config = load_cli_config(config.as_ref())?;
let uri = if uri.is_some() || cluster.is_some() {
resolve_storage_uri(
&config,
uri,
cluster.as_deref(),
cluster_graph.as_deref(),
"repair",
)
.await?
} else {
// RFC-011: no explicit per-command address — consult the scope.
let scope = scope::resolve_scope(
&operator::load_operator_config()?,
planes::Capability::Direct,
scope::ScopeFlags {
profile: cli.profile.as_deref(),
store: cli.store.as_deref(),
server: None,
graph: cli.graph.as_deref(),
uri: None,
},
)?;
resolve_storage_uri(
&config,
scope.uri,
scope.cluster.as_deref(),
scope.cluster_graph.as_deref(),
"repair",
)
.await?
};
let uri = resolve_maintenance_uri(
&config,
cli.profile.as_deref(),
cli.store.as_deref(),
cli.cluster.as_deref(),
cli.graph.as_deref(),
uri,
"repair",
)
.await?;
let db = Omnigraph::open(&uri).await?;
let stats = db
.repair(omnigraph::db::RepairOptions { confirm, force })
@ -979,45 +928,22 @@ async fn main() -> Result<()> {
Command::Cleanup {
uri,
config,
cluster,
cluster_graph,
keep,
older_than,
confirm,
json,
} => {
let config = load_cli_config(config.as_ref())?;
let uri = if uri.is_some() || cluster.is_some() {
resolve_storage_uri(
&config,
uri,
cluster.as_deref(),
cluster_graph.as_deref(),
"cleanup",
)
.await?
} else {
// RFC-011: no explicit per-command address — consult the scope.
let scope = scope::resolve_scope(
&operator::load_operator_config()?,
planes::Capability::Direct,
scope::ScopeFlags {
profile: cli.profile.as_deref(),
store: cli.store.as_deref(),
server: None,
graph: cli.graph.as_deref(),
uri: None,
},
)?;
resolve_storage_uri(
&config,
scope.uri,
scope.cluster.as_deref(),
scope.cluster_graph.as_deref(),
"cleanup",
)
.await?
};
let uri = resolve_maintenance_uri(
&config,
cli.profile.as_deref(),
cli.store.as_deref(),
cli.cluster.as_deref(),
cli.graph.as_deref(),
uri,
"cleanup",
)
.await?;
let older_than_dur = older_than.as_deref().map(parse_duration_arg).transpose()?;

View file

@ -177,35 +177,78 @@ pub(crate) fn command_label(cmd: &Command) -> &'static str {
}
}
/// 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.
/// The verbs that address an existing graph through a cluster scope
/// (`--cluster <root> --graph <id>`): the storage-maintenance commands.
/// `init` is storage-plane too but *creates* a graph (cluster graphs are born
/// from `cluster apply`, not `init`), and `schema plan` / `lint` take a
/// positional URI — none consume cluster addressing, so the guard rejects
/// `--cluster`/`--graph` on them rather than silently dropping the flag.
pub(crate) fn accepts_cluster_addressing(cmd: &Command) -> bool {
matches!(
cmd,
Command::Optimize { .. } | Command::Repair { .. } | Command::Cleanup { .. }
)
}
/// Reject a scope-addressing flag (`--server`/`--cluster`/`--graph`) on a verb
/// that cannot consume it, rather than silently dropping it (the old behavior:
/// e.g. `optimize --server prod` dropped `--server` and failed later with an
/// unrelated message). Each flag has a distinct valid surface:
/// - `--server` → served-graph scopes (`any`/`served`);
/// - `--cluster` → the cluster-maintenance verbs (optimize/repair/cleanup);
/// - `--graph` → any multi-graph scope: a served scope *or* a cluster one.
/// RFC-010 Slice 1, generalized for RFC-011 cluster addressing.
pub(crate) fn guard_addressing(cli: &Cli) -> Result<()> {
if cli.server.is_none() && cli.graph.is_none() {
if cli.server.is_none() && cli.cluster.is_none() && cli.graph.is_none() {
return Ok(());
}
let capability = command_capability(&cli.command);
if capability.accepts_server_addressing() {
return Ok(());
}
let label = command_label(&cli.command);
let how = match capability {
Capability::Direct => match cli.command {
Command::Init { .. } => "Pass a storage URI.",
_ => "Pass a storage URI, or --cluster <dir> --cluster-graph <id>.",
let cluster_ok = accepts_cluster_addressing(&cli.command);
if cli.server.is_some() && !capability.accepts_server_addressing() {
bail!(
"`{label}` is a {} command; --server addresses a served graph and does not apply.{}",
capability.describe(),
remediation(capability, &cli.command),
);
}
if cli.cluster.is_some() && !cluster_ok {
bail!(
"`{label}` is a {} command; --cluster addresses a cluster-managed graph for \
maintenance (optimize/repair/cleanup) and does not apply.{}",
capability.describe(),
remediation(capability, &cli.command),
);
}
if cli.graph.is_some() && !(capability.accepts_server_addressing() || cluster_ok) {
bail!(
"`{label}` is a {} command; --graph selects a graph within a server or cluster \
scope and does not apply.{}",
capability.describe(),
remediation(capability, &cli.command),
);
}
Ok(())
}
/// The "what to do instead" tail for a wrong-address error, by capability.
/// Includes its own leading space when non-empty so the caller appends it
/// directly — an empty tail (the served-addressing capabilities, which only
/// reach this fn for a misplaced `--cluster`/`--graph`) leaves no trailing space.
fn remediation(capability: Capability, cmd: &Command) -> &'static str {
match capability {
Capability::Direct => match cmd {
Command::Init { .. } => " Pass a storage URI.",
Command::Optimize { .. } | Command::Repair { .. } | Command::Cleanup { .. } => {
" Pass a storage URI, or --cluster <dir> --graph <id>."
}
_ => " Pass a storage URI.",
},
Capability::Control => "It operates on a cluster (pass --config <dir>).",
Capability::Local => "It does not address a graph.",
Capability::Any | Capability::Served => {
unreachable!("served-addressing capabilities returned early")
}
};
bail!(
"`{label}` is a {} command; --server/--graph address a served graph and do not apply. {how}",
capability.describe()
);
Capability::Control => " It operates on a cluster (pass --config <dir>).",
Capability::Local => " It does not address a graph.",
Capability::Any | Capability::Served => "",
}
}
#[cfg(test)]

View file

@ -42,6 +42,7 @@ pub(crate) struct ScopeFlags<'a> {
pub(crate) profile: Option<&'a str>,
pub(crate) store: Option<&'a str>,
pub(crate) server: Option<&'a str>,
pub(crate) cluster: Option<&'a str>,
pub(crate) graph: Option<&'a str>,
pub(crate) uri: Option<String>,
}
@ -56,17 +57,49 @@ pub(crate) fn resolve_scope(
capability: Capability,
flags: ScopeFlags<'_>,
) -> Result<ResolvedScope> {
// `--store` is its own way to address a graph; combining it with a positional
// URI or `--server` is a contradiction, not a silent precedence.
if flags.store.is_some() && (flags.uri.is_some() || flags.server.is_some()) {
// At most one explicit scope primitive may address a command — a positional
// URI, `--store`, `--server`, or `--cluster` are mutually exclusive ways to
// name the graph. Combining them is a contradiction, not a silent precedence.
let primitives: Vec<&str> = [
flags.uri.as_deref().map(|_| "a positional URI"),
flags.store.map(|_| "--store"),
flags.server.map(|_| "--server"),
flags.cluster.map(|_| "--cluster"),
]
.into_iter()
.flatten()
.collect();
if primitives.len() > 1 {
bail!(
"--store is exclusive with a positional URI and --server — pick one way to \
address the graph"
"{} are mutually exclusive — pick one way to address the graph",
primitives.join(" and ")
);
}
// 1. Any explicit address wins; reproduce today's behavior untouched.
// `--store` is an explicit store URI — fold it into `uri`.
// 1a. `--cluster` is the cluster scope primitive (maintenance): resolve its
// root + select the graph with `--graph`.
if let Some(cluster) = flags.cluster {
return scope_from_binding(
op,
capability,
ScopeBinding::Cluster(cluster.to_string()),
flags.graph.map(str::to_string),
"--cluster",
);
}
// 1b. Any other explicit address wins; reproduce today's behavior untouched.
// `--store` is an explicit store URI — fold it into `uri`.
if flags.uri.is_some() || flags.server.is_some() || flags.store.is_some() {
// `--graph` selects within a multi-graph scope; a bare positional URI /
// `--store` is already a single graph, so a stray `--graph` is an error
// rather than a silently-dropped flag.
if flags.graph.is_some() && flags.server.is_none() {
bail!(
"--graph selects a graph within a server or cluster scope; a positional \
URI / --store is already a single graph"
);
}
return Ok(ResolvedScope {
server: flags.server.map(str::to_string),
graph: flags.graph.map(str::to_string),
@ -128,8 +161,8 @@ fn scope_from_binding(
if capability == Capability::Direct {
bail!(
"this command needs direct storage access, but {source} resolves a \
server scope; name storage explicitly with --store <uri> (or a \
--cluster/--cluster-graph for a managed graph)"
server scope; name storage explicitly with --store <uri> (or \
--cluster <dir> --graph <id> for a managed graph)"
);
}
Ok(ResolvedScope {
@ -146,21 +179,25 @@ fn scope_from_binding(
direct access"
);
}
// A cluster binding is a config name (resolved against `clusters:`)
// or a literal root URI.
let root = if let Some(root) = op.cluster_root(&cluster) {
root.to_string()
} else if cluster.contains("://") {
cluster
} else {
// A cluster value is a config name (resolved against `clusters:`)
// or a literal root: an `s3://`/`file://` URI or a local cluster
// directory. Only a configured name is rewritten; anything else is
// passed through to the cluster-state resolver verbatim, so a bare
// directory path keeps working as it did for per-command `--cluster`.
let root = op
.cluster_root(&cluster)
.map(str::to_string)
.unwrap_or(cluster);
// A cluster holds many graphs; maintenance addresses one at a time.
let Some(graph) = graph else {
bail!(
"unknown cluster '{cluster}' ({source}); define it under `clusters:` \
in operator config, or use a literal root URI"
"{source} resolves a cluster scope; pass --graph <id> to select which \
graph to maintain"
);
};
Ok(ResolvedScope {
cluster: Some(root),
cluster_graph: graph,
cluster_graph: Some(graph),
..Default::default()
})
}
@ -192,6 +229,7 @@ mod tests {
profile: None,
store: None,
server: None,
cluster: None,
graph: None,
uri: None,
}
@ -230,7 +268,7 @@ mod tests {
}
#[test]
fn store_is_exclusive_with_positional_uri_and_server() {
fn scope_primitives_are_mutually_exclusive() {
let op = OperatorConfig::default();
for flags in [
ScopeFlags {
@ -243,9 +281,93 @@ mod tests {
server: Some("prod"),
..flags()
},
ScopeFlags {
cluster: Some("./brain"),
uri: Some("file://other.omni".into()),
..flags()
},
ScopeFlags {
cluster: Some("./brain"),
server: Some("prod"),
..flags()
},
] {
let err = resolve_scope(&op, Capability::Any, flags).unwrap_err().to_string();
assert!(err.contains("--store is exclusive"), "{err}");
let err = resolve_scope(&op, Capability::Direct, flags)
.unwrap_err()
.to_string();
assert!(err.contains("mutually exclusive"), "{err}");
}
}
#[test]
fn cluster_flag_resolves_root_and_graph_for_maintenance() {
let op = cfg("clusters:\n brain:\n root: s3://acme/brain\n");
let scope = resolve_scope(
&op,
Capability::Direct,
ScopeFlags {
cluster: Some("brain"),
graph: Some("knowledge"),
..flags()
},
)
.unwrap();
assert_eq!(scope.cluster.as_deref(), Some("s3://acme/brain"));
assert_eq!(scope.cluster_graph.as_deref(), Some("knowledge"));
}
#[test]
fn cluster_flag_accepts_a_literal_root_uri() {
let op = OperatorConfig::default();
let scope = resolve_scope(
&op,
Capability::Direct,
ScopeFlags {
cluster: Some("s3://bucket/clusters/brain"),
graph: Some("knowledge"),
..flags()
},
)
.unwrap();
assert_eq!(scope.cluster.as_deref(), Some("s3://bucket/clusters/brain"));
assert_eq!(scope.cluster_graph.as_deref(), Some("knowledge"));
}
#[test]
fn cluster_scope_without_a_graph_is_a_loud_error() {
let op = cfg("clusters:\n brain:\n root: s3://acme/brain\n");
let err = resolve_scope(
&op,
Capability::Direct,
ScopeFlags {
cluster: Some("brain"),
..flags()
},
)
.unwrap_err()
.to_string();
assert!(err.contains("--graph <id>"), "{err}");
}
#[test]
fn graph_on_a_bare_store_or_uri_is_rejected() {
let op = OperatorConfig::default();
for flags in [
ScopeFlags {
uri: Some("graph.omni".into()),
graph: Some("knowledge"),
..flags()
},
ScopeFlags {
store: Some("s3://b/g.omni"),
graph: Some("knowledge"),
..flags()
},
] {
let err = resolve_scope(&op, Capability::Any, flags)
.unwrap_err()
.to_string();
assert!(err.contains("already a single graph"), "{err}");
}
}
@ -294,6 +416,27 @@ mod tests {
assert_eq!(scope.cluster_graph.as_deref(), Some("knowledge"));
}
#[test]
fn profile_cluster_scope_with_graph_override() {
// The deferral closed by this slice: a `--graph` flag overrides a
// profile cluster's default_graph, exactly as it does for a server scope.
let op = cfg(
"clusters:\n brain:\n root: s3://acme/brain\nprofiles:\n admin:\n cluster: brain\n default_graph: knowledge\n",
);
let scope = resolve_scope(
&op,
Capability::Direct,
ScopeFlags {
profile: Some("admin"),
graph: Some("archive"),
..flags()
},
)
.unwrap();
assert_eq!(scope.cluster.as_deref(), Some("s3://acme/brain"));
assert_eq!(scope.cluster_graph.as_deref(), Some("archive")); // flag beats profile default
}
#[test]
fn server_scope_on_maintenance_verb_errors() {
let op = cfg("defaults:\n server: prod\nservers:\n prod:\n url: https://x\n");