From 7f28e1a5ed9a1dc4a7c65d393d692a3805203d0d Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Fri, 29 May 2026 13:17:20 +0200 Subject: [PATCH] refactor(cli): demote `check` from visible_alias to deprecation shim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `omnigraph check` was a clap `visible_alias` on `lint`, advertised in `--help` as an equivalent canonical name. Per MR-981 §6 (long-form flags as canonical, short forms as visible aliases), visible aliases on subcommand names hurt agent CX: agents emit either spelling depending on training-data drift, and there's no length signal pointing at the canonical name. Changes: * Remove `#[command(visible_alias = "check")]` from the `Lint` variant. `omnigraph --help` now shows only `lint`. * Add bare `check` to `rewrite_deprecated_argv` so `omnigraph check ` still works — it rewrites to `omnigraph lint ` and emits a one-line stderr deprecation warning, matching the existing pattern for `read` / `change` / `query lint` / `query check`. * Fix the nested `query check` shim to substitute `check` -> `lint` in the rewritten argv (previously it relied on `check` being a visible_alias to reach the `Lint` variant). * New test `deprecated_check_top_level_rewrites_to_lint` covers: bare `check` produces identical stdout to `lint`, emits the deprecation warning, and `check` does NOT appear as an alias in `omnigraph --help`. * Release notes updated to reflect the deprecation-shim treatment and cross-reference MR-981 §6 reasoning. Cargo / Go users typing `check` still work indefinitely; one stderr nudge per invocation teaches the canonical name. Agents see only `lint` in `--help --json` so they emit one canonical form. 67/0 omnigraph-cli tests pass; 39 workspace test suites green. --- crates/omnigraph-cli/src/main.rs | 51 +++++++++++++++++----- crates/omnigraph-cli/tests/cli.rs | 71 +++++++++++++++++++++++++++++++ docs/releases/v0.6.0.md | 2 +- 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 1e3658c..b7e3041 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -130,10 +130,15 @@ enum Command { }, /// Validate queries against a schema (offline) or repo (repo-backed). /// - /// Replaces `omnigraph query lint` / `omnigraph query check`, which - /// are kept as deprecated argv-level shims (a one-line warning is - /// printed and the invocation is rewritten to `omnigraph lint`). - #[command(visible_alias = "check")] + /// Canonical name is `lint` (matches the `omnigraph_compiler::lint` + /// module and the `OG-XXX-NNN` lint-code vocabulary). Replaces the + /// deprecated `omnigraph query lint` / `omnigraph query check` / + /// `omnigraph check` invocations — each is kept as an argv-level + /// shim that prints a one-line stderr warning and rewrites to + /// `omnigraph lint`. Aliases are deliberately *not* exposed via + /// clap's `visible_alias` because that would advertise two + /// equivalent canonical names, which agents emit interchangeably + /// (see MR-981). Lint { /// Graph URI uri: Option, @@ -1776,11 +1781,19 @@ async fn execute_export_remote_to_writer( /// Rewrite deprecated CLI invocations into their canonical form. /// -/// The current rename pass moves three subcommands: -/// - `omnigraph read` -> `omnigraph query` (visible_alias handles parsing; we just warn) -/// - `omnigraph change` -> `omnigraph mutate` (visible_alias handles parsing; we just warn) +/// The current rename pass moves four subcommands: +/// - `omnigraph read` -> `omnigraph query` (clap `visible_alias` handles parsing; we warn) +/// - `omnigraph change` -> `omnigraph mutate` (clap `visible_alias` handles parsing; we warn) +/// - `omnigraph check` -> `omnigraph lint` (rewrite required; no visible_alias by design) /// - `omnigraph query lint` -> `omnigraph lint` (rewrite required; `query` is now the read-runner) -/// - `omnigraph query check` -> `omnigraph lint` (`check` is still a visible alias on `lint`) +/// - `omnigraph query check` -> `omnigraph lint` (rewrite required) +/// +/// `check` is *not* a clap visible_alias on `lint` even though they're +/// semantically equivalent. Visible aliases create two canonical names +/// that agents emit interchangeably depending on training-data drift +/// (see MR-981 §6 for the policy). The argv-shim + stderr warning +/// pattern preserves back-compat for human users while pointing every +/// caller at the single canonical name in `--help`. /// /// Returns the (possibly rewritten) argv that clap should parse. fn rewrite_deprecated_argv(args: Vec) -> Vec { @@ -1790,12 +1803,17 @@ fn rewrite_deprecated_argv(args: Vec) -> Vec { if sub == Some("query") && matches!(sub2, Some("lint") | Some("check")) { let suffix = sub2.unwrap(); eprintln!( - "warning: `omnigraph query {suffix}` is deprecated; use `omnigraph lint` (alias: `omnigraph check`) instead" + "warning: `omnigraph query {suffix}` is deprecated; use `omnigraph lint` instead" ); - // Drop the leading `query` token, leaving e.g. `lint --query ./foo.gq`. + // Drop the leading `query` token AND normalize `check` -> `lint`. + // `check` is no longer a clap visible_alias (MR-981 §6), so the + // rewritten argv must reach the canonical `lint` subcommand + // directly. Result for `omnigraph query check --query foo.gq`: + // `omnigraph lint --query foo.gq`. let mut out = Vec::with_capacity(args.len() - 1); out.push(args[0].clone()); - out.extend(args[2..].iter().cloned()); + out.push(OsString::from("lint")); + out.extend(args[3..].iter().cloned()); return out; } } @@ -1807,6 +1825,17 @@ fn rewrite_deprecated_argv(args: Vec) -> Vec { "change" => eprintln!( "warning: `omnigraph change` is deprecated; use `omnigraph mutate` instead" ), + "check" => { + eprintln!( + "warning: `omnigraph check` is deprecated; use `omnigraph lint` instead" + ); + // Rewrite the top-level subcommand to `lint`; pass through the rest. + let mut out = Vec::with_capacity(args.len()); + out.push(args[0].clone()); + out.push(OsString::from("lint")); + out.extend(args[2..].iter().cloned()); + return out; + } _ => {} } } diff --git a/crates/omnigraph-cli/tests/cli.rs b/crates/omnigraph-cli/tests/cli.rs index 0f7e0be..6e5de37 100644 --- a/crates/omnigraph-cli/tests/cli.rs +++ b/crates/omnigraph-cli/tests/cli.rs @@ -714,6 +714,77 @@ query list_people() { ); } +/// Bare `omnigraph check` is NOT a clap `visible_alias` on `lint` (MR-981 §6: +/// visible aliases give agents two canonical names to emit interchangeably). +/// It's an argv-level shim: rewrites to `omnigraph lint`, prints a one-line +/// stderr deprecation warning, and produces identical stdout to the canonical +/// invocation. Cargo/Go users typing `check` keep working; help text shows +/// only `lint`. +#[test] +fn deprecated_check_top_level_rewrites_to_lint() { + let temp = tempdir().unwrap(); + let schema_path = temp.path().join("schema.pg"); + let query_path = temp.path().join("queries.gq"); + write_file( + &schema_path, + r#" +node Person { + name: String +} +"#, + ); + write_query_file( + &query_path, + r#" +query list_people() { + match { $p: Person } + return { $p.name } +} +"#, + ); + + let canonical = output_success( + cli() + .arg("lint") + .arg("--query") + .arg(&query_path) + .arg("--schema") + .arg(&schema_path) + .arg("--json"), + ); + let deprecated_check = output_success( + cli() + .arg("check") + .arg("--query") + .arg(&query_path) + .arg("--schema") + .arg(&schema_path) + .arg("--json"), + ); + + assert_eq!(stdout_string(&canonical), stdout_string(&deprecated_check)); + + let check_stderr = String::from_utf8(deprecated_check.stderr).unwrap(); + assert!( + check_stderr.contains("`omnigraph check` is deprecated") + && check_stderr.contains("`omnigraph lint`"), + "expected `omnigraph check` deprecation warning pointing at `omnigraph lint`; got: {check_stderr}" + ); + + // `check` must NOT appear in the canonical `omnigraph --help` output — + // agents copy the surface from help text and would otherwise emit both + // names interchangeably. + let help = cli().arg("--help").output().unwrap(); + let stdout = String::from_utf8(help.stdout).unwrap(); + let check_aliased = stdout + .lines() + .any(|line| line.trim_start().starts_with("lint") && line.contains("check")); + assert!( + !check_aliased, + "`check` must not be advertised as a visible alias of `lint`; help output: {stdout}" + ); +} + /// `omnigraph read` and `omnigraph change` are kept as visible clap /// aliases for the new canonical `query` / `mutate` subcommands, plus an /// argv-level deprecation warning. The warning is emitted to stderr; the diff --git a/docs/releases/v0.6.0.md b/docs/releases/v0.6.0.md index 12b3e22..7984056 100644 --- a/docs/releases/v0.6.0.md +++ b/docs/releases/v0.6.0.md @@ -35,7 +35,7 @@ Runtime add/remove (`POST /graphs`, `DELETE /graphs/{id}`, `omnigraph graphs cre ### Query / mutation rename - **`ChangeRequest` field rename**: `query_source` → `query`, `query_name` → `name`. Both legacy names continue to deserialize via `#[serde(alias = "...")]`, so existing clients sending the old JSON keys keep working. CLI remote calls against `/change` still emit the legacy keys verbatim through the `legacy_change_request_body` helper so a newer CLI talking to an older server keeps working byte-for-byte. -- **CLI `omnigraph query lint` / `omnigraph query check`** are now top-level `omnigraph lint` / `omnigraph check`. The legacy invocations remain as argv-level shims that rewrite to the canonical form and print a one-line deprecation warning to stderr. +- **CLI `omnigraph query lint` / `omnigraph query check`** are now top-level — canonical name is **`omnigraph lint`**. The three deprecated invocations (`omnigraph query lint`, `omnigraph query check`, and bare `omnigraph check`) remain as argv-level shims that rewrite to `omnigraph lint` and print a one-line stderr deprecation warning. `check` is deliberately **not** a clap `visible_alias` on `lint` — two equivalent canonical names would split agent emissions between them depending on training-data drift, so the deprecation pattern (rewrite + warn) gives one unambiguous canonical name in `omnigraph --help`. ## New