mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-27 02:39:38 +02:00
fix(cli): address review — honor the one-thing contract, restore docs, untangle test phases
- resolve_cluster_actor uses load_config directly: load_cli_config also loads auth.env_file into the process env — a second thing, violating the documented 'exactly one thing' omnigraph.yaml contract for cluster ops. - resolve_cli_actor gets its doc comment back (the inserted helper had absorbed the contiguous /// block). - The actor-default test imports once as setup and asserts on apply alone, idempotently, instead of re-importing inside the assertion helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
fbe9726ac7
commit
3b2bf755ae
2 changed files with 34 additions and 50 deletions
|
|
@ -1251,28 +1251,31 @@ async fn open_local_db_with_policy(graph: &ResolvedCliGraph) -> Result<Omnigraph
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Actor resolution for cluster operations. Cluster FACTS stay unlayered
|
||||||
|
/// (cluster.yaml only), but the operator's identity is a per-operator fact —
|
||||||
|
/// the per-operator config's permanent job. An explicit --as never touches
|
||||||
|
/// any config (containers and CI stay config-free); without it, the standard
|
||||||
|
/// cwd omnigraph.yaml search supplies `cli.actor`, and a malformed config
|
||||||
|
/// fails loudly rather than silently dropping attribution. Deliberately
|
||||||
|
/// `load_config`, NOT `load_cli_config`: the latter also loads
|
||||||
|
/// `auth.env_file` into the process env — a second thing, violating the
|
||||||
|
/// documented "exactly one thing" contract.
|
||||||
|
fn resolve_cluster_actor(cli_as: Option<&str>) -> Result<Option<String>> {
|
||||||
|
if let Some(actor) = cli_as {
|
||||||
|
return Ok(Some(actor.to_string()));
|
||||||
|
}
|
||||||
|
let config = load_config(None).wrap_err(
|
||||||
|
"resolving the default actor from the per-operator omnigraph.yaml (pass --as <ACTOR> to skip this lookup)",
|
||||||
|
)?;
|
||||||
|
Ok(config.cli.actor.clone())
|
||||||
|
}
|
||||||
|
|
||||||
/// Resolve the CLI's effective actor identity for engine-layer policy
|
/// Resolve the CLI's effective actor identity for engine-layer policy
|
||||||
/// (MR-722). Precedence: `--as <ACTOR>` (top-level flag) overrides
|
/// (MR-722). Precedence: `--as <ACTOR>` (top-level flag) overrides
|
||||||
/// `cli.actor` from `omnigraph.yaml`; both unset returns `None`. When
|
/// `cli.actor` from `omnigraph.yaml`; both unset returns `None`. When
|
||||||
/// policy is configured and this returns `None`, the engine-layer
|
/// policy is configured and this returns `None`, the engine-layer
|
||||||
/// footgun guard intentionally denies — silent bypass via "I forgot the
|
/// footgun guard intentionally denies — silent bypass via "I forgot the
|
||||||
/// actor" is what the guard prevents.
|
/// actor" is what the guard prevents.
|
||||||
/// Actor resolution for cluster operations. Cluster FACTS stay unlayered
|
|
||||||
/// (cluster.yaml only), but the operator's identity is a per-operator fact —
|
|
||||||
/// the per-operator config's permanent job. An explicit --as never touches
|
|
||||||
/// any config (containers and CI stay config-free); without it, the standard
|
|
||||||
/// cwd omnigraph.yaml search supplies `cli.actor`, and a malformed config
|
|
||||||
/// fails loudly rather than silently dropping attribution.
|
|
||||||
fn resolve_cluster_actor(cli_as: Option<&str>) -> Result<Option<String>> {
|
|
||||||
if let Some(actor) = cli_as {
|
|
||||||
return Ok(Some(actor.to_string()));
|
|
||||||
}
|
|
||||||
let config = load_cli_config(None).wrap_err(
|
|
||||||
"resolving the default actor from the per-operator omnigraph.yaml (pass --as <ACTOR> to skip this lookup)",
|
|
||||||
)?;
|
|
||||||
Ok(config.cli.actor.clone())
|
|
||||||
}
|
|
||||||
|
|
||||||
fn resolve_cli_actor<'a>(cli_as: Option<&'a str>, config: &'a OmnigraphConfig) -> Option<&'a str> {
|
fn resolve_cli_actor<'a>(cli_as: Option<&'a str>, config: &'a OmnigraphConfig) -> Option<&'a str> {
|
||||||
cli_as.or(config.cli.actor.as_deref())
|
cli_as.or(config.cli.actor.as_deref())
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -4339,22 +4339,19 @@ fn cluster_apply_uses_cli_actor_from_local_config() {
|
||||||
"cli:\n actor: act-local\n",
|
"cli:\n actor: act-local\n",
|
||||||
)
|
)
|
||||||
.unwrap();
|
.unwrap();
|
||||||
let run = |extra: &[&str]| {
|
// Phase 1: import once (setup, not under test).
|
||||||
let mut command = cli();
|
let output = cli()
|
||||||
command.current_dir(temp.path());
|
.current_dir(temp.path())
|
||||||
for arg in extra {
|
.arg("cluster")
|
||||||
command.arg(arg);
|
.arg("import")
|
||||||
}
|
.arg("--config")
|
||||||
let output = command
|
.arg(temp.path())
|
||||||
.arg("cluster")
|
.output()
|
||||||
.arg("import")
|
.unwrap();
|
||||||
.arg("--config")
|
assert!(output.status.success(), "{output:?}");
|
||||||
.arg(temp.path())
|
|
||||||
.arg("--json")
|
// Phase 2: apply alone, capturing the echoed actor (idempotent re-runs).
|
||||||
.output()
|
let apply = |extra: &[&str]| {
|
||||||
.unwrap();
|
|
||||||
assert!(output.status.success(), "{output:?}");
|
|
||||||
// apply, capturing the echoed actor
|
|
||||||
let mut command = cli();
|
let mut command = cli();
|
||||||
command.current_dir(temp.path());
|
command.current_dir(temp.path());
|
||||||
for arg in extra {
|
for arg in extra {
|
||||||
|
|
@ -4372,24 +4369,8 @@ fn cluster_apply_uses_cli_actor_from_local_config() {
|
||||||
serde_json::from_str(String::from_utf8_lossy(&output.stdout).trim()).unwrap();
|
serde_json::from_str(String::from_utf8_lossy(&output.stdout).trim()).unwrap();
|
||||||
json["actor"].clone()
|
json["actor"].clone()
|
||||||
};
|
};
|
||||||
assert_eq!(run(&[]), "act-local", "cli.actor is the no-flag default");
|
assert_eq!(apply(&[]), "act-local", "cli.actor is the no-flag default");
|
||||||
|
assert_eq!(apply(&["--as", "andrew"]), "andrew", "--as overrides cli.actor");
|
||||||
// A fresh dir (state already imported above): the flag wins over config.
|
|
||||||
let mut command = cli();
|
|
||||||
command.current_dir(temp.path());
|
|
||||||
let output = command
|
|
||||||
.arg("--as")
|
|
||||||
.arg("andrew")
|
|
||||||
.arg("cluster")
|
|
||||||
.arg("apply")
|
|
||||||
.arg("--config")
|
|
||||||
.arg(temp.path())
|
|
||||||
.arg("--json")
|
|
||||||
.output()
|
|
||||||
.unwrap();
|
|
||||||
let json: serde_json::Value =
|
|
||||||
serde_json::from_str(String::from_utf8_lossy(&output.stdout).trim()).unwrap();
|
|
||||||
assert_eq!(json["actor"], "andrew", "--as overrides cli.actor");
|
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue