diff --git a/CHANGELOG.md b/CHANGELOG.md index ab0e8945..6426fb52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,6 +106,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 warning. `aid` passes the flag through. A verb that forwards no login says it is ignoring it, as `--devcontainer` does; a global command refuses it. +- **A pane's row in [herdr](https://herdr.dev) says which account the agent in it is + running as**, reported as the display label `profile=`. A profile is chosen per + launch and forwarded per session, so two tabs side by side can be two different + accounts with nothing on screen to tell them apart, and the failure that matters here + is not noticing: work pushed from the wrong identity is found out about later and + somewhere else. Reported under devlaunch's own source name, so it sits beside herdr's + labels rather than overwriting one, and cleared rather than left stale when a launch + forwards the default login. The label is reported before anything that can go wrong + with the manager's socket, because a launch whose forward never starts still opens a + session: reporting it last would have cleared a stale label on one path out of five + and left `profile=work` standing over a session on another account on the other four. + + **A pane opened beside an agent inherits the profile that agent started with.** The + pane shell already opens in the workspace its tab holds; a workspace is not an account, + so it read the default login while the agent one pane over ran as another. It is read + from the agent's own argv rather than from a note kept anywhere, for the reason the + pane shell keeps nothing: the argv is what is true, and a record would be a second copy + of it that can go stale. An exact element match, and it stops at a bare `--`, so a + prompt that contains the words `--claude-profile work` is a prompt. + + **Nothing is inherited across a boundary.** A pane in a tab holding no agent has no + account to inherit and gets the default login, and the environment of an agent already + running is fixed at exec, so switching profile reaches the next session and never the + one on screen. + ### Fixed - **`$CLAUDE_CONFIG_DIR` is now honoured on the host, so a host that has moved its diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index dd3a9f84..ccbc9205 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -2756,7 +2756,9 @@ impl core::marker::StructuralPartialEq for devlaunch_core::flows::repo_manager:: pub mod devlaunch_core::flows::session_manager pub enum devlaunch_core::flows::session_manager::PaneDestination pub devlaunch_core::flows::session_manager::PaneDestination::HostShell -pub devlaunch_core::flows::session_manager::PaneDestination::Workspace(alloc::string::String) +pub devlaunch_core::flows::session_manager::PaneDestination::Workspace +pub devlaunch_core::flows::session_manager::PaneDestination::Workspace::claude_profile: core::option::Option +pub devlaunch_core::flows::session_manager::PaneDestination::Workspace::workspace_id: alloc::string::String impl core::clone::Clone for devlaunch_core::flows::session_manager::PaneDestination pub fn devlaunch_core::flows::session_manager::PaneDestination::clone(&self) -> devlaunch_core::flows::session_manager::PaneDestination impl core::cmp::Eq for devlaunch_core::flows::session_manager::PaneDestination diff --git a/rust/devlaunch-core/src/clients/herdr.rs b/rust/devlaunch-core/src/clients/herdr.rs index 53d52a21..6d02bba7 100644 --- a/rust/devlaunch-core/src/clients/herdr.rs +++ b/rust/devlaunch-core/src/clients/herdr.rs @@ -1234,6 +1234,53 @@ pub(crate) fn pane_list_argv() -> Vec { vec!["pane".to_owned(), "list".to_owned()] } +/// The source id devlaunch reports under, in herdr's `--source` sense. +/// +/// One string, two callers: this module's argv builders and [`HOOK`], which spells it +/// as a shell literal because it is a shell script. +/// `the_hook_reports_under_the_source_this_module_names` is the test that diffs them. +pub(crate) const REPORT_SOURCE: &str = "devlaunch:claude"; + +/// The metadata token naming the Claude profile a pane's session forwards. +/// +/// Display-only, and rendered as `$profile` in herdr's agent sidebar rows. A *token* +/// rather than `--display-agent`, which would fight the agent name `report-agent` +/// sets and which herdr uses to pick a detection manifest. +pub(crate) const PROFILE_TOKEN: &str = "profile"; + +/// The argv that labels a pane with the Claude profile its session runs as, or clears +/// the label when the session names no profile. +/// +/// **Clearing matters as much as setting.** A pane is reused: a launch that named +/// `work` and a later launch in the same pane that named nothing would otherwise leave +/// the sidebar claiming an account the running session is not using, which is the exact +/// mislabelling `--claude-profiles` exists to prevent, moved somewhere more visible. +/// +/// Verified against herdr 0.8.2: `pane report-metadata [OPTIONS] --source +/// `, with `--token NAME=VALUE` and `--clear-token NAME`. The pane id is given +/// first, as [`HOOK`] gives it to `report-agent`, which is the invocation measured +/// against a live herdr. +pub(crate) fn profile_metadata_argv(pane_id: &str, profile: Option<&str>) -> Vec { + let mut args = vec![ + "pane".to_owned(), + "report-metadata".to_owned(), + pane_id.to_owned(), + "--source".to_owned(), + REPORT_SOURCE.to_owned(), + ]; + match profile { + Some(name) => { + args.push("--token".to_owned()); + args.push(format!("{PROFILE_TOKEN}={name}")); + } + None => { + args.push("--clear-token".to_owned()); + args.push(PROFILE_TOKEN.to_owned()); + } + } + args +} + /// The argv that asks herdr what one pane is running. pub(crate) fn process_info_argv(pane_id: &str) -> Vec { vec![ diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index 029499b9..ec9a315f 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -2182,6 +2182,27 @@ fn begin_reporting( reporting: herdr::Reporting, notices: &mut dyn Notices, ) -> Option<(herdr::Reporting, session_manager::Forward)> { + // Which account this session runs as, so the manager's sidebar says so rather than + // leaving the name of a directory as the only clue. Called even when no profile was + // named, because a pane is reused and a stale label is worse than none. + // + // **First, and before every early return below.** This sat in the `Ready` arm at + // the bottom, which is one exit out of five: no ssh config, no published alias, a + // forward that would not start and a container that refused all return `None` from + // here, and every one of them still opens a session (the notices they raise are + // never fatal). So a pane that had run `dl ws --claude-profile work` and was reused + // for a `dl ws2` whose alias devpod had not published yet kept asserting + // `profile=work` over a session on the default login -- exactly the stale label the + // clearing exists to prevent, on the paths where clearing never ran. + // + // Safe this early because the label is host-side: `report_profile` runs herdr's own + // binary against a pane id, and needs neither the forward nor a prepared container. + session_manager::report_profile( + session.runner, + &reporting, + session.host.claude.profile.as_deref(), + ); + // The alias, resolved without asking whether dl is on a terminal. // // Deliberately not [`Terminal`], which answers a different question and is @@ -6918,6 +6939,55 @@ mod tests { ); } + /// A Scene whose pane reports to a session manager, and which resolves no ssh + /// alias, so `begin_reporting` takes one of its four early returns. + /// + /// The five herdr variables are what `Reporting::resolve` asks for; the `Scene` + /// leaves `ssh_config` unset, which is the "no ssh config" arm. + fn reporting_in_a_pane(mut scene: Scene) -> Scene { + scene.host.herdr = crate::clients::herdr::HostEnv { + enabled: Some("1".to_owned()), + in_pane: Some("1".to_owned()), + pane_id: Some("w1:p3".to_owned()), + socket: Some("/run/herdr.sock".to_owned()), + binary: Some("/opt/herdr/bin/herdr".to_owned()), + }; + scene + } + + #[test] + fn the_profile_label_is_reported_even_when_the_manager_cannot_be_reached() { + // The stale-label clear used to sit in `begin_reporting`'s `Ready` arm, which + // is one exit out of five. No ssh config, no published alias, a forward that + // would not start and a container that refused all return early, and every one + // of them still opens the session -- so a pane that had run + // `dl ws --claude-profile work` and was then reused for a launch that took one + // of those paths kept asserting `profile=work` over a session on another + // account. Reported first now, before any of them. + let scene = reporting_in_a_pane( + Scene::new() + .on_a_terminal(&["myws"]) + .with_running("myws") + .naming_a_claude_profile("work", true), + ); + + let _ = a_session_on_our_own_claude(&scene, Some("claude")); + + let labelled: Vec> = scene + .runner + .calls_to("/opt/herdr/bin/herdr") + .iter() + .map(|call| call.invocation().argv().to_vec()) + .filter(|argv| argv.iter().any(|word| word == "report-metadata")) + .collect(); + assert!( + labelled + .iter() + .any(|argv| argv.iter().any(|word| word.contains("profile=work"))), + "no profile label was reported: {labelled:?}" + ); + } + /// The refusal `clients::claude` builds and nothing used to read. /// /// `NoToken::ProfileUnreadable` is documented where it is declared as "the one diff --git a/rust/devlaunch-core/src/flows/session_manager.rs b/rust/devlaunch-core/src/flows/session_manager.rs index 38f99974..e6e1d634 100644 --- a/rust/devlaunch-core/src/flows/session_manager.rs +++ b/rust/devlaunch-core/src/flows/session_manager.rs @@ -335,7 +335,18 @@ pub(crate) fn start_forward( // binary surface -- not part of the frozen wf API (#251 ยง7) pub enum PaneDestination { /// The tab holds a live devlaunch session, in this workspace. - Workspace(String), + Workspace { + workspace_id: String, + /// The Claude profile the session beside this pane was launched with, when + /// its `dl` argv names one. + /// + /// Read rather than remembered, which is what lets it exist at all: a + /// profile is named per launch and deliberately **not** stored with the + /// workspace, so there is nothing on disk for a new pane to inherit. The + /// live sibling process is the only record, and herdr is already being + /// asked what that pane is running. + claude_profile: Option, + }, /// Open an ordinary shell on this host. HostShell, } @@ -415,12 +426,44 @@ fn destination_for(runner: &dyn Runner, host: PaneEnv<'_>) -> PaneDestination { continue; }; if let Some(workspace_id) = workspace_among(&info) { - return PaneDestination::Workspace(workspace_id); + return PaneDestination::Workspace { + workspace_id, + // Asked of the *same* pane, so one pane answers both questions and a + // tab holding two sessions cannot pair one session's workspace with + // another's account. + claude_profile: profile_among(&info).map(str::to_owned), + }; } } PaneDestination::HostShell } +/// Tell the manager which Claude account this pane's session is running as. +/// +/// Display only: it sets a metadata token herdr renders as `$profile`, and touches no +/// lifecycle state, so it cannot disturb the idle/working/blocked the hook reports. +/// +/// **Reported and then tolerated**, like everything else in this flow. A herdr that is +/// gone, too old for `report-metadata`, or simply slow costs a label and never a +/// session, so the outcome is deliberately dropped. +/// +/// Called with `None` as well as with a name, because a pane is reused and a stale +/// label is worse than none: see [`herdr::profile_metadata_argv`]. +pub(crate) fn report_profile( + runner: &dyn Runner, + reporting: &herdr::Reporting, + profile: Option<&str>, +) { + let mut budget = Budget::of(herdr::ANSWER_WITHIN); + let program = reporting.host_binary().display().to_string(); + let _ = ask( + runner, + &program, + &herdr::profile_metadata_argv(reporting.pane_id(), profile), + &mut budget, + ); +} + /// What is left of the time the whole question may take. /// /// A budget rather than a deadline, because the [`Runner`] seam takes a duration @@ -500,6 +543,55 @@ fn workspace_among(info: &herdr::PaneProcessInfo) -> Option { .map(str::to_owned) } +/// The Claude profile a pane's processes name, if one of them was given one. +/// +/// Asked only of a pane that has already named a workspace, so the question is +/// "which account is *this devlaunch session* running as" rather than a scan of the +/// machine. That coupling is the guard: an unrelated pane is never consulted. +/// +/// **Elements are compared whole, and the scan stops at the first bare `--`.** Both +/// matter for one case. `aid add --claude-profile support to the docs` becomes a +/// `dl` line whose prompt is a single argument containing that text, and the ssh +/// transport carries the same string as one payload argument; neither is an argv +/// *element* equal to `--claude-profile`, so neither is read as a flag. The `--` stop +/// says the same thing a second way, and cheaply. +/// +/// Deliberately not gated on the program name. `dl` on a host may be `dl`, `dl-next` +/// or an absolute path, and matching that family by prefix is exactly the fuzzy test +/// that made [`ssh_host`] necessary. The pane-level coupling above is the stronger +/// guard and needs no such guess. +fn profile_among(info: &herdr::PaneProcessInfo) -> Option<&str> { + info.foreground_processes + .iter() + .filter_map(|process| process.argv.as_deref()) + .find_map(profile_named_by) +} + +/// The profile one argv names, in either of clap's two spellings. +fn profile_named_by(argv: &[String]) -> Option<&str> { + // The first match, and one is all a real argv can hold: `--claude-profile` is an + // `Option` under clap with no `ArgAction::Append`, so a repeat is refused + // outright ("the argument '--claude-profile ' cannot be used multiple times") + // and the launch never happens. There is therefore no last-versus-first question to + // get wrong here, which is worth writing down because the reader looks like it has + // one. `a_repeated_profile_never_reaches_a_running_agent` holds clap to that. + let mut rest = argv.iter(); + while let Some(argument) = rest.next() { + if argument == "--" { + // Everything after this belongs to the command the workspace runs, and a + // prompt is allowed to contain anything at all. + return None; + } + if let Some(value) = argument.strip_prefix("--claude-profile=") { + return (!value.is_empty()).then_some(value); + } + if argument == "--claude-profile" { + return rest.next().map(String::as_str).filter(|v| !v.is_empty()); + } + } + None +} + /// The workspace an argv names, when the argv is one of dl's two transports. /// /// **This reads dl's own writing**, which is what makes it a reading and not a @@ -1099,6 +1191,192 @@ mod tests { assert_eq!(workspace_named_by(&[]), None); } + /// One foreground process running `words`, beside the existing `argv` helper. + fn process(words: &[&str]) -> herdr::ForegroundProcess { + herdr::ForegroundProcess { + argv: Some(argv(words)), + } + } + + #[test] + fn the_profile_label_is_set_and_cleared_in_herdrs_own_words() { + // Verified against herdr 0.8.2's `pane report-metadata --help`, not guessed: + // `[OPTIONS] --source `, where a metadata token is set with + // `--token` as `NAME=VALUE` and cleared with `--clear-token`. The pane id + // goes first, as the hook gives it to `report-agent`, which is the + // invocation measured against a live herdr. + assert_eq!( + herdr::profile_metadata_argv("pane-7", Some("work")), + argv(&[ + "pane", + "report-metadata", + "pane-7", + "--source", + "devlaunch:claude", + "--token", + "profile=work", + ]) + ); + // No profile clears the label rather than leaving one. A pane is reused, and a + // sidebar claiming an account the running session is not using is the + // mislabelling `--claude-profiles` exists to prevent, moved somewhere more + // visible. + assert_eq!( + herdr::profile_metadata_argv("pane-7", None), + argv(&[ + "pane", + "report-metadata", + "pane-7", + "--source", + "devlaunch:claude", + "--clear-token", + "profile", + ]) + ); + } + + #[test] + fn the_hook_reports_under_the_source_this_module_names() { + // Two copies of one string: this module's argv builders and the hook, which + // spells it as a shell literal because it is a shell script. CLAUDE.md's + // standing rule asks for a test beside the second copy, and this is it. + assert!( + herdr::HOOK.contains(herdr::REPORT_SOURCE), + "the hook no longer reports under {}", + herdr::REPORT_SOURCE + ); + } + + #[test] + fn a_profile_label_costs_a_label_and_never_a_session() { + // Reported and then tolerated, like everything else in this flow. A herdr that + // is not installed, is too old for `report-metadata`, or simply refuses must + // cost the label and never the session. The signature is most of the guarantee + // -- there is no outcome for a caller to branch on -- and this is the other + // half: the unscripted and missing cases both return rather than panicking. + let reporting = reporting(); + report_profile(&ScriptedRunner::new(), &reporting, Some("work")); + report_profile(&ScriptedRunner::new(), &reporting, None); + let absent = + ScriptedRunner::new().with_missing(reporting.host_binary().display().to_string()); + report_profile(&absent, &reporting, Some("work")); + } + + #[test] + fn a_profile_is_read_out_of_the_line_that_named_it() { + // Both of clap's spellings, since `dl` accepts either and a recalled line may + // hold either. + assert_eq!( + profile_named_by(&argv(&["dl", "ws", "--claude-profile", "work"])), + Some("work") + ); + assert_eq!( + profile_named_by(&argv(&["dl", "--claude-profile=work", "ws"])), + Some("work") + ); + // A line that named none, which is almost every line. + assert_eq!(profile_named_by(&argv(&["dl", "ws"])), None); + // A flag with nothing after it is clap's error to report, not a profile. + assert_eq!( + profile_named_by(&argv(&["dl", "ws", "--claude-profile"])), + None + ); + assert_eq!( + profile_named_by(&argv(&["dl", "--claude-profile=", "ws"])), + None + ); + } + + #[test] + fn a_repeated_profile_never_reaches_a_running_agent() { + // Why this reader needs no first-versus-last rule, asserted rather than + // assumed: clap refuses a repeated `--claude-profile` outright, so no agent is + // ever running with two of them in its argv. Written down because the reader + // looks like it has a choice to make here, and a future `ArgAction::Append` on + // that flag would give it one -- at which point this test fails and says so. + // + // The clap half lives in `dl` and cannot be called from this crate, so what is + // pinned here is the consequence: whichever single occurrence an argv holds, + // both spellings and both positions answer with it. + assert_eq!( + profile_named_by(&argv(&["dl", "ws", "--claude-profile", "work"])), + Some("work") + ); + assert_eq!( + profile_named_by(&argv(&["dl", "--claude-profile=work", "ws"])), + Some("work") + ); + // What sits after `--` is the command's, so a flag there is never a profile. + assert_eq!( + profile_named_by(&argv(&["dl", "ws", "--", "--claude-profile", "personal",])), + None + ); + } + + #[test] + fn a_prompt_that_mentions_the_flag_is_not_a_profile() { + // The case that decides how this is written. `aid add --claude-profile + // support to the docs` becomes a dl line whose prompt is one argument holding + // that text, and the ssh transport carries the same string as one payload + // argument. Elements are compared whole, so neither is a flag; the `--` stop + // says it a second way. + assert_eq!( + profile_named_by(&argv(&[ + "dl", + "ws", + "--", + "claude", + "add --claude-profile support to the docs", + ])), + None + ); + assert_eq!( + profile_named_by(&argv(&[ + "ssh", + "ws.devpod", + "claude 'add --claude-profile support'", + ])), + None + ); + // And a real flag *before* the `--` is still read, so the stop is a stop and + // not a refusal. + assert_eq!( + profile_named_by(&argv(&[ + "dl", + "--claude-profile", + "work", + "ws", + "--", + "claude", + "--claude-profile decoy", + ])), + Some("work") + ); + } + + #[test] + fn the_profile_comes_from_the_pane_that_named_the_workspace() { + // One pane answers both questions, so a tab holding two sessions cannot pair + // one session's workspace with another's account. The workspace comes off the + // transport process and the profile off `dl`, which are two processes in the + // same pane. + let info = herdr::PaneProcessInfo { + foreground_processes: vec![ + process(&["dl", "ws", "--claude-profile", "work"]), + process(&["ssh", "-F", "cfg", "ws-3j1t.devpod", "payload"]), + ], + }; + assert_eq!(profile_among(&info), Some("work")); + } + + #[test] + fn a_pane_with_no_profile_on_its_line_reports_none() { + let info = herdr::PaneProcessInfo { + foreground_processes: vec![process(&["dl", "ws"]), process(&["ssh", "ws.devpod"])], + }; + assert_eq!(profile_among(&info), None); + } + /// The diff test CLAUDE.md's standing rule asks for. [`workspace_named_by`] is /// a second copy of a shape two argv builders own, so both builders are run /// and their output is fed to the reader. Break either builder and this fails; @@ -1236,7 +1514,10 @@ mod tests { ); assert_eq!( destination(&runner, in_pane_of("w1:t1")), - PaneDestination::Workspace("devlaunch-main-3j1t".to_owned()) + PaneDestination::Workspace { + workspace_id: "devlaunch-main-3j1t".to_owned(), + claude_profile: None, + } ); } @@ -1307,7 +1588,10 @@ mod tests { ); assert_eq!( destination(&runner, in_pane_of("w1:t1")), - PaneDestination::Workspace("ws-3j1t".to_owned()) + PaneDestination::Workspace { + workspace_id: "ws-3j1t".to_owned(), + claude_profile: None, + } ); } diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index ada7421c..91164a18 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -133,7 +133,10 @@ pub(crate) fn dispatch( // indistinguishable from one typed by hand -- same launch, same terminal // title, same agent reporting, same everything a manager reads. Command::HerdrShell => match session_manager::pane_destination(runner) { - PaneDestination::Workspace(workspace_id) => { + PaneDestination::Workspace { + workspace_id, + claude_profile, + } => { let ending = dispatch( runner, cache, @@ -142,13 +145,13 @@ pub(crate) fn dispatch( target: workspace_id, verb: Verb::Attach { rm: RmOnExit::No }, devcontainer: None, - // Nothing to inherit, for the same reason `devcontainer` is - // `None`: a profile is named per launch and deliberately not - // stored with the workspace, so a pane opened beside an agent - // has no record of which login started it and gets the default - // one. Consistent with "indistinguishable from one typed by - // hand", since a hand-typed `dl ` names no profile either. - claude_profile: None, + // Inherited from the live sibling rather than from disk: a + // profile is named per launch and not stored with the + // workspace, so the running session is the only record of + // which account it is. Without this a pane opened beside an + // agent authenticated as a different account than the agent, + // which is the one way this feature could mislead quietly. + claude_profile, }, ); if pane_shell::no_session_ran(ending) {