From a5e4e1104ba5507027539eb1075a604ff3a86675 Mon Sep 17 00:00:00 2001 From: Joshua Smith Date: Sat, 19 Sep 2026 16:14:42 +0100 Subject: [PATCH 1/3] feat: `--claude-profiles --json` says what a machine actually has corral keeps an allowlist of Claude profile names on the gateway while the directories they name live on the target. Those are two copies of one fact and they drifted: the gateway went on offering profiles the target no longer had, and a launch naming one fell back to forwarding the host's own login instead of refusing. The operator saw a successful launch using the wrong credential. Reading the machine is the fix, and this is what makes reading it reliable. `--claude-profiles` already knew the answer; it could only say it in a table. The shape distinguishes the three readings the table's account column already has, without inventing a fourth: - `state` is `authed` or `not-logged-in`, mirroring `ProfileState`; - `account` is null both when there is no credential and when there is one that names nobody, so `authed` with a null account is the table's "unknown"; - `default` is a flag rather than a name comparison, because that row is pushed into the listing whether or not its directory exists and a consumer must not infer that from the string. The state strings are spelled independently of the table's, so a test diffs the two renderings over every state the columns distinguish -- this repo's rule about a second hand-maintained copy of a fact. `--json` is now required to accompany `--ls` or `--claude-profiles`, expressed as a clap group rather than a hand-rolled check, so asking for it anywhere else is refused by the parser with the alternatives named. Claude-Session: https://claude.ai/code/session_01AdSFnBdxie6TosHVmjLY28 --- README.md | 5 +- rust/devlaunch-core/public-api.rest.txt | 1 + .../src/flows/claude_profiles.rs | 89 ++++++++++++++ rust/dl/src/cli.rs | 77 ++++++++++-- rust/dl/src/commands.rs | 46 ++++--- rust/dl/src/render.rs | 113 ++++++++++++++++++ 6 files changed, 306 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 359743d9..586b319e 100644 --- a/README.md +++ b/README.md @@ -323,6 +323,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 | @@ -361,7 +362,9 @@ 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. `--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 99d4e061..306a68c6 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -1324,6 +1324,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..f485555b 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. @@ -237,6 +239,93 @@ fn row(name: String, path: PathBuf) -> ProfileSummary { } } +// --------------------------------------------------------------------------- +// 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. +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, +} + +/// [`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(), + }; + serde_json::to_value(wire).expect("RowWire holds only strings, bools and options of them") +} + #[cfg(test)] mod tests { use super::*; diff --git a/rust/dl/src/cli.rs b/rust/dl/src/cli.rs index a667f07b..1726a6af 100644 --- a/rust/dl/src/cli.rs +++ b/rust/dl/src/cli.rs @@ -389,8 +389,9 @@ 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 +604,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 +674,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 +1067,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 +2011,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 +2029,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 +2179,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 c2db1b8a..08a590d3 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,24 +399,42 @@ 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; - } - for line in render::claude_profile_lines(&rows) { - println!("{line}"); + 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}"); + } + } + } } Ending::Done } diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 83db1efe..55181e86 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -4299,6 +4299,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 From b4e9db90f17c63bb51d08dae27bc0c3567f24a8b Mon Sep 17 00:00:00 2001 From: Joshua Smith Date: Mon, 28 Sep 2026 15:49:42 +0100 Subject: [PATCH 2/3] feat: `dl --claude-profiles --json` carries a profile's usage snapshot A gateway forwarding a Claude login already asks this host what it has over one ssh round trip; usage data was a second round trip away. Reading usage-snapshot.json beside the credential (never globbed, capped at 64KiB, only when the profile is authed, same rule as the account field) and adding it to the wire as an always-present `usageSnapshot` key lets a caller get both in the one call. The key is never omitted, because a missing key and a null value answer different questions: missing means this dl predates the field, null means it looked and found nothing -- collapsing the two would let an un-upgraded host report zero usage with the same shape as a host that genuinely has none. Also makes `--claude-profiles --json` exit non-zero when the profiles directory exists but can't be read, so "couldn't look" stops being indistinguishable from "nothing there" for a script that isn't reading stderr. Claude-Session: https://claude.ai/code/session_01AdSFnBdxie6TosHVmjLY28 --- README.md | 7 +- rust/devlaunch-core/public-api.rest.txt | 1 + .../src/flows/claude_profiles.rs | 164 ++++++++++++++++++ rust/dl/src/cli.rs | 4 +- rust/dl/src/commands.rs | 112 ++++++++++-- rust/dl/src/render.rs | 1 + 6 files changed, 277 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 586b319e..e43f2619 100644 --- a/README.md +++ b/README.md @@ -364,7 +364,12 @@ so profiles you already have work with no re-login, and it never writes there: c deleting them stays with whatever made the directory. By hand it is `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. +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 306a68c6..5a7882e8 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -1314,6 +1314,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 diff --git a/rust/devlaunch-core/src/flows/claude_profiles.rs b/rust/devlaunch-core/src/flows/claude_profiles.rs index f485555b..44719ab5 100644 --- a/rust/devlaunch-core/src/flows/claude_profiles.rs +++ b/rust/devlaunch-core/src/flows/claude_profiles.rs @@ -50,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 { @@ -85,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. @@ -236,9 +266,36 @@ 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 // --------------------------------------------------------------------------- @@ -279,6 +336,15 @@ fn row(name: String, path: PathBuf) -> ProfileSummary { /// `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()) } @@ -294,6 +360,10 @@ struct RowWire { 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 @@ -322,6 +392,7 @@ fn json_row(row: &ProfileSummary) -> serde_json::Value { 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") } @@ -618,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 1726a6af..0079a9ef 100644 --- a/rust/dl/src/cli.rs +++ b/rust/dl/src/cli.rs @@ -391,7 +391,9 @@ pub(crate) enum Command { Repos, /// `dl --claude-profiles [--json]` — the Claude logins `--claude-profile` /// can name. - ClaudeProfiles { output: ListOutput }, + ClaudeProfiles { + output: ListOutput, + }, /// `dl --completion-data` — the whole completion cache, as one JSON line. CompletionData, /// `dl --update-cache [--force]` — the silent background refresh. diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index 08a590d3..8eb68b47 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -413,7 +413,7 @@ fn render_json( /// 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(); + 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 @@ -436,10 +436,29 @@ fn render_claude_profiles(output: cli::ListOutput) -> Ending { } } } + 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 @@ -450,22 +469,42 @@ fn render_claude_profiles(output: cli::ListOutput) -> 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 [`std::fs::read_dir`] gives for `root`, 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. +/// +/// 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 { + match std::fs::read_dir(root) { + Ok(_) => None, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, + Err(error) => Some(error), + } } /// The known `owner/repo` strings, one per line. @@ -1936,3 +1975,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 55181e86..a77dd4c5 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -4066,6 +4066,7 @@ mod tests { state, account, shares_account_with: shares_with.iter().map(|n| (*n).to_owned()).collect(), + usage_snapshot: None, } } From 4724f981ea08148d5af75b2adc4ae32e8ee0e64d Mon Sep 17 00:00:00 2001 From: Joshua Smith Date: Fri, 2 Oct 2026 11:49:11 +0100 Subject: [PATCH 3/3] fix: an unreadable profiles root is caught per entry, not just on open `profiles_root_read_error` asked only whether 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 in silence. So the check reported "fine" for the exact outcome it exists to catch: a listing short a profile, no warning, exit 0 -- "a host with five profiles being told it has none", arrived at one entry at a time instead of all at once. It now walks the iterator and returns the first entry error. Walking it twice, here and in `summarise`, is deliberate: `summarise` is pure and has no channel to report this, and widening its return type for a case only the binary can print would charge every caller for it. The directory holds one entry per Claude login. The arm has no test and the doc says so. A per-entry readdir failure is not something a portable unit test can provoke -- removing entries mid-walk does not error, nor does a non-UTF-8 name, and a stale NFS handle cannot be arranged from inside the suite. Reported by review on #650. Claude-Session: https://claude.ai/code/session_01AdSFnBdxie6TosHVmjLY28 --- rust/dl/src/commands.rs | 39 ++++++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index 8eb68b47..5a10d432 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -488,23 +488,48 @@ fn warn_if_the_profiles_root_could_not_be_read() -> bool { true } -/// The error [`std::fs::read_dir`] gives for `root`, unless it is the one that means -/// "there is nothing here at all" rather than "I could not look". +/// 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 { - match std::fs::read_dir(root) { - Ok(_) => None, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, - Err(error) => Some(error), - } + 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.