diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index a0fb3b5..3dd0f88 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -1679,19 +1679,36 @@ struct QueriesListOutput { queries: Vec, } -/// Resolve the stored-query registry entries for the selected graph via -/// the same `query_entries_for` the server boot uses, so the CLI -/// validates exactly the block the server would load. A `--target` -/// overrides the configured default graph. -fn registry_entries<'a>( - config: &'a OmnigraphConfig, - target: Option<&str>, -) -> &'a std::collections::BTreeMap { - config.query_entries_for(target.or_else(|| config.cli_graph_name())) +/// Resolve the selected graph to `(local URI, registry selection)` from one +/// precedence, so a command's schema and its stored-query registry can never +/// come from different graphs. A **positional URI is anonymous** (top-level +/// registry, ignoring the configured default graph); otherwise `--target` +/// or the configured `cli.graph` names the graph (its per-graph block). +/// Mirrors the server's single-mode identity rule. +fn resolve_selected_graph( + config: &OmnigraphConfig, + cli_uri: Option, + cli_target: Option<&str>, + operation: &str, +) -> Result<(String, Option)> { + let selected = if cli_uri.is_some() { + None + } else { + cli_target + .map(str::to_string) + .or_else(|| config.cli_graph_name().map(str::to_string)) + }; + let uri = resolve_local_uri(config, cli_uri, cli_target, operation)?; + Ok((uri, selected)) } -fn load_registry_or_report(config: &OmnigraphConfig, target: Option<&str>) -> Result { - QueryRegistry::load(config, registry_entries(config, target)).map_err(|errors| { +/// Load the stored-query registry for an already-resolved graph selection +/// (`None` = anonymous → top-level; `Some(name)` = that graph's block). +fn load_registry_or_report( + config: &OmnigraphConfig, + selected: Option<&str>, +) -> Result { + QueryRegistry::load(config, config.query_entries_for(selected)).map_err(|errors| { color_eyre::eyre::eyre!( "stored-query registry failed to load:\n {}", errors @@ -1710,8 +1727,11 @@ async fn execute_queries_validate( json: bool, ) -> Result<()> { let config = load_cli_config(config_path)?; - let registry = load_registry_or_report(&config, target.as_deref())?; - let uri = resolve_local_uri(&config, uri, target.as_deref(), "queries validate")?; + // One selection drives both the schema URI and the registry, so a + // positional URI and a `--target` can't validate different graphs. + let (uri, selected) = + resolve_selected_graph(&config, uri, target.as_deref(), "queries validate")?; + let registry = load_registry_or_report(&config, selected.as_deref())?; let db = Omnigraph::open(&uri).await?; let report = check(®istry, &db.catalog()); @@ -1766,7 +1786,10 @@ fn execute_queries_list( json: bool, ) -> Result<()> { let config = load_cli_config(config_path)?; - let registry = load_registry_or_report(&config, target.as_deref())?; + // `list` takes no URI, so the selection is the target or the configured + // default graph (named → its per-graph block; else top-level). + let selected = target.as_deref().or_else(|| config.cli_graph_name()); + let registry = load_registry_or_report(&config, selected)?; let output = QueriesListOutput { queries: registry diff --git a/crates/omnigraph-cli/tests/cli.rs b/crates/omnigraph-cli/tests/cli.rs index 2d08965..0cb936c 100644 --- a/crates/omnigraph-cli/tests/cli.rs +++ b/crates/omnigraph-cli/tests/cli.rs @@ -2493,3 +2493,49 @@ fn queries_validate_exits_nonzero_on_duplicate_tool_name() { "duplicate tool name should be reported naming both queries; stderr:\n{stderr}" ); } + +#[test] +fn queries_validate_positional_uri_ignores_default_graph() { + // A positional URI is anonymous → the schema AND the registry both come + // from top-level, even when `cli.graph` names a graph whose per-graph + // queries would fail. Pins that the URI and registry can't diverge. + let graph = SystemGraph::loaded(); + graph.write_query( + "clean.gq", + "query clean($name: String) { match { $p: Person { name: $name } } return { $p.age } }", + ); + // `Widget` is not in the fixture schema — the default graph's per-graph + // query would break validate if it were (wrongly) selected. + graph.write_query("broken.gq", "query broken() { match { $w: Widget } return { $w.name } }"); + let config = graph.write_config( + "omnigraph.yaml", + concat!( + "cli:\n graph: prod\n", + "graphs:\n", + " prod:\n", + " uri: /nonexistent-prod.omni\n", + " queries:\n", + " broken:\n", + " file: ./broken.gq\n", + "queries:\n", + " clean:\n", + " file: ./clean.gq\n", + "policy: {}\n", + ), + ); + // Positional URI = the real loaded graph; selection is anonymous, so the + // CLEAN top-level registry validates (not prod's broken one). + let output = output_success( + cli() + .arg("queries") + .arg("validate") + .arg(graph.path()) + .arg("--config") + .arg(&config), + ); + let stdout = stdout_string(&output); + assert!( + stdout.contains("OK"), + "positional URI must validate the top-level registry, not the cli.graph default; stdout:\n{stdout}" + ); +} diff --git a/crates/omnigraph-server/src/config.rs b/crates/omnigraph-server/src/config.rs index 0b621e1..996feaa 100644 --- a/crates/omnigraph-server/src/config.rs +++ b/crates/omnigraph-server/src/config.rs @@ -331,6 +331,36 @@ impl OmnigraphConfig { } } + /// The policy file that applies for a graph selection — the policy + /// sibling of [`OmnigraphConfig::query_entries_for`], so policy and + /// queries resolve by the same identity rule. A named graph in + /// `graphs:` uses its per-graph `policy.file` with **no** top-level + /// fallback (a named graph with no per-graph policy has no policy — + /// that keeps the boot-time coherence check meaningful); anything else + /// (no selection, or a bare URI) uses the top-level `policy.file`. + pub fn resolve_policy_file_for(&self, graph: Option<&str>) -> Option { + match graph { + Some(name) if self.graphs.contains_key(name) => self.resolve_target_policy_file(name), + _ => self.resolve_policy_file(), + } + } + + /// Names of any top-level config blocks (`policy.file`, `queries:`) + /// that are populated. Used by the boot-time coherence check: when a + /// **named** graph is served (single-mode by name, or multi-mode), + /// the top-level blocks are not honored, so a populated one is a + /// configuration error rather than a silent no-op. + pub fn populated_top_level_blocks(&self) -> Vec<&'static str> { + let mut blocks = Vec::new(); + if self.policy.file.is_some() { + blocks.push("policy.file"); + } + if !self.queries.is_empty() { + blocks.push("queries"); + } + blocks + } + /// Resolve a stored-query `.gq` file path (from a registry entry), /// relative to the config's `base_dir`. Mirrors policy-file /// resolution; the registry loader calls this to turn each entry's @@ -640,6 +670,42 @@ queries: ); } + #[test] + fn resolve_policy_file_for_follows_identity() { + let temp = tempdir().unwrap(); + fs::write( + temp.path().join("omnigraph.yaml"), + "policy:\n file: ./top.yaml\ngraphs:\n prod:\n uri: s3://b/prod\n \ + policy:\n file: ./prod.yaml\n bare:\n uri: s3://b/bare\n", + ) + .unwrap(); + let config = load_config_in(temp.path(), None).unwrap(); + + // Named graph with its own policy → per-graph (not top-level). + assert!( + config + .resolve_policy_file_for(Some("prod")) + .unwrap() + .ends_with("prod.yaml") + ); + // Named graph with NO per-graph policy → None (no top-level fallback; + // load-bearing for the boot coherence check). + assert!(config.resolve_policy_file_for(Some("bare")).is_none()); + // Anonymous (bare URI) or an unknown name → top-level. + assert!( + config + .resolve_policy_file_for(None) + .unwrap() + .ends_with("top.yaml") + ); + assert!( + config + .resolve_policy_file_for(Some("nope")) + .unwrap() + .ends_with("top.yaml") + ); + } + #[test] fn queries_block_absent_yields_empty_registry() { let temp = tempdir().unwrap(); diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 168c9fb..ce62c52 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -927,11 +927,34 @@ pub fn load_server_settings( let uri = normalize_root_uri(&raw_uri).wrap_err_with(|| { format!("normalize single-graph URI '{raw_uri}' from server settings") })?; - let policy_file = config.resolve_policy_file(); - // Single mode uses the top-level `queries:` (mirrors top-level - // `policy.file`). Load + identity-check now (no engine needed); - // the schema type-check happens when the engine opens. - let queries = QueryRegistry::load(&config, config.query_entries_for(None)) + // Config follows graph IDENTITY, not mode: a bare URI is anonymous + // (top-level config); a graph chosen by name uses its per-graph + // `graphs..{policy,queries}`. `resolve_target_uri` already + // errored on an unknown name, so a `Some(name)` here is a known graph. + let selected: Option<&str> = if has_cli_uri { + None + } else { + cli_target.as_deref().or_else(|| config.server_graph_name()) + }; + // A named selection must not leave a populated top-level block + // silently unused — refuse boot and point at the per-graph block. + if let Some(name) = selected { + let unhonored = config.populated_top_level_blocks(); + if !unhonored.is_empty() { + bail!( + "serving named graph '{name}', but top-level {} {} set — a named graph \ + uses its own `graphs.{name}.…` block, so the top-level value is ignored. \ + Move it to `graphs.{name}` (e.g. `graphs.{name}.policy.file`, \ + `graphs.{name}.queries`).", + unhonored.join(" and "), + if unhonored.len() == 1 { "is" } else { "are" }, + ); + } + } + // Load + identity-check now (no engine needed); the schema + // type-check happens when the engine opens. + let policy_file = config.resolve_policy_file_for(selected); + let queries = QueryRegistry::load(&config, config.query_entries_for(selected)) .map_err(|errs| color_eyre::eyre::eyre!(format_registry_load_errors(&uri, &errs)))?; ServerConfigMode::Single { uri, @@ -939,12 +962,16 @@ pub fn load_server_settings( queries, } } else if has_explicit_config && has_graphs_map { - if config.resolve_policy_file().is_some() { + // Multi mode: every graph uses its per-graph block; top-level + // policy/queries are never honored, so a populated one is an error. + let unhonored = config.populated_top_level_blocks(); + if !unhonored.is_empty() { bail!( - "top-level `policy.file` is single-graph/CLI-local policy only; \ - in multi-graph mode move per-graph rules to \ - `graphs..policy.file` and move `graph_list` rules to \ - `server.policy.file`." + "multi-graph mode: top-level {} {} not honored — each graph uses its own \ + `graphs..…` block. Move per-graph rules there (and any \ + `graph_list` policy to `server.policy.file`).", + unhonored.join(" and "), + if unhonored.len() == 1 { "is" } else { "are" }, ); } // Rule 4 → Multi mode. Build a startup config per graph. diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 7770b18..1572f7c 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -5506,11 +5506,11 @@ graphs: let err = load_server_settings(Some(&config_path), None, None, None, true).unwrap_err(); let msg = err.to_string(); assert!( - msg.contains("top-level `policy.file` is single-graph/CLI-local policy only"), - "expected single-graph policy guidance, got: {msg}" + msg.contains("top-level") && msg.contains("policy.file") && msg.contains("not honored"), + "expected top-level-not-honored guidance, got: {msg}" ); assert!( - msg.contains("graphs..policy.file"), + msg.contains("graphs."), "expected per-graph migration guidance, got: {msg}" ); assert!( @@ -5519,6 +5519,86 @@ graphs: ); } + #[test] + fn mode_inference_multi_rejects_top_level_queries() { + // Symmetric to the policy guard: a top-level `queries:` block in + // multi-graph mode is not honored (each graph uses its own), so it + // is a loud error rather than a silent no-op. + let temp = tempfile::tempdir().unwrap(); + let config_path = temp.path().join("omnigraph.yaml"); + fs::write( + &config_path, + "queries:\n q:\n file: ./q.gq\ngraphs:\n alpha:\n uri: /tmp/alpha.omni\n", + ) + .unwrap(); + let err = load_server_settings(Some(&config_path), None, None, None, true).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("queries") && msg.contains("not honored"), + "top-level queries must be rejected in multi-graph mode: {msg}" + ); + } + + #[test] + fn single_mode_named_graph_rejects_top_level_blocks() { + // Serving a graph by name (`--target`/`server.graph`) uses its + // per-graph block; a populated top-level block would be silently + // shadowed, so boot refuses and names the per-graph location. + let temp = tempfile::tempdir().unwrap(); + let config_path = temp.path().join("omnigraph.yaml"); + fs::write( + &config_path, + "policy:\n file: ./top.yaml\ngraphs:\n prod:\n uri: /tmp/prod.omni\n", + ) + .unwrap(); + let err = + load_server_settings(Some(&config_path), None, Some("prod".to_string()), None, true) + .unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("prod") && msg.contains("policy.file") && msg.contains("graphs.prod"), + "named single-mode + top-level policy must refuse, naming the graph: {msg}" + ); + } + + #[test] + fn single_mode_named_graph_uses_per_graph_policy_and_queries() { + // The identity rule: `--target prod` attaches `graphs.prod`'s own + // policy + queries, not the top-level ones (which are absent here). + let temp = tempfile::tempdir().unwrap(); + fs::write( + temp.path().join("prod.gq"), + "query pq() { match { $u: User } return { $u.name } }", + ) + .unwrap(); + let config_path = temp.path().join("omnigraph.yaml"); + fs::write( + &config_path, + "graphs:\n prod:\n uri: /tmp/prod.omni\n policy:\n file: ./prod-policy.yaml\n \ + queries:\n pq:\n file: ./prod.gq\n", + ) + .unwrap(); + let settings = + load_server_settings(Some(&config_path), None, Some("prod".to_string()), None, true) + .unwrap(); + match settings.mode { + ServerConfigMode::Single { + policy_file, + queries, + .. + } => { + assert!( + policy_file + .as_ref() + .is_some_and(|p| p.ends_with("prod-policy.yaml")), + "per-graph policy attached: {policy_file:?}" + ); + assert!(queries.lookup("pq").is_some(), "per-graph query attached"); + } + other => panic!("expected Single mode, got {other:?}"), + } + } + #[test] fn mode_inference_normalizes_multi_graph_uris() { let temp = tempfile::tempdir().unwrap(); diff --git a/docs/user/cli-reference.md b/docs/user/cli-reference.md index 2f9e198..931b7f9 100644 --- a/docs/user/cli-reference.md +++ b/docs/user/cli-reference.md @@ -67,7 +67,7 @@ aliases: graph: branch: format: -queries: # top-level stored-query registry (single-graph mode); mirrors top-level `policy` +queries: # top-level registry — applies only to a bare-URI (anonymous) graph; a graph served by name uses its `graphs..queries`. Mirrors top-level `policy`. : { file: } # mcp.expose defaults to true policy: file: ./policy.yaml diff --git a/docs/user/policy.md b/docs/user/policy.md index 21acda8..77d6aed 100644 --- a/docs/user/policy.md +++ b/docs/user/policy.md @@ -47,10 +47,15 @@ graphs: # no per-graph policy → no engine-layer Cedar enforcement on beta ``` -Top-level `policy.file` is single-graph / CLI-local policy only. Multi-graph -server startup rejects it because applying one graph policy to every configured -graph is ambiguous. Move per-graph rules to `graphs..policy.file` and -move `graph_list` rules to `server.policy.file`. +**Config follows graph identity, not server mode.** A graph served by **name** +(`--target ` or `server.graph`) uses its own `graphs..policy.file`, +exactly as in multi-graph mode. Top-level `policy.file` applies only to an +**anonymous** graph — one served by a bare `` with no `graphs:` entry. +Serving a **named** graph (single- or multi-graph mode) while top-level +`policy.file` (or `queries:`) is populated **refuses boot**, naming the block, +since the top-level value would otherwise be silently shadowed by the per-graph +block. Move per-graph rules to `graphs..policy.file` and `graph_list` +rules to `server.policy.file`. Each graph's HTTP request flows through its own per-graph policy. The management endpoint (`GET /graphs`) flows through the server-level policy. When `server.policy.file` is unset, `GET /graphs` is denied in every runtime state, including `--unauthenticated`; with bearer tokens configured, it returns 403 after admission control because `graph_list` is not a `read`-equivalent action. The operator must explicitly authorize via `server-policy.yaml` to expose `/graphs`. diff --git a/docs/user/server.md b/docs/user/server.md index d67002e..3a65f85 100644 --- a/docs/user/server.md +++ b/docs/user/server.md @@ -6,7 +6,9 @@ Axum 0.8 + tokio + utoipa-generated OpenAPI. **Two modes** (v0.6.0+): single-gra ### Single-graph mode (legacy) -`omnigraph-server ` or `omnigraph-server --target --config omnigraph.yaml`. Routes are flat — `/snapshot`, `/read`, `/branches`, etc. Behavior unchanged from v0.6.0. +`omnigraph-server ` or `omnigraph-server --target --config omnigraph.yaml`. Routes are flat — `/snapshot`, `/read`, `/branches`, etc. + +**Config follows graph identity.** A bare `` is an *anonymous* graph and uses the **top-level** `policy.file` / `queries:`. A graph chosen by **name** (`--target` / `server.graph`) uses its own `graphs..{policy.file, queries}` — the same block multi-graph mode uses. ⚠️ *Changed from v0.6.0, which always used top-level config in single mode: a named-graph config that puts `policy`/`queries` at top-level now **refuses boot** and points you at `graphs..…` (move the block there). Bare-`` single mode is unchanged.* ### Multi-graph mode (v0.6.0+)