diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e22c70a..d067470b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. + + **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 57d32b47..a869c661 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -2205,6 +2205,15 @@ fn begin_reporting( pane_id: reporting.pane_id().to_owned(), socket: reporting.container_socket(), }); + // 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. + session_manager::report_profile( + session.runner, + &reporting, + session.host.claude.profile.as_deref(), + ); Some((reporting, forward)) } session_manager::Prepared::Refused { reason } => { diff --git a/rust/devlaunch-core/src/flows/session_manager.rs b/rust/devlaunch-core/src/flows/session_manager.rs index 38f99974..e306c400 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 @@ -526,6 +569,49 @@ fn workspace_among(info: &herdr::PaneProcessInfo) -> Option { /// /// The program is compared by its last path component, so a `/usr/bin/ssh` and a /// bare `ssh` answer alike, exactly as [`herdr::agent_in`] does it. +/// 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> { + 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 +} + fn workspace_named_by(argv: &[String]) -> Option<&str> { let program = argv.first()?.rsplit('/').next()?; if program == devpod::PROGRAM { @@ -1104,6 +1190,165 @@ mod tests { /// and their output is fed to the reader. Break either builder and this fails; /// without it the pane shell would quietly stop finding workspaces and no test /// in the tree would notice. + /// 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 `, with --token NAME=VALUE and + // --clear-token NAME. 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_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); + } + #[test] fn both_transports_name_the_workspace_they_were_built_for() { let workspace_id = "devlaunch-main-3j1t"; @@ -1236,7 +1481,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 +1555,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 d8cd7ec7..bc4968bd 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) {