diff --git a/experimental/papyrus/server/services/silverwood.ts b/experimental/papyrus/server/services/silverwood.ts index e89b2e7..a46b830 100644 --- a/experimental/papyrus/server/services/silverwood.ts +++ b/experimental/papyrus/server/services/silverwood.ts @@ -111,14 +111,14 @@ export function create(params: { }): Promise { // Blocks on `jj git clone --colocate`; the checkout carries a pending→ready // state machine, so the returned workstream is the finished (ready/failed) one. + // Nested subcommands mirror the data model: `new `. return run([ "new", + "basic", + params.mode || "jj-colocated", + params.source, "--name", params.name, - "--source", - params.source, - "--mode", - params.mode || "jj-colocated", ]); } diff --git a/experimental/silverwood/TODO.md b/experimental/silverwood/TODO.md index c38df6d..949ad49 100644 --- a/experimental/silverwood/TODO.md +++ b/experimental/silverwood/TODO.md @@ -75,7 +75,9 @@ Part 2 notes: ## Part 3 — CLI surface (`--json`) — DONE - [x] **wrapped `packages.default` with `makeWrapper` (jujutsu + git on PATH)** — `packages.unwrapped` is the bare crane build; checks use unwrapped - [x] subcommands: `new`, `ls [--all]`, `show `, `archive `, `kv (set/get/ls/unset)`, `session (attach/ls/rename/detach)` -- [x] all inputs explicit (no defaults): `new --name --source --mode jj-colocated`; `--mode` is a clap ValueEnum (`ModeArg`, keeps clap out of core) +- [x] all inputs explicit (no defaults): the workstream variant → checkout mode → seed are nested subcommands mirroring the data + model — `new basic jj-colocated ` (and `…jj-colocated-direnv-unsafe `); `--name` is a + global option accepted at any `new` level; `silverwood modes` metadata is derived from the `new basic` subcommand tree - [x] dual output: human-readable default + `--json` (global flag); mutating cmds print the affected object after the change - [x] verify (structural): `nix flake check` green + `cli.rs` smoke tests (info json, ls empty=`[]`, bad-id exits non-zero) — no network needed - [x] verify (behavioral): drove the WRAPPED binary (`nix build .#default`) new→ls→kv→session→show→archive with a real clone of diff --git a/experimental/silverwood/crates/silverwood-cli/src/main.rs b/experimental/silverwood/crates/silverwood-cli/src/main.rs index 668becb..7b866e3 100644 --- a/experimental/silverwood/crates/silverwood-cli/src/main.rs +++ b/experimental/silverwood/crates/silverwood-cli/src/main.rs @@ -11,7 +11,7 @@ use std::path::PathBuf; use std::process::ExitCode; use std::str::FromStr; -use clap::{Parser, Subcommand, ValueEnum}; +use clap::{Parser, Subcommand}; use silverwood_core::{ agent_shell_plan, base_shell_plan, AgentKind, AgentSession, CheckoutState, Forest, HttpsGitUrl, LocationWithinForest, NewCheckoutMode, NewKind, NewWorkstream, SpawnSeed, UpgradeReport, @@ -42,15 +42,11 @@ enum Command { /// Create a workstream, provisioning its checkout. New { - /// Human-friendly name. - #[arg(long)] - name: String, - /// HTTPS git endpoint to clone from. - #[arg(long, value_name = "HTTPS_URL")] - source: String, - /// How the checkout is materialized. - #[arg(long, value_enum)] - mode: ModeArg, + /// Human-friendly name (accepted at any `new` subcommand level). + #[arg(long, global = true)] + name: Option, + #[command(subcommand)] + variant: NewVariant, }, /// List workstreams. @@ -95,7 +91,7 @@ enum Command { dry_run: bool, }, - /// List the available checkout modes for `new --mode`. + /// List the available checkout modes for `new basic `. Modes, /// Exec the interactive agent shell for a session in its checkout. The command @@ -229,71 +225,83 @@ enum SessionCreate { }, } -/// CLI mirror of the checkout-mode selector (keeps `clap` out of `silverwood-core`). -#[derive(Clone, Copy, ValueEnum)] -enum ModeArg { - #[value(name = "jj-colocated")] - JjColocated, - #[value(name = "jj-colocated-direnv-unsafe")] - JjColocatedDirenvUnsafe, +/// The workstream variant (kind) to create — the first `new` subcommand level. Only +/// `basic` today (`WorkstreamKind` is `#[non_exhaustive]`); the kebab name matches the +/// stored `kind` tag. +#[derive(Subcommand)] +enum NewVariant { + /// A basic workstream, materialized by a checkout mode. + Basic { + #[command(subcommand)] + mode: NewModeArg, + }, +} + +/// A `basic` workstream's checkout mode — the second `new` subcommand level. Each variant +/// carries that mode's creation seed as positionals; the kebab name matches the stored +/// `checkout_mode` tag. +#[derive(Subcommand)] +enum NewModeArg { + /// jj/git colocated clone (`jj git clone --colocate`). + JjColocated { + /// HTTPS git endpoint to clone from. + #[arg(value_name = "SOURCE_HTTPS_URL")] + source: String, + }, + /// jj-colocated clone, then `direnv allow` on the checkout (pre-approves .envrc; unsafe). + JjColocatedDirenvUnsafe { + /// HTTPS git endpoint to clone from. + #[arg(value_name = "SOURCE_HTTPS_URL")] + source: String, + }, +} + +impl NewModeArg { + /// Build the creation-side mode from the selector + its seed, validating the seed. + /// Consumes `self` (the seed `String` moves into the returned mode). + fn into_new_mode(self) -> Result> { + Ok(match self { + NewModeArg::JjColocated { source } => NewCheckoutMode::JjColocated { + initial_source: HttpsGitUrl::parse(&source)?, + }, + NewModeArg::JjColocatedDirenvUnsafe { source } => { + NewCheckoutMode::JjColocatedDirenvUnsafe { + initial_source: HttpsGitUrl::parse(&source)?, + } + } + }) + } } /// A checkout mode's metadata, for `silverwood modes` (drives a frontend picker). #[derive(serde::Serialize)] struct ModeInfo { - /// The kebab tag passed to `new --mode` (matches the stored `checkout_mode`). + /// The kebab tag naming the `new basic ` subcommand (matches the stored + /// `checkout_mode`). mode: String, /// One-line human description. - description: &'static str, - /// Whether this mode requires `--source` (all modes today do). + description: String, + /// Whether this mode takes a source seed (has a `source` positional). requires_source: bool, } -impl ModeArg { - /// Build the creation-side mode from the selector + its seed (the source url). - fn to_new_mode(self, initial_source: HttpsGitUrl) -> NewCheckoutMode { - match self { - ModeArg::JjColocated => NewCheckoutMode::JjColocated { initial_source }, - ModeArg::JjColocatedDirenvUnsafe => { - NewCheckoutMode::JjColocatedDirenvUnsafe { initial_source } - } - } - } - - /// One-line description of the mode (kept in sync with the variants by the - /// exhaustive match — a new variant forces filling this in). - fn description(self) -> &'static str { - match self { - ModeArg::JjColocated => "jj/git colocated clone (`jj git clone --colocate`).", - ModeArg::JjColocatedDirenvUnsafe => { - "jj-colocated clone, then `direnv allow` on the checkout (pre-approves .envrc; unsafe)." - } - } - } - - /// Whether `new --mode ` requires `--source` (all modes today do). - fn requires_source(self) -> bool { - match self { - ModeArg::JjColocated | ModeArg::JjColocatedDirenvUnsafe => true, - } - } - - /// Metadata for every mode, in declaration order. The `mode` tag is sourced - /// from clap's `#[value(name=...)]` so it stays the single source of truth. - fn all_infos() -> Vec { - Self::value_variants() - .iter() - .map(|m| ModeInfo { - mode: m - .to_possible_value() - .expect("no ModeArg variant is value-skipped") - .get_name() - .to_string(), - description: m.description(), - requires_source: m.requires_source(), - }) - .collect() - } +/// Metadata for every checkout mode, derived from the `new basic` subcommand tree so the +/// clap definitions stay the single source of truth (tag = subcommand name, description = +/// its help, `requires_source` = whether it has the `source` positional). +fn mode_infos() -> Vec { + let basic = NewVariant::augment_subcommands(clap::Command::new("new")); + basic + .find_subcommand("basic") + .expect("basic subcommand exists") + .get_subcommands() + .map(|sc| ModeInfo { + mode: sc.get_name().to_string(), + description: sc.get_about().map(|a| a.to_string()).unwrap_or_default(), + requires_source: sc + .get_positionals() + .any(|a| a.get_id().as_str() == "source"), + }) + .collect() } fn main() -> ExitCode { @@ -319,7 +327,7 @@ fn run(cli: Cli) -> CliResult { // `modes` is pure metadata — handle it before opening (and thereby creating) // the forest, so listing modes never touches `$HOME/.silverwood`. if let Command::Modes = cli.command { - let modes = ModeArg::all_infos(); + let modes = mode_infos(); emit(json, &modes, || { for m in &modes { println!("{:28} {}", m.mode, m.description); @@ -333,17 +341,7 @@ fn run(cli: Cli) -> CliResult { match cli.command { Command::Info => info(&forest, json), - Command::New { name, source, mode } => { - let source = HttpsGitUrl::parse(&source)?; - let ws = forest.create_workstream(NewWorkstream { - name, - kind: NewKind::Basic { - mode: mode.to_new_mode(source), - }, - })?; - emit(json, &ws, || print_workstream(&ws)); - Ok(()) - } + Command::New { name, variant } => run_new(&forest, json, name, variant), Command::Ls { all } => { let list = forest.list(all)?; @@ -402,6 +400,19 @@ fn run(cli: Cli) -> CliResult { } } +fn run_new(forest: &Forest, json: bool, name: Option, variant: NewVariant) -> CliResult { + let name = name.ok_or("`--name ` is required")?; + let NewVariant::Basic { mode } = variant; + let ws = forest.create_workstream(NewWorkstream { + name, + kind: NewKind::Basic { + mode: mode.into_new_mode()?, + }, + })?; + emit(json, &ws, || print_workstream(&ws)); + Ok(()) +} + fn run_kv(forest: &Forest, json: bool, cmd: KvCommand) -> CliResult { match cmd { KvCommand::Set { diff --git a/experimental/silverwood/crates/silverwood-cli/tests/cli.rs b/experimental/silverwood/crates/silverwood-cli/tests/cli.rs index 0b20d4b..a67388c 100644 --- a/experimental/silverwood/crates/silverwood-cli/tests/cli.rs +++ b/experimental/silverwood/crates/silverwood-cli/tests/cli.rs @@ -65,24 +65,22 @@ fn new_with_non_https_source_is_rejected_without_cloning() { &dir, &[ "new", + "basic", + "jj-colocated", + "git@github.com:a/b.git", "--name", "x", - "--source", - "git@github.com:a/b.git", - "--mode", - "jj-colocated", ], ); fails( &dir, &[ "new", + "basic", + "jj-colocated", + "http://github.com/a/b.git", "--name", "x", - "--source", - "http://github.com/a/b.git", - "--mode", - "jj-colocated", ], ); @@ -90,6 +88,46 @@ fn new_with_non_https_source_is_rejected_without_cloning() { assert_eq!(json(&dir, &["--json", "ls"]), serde_json::json!([])); } +#[test] +fn new_subcommand_structure_mirrors_the_data_model() { + let dir = forest(); + + // Each level demands its child: `new` needs a variant, `basic` needs a mode. + // Both are clap parse errors (exit before the forest is touched). + fails(&dir, &["new"]); + fails(&dir, &["new", "basic"]); + + // A mode leaf demands its seed: the `` positional. + let stderr = fails(&dir, &["new", "basic", "jj-colocated"]); + assert!(stderr.contains("SOURCE_HTTPS_URL"), "got: {stderr}"); + + // `--name` is required (enforced in the handler); omitting it fails before the + // source is ever validated. + let stderr = fails( + &dir, + &["new", "basic", "jj-colocated", "http://github.com/a/b.git"], + ); + assert!(stderr.contains("--name"), "got: {stderr}"); + + // `--name` is global: placing it at the `new` level (before the mode) parses, + // and the command then reaches source validation (rejecting the non-https url). + let stderr = fails( + &dir, + &[ + "new", + "--name", + "x", + "basic", + "jj-colocated", + "http://github.com/a/b.git", + ], + ); + assert!(stderr.contains("scheme must be https"), "got: {stderr}"); + + // None of the above created anything. + assert_eq!(json(&dir, &["--json", "ls"]), serde_json::json!([])); +} + #[test] fn show_rejects_bad_and_absent_ids() { let dir = forest(); diff --git a/experimental/silverwood/crates/silverwood-cli/tests/common/mod.rs b/experimental/silverwood/crates/silverwood-cli/tests/common/mod.rs index 5958569..2f57ab5 100644 --- a/experimental/silverwood/crates/silverwood-cli/tests/common/mod.rs +++ b/experimental/silverwood/crates/silverwood-cli/tests/common/mod.rs @@ -65,12 +65,11 @@ pub fn create(dir: &TempDir, name: &str) -> String { &[ "--json", "new", + "basic", + "jj-colocated", + EXAMPLE_SOURCE, "--name", name, - "--source", - EXAMPLE_SOURCE, - "--mode", - "jj-colocated", ], ); value["id"].as_str().expect("id field").to_string() diff --git a/experimental/silverwood/crates/silverwood-cli/tests/e2e.rs b/experimental/silverwood/crates/silverwood-cli/tests/e2e.rs index 6230ca8..999634a 100644 --- a/experimental/silverwood/crates/silverwood-cli/tests/e2e.rs +++ b/experimental/silverwood/crates/silverwood-cli/tests/e2e.rs @@ -50,12 +50,11 @@ fn new_creates_a_ready_colocated_checkout() { &[ "--json", "new", + "basic", + "jj-colocated", + EXAMPLE_SOURCE, "--name", "auth-refactor", - "--source", - EXAMPLE_SOURCE, - "--mode", - "jj-colocated", ], ); @@ -109,12 +108,11 @@ fn new_direnv_unsafe_mode_is_ready() { &[ "--json", "new", + "basic", + "jj-colocated-direnv-unsafe", + EXAMPLE_SOURCE, "--name", "with-direnv", - "--source", - EXAMPLE_SOURCE, - "--mode", - "jj-colocated-direnv-unsafe", ], ); assert_eq!(ws["mode"]["checkout_mode"], "jj-colocated-direnv-unsafe"); @@ -139,12 +137,11 @@ fn spawn_plan_reflects_checkout_mode() { &[ "--json", "new", + "basic", + "jj-colocated", + EXAMPLE_SOURCE, "--name", "plain", - "--source", - EXAMPLE_SOURCE, - "--mode", - "jj-colocated", ], ); let id = ws["id"].as_str().unwrap(); @@ -168,12 +165,11 @@ fn spawn_plan_reflects_checkout_mode() { &[ "--json", "new", + "basic", + "jj-colocated-direnv-unsafe", + EXAMPLE_SOURCE, "--name", "with-direnv", - "--source", - EXAMPLE_SOURCE, - "--mode", - "jj-colocated-direnv-unsafe", ], ); let id2 = ws2["id"].as_str().unwrap(); @@ -345,12 +341,11 @@ fn archive_tombstones_but_keeps_checkout_and_persists() { &[ "--json", "new", + "basic", + "jj-colocated", + EXAMPLE_SOURCE, "--name", "w", - "--source", - EXAMPLE_SOURCE, - "--mode", - "jj-colocated", ], ); let id = ws["id"].as_str().unwrap().to_string();