diff --git a/AGENTS.md b/AGENTS.md index 394ebb4..fe224ee 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,6 +13,18 @@ messages. Scope and non-scope are in [README.md](README.md). There is no central server, no owner, no registration and no majority vote. Design accordingly: a majority is not a root of trust. +## User experience + +User convenience is a core requirement. The CLI should feel logical and +predictable to an experienced administrator: use consistent selectors and +flags, sensible defaults, and actionable errors. Let users refer to configured +networks by their exact name as well as by id or a unique id prefix; do not +make them copy an opaque identifier when the stored name identifies one network. +When a selector is ambiguous, list the matching names and ids and change +nothing. Never guess which resource a destructive command should affect. +Keep help text and examples in sync with behavior, and preserve existing +scripts when improving the interactive workflow. + ## Architectural boundaries Keep these separate. Crossing them is the main thing to review for. diff --git a/README.md b/README.md index ccf7efa..a997ddf 100644 --- a/README.md +++ b/README.md @@ -362,11 +362,11 @@ tsunagi id key rotate replace that key tsunagi network the networks this device belongs to tsunagi network join -n lab -s tsn1… join one; adds it to a running agent tsunagi network join -n lab resume one this device has, or make it on the spot -tsunagi network stop stop serving it, keeping everything -tsunagi network start serve it again, from where it left off -tsunagi network leave give up the address and name, then forget it +tsunagi network stop lab stop serving it, keeping everything +tsunagi network start lab serve it again, from where it left off +tsunagi network leave lab give up the address and name, then forget it tsunagi network secret the secret of each joined network -tsunagi network secret just that one, for copying +tsunagi network secret lab just that one, for copying tsunagi network secret generate a fresh secret for a network that does not exist yet tsunagi dns whether the local resolver is serving, and what @@ -599,8 +599,8 @@ received broadcasts never trigger another fanout. ```sh tsunagi join -n games --no-broadcast tsunagi join -n games --broadcast -tsunagi network broadcast off -tsunagi network broadcast on +tsunagi network broadcast games off +tsunagi network broadcast games on ``` The choice persists across restarts and a plain `join`. It can be changed while @@ -674,7 +674,7 @@ Membership outlives a session, so it also has to be possible to end it. ```bash tsunagi network # what this device belongs to -tsunagi network leave # give up the address and the name, then forget it +tsunagi network leave lab # give up the address and the name, then forget it tsunagi wipe --yes # remove everything and be a stranger again ``` @@ -703,9 +703,12 @@ nothing can sign or send it: the command says so and refuses, and `--offline` drops the network locally while leaving the others holding the old claim. The protocol key for that network goes too — rejoining is joining, not resuming. -A network is named by its id, never by its name: two networks can share a -name, and choosing between them for the user is how the wrong one gets left. A -unique prefix is enough. +`leave`, `stop`, `start`, `secret` and `broadcast` accept an exact stored network +name, its full id, or a unique id prefix (including a copied report ellipsis). +Names are case-sensitive. If several networks share a name, or a name matches +another network's id prefix, the command lists matching names and full ids and +changes nothing. Use a full id to choose explicitly; a full id always takes +precedence over a name. No secret is needed to select a configured network. `wipe` removes both directories' contents: the device identity, every network, every signed record and everything a protocol kept beside them. It refuses diff --git a/crates/tsunagi-cli/src/main.rs b/crates/tsunagi-cli/src/main.rs index 455f3fa..6f74cb7 100644 --- a/crates/tsunagi-cli/src/main.rs +++ b/crates/tsunagi-cli/src/main.rs @@ -147,7 +147,7 @@ struct JoinArgs { enum NetworkAction { /// Shows or changes this network's local LAN broadcast participation. Broadcast { - /// Network id or unique id prefix. + /// Exact network name, full id or unique id prefix. network: String, /// New choice; omit to show the stored choice. #[arg(value_enum)] @@ -168,12 +168,12 @@ enum NetworkAction { /// simply away, as if it had been switched off. It stays stopped /// across restarts until `tsunagi network start`. Stop { - /// Which network, by id. A unique prefix is enough. + /// Exact network name, full id or unique id prefix. network: String, }, /// Serves a stopped network again, from where it left off. Start { - /// Which network, by id. A unique prefix is enough. + /// Exact network name, full id or unique id prefix. network: String, }, /// Gives up this device's address and name in a network, and forgets it. @@ -186,8 +186,8 @@ enum NetworkAction { /// network's signed records, its cached hints and the protocol key it /// used. `stop` is the one that keeps them. Leave { - /// Which network, by id. A unique prefix is enough; the name is not, - /// because two networks may share one. + /// Exact network name, full id or unique id prefix. + /// If the name is shared, use the id to select one network. network: String, /// Remove it without telling anybody. @@ -202,8 +202,7 @@ enum NetworkAction { /// Printed only when asked for, never as part of an overview: these /// reports get pasted into chats and issue trackers. Secret { - /// Which network, by id; a unique prefix is enough. All of them if - /// omitted. + /// Exact network name, full id or unique id prefix. All of them if omitted. network: Option, #[command(subcommand)] @@ -1498,44 +1497,51 @@ fn stored_networks(paths: &StoragePaths) -> Vec .unwrap_or_default() } -/// Finds the one configured network whose id starts with `wanted`. +/// Resolves a stored network by exact name, full id or unique id prefix. /// -/// A prefix, because the ids are 52 characters and `status` prints them -/// shortened; the name is deliberately not accepted, since two networks can -/// share one and choosing for the user is how the wrong network gets left. +/// Full ids are definitive. Otherwise name and prefix matches must identify +/// exactly one network: a shared name or a name colliding with another id's +/// prefix must never select an arbitrary network for a destructive command. fn resolve_network<'a>( networks: &'a [tsunagi::storage::StoredNetwork], wanted: &str, ) -> Result<&'a tsunagi::storage::StoredNetwork, String> { - let wanted = wanted.trim().trim_end_matches('…'); + let wanted = wanted.trim_matches(|c: char| c.is_ascii_whitespace()); if wanted.is_empty() { - return Err("name a network by its id; `tsunagi network` lists them".to_string()); + return Err("select a network by name or id; `tsunagi network` lists them".to_string()); } + if let Some(network) = networks + .iter() + .find(|network| network.network_id.to_string() == wanted) + { + return Ok(network); + } + // Only id prefixes lose a copied report ellipsis; names are used verbatim. + let prefix = wanted.trim_end_matches('…'); let matched: Vec<&tsunagi::storage::StoredNetwork> = networks .iter() - .filter(|network| network.network_id.to_string().starts_with(wanted)) + .filter(|network| { + network.name.as_str() == wanted + || (!prefix.is_empty() && network.network_id.to_string().starts_with(prefix)) + }) .collect(); match matched.as_slice() { [one] => Ok(one), - [] => { - if networks + [] => Err(format!( + "no configured network matches name or id `{wanted}`; \ + `tsunagi network` lists them" + )), + several => { + let candidates = several .iter() - .any(|network| network.name.as_str() == wanted) - { - return Err(format!( - "`{wanted}` is a network name, not an id. Two networks can share a name, \ - so this takes the id; `tsunagi network` lists them." - )); - } + .map(|network| format!(" `{}` {}", network.name, network.network_id)) + .collect::>() + .join("\n"); Err(format!( - "no configured network has an id starting `{wanted}`; \ - `tsunagi network` lists them" + "`{wanted}` matches {} networks; use the full id or a unique id prefix:\n{candidates}", + several.len() )) } - several => Err(format!( - "`{wanted}` matches {} networks; use more of the id", - several.len() - )), } } @@ -4320,21 +4326,68 @@ mod network_tests { assert_eq!(picked.network_id, networks[0].network_id); let err = resolve_network(&networks, "").unwrap_err(); - assert!(err.contains("by its id"), "{err}"); + assert!(err.contains("by name or id"), "{err}"); } #[test] - fn a_name_is_refused_because_two_networks_can_share_one() { + fn a_unique_exact_name_selects_the_stored_network_without_normalizing_it() { + let networks = vec![ + configured("lab", "one"), + configured("Lab", "two"), + configured("Игры дома…", "three"), + ]; + for network in &networks { + assert_eq!( + resolve_network(&networks, network.name.as_str()) + .unwrap() + .network_id, + network.network_id + ); + } + assert!(resolve_network(&networks, "LAB").is_err()); + assert!(resolve_network(&networks, "Игры дома").is_err()); + assert!(resolve_network(&networks, "Игры").is_err()); + assert!(resolve_network(&networks, "…").is_err()); + } + + #[test] + fn a_shared_name_lists_the_ids_without_choosing_or_disclosing_secrets() { // Exactly the situation this command exists for: two networks called // `lab`, one of them joined with a mistyped secret. Choosing for the // user here is how the wrong one gets left. let networks = vec![configured("lab", "one"), configured("lab", "two")]; let err = resolve_network(&networks, "lab").unwrap_err(); - assert!(err.contains("not an id"), "{err}"); - assert!( - err.contains("tsunagi network"), - "it says where to look: {err}" + assert!(err.contains("matches 2 networks"), "{err}"); + for network in &networks { + assert!(err.contains(&network.network_id.to_string()), "{err}"); + assert!(!err.contains(network.secret.encode().as_str()), "{err}"); + } + } + + #[test] + fn a_name_colliding_with_an_id_prefix_requires_a_full_id() { + let first = configured("lab", "one"); + let id = first.network_id.to_string(); + let networks = vec![ + first, + configured(&id[..10], "two"), + configured(&id, "three"), + ]; + let err = resolve_network(&networks, &id[..10]).unwrap_err(); + assert!(err.contains("matches 2 networks"), "{err}"); + // An explicit full id remains definitive, even if another name is an id. + assert_eq!( + resolve_network(&networks, &id).unwrap().network_id, + networks[0].network_id ); + for network in &networks[1..] { + assert_eq!( + resolve_network(&networks, &network.network_id.to_string()) + .unwrap() + .network_id, + network.network_id + ); + } } #[test] @@ -4346,21 +4399,26 @@ mod network_tests { #[test] fn a_prefix_shared_by_two_networks_is_refused_rather_than_guessed() { - let networks = vec![configured("lab", "one"), configured("other", "two")]; - let shared = &networks[0].network_id.to_string()[..1]; - let both = networks + // 33 distinct ids must share at least one initial base32 character. + let networks: Vec<_> = (0..33) + .map(|i| configured(&format!("network-{i}"), "secret")) + .collect(); + let shared = networks .iter() - .filter(|network| network.network_id.to_string().starts_with(shared)) - .count(); - if both < 2 { - // The two derived ids happen not to share a first character; - // the empty prefix is the same question with a certain answer. - let err = resolve_network(&networks, "").unwrap_err(); - assert!(err.contains("by its id"), "{err}"); - return; - } - let err = resolve_network(&networks, shared).unwrap_err(); - assert!(err.contains("use more of the id"), "{err}"); + .map(|network| network.network_id.to_string()[..1].to_string()) + .find(|prefix| { + networks + .iter() + .filter(|network| network.network_id.to_string().starts_with(prefix)) + .count() + > 1 + }) + .unwrap(); + let err = resolve_network(&networks, &shared).unwrap_err(); + assert!( + err.contains("use the full id or a unique id prefix"), + "{err}" + ); } } diff --git a/crates/tsunagi-cli/tests/network_cli.rs b/crates/tsunagi-cli/tests/network_cli.rs index 87e9897..b583248 100644 --- a/crates/tsunagi-cli/tests/network_cli.rs +++ b/crates/tsunagi-cli/tests/network_cli.rs @@ -25,17 +25,28 @@ impl Drop for Running { impl Running { fn run(&self, args: &[&str]) -> std::process::Output { - std::process::Command::new(env!("CARGO_BIN_EXE_tsunagi")) - .args(args) - .arg("--state-dir") - .arg(self.dir.path().join("state")) - .arg("--cache-dir") - .arg(self.dir.path().join("cache")) - .output() - .expect("the agent binary runs") + run_in(&self.dir, args) } } +fn run_in(dir: &TempDir, args: &[&str]) -> std::process::Output { + std::process::Command::new(env!("CARGO_BIN_EXE_tsunagi")) + .args(args) + .arg("--state-dir") + .arg(dir.path().join("state")) + .arg("--cache-dir") + .arg(dir.path().join("cache")) + .output() + .expect("the agent binary runs") +} + +fn read_networks(dir: &TempDir) -> Vec { + tsunagi::storage::StateStore::open(dir.path().join("state/state.sqlite")) + .unwrap() + .list_networks() + .unwrap() +} + /// An agent with no network at all, the way a daemon is started before /// anything has been decided. fn start_bare(port: u16) -> Running { @@ -155,7 +166,7 @@ fn a_network_can_be_stopped_and_resumed_without_losing_anything() { .to_string(); assert!(secret_before.starts_with("tsn1")); - let stopped = agent.run(&["network", "stop", &id[..10]]); + let stopped = agent.run(&["network", "stop", "resident"]); assert!( stopped.status.success(), "{}", @@ -181,7 +192,7 @@ fn a_network_can_be_stopped_and_resumed_without_losing_anything() { String::from_utf8_lossy(&again.stdout) ); - let started = agent.run(&["network", "start", &id[..10]]); + let started = agent.run(&["network", "start", "resident"]); assert!(started.status.success()); assert!( String::from_utf8_lossy(&started.stdout).contains("started `resident`"), @@ -192,7 +203,7 @@ fn a_network_can_be_stopped_and_resumed_without_losing_anything() { assert!(listed.contains("running"), "{listed}"); // And nothing was given up on the way: the same network, same secret. - let secret_after = agent.run(&["network", "secret", &id[..10]]); + let secret_after = agent.run(&["network", "secret", "resident"]); assert_eq!( String::from_utf8_lossy(&secret_after.stdout).trim(), secret_before, @@ -290,7 +301,7 @@ fn broadcast_choice_is_per_network_live_persistent_and_preserved_by_rejoin() { .broadcast ); let id = disabled.network_id.to_string(); - let change = agent.run(&["network", "broadcast", &id, "on"]); + let change = agent.run(&["network", "broadcast", "broadcast-off", "on"]); assert!( change.status.success(), "{}", @@ -312,3 +323,98 @@ fn broadcast_choice_is_per_network_live_persistent_and_preserved_by_rejoin() { .success() ); } + +#[test] +fn leaving_by_name_removes_only_that_network_from_a_running_agent() { + let agent = start("resident", 0); + assert!(agent.run(&["join", "-n", "games"]).status.success()); + let resident = read_networks(&agent.dir) + .into_iter() + .find(|network| network.name.as_str() == "resident") + .unwrap(); + + let left = agent.run(&["network", "leave", "games"]); + assert!( + left.status.success(), + "{}", + String::from_utf8_lossy(&left.stderr) + ); + assert!(String::from_utf8_lossy(&left.stdout).contains("left `games`")); + let remaining = read_networks(&agent.dir); + assert_eq!(remaining.len(), 1); + assert_eq!(remaining[0].network_id, resident.network_id); + assert_eq!( + remaining[0].secret.encode().as_str(), + resident.secret.encode().as_str() + ); + let status = agent.run(&["network"]); + assert!(status.status.success()); + let out = String::from_utf8_lossy(&status.stdout); + assert!(out.contains("resident") && out.contains("running"), "{out}"); + assert!(!out.contains("games"), "{out}"); +} + +#[test] +fn leaving_by_name_offline_keeps_the_explicit_offline_requirement() { + let dir = TempDir::new().unwrap(); + for name in ["resident", "games"] { + assert!(run_in(&dir, &["join", "-n", name]).status.success()); + } + let refused = run_in(&dir, &["network", "leave", "games"]); + assert!(!refused.status.success()); + assert!(String::from_utf8_lossy(&refused.stderr).contains("--offline")); + assert_eq!(read_networks(&dir).len(), 2); + + let left = run_in(&dir, &["network", "leave", "games", "--offline"]); + assert!( + left.status.success(), + "{}", + String::from_utf8_lossy(&left.stderr) + ); + let remaining = read_networks(&dir); + assert_eq!(remaining.len(), 1); + assert_eq!(remaining[0].name.as_str(), "resident"); +} + +#[test] +fn ambiguous_and_unknown_names_leave_every_network_untouched() { + let agent = start_bare(0); + for secret in [ + "first-secret-for-the-cli-test", + "second-secret-for-the-cli-test", + ] { + assert!( + agent + .run(&["join", "-n", "games", "-s", secret]) + .status + .success() + ); + } + let before = read_networks(&agent.dir); + assert_eq!(before.len(), 2); + for name in ["games", "missing-network"] { + let result = agent.run(&["network", "leave", name]); + assert!(!result.status.success()); + let err = String::from_utf8_lossy(&result.stderr); + if name == "games" { + assert!(err.contains("matches 2 networks"), "{err}"); + for network in &before { + assert!(err.contains(&network.network_id.to_string()), "{err}"); + assert!(!err.contains(network.secret.encode().as_str()), "{err}"); + } + } else { + assert!(err.contains("no configured network"), "{err}"); + } + assert_eq!(read_networks(&agent.dir).len(), 2); + } + // The suggested full id still selects exactly one of the same-name networks. + let left = agent.run(&["network", "leave", &before[0].network_id.to_string()]); + assert!( + left.status.success(), + "{}", + String::from_utf8_lossy(&left.stderr) + ); + let remaining = read_networks(&agent.dir); + assert_eq!(remaining.len(), 1); + assert_eq!(remaining[0].network_id, before[1].network_id); +} diff --git a/docs/broadcast.md b/docs/broadcast.md index 2cbb4b2..eeca189 100644 --- a/docs/broadcast.md +++ b/docs/broadcast.md @@ -8,14 +8,16 @@ control session. It is not a network-wide vote or signed shared configuration. ```sh tsunagi join -n games --no-broadcast tsunagi join -n games --broadcast -tsunagi network broadcast off -tsunagi network broadcast on -tsunagi network broadcast +tsunagi network broadcast games off +tsunagi network broadcast games on +tsunagi network broadcast games ``` Joining without either flag preserves the saved choice. The `network broadcast` command can change a running network immediately or configure it while the -agent is stopped. `status` reports the local setting. There is no global `up` +agent is stopped. It accepts the exact network name, full id or a unique id +prefix; ambiguous selectors list candidates without changing anything. +`status` reports the local setting. There is no global `up` override: starting the agent preserves every network's independent policy. ## Delivery diff --git a/docs/testing.md b/docs/testing.md index 5f0c73c..4291987 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -108,6 +108,11 @@ with no secret invents one, prints it in full and prints a line the other side can paste unchanged; a bare name this device already knows resumes that network instead of inventing another of the same name; and a network can be stopped and started again with its secret and its place intact. +Network selection accepts exact names consistently across commands. Leave-by-name +is tested with a live agent and with offline state, preserving other networks; +duplicate and unknown names change nothing and full ids resolve ambiguity. +Selector unit tests cover case-sensitive names, Unicode/ellipsis names, copied +id prefixes and collisions between names and ids without revealing secrets. `crates/tsunagi-cli/tests/dns_service.rs` runs the real binary: the resolver comes up with no interface to attach it to, the listener is not rebuilt on