diff --git a/README.md b/README.md index e1771f0c..49524e23 100644 --- a/README.md +++ b/README.md @@ -324,6 +324,7 @@ instead. [docs/cli.md](docs/cli.md) has the full `--rm` contract, including whic | `--herdr-workspace ID` | Target a workspace when using `--herdr-env` | | `dl --refresh` | Rebuild the completion cache now | | `dl --claude-profiles` | List the Claude logins `--claude-profile` can name, and the account each is signed in as | +| `dl --claude-profiles --json` | The same, machine-readable, for a caller (such as corral) asking this host what it actually has rather than trusting its own copy of the list | | `dl --version` | Print the version | | `dl --herdr-shell` | The shell a new [herdr](https://herdr.dev) pane opens: inside the workspace its tab holds, or on this host | | `dl --help`, `-h` | Print help | @@ -362,7 +363,14 @@ holding the `.credentials.json` that a `claude` login writes. Each is a `CLAUDE_ own, which is what makes the logins independent. `dl` reads that layout rather than inventing one, so profiles you already have work with no re-login, and it never writes there: creating and deleting them stays with whatever made the directory. By hand it is -`CLAUDE_CONFIG_DIR=~/.claude-profiles/work claude`, then log in. +`CLAUDE_CONFIG_DIR=~/.claude-profiles/work claude`, then log in. `dl --claude-profiles --json` +answers the same question machine-readably, so a tool that names a profile can check this host +actually has it before asking for it. Each authed row also carries `usageSnapshot`: the exact +bytes of a `usage-snapshot.json` sitting beside the credential, or `null` when there is none, so a +gateway can read a profile's usage over the same round trip instead of a second one. The key is +always present, even when its value is `null`, because a missing key and a `null` value mean +different things: missing says this `dl` predates the field, `null` says it looked and found +nothing. `--claude-profile default` means the login you would get anyway, so a recalled line has a way to say "not the profile I used last time". diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index b9d88ac6..c1a56193 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -1328,6 +1328,7 @@ pub devlaunch_core::flows::claude_profiles::ProfileSummary::name: alloc::string: pub devlaunch_core::flows::claude_profiles::ProfileSummary::path: std::path::PathBuf pub devlaunch_core::flows::claude_profiles::ProfileSummary::shares_account_with: alloc::vec::Vec pub devlaunch_core::flows::claude_profiles::ProfileSummary::state: devlaunch_core::flows::claude_profiles::ProfileState +pub devlaunch_core::flows::claude_profiles::ProfileSummary::usage_snapshot: core::option::Option impl core::clone::Clone for devlaunch_core::flows::claude_profiles::ProfileSummary pub fn devlaunch_core::flows::claude_profiles::ProfileSummary::clone(&self) -> devlaunch_core::flows::claude_profiles::ProfileSummary impl core::cmp::Eq for devlaunch_core::flows::claude_profiles::ProfileSummary @@ -1338,6 +1339,7 @@ pub fn devlaunch_core::flows::claude_profiles::ProfileSummary::fmt(&self, &mut c impl core::marker::StructuralPartialEq for devlaunch_core::flows::claude_profiles::ProfileSummary pub const devlaunch_core::flows::claude_profiles::DEFAULT_PROFILE: &str pub fn devlaunch_core::flows::claude_profiles::from_process() -> alloc::vec::Vec +pub fn devlaunch_core::flows::claude_profiles::json_document(&[devlaunch_core::flows::claude_profiles::ProfileSummary]) -> serde_json::value::Value pub fn devlaunch_core::flows::claude_profiles::summarise(core::option::Option<&std::path::Path>, core::option::Option<&std::path::Path>) -> alloc::vec::Vec pub mod devlaunch_core::flows::completion pub enum devlaunch_core::flows::completion::FileState diff --git a/rust/devlaunch-core/src/flows/claude_profiles.rs b/rust/devlaunch-core/src/flows/claude_profiles.rs index 1bebe1b1..44719ab5 100644 --- a/rust/devlaunch-core/src/flows/claude_profiles.rs +++ b/rust/devlaunch-core/src/flows/claude_profiles.rs @@ -26,6 +26,8 @@ use std::path::{Path, PathBuf}; +use serde::Serialize; + use crate::clients::claude; /// The account behind a profile, at a path a caller outside this crate can name. @@ -48,6 +50,23 @@ pub use crate::clients::claude::Account; /// leaving one to drift. pub const DEFAULT_PROFILE: &str = "default"; +/// The exact filename a usage snapshot is read from, beside the credential. +/// +/// Named, not globbed: Claude Code's own writers use temp-then-rename (a file named +/// `.usage-snapshot.json.` briefly exists beside the real one while a write is in +/// flight), and a glob over the directory would pick up that half-written temp file as +/// though it were the snapshot. Joining this exact name never can. +const USAGE_SNAPSHOT_NAME: &str = "usage-snapshot.json"; + +/// The most a usage snapshot read will ever return, regardless of the file's real +/// size. +/// +/// A cap, not a validation: this crate does not parse the file, so it cannot tell an +/// oversize snapshot from a truncated one and does not try to. It exists so that a +/// gateway relying on one ssh round trip cannot be handed an unbounded amount of data +/// by a file it does not own. +const USAGE_SNAPSHOT_MAX_BYTES: u64 = 64 * 1024; + /// Whether a profile can be launched with. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum ProfileState { @@ -83,6 +102,19 @@ pub struct ProfileSummary { /// proves nothing. Decided on `accountUuid`, never on a display field, for that /// reason. pub shares_account_with: Vec, + /// The bytes of `usage-snapshot.json` beside the credential, when there is a + /// credential and a file to read. + /// + /// `None` covers two different absences a caller does not need told apart: no + /// credential (nobody signed in to have a snapshot) and a credential with no such + /// file yet (nothing has written one). What a caller *does* need told apart is + /// this crate's own absence from an older `dl` that never read the file at all -- + /// [`crate::flows::claude_profiles::json_document`] carries that distinction by + /// always emitting the wire key, `null` or not, rather than omitting it. + /// + /// Read whole and undecoded: this crate does not parse whatever a usage snapshot + /// holds, only forwards the bytes a gateway asked for over one round trip. + pub usage_snapshot: Option, } /// Every profile this host offers, `default` first and the rest by name. @@ -234,9 +266,137 @@ fn row(name: String, path: PathBuf) -> ProfileSummary { // Filled in by `note_shared_accounts` once the whole listing exists: it is a // fact about a row's neighbours, so no row can answer it alone. shares_account_with: Vec::new(), + // Same rule as `account`, and the same reason: a usage snapshot belongs to the + // account that wrote it, and a profile with no credential has no account to + // have written one. + usage_snapshot: authed.then(|| read_usage_snapshot(&path)).flatten(), } } +/// The bytes of `usage-snapshot.json` in `dir`, if there is one to read. +/// +/// `None` for "no such file" and for "could not be read" alike -- this is a listing, +/// not a diagnostic, and, like [`claude::account_at`] beside it, has no stderr of its +/// own to put a reason on. +/// +/// **Never writes, creates or renames anything**, and checks [`Path::is_file`] before +/// opening so that a FIFO or other special file left in the directory cannot hang this +/// read. Reads at most [`USAGE_SNAPSHOT_MAX_BYTES`] and decodes what it got lossily, +/// because this crate does not parse the contents and a snapshot writer using bytes +/// this is not prepared for is not this function's failure to report. +fn read_usage_snapshot(dir: &Path) -> Option { + let path = dir.join(USAGE_SNAPSHOT_NAME); + if !path.is_file() { + return None; + } + let file = std::fs::File::open(&path).ok()?; + let mut capped = std::io::Read::take(file, USAGE_SNAPSHOT_MAX_BYTES); + let mut buf = Vec::new(); + std::io::Read::read_to_end(&mut capped, &mut buf).ok()?; + Some(String::from_utf8_lossy(&buf).into_owned()) +} + +// --------------------------------------------------------------------------- +// the JSON document -- what corral parses instead of trusting its own list +// --------------------------------------------------------------------------- + +/// `dl --claude-profiles --json`. +/// +/// One row per [`ProfileSummary`] the human listing prints, over the same `rows` -- +/// this reads no filesystem of its own, so the table and the document cannot +/// disagree about what a profile is, and there is nothing here for a second +/// listing's worth of drift to hide in. +/// +/// Written for corral's gateway (see the module doc): it kept an allowlist of +/// profile names while the directories live here, the two drifted, and a launch +/// naming a profile the gateway offered but this host no longer had fell back to +/// forwarding the host login instead of refusing. Reading this document is the +/// fix, so its three fields are the three things that answer that: +/// +/// - `name` is the row at all. A profile the gateway remembers and this host does +/// not is not a row here to begin with -- `summarise` only lists directories +/// [`std::fs::read_dir`] actually found -- so a name missing from this array +/// answers "gone" without a state to interpret. +/// - `state` and `account` together are the three readings the human columns +/// already carry, spelled for a parser instead of a person: `"authed"` with an +/// `account` object is signed in and nameably so; `"authed"` with `account: null` +/// is signed in as somebody this could not name (Claude Code's state file was +/// absent or has moved on from the shape this reads); `"not-logged-in"` (always +/// paired with `account: null`) is a directory nobody has logged in to yet. The +/// pairing is [`ProfileSummary::state`] and [`ProfileSummary::account`] +/// unmodified, not a fourth value invented for the wire. +/// - `default` is true for exactly the row named [`DEFAULT_PROFILE`], which +/// `summarise` pushes whether or not its directory exists at all +/// (`$CLAUDE_CONFIG_DIR`, else `~/.claude`) -- the one row here that is not +/// backed by an entry `read_dir` found. A caller that treated every row as "a +/// directory dl walked" would treat `default`'s absence of one as a fact about +/// the login instead of a fact about how the name resolves; this field says so +/// rather than leaving it to be inferred from the string `"default"`. +/// +/// `sharesAccountWith` carries [`ProfileSummary::shares_account_with`] unmodified, +/// for the same reason it is a field there: which names are spare copies of one +/// login is a fact about the whole listing, not about a name alone. +/// +/// `usageSnapshot` carries [`ProfileSummary::usage_snapshot`] unmodified, and is +/// **always present as a key**, never dropped for `null` -- unlike every other +/// optional field here. That is deliberate and the opposite default from +/// `account`'s: a caller reading this over one ssh round trip has to be able to tell +/// "this `dl` is old enough that it never read for a snapshot at all" (the key is +/// missing) from "this `dl` looked and there was nothing to find" (the key is +/// present and `null`). Collapsing the two would let an un-upgraded host report zero +/// usage with the same shape as a host that genuinely has none. +pub fn json_document(rows: &[ProfileSummary]) -> serde_json::Value { + serde_json::Value::Array(rows.iter().map(json_row).collect()) +} + +/// The four fields every row carries, in the order the wire carries them. +#[derive(Debug, Serialize)] +struct RowWire { + name: String, + default: bool, + /// `"authed"` or `"not-logged-in"` -- [`ProfileState`], spelled for the wire + /// rather than for `--help`. + state: &'static str, + account: Option, + #[serde(rename = "sharesAccountWith")] + shares_account_with: Vec, + /// Always serialised, `null` or not -- see [`json_document`]'s doc comment for + /// why a missing key and a `null` value must stay distinguishable. + #[serde(rename = "usageSnapshot")] + usage_snapshot: Option, +} + +/// [`Account`], on the wire. No `accountUuid`: it exists to tell two profiles of +/// one login apart from two that merely look alike, which [`ProfileSummary`] +/// already did on this listing's behalf (`shares_account_with`), so repeating the +/// id here would hand a caller a second, weaker way to reach the same fact. +#[derive(Debug, Serialize)] +struct AccountWire { + email: Option, + organization: Option, + #[serde(rename = "seatTier")] + seat_tier: Option, +} + +fn json_row(row: &ProfileSummary) -> serde_json::Value { + let wire = RowWire { + name: row.name.clone(), + default: row.name == DEFAULT_PROFILE, + state: match row.state { + ProfileState::Authed => "authed", + ProfileState::NoCredential => "not-logged-in", + }, + account: row.account.as_ref().map(|account| AccountWire { + email: account.email.clone(), + organization: account.organization.clone(), + seat_tier: account.seat_tier.clone(), + }), + shares_account_with: row.shares_account_with.clone(), + usage_snapshot: row.usage_snapshot.clone(), + }; + serde_json::to_value(wire).expect("RowWire holds only strings, bools and options of them") +} + #[cfg(test)] mod tests { use super::*; @@ -529,4 +689,97 @@ mod tests { let offered: Vec<&str> = rows.iter().map(|row| row.name.as_str()).collect(); assert_eq!(offered, ["alpha", "mid", "zeta"]); } + + #[test] + fn an_authed_profile_with_a_usage_snapshot_returns_its_exact_bytes() { + let root = tempfile::tempdir().expect("a scratch root"); + let dir = profile(root.path(), "work", true, None); + std::fs::write(dir.join(USAGE_SNAPSHOT_NAME), r#"{"tokens":123}"#).expect("a snapshot"); + + let rows = summarise(Some(root.path()), None); + assert_eq!(rows[0].usage_snapshot.as_deref(), Some(r#"{"tokens":123}"#)); + } + + #[test] + fn an_authed_profile_with_no_usage_snapshot_returns_none() { + let root = tempfile::tempdir().expect("a scratch root"); + profile(root.path(), "work", true, None); + + let rows = summarise(Some(root.path()), None); + assert_eq!(rows[0].usage_snapshot, None); + } + + #[test] + fn a_profile_with_no_credential_names_no_usage_snapshot_even_with_one_on_disk() { + // Same rule as `account`: a snapshot belongs to the account that wrote it, and + // a profile with no credential has no account to have written one. + let root = tempfile::tempdir().expect("a scratch root"); + let dir = profile(root.path(), "stale", false, None); + std::fs::write(dir.join(USAGE_SNAPSHOT_NAME), r#"{"tokens":123}"#).expect("a snapshot"); + + let rows = summarise(Some(root.path()), None); + assert_eq!(rows[0].usage_snapshot, None); + } + + #[test] + fn a_temp_written_snapshot_is_not_picked_up() { + // The writer's own temp-then-rename artifact, left beside the real name -- + // never globbed for, so it is never mistaken for one. + let root = tempfile::tempdir().expect("a scratch root"); + let dir = profile(root.path(), "work", true, None); + std::fs::write(dir.join(".usage-snapshot.json.123"), r#"{"tokens":123}"#) + .expect("a temp file"); + + let rows = summarise(Some(root.path()), None); + assert_eq!(rows[0].usage_snapshot, None); + } + + #[test] + fn an_oversize_usage_snapshot_is_truncated_at_the_cap() { + let root = tempfile::tempdir().expect("a scratch root"); + let dir = profile(root.path(), "work", true, None); + let oversize = "a".repeat(USAGE_SNAPSHOT_MAX_BYTES as usize + 1024); + std::fs::write(dir.join(USAGE_SNAPSHOT_NAME), &oversize).expect("a snapshot"); + + let rows = summarise(Some(root.path()), None); + let read = rows[0].usage_snapshot.as_ref().expect("a truncated read"); + assert_eq!(read.len(), USAGE_SNAPSHOT_MAX_BYTES as usize); + } + + #[test] + fn the_wire_always_carries_the_usage_snapshot_key() { + let root = tempfile::tempdir().expect("a scratch root"); + let dir = profile(root.path(), "work", true, None); + std::fs::write(dir.join(USAGE_SNAPSHOT_NAME), r#"{"tokens":123}"#).expect("a snapshot"); + profile(root.path(), "fresh", false, None); + + let rows = summarise(Some(root.path()), None); + let document = json_document(&rows); + let by_name = |name: &str| -> &serde_json::Value { + document + .as_array() + .expect("an array") + .iter() + .find(|row| row["name"] == name) + .unwrap_or_else(|| panic!("{name} missing from {document:?}")) + }; + // Present and non-null for the profile that has one. + assert_eq!(by_name("work")["usageSnapshot"], r#"{"tokens":123}"#); + // Present, but null, for a profile with nothing to report -- the key must + // never simply be absent, which is what would make an un-upgraded `dl` + // indistinguishable from a host with no usage at all. + assert!( + by_name("work") + .as_object() + .unwrap() + .contains_key("usageSnapshot") + ); + assert!( + by_name("fresh") + .as_object() + .unwrap() + .contains_key("usageSnapshot") + ); + assert!(by_name("fresh")["usageSnapshot"].is_null()); + } } diff --git a/rust/dl/src/cli.rs b/rust/dl/src/cli.rs index a667f07b..0079a9ef 100644 --- a/rust/dl/src/cli.rs +++ b/rust/dl/src/cli.rs @@ -389,8 +389,11 @@ pub(crate) enum Command { }, /// `dl --repos` — the known `owner/repo` strings, for completion. Repos, - /// `dl --claude-profiles` — the Claude logins `--claude-profile` can name. - ClaudeProfiles, + /// `dl --claude-profiles [--json]` — the Claude logins `--claude-profile` + /// can name. + ClaudeProfiles { + output: ListOutput, + }, /// `dl --completion-data` — the whole completion cache, as one JSON line. CompletionData, /// `dl --update-cache [--force]` — the silent background refresh. @@ -603,11 +606,21 @@ pub(crate) struct Cli { command: Vec, /// List every workspace on this machine. - #[arg(long, group = "what")] + #[arg(long, group = "what", group = "json_capable")] ls: bool, - /// With `--ls`: the machine-readable listing, for tools that decide which - /// workspaces to clean up. - #[arg(long, requires = "ls")] + /// With `--ls` or `--claude-profiles`: the machine-readable listing — the + /// workspaces to clean up, or the logins `--claude-profile` can actually + /// name, for a caller (corral) that would otherwise have to trust its own + /// copy of that list. + /// + /// `requires = "json_capable"` rather than `requires = "ls"`: `json_capable` + /// is the group `ls` and `claude_profiles` both carry below, and a group in + /// `requires` is satisfied by *any* member being present — clap's own + /// resolution, not a rule this file re-implements. So `--json` alone is + /// still refused (`json_and_size_are_only_meaningful_with_ls`), and so is + /// `--json` on every other command in `global_command`'s `what` group, + /// exactly as it was when the requirement named `ls` alone. + #[arg(long, requires = "json_capable")] json: bool, /// With `--ls`: what deleting each workspace's clone would free. Off by /// default — it walks every file in the clone. @@ -663,8 +676,9 @@ pub(crate) struct Cli { autorm: bool, /// List the Claude logins `--claude-profile` can name, and the account each is - /// signed in as. Reads them; never writes. - #[arg(long = "claude-profiles", group = "what")] + /// signed in as. Reads them; never writes. `--json` names the same rows + /// machine-readably. + #[arg(long = "claude-profiles", group = "what", group = "json_capable")] claude_profiles: bool, /// The known `owner/repo` strings, one per line (for shell completion). @@ -1055,7 +1069,13 @@ fn global_command(cli: &Cli, chosen: Chosen) -> Result { Chosen::Purge => Command::Purge { yes: cli.yes }, Chosen::Version => Command::Version, Chosen::Repos => Command::Repos, - Chosen::ClaudeProfiles => Command::ClaudeProfiles, + Chosen::ClaudeProfiles => Command::ClaudeProfiles { + output: if cli.json { + ListOutput::Json + } else { + ListOutput::Table + }, + }, Chosen::CompletionData => Command::CompletionData, Chosen::UpdateCache => Command::UpdateCache { force: cli.force }, Chosen::HerdrShell => Command::HerdrShell, @@ -1993,7 +2013,12 @@ mod tests { // A global command, so it takes no workspace and no modifier: the two flags // read alike and mean opposite things, one naming a login to use and one // asking which exist. - assert_eq!(parse(&["--claude-profiles"]), Ok(Command::ClaudeProfiles)); + assert_eq!( + parse(&["--claude-profiles"]), + Ok(Command::ClaudeProfiles { + output: ListOutput::Table + }) + ); assert_eq!( parse(&["--claude-profiles", "--claude-profile", "work"]), Err(GrammarError::ClaudeProfileNotAllowed { @@ -2006,6 +2031,21 @@ mod tests { )); } + #[test] + fn claude_profiles_takes_json_the_way_ls_does() { + // `--json` used to require `--ls` by name; it now requires either half + // of the `json_capable` group, and this is the other half. Refused + // combinations (`--json` alone, `--json` on an unrelated command) are + // `json_and_size_are_only_meaningful_with_ls` and + // `json_still_requires_ls_or_claude_profiles_and_nothing_else`. + assert_eq!( + parse(&["--claude-profiles", "--json"]), + Ok(Command::ClaudeProfiles { + output: ListOutput::Json + }) + ); + } + #[test] fn a_claude_profile_is_refused_on_a_command_that_forwards_no_login() { // The same line `--devcontainer` draws below: a global command has no @@ -2141,6 +2181,25 @@ mod tests { ); } + /// `--json` now accepts either half of the `json_capable` group, but that is + /// an *or* between two named flags, not a widening to every command: a command + /// outside `json_capable` altogether still refuses `--json` at the same clap + /// level, `MissingRequiredArgument`, because neither `ls` nor `claude_profiles` + /// is on the line. `--size` is untouched by this change and keeps naming `ls` + /// alone; it is not asked here because `ls` and `claude_profiles` are already + /// mutually exclusive (clap's `what` group), and clap treats a required arg + /// that a *present* one conflicts with as satisfied-by-conflict rather than + /// missing -- `--claude-profiles --size` was accepted before this change too, + /// for that reason, and still is. + #[test] + fn json_still_requires_ls_or_claude_profiles_and_nothing_else() { + assert_eq!( + refused(&["--repos", "--json"]), + clap::error::ErrorKind::MissingRequiredArgument, + "--repos is not in json_capable, so --json is still unmet" + ); + } + #[test] fn a_third_positional_word_is_refused() { assert_eq!( diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index 69b49b2a..9d4a59be 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -114,7 +114,7 @@ pub(crate) fn dispatch( Command::Version => render_version(), Command::List { output, sizes } => render_list(runner, &mut context, cache, output, sizes), Command::Repos => render_repos(&mut context, cache), - Command::ClaudeProfiles => render_claude_profiles(), + Command::ClaudeProfiles { output } => render_claude_profiles(output), Command::CompletionData => render_completion_data(&mut context, cache), Command::UpdateCache { force } => render_update_cache(runner, &mut context, cache, force), Command::Refresh => render_refresh(&mut context, cache), @@ -399,29 +399,66 @@ fn render_json( // the completion commands // --------------------------------------------------------------------------- -/// `dl --claude-profiles`: the logins `--claude-profile` can name, and who each is. +/// `dl --claude-profiles [--json]`: the logins `--claude-profile` can name, and who +/// each is. /// -/// Plumbing only. The columns, the two absences the account column distinguishes and -/// the shared-account footnote are `render::claude_profile_lines`, which is where the -/// rest of dl's rendering lives and where it is tested. +/// Plumbing only, for either rendering. The table's columns, the two absences the +/// account column distinguishes and the shared-account footnote are +/// `render::claude_profile_lines`; the JSON document is +/// `claude_profiles::json_document`. Both read the same `rows`, so the two cannot +/// disagree about what a profile is — corral, which is why `--json` exists here at +/// all, parses the second rather than screen-scraping the first. /// /// Nothing here reads a token. "authed" is the credential file's existence, so a -/// listing has never touched a secret. -fn render_claude_profiles() -> Ending { +/// listing has never touched a secret, in either rendering. +fn render_claude_profiles(output: cli::ListOutput) -> Ending { let rows = claude_profiles::from_process(); - warn_if_the_profiles_root_could_not_be_read(); - if rows.is_empty() { - // stderr, because it is the reason there is no listing rather than a listing. - eprintln!("{}", render::no_claude_profiles()); - return Ending::Done; + let root_unreadable = warn_if_the_profiles_root_could_not_be_read(); + match output { + cli::ListOutput::Json => { + // No empty-listing sentinel here: `[]` already says "no profiles" to a + // parser, where the sentence below is for a person who would otherwise + // read silence as a hang. + println!( + "{}", + render::python_json_document(&claude_profiles::json_document(&rows)) + ); + } + cli::ListOutput::Table => { + if rows.is_empty() { + // stderr, because it is the reason there is no listing rather than a + // listing. + eprintln!("{}", render::no_claude_profiles()); + } else { + for line in render::claude_profile_lines(&rows) { + println!("{line}"); + } + } + } } - for line in render::claude_profile_lines(&rows) { - println!("{line}"); + claude_profiles_ending(output, root_unreadable) +} + +/// Whether `render_claude_profiles` should exit non-zero. +/// +/// Only `--json` can: a caller parsing that document has no other way to tell "I +/// looked and there is nothing" from "I could not look" -- both print `[]` on stdout, +/// since `claude_profiles::summarise` is pure and has no channel of its own to say +/// which. The table rendering already has [`render::no_claude_profiles`] on stderr for +/// a person reading it, and does not change here. +/// +/// Split from [`render_claude_profiles`] so the decision is a function of the two +/// facts it is actually made from, tested without a filesystem or a process +/// environment in the way. +fn claude_profiles_ending(output: cli::ListOutput, root_unreadable: bool) -> Ending { + if matches!(output, cli::ListOutput::Json) && root_unreadable { + return Ending::Refused; } Ending::Done } -/// Say so when the profiles root is there and could not be read. +/// Say so when the profiles root is there and could not be read, and report whether it +/// was. /// /// `claude_profiles::summarise` is pure and returns a list, so every reason it found /// no profiles looks the same from the outside: a root that was never created, and one @@ -432,22 +469,67 @@ fn render_claude_profiles() -> Ending { /// /// So the reason is asked for here, where there is a stderr to put it on, rather than /// widening the return type of a pure function for a case only the binary can report. -/// `NotFound` is the silent arm; a directory that reads fine says nothing either. -fn warn_if_the_profiles_root_could_not_be_read() { +/// `NotFound` is the silent arm; a directory that reads fine says nothing either. The +/// `bool` this returns is [`claude_profiles_ending`]'s way of turning the same fact +/// into an exit code, since a script reading stdout alone never sees the line printed +/// here. +fn warn_if_the_profiles_root_could_not_be_read() -> bool { let Ok(root) = xdg::claude_profiles_root() else { - return; + return false; }; - let Err(error) = std::fs::read_dir(&root) else { - return; + let Some(error) = profiles_root_read_error(&root) else { + return false; }; - if error.kind() == std::io::ErrorKind::NotFound { - return; - } eprintln!( "Could not read the Claude profiles directory {} ({error}), so any profiles in it are \ missing from this listing.", root.display() ); + true +} + +/// The error reading `root` gives, unless it is the one that means "there is nothing +/// here at all" rather than "I could not look". +/// +/// `NotFound` is that ordinary absence: most hosts have never made this directory, +/// and that is not a failure to read it. Every other error -- permissions, a plain +/// file where a directory should be -- is a real "I could not look" and this is `Some` +/// of it. +/// +/// **The whole iterator is walked, not just opened.** `read_dir` returning `Ok` +/// only says the directory could be OPENED; each entry is a second fallible read, +/// and `claude_profiles::summarise` consumes them with `entries.flatten()`, which +/// drops a failing one silently. Checking the open alone therefore reported "fine" +/// for the exact case this function exists to catch: a listing that is short a +/// profile, printed with no warning and exit 0. That is the "a host with five +/// profiles being told it has none" outcome named in +/// [`warn_if_the_profiles_root_could_not_be_read`], arrived at one entry at a time. +/// +/// Walking it twice (here and in `summarise`) is deliberate. `summarise` is pure +/// and returns a list, so it has no channel to report this, and widening its return +/// type for a case only the binary can print would put the cost on every caller. +/// The directory holds one entry per Claude login; two walks of it is not a cost +/// worth shaping an API around. +/// +/// Reported by review on #650. +/// +/// **Not covered by a test, and said rather than hidden.** A per-entry `readdir` +/// failure is not something a portable unit test can provoke: removing entries +/// mid-walk does not error, nor does a name that is not UTF-8, and the kernel +/// paths that do fail (a stale NFS handle, a disappearing mount) cannot be +/// arranged from inside the suite. The three tests below pin what can be pinned +/// -- absent, readable, not a directory. This arm rests on the type. +/// +/// Split out so a test can hand this a path it built (a plain file where a directory +/// should be) instead of shaping a process environment every other test in the binary +/// shares. +fn profiles_root_read_error(root: &Path) -> Option { + let entries = match std::fs::read_dir(root) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return None, + Err(error) => return Some(error), + }; + entries.filter_map(Result::err).next() } /// The known `owner/repo` strings, one per line. @@ -1923,3 +2005,56 @@ mod herdr_editor_tests { assert_eq!(started_agent(&Verb::Attach { rm: RmOnExit::No }), None); } } + +#[cfg(test)] +mod claude_profiles_ending_tests { + use super::{Ending, claude_profiles_ending, profiles_root_read_error}; + use crate::cli::ListOutput; + + #[test] + fn json_mode_exits_non_zero_when_the_root_could_not_be_read() { + assert_eq!( + claude_profiles_ending(ListOutput::Json, true).code(), + Ending::Refused.code() + ); + } + + #[test] + fn json_mode_is_done_when_the_root_read_fine() { + assert_eq!( + claude_profiles_ending(ListOutput::Json, false).code(), + Ending::Done.code() + ); + } + + #[test] + fn table_mode_never_refuses_over_an_unreadable_root() { + // The table already has `render::no_claude_profiles` on stderr for a person; + // this command's exit code does not change for it. + assert_eq!( + claude_profiles_ending(ListOutput::Table, true).code(), + Ending::Done.code() + ); + } + + #[test] + fn a_root_that_was_never_created_is_not_a_read_error() { + let root = tempfile::tempdir().expect("a scratch dir"); + let never_created = root.path().join("does-not-exist"); + assert!(profiles_root_read_error(&never_created).is_none()); + } + + #[test] + fn a_root_that_reads_fine_is_not_a_read_error() { + let root = tempfile::tempdir().expect("a scratch dir"); + assert!(profiles_root_read_error(root.path()).is_none()); + } + + #[test] + fn a_plain_file_where_a_directory_should_be_is_a_read_error() { + let root = tempfile::tempdir().expect("a scratch dir"); + let not_a_directory = root.path().join("profiles"); + std::fs::write(¬_a_directory, "not a directory").expect("a plain file"); + assert!(profiles_root_read_error(¬_a_directory).is_some()); + } +} diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 1c1030b9..70681360 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -4097,6 +4097,7 @@ mod tests { state, account, shares_account_with: shares_with.iter().map(|n| (*n).to_owned()).collect(), + usage_snapshot: None, } } @@ -4330,6 +4331,119 @@ mod tests { assert!(no_claude_profiles().contains("no profiles to list")); } + // ---------------------------------------------------------- --json's document + + /// The table's three account-column readings, agreed with `--json`'s. + /// + /// `claude_profiles::json_document` and `claude_profile_lines` both read + /// `ProfileSummary` and neither re-derives the other's answer, but the *words* + /// each picks (`"not logged in"` here, `"not-logged-in"` there; `"unknown"` + /// here, `account: null` there) are a second hand-maintained copy of the same + /// three facts, per this repo's standing rule about those. This is the diff + /// that keeps them from drifting: for every `(state, account)` pair the table + /// distinguishes, the JSON row is asserted to say the same thing in its own + /// words. + #[test] + fn the_json_document_agrees_with_the_table_on_every_state_the_columns_distinguish() { + let cases = [ + // authed, with an account the state file names + ( + profile( + "work", + claude_profiles::ProfileState::Authed, + Some(account(Some("me@example.com"), None, None)), + &[], + ), + "authed", + true, + ), + // authed, but the state file named nobody -- the table's "unknown" + ( + profile("odd", claude_profiles::ProfileState::Authed, None, &[]), + "authed", + false, + ), + // never logged in -- the table's "-" + ( + profile( + "fresh", + claude_profiles::ProfileState::NoCredential, + None, + &[], + ), + "not-logged-in", + false, + ), + ]; + for (row, expected_state, has_account) in cases { + let table = claude_profile_lines(std::slice::from_ref(&row)); + let json = claude_profiles::json_document(std::slice::from_ref(&row)); + let wire_state = json[0]["state"].as_str().expect("a state string"); + assert_eq!(wire_state, expected_state, "{}: {table:#?}", row.name); + assert_eq!( + json[0]["account"].is_null(), + !has_account, + "{}: {table:#?}", + row.name + ); + match (wire_state, has_account) { + ("authed", true) => assert!(table[1].contains("authed") && !table[1].contains('-')), + ("authed", false) => assert!(table[1].contains("unknown"), "{table:#?}"), + ("not-logged-in", false) => { + assert!(table[1].contains("not logged in"), "{table:#?}") + } + other => panic!("an untested combination: {other:?}"), + } + } + } + + #[test] + fn the_json_document_names_the_account_and_the_shared_group() { + let json = claude_profiles::json_document(&[profile( + "work", + claude_profiles::ProfileState::Authed, + Some(account(Some("me@example.com"), Some("Acme"), Some("max"))), + &["spare"], + )]); + let row = &json[0]; + assert_eq!(row["name"], "work"); + assert_eq!(row["default"], false); + assert_eq!(row["account"]["email"], "me@example.com"); + assert_eq!(row["account"]["organization"], "Acme"); + assert_eq!(row["account"]["seatTier"], "max"); + assert_eq!(row["sharesAccountWith"], serde_json::json!(["spare"])); + // No accountUuid on the wire: `shares_account_with` already carries what a + // caller would use it for. + assert!(row["account"].get("accountUuid").is_none()); + } + + #[test] + fn the_default_row_says_so_in_json_without_a_directory_of_its_own() { + // `default` is pushed into the listing whether or not its directory + // exists (see `claude_profiles::summarise`), so a JSON consumer needs a + // way to tell it apart from a name `read_dir` actually found -- rather + // than hard-coding the string `"default"` a second time. + let json = claude_profiles::json_document(&[ + profile( + claude_profiles::DEFAULT_PROFILE, + claude_profiles::ProfileState::NoCredential, + None, + &[], + ), + profile("work", claude_profiles::ProfileState::Authed, None, &[]), + ]); + assert_eq!(json[0]["default"], true); + assert_eq!(json[1]["default"], false); + } + + #[test] + fn an_empty_listing_is_an_empty_json_array() { + // Unlike the table, which says why on stderr and prints nothing: a parser + // reading stdout wants a value every time, and `[]` already says "no + // profiles" without a sentinel to special-case. + assert_eq!(claude_profiles::json_document(&[]), serde_json::json!([])); + } + /// The three sentences a refused `--claude-profile` produces. /// /// Worth pinning as text rather than as "it errored", because the whole argument