Skip to content

Commit c100743

Browse files
refactor(cli): delete GlobalArgs and the dead ops descriptor spec column (#10554)
* refactor(cli): delete the fieldless GlobalArgs handler parameter `GlobalArgs` was a zero-field struct with no derives and no impls, built from nothing at dispatch time (`GlobalArgs {}`) and threaded by reference through 98 function signatures across 67 files, two function-type aliases (`JsonCommandExecutor`, `BoundCommandAdapter::run`), one function-pointer field (`ReviewStageDescriptor::run`), and ~140 call sites. It never participated in argument parsing: it does not derive `clap::Args`, is never a struct field, never appears in a `#[command(flatten)]`, and is never referenced from `cli_surface::Cli`. Real process-wide CLI state lives on `Cli` as `global = true` clap arguments. Removing the type therefore cannot change the rendered `--help` surface or the accepted argv surface. Adds `root_global_flag_surface_is_pinned` to lock the globally-propagated flag set, because that set *is* rendered help and `homeboy-lab-runner` negotiates its lease-less recovery contract by parsing a remote binary's rendered `Options:` block. Refs #10322 * refactor(cli): drop the dead spec column from ops_command_descriptors `ops_command_descriptors!` is expanded exactly twice: `commands::register_ops_command_modules` (uses $module) and `json_output::ops::registered_ops_dispatch` (uses $variant and $handler). Neither consumer ever referenced $args or $spec, so the fifteen `CommandSpec` expressions duplicated from `ops_command_spec!` expanded to no code at all — they were pure maintenance load, requiring every safety-metadata edit to be made in two places with nothing enforcing the pairing. Delete both dead columns. Each row is now `(module, Variant, handler)`, the ops `CommandSpec` lives once in `ops_command_spec!`, and the descriptor table fits on readable lines — which matters because rustfmt does not format `macro_rules!` bodies. Adds `every_ops_descriptor_is_registered_in_the_ops_json_family`, which asserts the invariant the deleted column only ever documented. Refs #10323 * style(cli): rustfmt the GlobalArgs edits cargo fmt cannot reach `cargo fmt --all` does not visit the fifteen ops command modules. They are declared by `$(pub mod $module;)*` inside `register_ops_command_modules!`, and rustfmt does not expand macros, so `commands/{daemon,deploy,logs,schedule, ssh,status,triage,upgrade,...}.rs` and everything beneath them — including the `#[cfg(test)] #[path = "../../tests/..."]` modules they pull in — are invisible to the formatter. Verified empirically: appending `fn __fmt_probe(){let x=1;}` to daemon.rs survives `cargo fmt --all` unformatted. Consequence: those files carry pre-existing formatting drift (daemon.rs 11 blocks, deploy.rs 14, ssh.rs 3, triage.rs 3, status/mod.rs 5, ...) on main today. This commit does not sweep that up. It only rustfmts the call sites the GlobalArgs removal reflowed, so per-file `rustfmt --check` block counts are <= main everywhere and this branch adds no new drift. Refs #10322 * fix(cli): drop five forwarded _global arguments the sweep missed CI caught five call sites that forwarded the parameter by its underscore-prefixed name: contract/mod.rs:431, refactor.rs:455 and :460, runner/dispatch.rs:466, and self_cmd.rs:147. The underscore prefix only suppresses the unused-variable warning; it does not make the binding unusable, and these five handlers genuinely read and forwarded it. My verification grep matched the word `global`, which does not match `_global` — so the sweep left the definitions correct and the callers dangling (E0425 + E0061). Refs #10322 * fix(cli): clear the two audit findings the GlobalArgs sweep introduced The differential audit gate rejected the branch at current=21 base=20. Reproduced locally with `homeboy review audit --changed-since origin/main`, which named both: - `structural::cli_surface/mod.rs::GodFile` — main sits at 1490 lines against a 1500 threshold, so the 47-line pinning test tripped it. Moved the test to its own `cli_surface/global_flag_surface_tests.rs`; `mod.rs` is now main plus three lines. Deliberately not splitting the existing 679-line inline `mod tests` block: doing so just moves the finding to `HighItemCount` (35 top-level items against a threshold of 30), which I verified, and a proper `cli_surface/tests/` split is a separate change with real conflict surface. - `Commands::commands/deploy.rs::SignatureMismatch` — removing the parameter left `pub fn run(\n mut args: DeployArgs,\n)` wrapped across three lines, which the signature normalizer read as a 4-token signature. Collapsed to one line, which is what rustfmt wants anyway and cannot do here (deploy.rs is macro-declared, so `cargo fmt` never reaches it). `homeboy review audit` now reports zero unbaselined findings. Refs #10322 --------- Co-authored-by: chubes-bot <266378653+homeboy-ci[bot]@users.noreply.github.com>
1 parent 884b4ac commit c100743

94 files changed

Lines changed: 1637 additions & 1825 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

crates/homeboy-cli/src/cli_runtime.rs

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ use crate::commands;
1414
use crate::commands::cli;
1515
use crate::commands::output_runtime;
1616
use crate::commands::utils::{args, entity_suggest, resource_policy, response as output};
17-
use crate::commands::GlobalArgs;
1817
use homeboy::extension::{
1918
list_summaries, load_all_extensions, CliConfig,
2019
ExtensionManifest as InstalledExtensionManifest, ExtensionSummary,
@@ -359,7 +358,6 @@ impl CliRuntime {
359358
}
360359

361360
fn run_matches(&self, matches: ArgMatches, normalized: Vec<String>) -> std::process::ExitCode {
362-
let global = GlobalArgs {};
363361
let command_identity = command_identity_from_matches(&matches);
364362

365363
// Extract --output early so it's available for all code paths (including
@@ -389,7 +387,7 @@ impl CliRuntime {
389387
identifier: extension_cmd.project_id,
390388
args: extension_cmd.args,
391389
};
392-
let result = cli::run(cli_args, &global);
390+
let result = cli::run(cli_args);
393391

394392
let (json_result, exit_code) = output::map_cmd_result_to_json(result);
395393
output_runtime::emit_json_result_for_identity(
@@ -567,7 +565,6 @@ impl CliRuntime {
567565
commands::output_runtime::run_command(
568566
cli.command,
569567
command_spec,
570-
&global,
571568
output_file.as_deref(),
572569
&command_identity,
573570
)
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
//! Pins the globally-propagated clap argument surface.
2+
//!
3+
//! Lives beside `cli_surface/mod.rs` rather than inside its `mod tests` block
4+
//! because that file sits 7 lines under the audit's 1500-line `god_file`
5+
//! threshold; anything added inline trips it.
6+
7+
use super::Cli;
8+
use clap::CommandFactory;
9+
10+
/// The globally-propagated flag set is a wire protocol, not just documentation:
11+
/// `homeboy-lab-runner` negotiates the lease-less recovery contract by parsing
12+
/// a remote binary's rendered `Options:` block
13+
/// (`negotiate_leaseless_recovery_contract`), and every `global = true`
14+
/// argument is rendered into that block for every subcommand. Pin the set so
15+
/// any addition or removal is a deliberate, reviewed protocol change.
16+
///
17+
/// This also anchors the invariant that motivated deleting the fieldless
18+
/// `GlobalArgs` handler parameter: process-wide CLI state is carried by these
19+
/// clap arguments on `Cli`, never by a separate struct threaded through
20+
/// command handlers.
21+
#[test]
22+
fn root_global_flag_surface_is_pinned() {
23+
let mut longs: Vec<String> = Cli::command()
24+
.get_arguments()
25+
.filter(|arg| arg.is_global_set())
26+
.filter_map(|arg| arg.get_long())
27+
// `help`/`version` are clap-generated; this pins Homeboy-declared
28+
// globals only.
29+
.filter(|long| !matches!(*long, "help" | "version"))
30+
.map(str::to_string)
31+
.collect();
32+
longs.sort();
33+
34+
assert_eq!(
35+
longs.iter().map(String::as_str).collect::<Vec<_>>(),
36+
vec![
37+
"allow-dirty-lab-workspace",
38+
"artifact-root",
39+
"detach-after-handoff",
40+
"lab-env-json",
41+
"notification-route",
42+
"notification-transport",
43+
"output",
44+
"placement",
45+
"preserve-workspace-on-failure",
46+
"runner",
47+
"runner-env",
48+
"runner-workspace-root",
49+
"skip-deps-hydration",
50+
"wait",
51+
],
52+
"the globally-propagated flag surface changed; update remote \
53+
capability negotiation and docs before accepting this",
54+
);
55+
}

crates/homeboy-cli/src/cli_surface/mod.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1488,3 +1488,6 @@ mod tests {
14881488
}
14891489
}
14901490
}
1491+
1492+
#[cfg(test)]
1493+
mod global_flag_surface_tests;

crates/homeboy-cli/src/command_contract/descriptors.rs

Lines changed: 41 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2,39 +2,56 @@
22
33
/// Expands the Ops command descriptors into a consumer macro.
44
///
5-
/// Each row owns the command module, parsed Clap variant, contract metadata, and
6-
/// JSON handler binding. Consumers select the fields they need while preserving
7-
/// command-owned dynamic output and Lab predicates.
5+
/// Each row binds a command module to its parsed Clap variant and JSON handler.
6+
/// That is the whole descriptor: module registration and JSON dispatch are the
7+
/// only two things a `Commands` variant needs from this table.
8+
///
9+
/// Contract metadata (a `CommandSpec`) is deliberately **not** a column here. It
10+
/// lives once, in [`ops_command_spec`], because `spec.rs` splices the ops rows
11+
/// into a hand-ordered `COMMANDS` array at fifteen non-contiguous positions —
12+
/// a shape this block-emitting macro cannot produce. Carrying a second copy of
13+
/// the spec expressions in this table bought nothing: neither consumer ever
14+
/// referenced it, so the copies expanded to no code while still requiring every
15+
/// safety-metadata edit to be made twice.
16+
///
17+
/// Registry/parser parity is enforced by
18+
/// `cli_surface::tests::command_registry_manifest_and_docs_metadata_align`,
19+
/// which asserts a bijection between the clap subcommand set and
20+
/// `COMMAND_SPECS`. That guard is stronger than macro co-location: it catches a
21+
/// missing spec for *any* command, not just the ops family.
822
#[macro_export]
923
macro_rules! ops_command_descriptors {
1024
($consumer:ident) => {
1125
$consumer! {
12-
(ssh, Ssh, crate::commands::ssh::SshArgs, command_spec("ssh", CommandJsonFamily::Ops), crate::commands::ssh::run),
13-
(server, Server, crate::commands::server::ServerArgs, CommandSpec { subcommand_safety: SERVER_SUBCOMMAND_SAFETY, ..command_spec("server", CommandJsonFamily::Ops) }, crate::commands::server::run),
14-
(db, Db, crate::commands::db::DbArgs, CommandSpec { subcommand_safety: DB_SUBCOMMAND_SAFETY, ..command_spec("db", CommandJsonFamily::Ops) }, crate::commands::db::run),
15-
(file, File, crate::commands::file::FileArgs, CommandSpec { subcommand_safety: FILE_SUBCOMMAND_SAFETY, ..command_spec("file", CommandJsonFamily::Ops) }, crate::commands::file::run),
16-
(logs, Logs, crate::commands::logs::LogsArgs, command_spec("logs", CommandJsonFamily::Ops), crate::commands::logs::run),
17-
(triage, Triage, crate::commands::triage::TriageArgs, command_spec_with_safety("triage", CommandJsonFamily::Ops, operator_safety(None, TRIAGE_DANGEROUS_FLAGS)), crate::commands::triage::run),
18-
(deploy, Deploy, crate::commands::deploy::DeployArgs, command_spec_with_safety("deploy", CommandJsonFamily::Ops, operator_safety(Some("--dry-run"), DEPLOY_DANGEROUS_FLAGS)), crate::commands::deploy::run),
19-
(harvest, Harvest, crate::commands::harvest::HarvestArgs, command_spec_with_safety("harvest", CommandJsonFamily::Ops, operator_safety(Some("--dry-run"), &["--apply"])), crate::commands::harvest::run),
20-
(daemon, Daemon, crate::commands::daemon::DaemonArgs, command_spec("daemon", CommandJsonFamily::Ops), crate::commands::daemon::run),
21-
(schedule, Schedule, crate::commands::schedule::ScheduleArgs, command_spec("schedule", CommandJsonFamily::Ops), crate::commands::schedule::run),
22-
(status, Status, crate::commands::status::StatusArgs, command_spec("status", CommandJsonFamily::Ops), crate::commands::status::run),
23-
(git, Git, crate::commands::git::GitArgs, CommandSpec { subcommand_safety: GIT_SUBCOMMAND_SAFETY, ..command_spec("git", CommandJsonFamily::Ops) }, crate::commands::git::run),
24-
(self_cmd, SelfCmd, crate::commands::self_cmd::SelfArgs, CommandSpec { subcommand_safety: SELF_SUBCOMMAND_SAFETY, ..command_spec_with_output_notes("self", CommandJsonFamily::Ops, "inspects the active Homeboy runtime and renders built-in CLI documentation") }, crate::commands::self_cmd::run),
25-
(api, Api, crate::commands::api::ApiArgs, CommandSpec { subcommand_safety: API_SUBCOMMAND_SAFETY, ..command_spec("api", CommandJsonFamily::Ops) }, crate::commands::api::run),
26-
(upgrade, Upgrade, crate::commands::upgrade::UpgradeArgs, command_spec_with_output_notes_and_safety("upgrade", CommandJsonFamily::Ops, "upgrades the active Homeboy binary, extensions, runners, and services unless --check or skip flags are used", operator_safety(None, UPGRADE_DANGEROUS_FLAGS)), crate::commands::upgrade::run),
26+
(ssh, Ssh, crate::commands::ssh::run),
27+
(server, Server, crate::commands::server::run),
28+
(db, Db, crate::commands::db::run),
29+
(file, File, crate::commands::file::run),
30+
(logs, Logs, crate::commands::logs::run),
31+
(triage, Triage, crate::commands::triage::run),
32+
(deploy, Deploy, crate::commands::deploy::run),
33+
(harvest, Harvest, crate::commands::harvest::run),
34+
(daemon, Daemon, crate::commands::daemon::run),
35+
(schedule, Schedule, crate::commands::schedule::run),
36+
(status, Status, crate::commands::status::run),
37+
(git, Git, crate::commands::git::run),
38+
(self_cmd, SelfCmd, crate::commands::self_cmd::run),
39+
(api, Api, crate::commands::api::run),
40+
(upgrade, Upgrade, crate::commands::upgrade::run),
2741
}
2842
};
2943
}
3044

31-
/// Commands-free spec table for the ops command family.
45+
/// Canonical `CommandSpec` table for the ops command family.
46+
///
47+
/// This is the single source of truth for ops contract metadata. It is expanded
48+
/// inside `command_contract` (`spec.rs`), which cannot name `crate::commands`
49+
/// types, so it deliberately holds no Args type or handler binding — those live
50+
/// in [`ops_command_descriptors`], which is expanded only on the CLI side.
3251
///
33-
/// This mirrors the `$spec` field of [`ops_command_descriptors`] but omits the
34-
/// `crate::commands` Args type and handler binding, so it can be expanded inside
35-
/// `command_contract` (e.g. `spec.rs`) without depending on the `commands`
36-
/// module. The full descriptor macro (with Args + handler) is expanded only on
37-
/// the CLI side.
52+
/// The per-name arms exist because `spec.rs` interleaves these rows with
53+
/// non-ops entries in one hand-ordered array; a single block-emitting arm could
54+
/// not be spliced into those positions.
3855
#[macro_export]
3956
macro_rules! ops_command_spec {
4057
(ssh) => { command_spec("ssh", CommandJsonFamily::Ops) };

crates/homeboy-cli/src/commands/activity.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use super::utils::response::{
1616
CommandActionableMetadata, CommandAgentTaskRef, CommandJobRef, CommandNextAction,
1717
CommandNextActionKind, CommandResultRefs, CommandRunRef,
1818
};
19-
use super::{CmdResult, GlobalArgs};
19+
use super::CmdResult;
2020

2121
const TIMEOUT_EXIT_CODE: i32 = 124;
2222

@@ -101,7 +101,7 @@ pub struct ActivityWatchOutput {
101101
pub notify: Option<NotifyOutcome>,
102102
}
103103

104-
pub fn run(args: ActivityArgs, _global: &GlobalArgs) -> CmdResult<ActivityOutput> {
104+
pub fn run(args: ActivityArgs) -> CmdResult<ActivityOutput> {
105105
match args
106106
.command
107107
.unwrap_or(ActivityCommand::List(ActivityListArgs {

crates/homeboy-cli/src/commands/agent_task.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
use serde::Serialize;
88
use serde_json::Value;
99

10-
use super::{CmdResult, GlobalArgs};
10+
use super::CmdResult;
1111

1212
pub mod args;
1313
pub mod auth;
@@ -44,7 +44,7 @@ pub use args::{
4444
};
4545
pub(crate) use status::diagnostic_summary_from_aggregate;
4646

47-
pub fn run(args: AgentTaskArgs, _global: &GlobalArgs) -> CmdResult<Value> {
47+
pub fn run(args: AgentTaskArgs) -> CmdResult<Value> {
4848
// Announce durable identity exactly once, on the first progress event that
4949
// carries a run id, and do it outside the TTY gate. Phase chatter stays
5050
// TTY-gated so non-interactive logs are not spammed, but the operator

crates/homeboy-cli/src/commands/api/auth.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use homeboy::core::server::auth_profiles::{
1010
};
1111

1212
use crate::commands::utils::tty::{prompt, prompt_password};
13-
use crate::commands::{CmdResult, GlobalArgs};
13+
use crate::commands::CmdResult;
1414

1515
#[derive(Args)]
1616
pub struct AuthArgs {
@@ -146,7 +146,7 @@ pub enum AuthOutput {
146146
ProfileRemove(ProfileRemoveResult),
147147
}
148148

149-
pub fn run(args: AuthArgs, _global: &GlobalArgs) -> CmdResult<AuthOutput> {
149+
pub fn run(args: AuthArgs) -> CmdResult<AuthOutput> {
150150
match args.command {
151151
AuthCommand::Login {
152152
project,

crates/homeboy-cli/src/commands/api/http.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use clap::{Args, Subcommand};
22

33
use homeboy::core::http_request::{self, HttpRequestInput, HttpRequestOutput};
44

5-
use crate::commands::{parse_key_val, CmdResult, GlobalArgs};
5+
use crate::commands::{parse_key_val, CmdResult};
66

77
#[derive(Args)]
88
pub struct HttpArgs {
@@ -54,7 +54,7 @@ pub(crate) struct RequestArgs {
5454
form: Vec<(String, String)>,
5555
}
5656

57-
pub fn run(args: HttpArgs, _global: &GlobalArgs) -> CmdResult<HttpRequestOutput> {
57+
pub fn run(args: HttpArgs) -> CmdResult<HttpRequestOutput> {
5858
let input = match args.command {
5959
HttpCommand::Get(args) => build_input("GET", args),
6060
HttpCommand::Request {

crates/homeboy-cli/src/commands/api/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,10 @@ pub enum ApiCommandOutput {
9393
Http(homeboy::core::http_request::HttpRequestOutput),
9494
}
9595

96-
pub fn run(args: ApiArgs, global: &crate::commands::GlobalArgs) -> CmdResult<ApiCommandOutput> {
96+
pub fn run(args: ApiArgs) -> CmdResult<ApiCommandOutput> {
9797
match args.command {
98-
ApiCommand::Auth(args) => map_nested(auth::run(args, global), ApiCommandOutput::Auth),
99-
ApiCommand::Http(args) => map_nested(http::run(args, global), ApiCommandOutput::Http),
98+
ApiCommand::Auth(args) => map_nested(auth::run(args), ApiCommandOutput::Auth),
99+
ApiCommand::Http(args) => map_nested(http::run(args), ApiCommandOutput::Http),
100100
command => run_project(ApiArgs { command })
101101
.map(|(output, code)| (ApiCommandOutput::Project(output), code)),
102102
}

crates/homeboy-cli/src/commands/artifact_postprocess.rs

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use homeboy::core::artifacts::{
66
};
77
use serde::Serialize;
88

9-
use super::{CmdResult, GlobalArgs};
9+
use super::CmdResult;
1010

1111
#[derive(Args, Clone)]
1212
pub struct ArtifactPostprocessArgs {
@@ -37,10 +37,7 @@ pub struct ArtifactPostprocessCommandOutput {
3737
pub result: homeboy::core::artifacts::ArtifactPostprocessResult,
3838
}
3939

40-
pub fn run(
41-
args: ArtifactPostprocessArgs,
42-
_global: &GlobalArgs,
43-
) -> CmdResult<ArtifactPostprocessCommandOutput> {
40+
pub fn run(args: ArtifactPostprocessArgs) -> CmdResult<ArtifactPostprocessCommandOutput> {
4441
let raw = homeboy::core::config::read_json_spec_to_string(&args.plan)?;
4542
let plan: ArtifactPostprocessPlan = serde_json::from_str(&raw).map_err(|error| {
4643
homeboy::core::Error::validation_invalid_json(

0 commit comments

Comments
 (0)