diff --git a/CHANGELOG.md b/CHANGELOG.md index 49c18d09..51a7a26f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`$CLAUDE_CONFIG_DIR` is now honoured on the host, so a host that has moved its + Claude configuration forwards its login instead of reporting itself as not logged + in.** Claude Code reads that variable before `~/.claude`, and the probe `dl` runs + inside a container has always read it too, because a devcontainer feature may set + it. Only the host side did not, and the symptom was silent: `claude` in every + workspace asked for a login while the host was authenticated, with nothing to say + why, because "no credential file at `~/.claude`" is also the ordinary macOS state + and so is deliberately not warned about. + + The variable **replaces** `~/.claude` rather than being tried ahead of it, which + is how Claude Code treats it. Falling back would forward a credential out of a + directory Claude Code is not reading, and that is the same defect one level down: + invisible until the host has two logins, at which point it forwards the wrong one. + An empty value counts as unset, and the value is read as bytes rather than as + text, so a configuration directory whose name is not valid UTF-8 is opened as + named instead of being mangled into one that cannot be found. An exported + `CLAUDE_CODE_OAUTH_TOKEN` still wins over both, so a `dl` inside a workspace keeps + passing its own token further down, and `DEVLAUNCH_NO_CLAUDE_TOKEN` still beats + everything. + ## [0.29.0] - 2026-09-02 ### Added diff --git a/README.md b/README.md index 272f6eeb..37b8862c 100644 --- a/README.md +++ b/README.md @@ -349,8 +349,9 @@ anything to its `devcontainer.json`. - **Your Claude login.** `claude` starts in the container without asking you to log in again. The host's access token is forwarded as `CLAUDE_CODE_OAUTH_TOKEN`, only into the sessions `dl` itself opens, so a `postCreateCommand` from a repo you did not write never sees it. Nothing is written - to the container's disk. A repo whose own devcontainer bind-mounts `~/.claude` is detected and - left alone. `DEVLAUNCH_NO_CLAUDE_TOKEN=1` skips it. A workspace that existed before this feature + to the container's disk. The token is read from `$CLAUDE_CONFIG_DIR` when your host sets one and + from `~/.claude` otherwise, which is the order Claude Code itself reads them in. A repo whose own + devcontainer bind-mounts `~/.claude` is detected and left alone. `DEVLAUNCH_NO_CLAUDE_TOKEN=1` skips it. A workspace that existed before this feature needs one `dl up` before it picks the login up, and so does a workspace rebuilt by a `devpod up` devlaunch did not run; workspaces created since do not. - **`gh` and `claude` on `PATH`.** If the image has them, nothing happens. If not, `dl` streams @@ -416,11 +417,12 @@ Images are yours: `docker system df` is what shows those. | `HERDR_AGENT=` | Written, not read: an `aid` launch that starts an agent, or a `dl -- ` whose command is one, names it here so a session manager can see it. See [docs/workspace-tools.md](docs/workspace-tools.md) | | `DEVLAUNCH_TIMING=1\|json` | Write a timing summary to stderr. See [docs/performance.md](docs/performance.md) | | `DEVPOD_SSH_CONFIG=` | devpod's own, honoured rather than set: it is where `devpod up` publishes host aliases, so it is where `dl` looks for them. See [docs/cli.md](docs/cli.md) | +| `CLAUDE_CONFIG_DIR=` | Claude Code's own, honoured rather than set: it is where the host keeps its Claude configuration, so it is where `dl` reads the login to forward. It replaces `~/.claude` rather than being tried before it, exactly as Claude Code treats it. See [docs/workspace-tools.md](docs/workspace-tools.md) | Every switch here reads the same values: anything but empty, `0`, `false` or `no` counts as -set. On a "no" variable that means turn it off; on an opt-in one it means turn it on. Five -rows are not switches and do not follow it: `DEVLAUNCH_AID_AGENT` and `DEVPOD_SSH_CONFIG` -take a value, `DEVLAUNCH_TIMING` counts only empty and `0` as off, so `false` and `no` +set. On a "no" variable that means turn it off; on an opt-in one it means turn it on. Six +rows are not switches and do not follow it: `DEVLAUNCH_AID_AGENT`, `DEVPOD_SSH_CONFIG` and +`CLAUDE_CONFIG_DIR` take a value, `DEVLAUNCH_TIMING` counts only empty and `0` as off, so `false` and `no` turn it on, `DEVLAUNCH_AID_REMOTE_CONTROL` takes `1`/`true`/`on`/`yes` or `0`/`false`/`off`/`no` and refuses anything else rather than guessing, and `HERDR_AGENT` is the one written rather than read, so a value of your own survives only a line that starts diff --git a/docs/workspace-tools.md b/docs/workspace-tools.md index 27bc2c00..7bda4fc1 100644 --- a/docs/workspace-tools.md +++ b/docs/workspace-tools.md @@ -58,8 +58,9 @@ stays in place, including one it was given before you set ## Claude authentication `claude` starts in every workspace `dl` opens without asking for a login. The -host's access token is read from `~/.claude/.credentials.json` and forwarded as -`CLAUDE_CODE_OAUTH_TOKEN`. Only the variable's name reaches a command line, which +host's access token is read from its Claude configuration directory, which is +`$CLAUDE_CONFIG_DIR` when the host sets one and `~/.claude` otherwise, and +forwarded as `CLAUDE_CODE_OAUTH_TOKEN`. Only the variable's name reaches a command line, which is the discipline the GitHub token keeps too, by a different route: that one is staged in a private file because `devpod up` needs it, and this one rides `--send-env` on the session, so the value travels in the environment and no file @@ -72,6 +73,44 @@ working status line; a repo with no `.devcontainer/` at all had neither, and the reported symptom was a blank status bar in a workspace where `claude-statusline` was installed and the `settings.json` naming it had never been applied. +### Which credential, on a host that has moved it + +`$CLAUDE_CONFIG_DIR` is Claude Code's own name for where its configuration lives, +and `dl` honours it rather than setting it, the way it honours `DEVPOD_SSH_CONFIG`. +The full order a launch reads: + +1. `DEVLAUNCH_NO_CLAUDE_TOKEN`, which forwards nothing at all. +2. Any `CLAUDE_CODE_OAUTH_TOKEN` the host has already exported. This is what lets a + `dl` running inside a workspace pass the token it was given further down, so a + workspace can launch a workspace. +3. `$CLAUDE_CONFIG_DIR/.credentials.json`, or `~/.claude/.credentials.json` when + the variable is unset. One step, not two: see below. + +**The variable replaces the default rather than being tried ahead of it.** Claude +Code does not fall back from `$CLAUDE_CONFIG_DIR` to `~/.claude`, and neither does +this. A fallback would forward a credential out of a directory Claude Code is not +reading, which is the same defect as ignoring the variable and harder to notice, +because it only shows itself on a host with two logins. So a `$CLAUDE_CONFIG_DIR` +that names a directory holding no credential is a host that is not logged in, and +the launch forwards nothing rather than the other account. It does that silently, +which is deliberate and is the same silence a missing `~/.claude` gets: on macOS +the credential lives in the login keychain and no file is the ordinary state, so a +warning there would fire on every correctly configured Mac. + +An empty value counts as unset, which is what a shell exporting a bare variable +means and the rule the XDG directories already follow here. The value is read as +bytes rather than as text, so a directory whose name is not valid UTF-8 is opened +as named. + +The exported token stays above the variable because both are ambient, and the +nested-workspace case has to keep working when no variable is set. + +This closes an asymmetry rather than adding a feature. The probe `dl` runs inside a +container has always read `$CLAUDE_CONFIG_DIR`, because a devcontainer feature may +set it and Claude Code honours it there too. The host side read `~/.claude` +regardless, so a host that had moved its configuration reported itself as not +logged in while holding a perfectly good login. + ### A variable, not the credential file Claude Code authenticates from `CLAUDE_CODE_OAUTH_TOKEN` alone, with an otherwise diff --git a/rust/devlaunch-core/src/clients/claude.rs b/rust/devlaunch-core/src/clients/claude.rs index 60fa6af0..3eeb7227 100644 --- a/rust/devlaunch-core/src/clients/claude.rs +++ b/rust/devlaunch-core/src/clients/claude.rs @@ -37,12 +37,29 @@ //! - Every session re-reads the host's credential, which is what makes //! refresh-on-start free. //! +//! # Which credential, on a host that has moved it +//! +//! `$CLAUDE_CONFIG_DIR` is honoured here as Claude Code honours it, and as the +//! container-side probe in [`crate::flows::provision`] has always honoured it. The +//! host side reading `$HOME/.claude` regardless was an asymmetry with one visible +//! symptom: a host that had moved its configuration reported [`NoToken::NotLoggedIn`] +//! while holding a perfectly good login. +//! +//! The order is opt-out, then any exported `CLAUDE_CODE_OAUTH_TOKEN`, then one +//! credential file: `$CLAUDE_CONFIG_DIR`'s when the variable is set and +//! `$HOME/.claude`'s when it is not. Those last two are one step and not two, which +//! is the whole of [`config_dir`]'s argument and worth spelling here because a list +//! of four reads as a fallback chain. The exported token stays above the variable +//! because both are ambient and that hatch is what lets a `dl` inside a workspace +//! forward what it was handed. +//! //! It also means a session devlaunch did not open — `devpod ssh` by hand, VS Code //! through `dl code` — does not get it. Those already have the host's real //! `claude` available to them by other means, and widening this to cover them //! would mean widening it to cover `postCreateCommand` too. -use std::path::Path; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; use super::gh::{Forwarding, forwarding_disabled}; use crate::runner::EnvSpec; @@ -55,8 +72,28 @@ pub(crate) const TOKEN_VAR: &str = "CLAUDE_CODE_OAUTH_TOKEN"; /// Set this to opt a machine out of forwarding the Claude login entirely. pub(crate) const DISABLE_VAR: &str = "DEVLAUNCH_NO_CLAUDE_TOKEN"; -/// Where the host keeps the credential, relative to `$HOME`. -const CREDENTIALS_RELPATH: &str = ".claude/.credentials.json"; +/// `$CLAUDE_CONFIG_DIR`: Claude Code's own name for where its configuration lives. +/// +/// Honoured rather than set, the way [`super::ssh::CONFIG_VAR`] is: Claude Code reads +/// this before `$HOME/.claude`, so a host that has moved its configuration has moved +/// the credential too, and reading `$HOME/.claude` regardless would inspect a +/// directory Claude Code does not use. The container side of this has always honoured +/// it -- see [`crate::flows::provision`]'s `claude_config_lines`, whose +/// `CLAUDE_CONFIG_DIR:-$HOME/.claude` is the same rule in shell -- and the host side +/// ignoring it was the asymmetry this closes. +const CONFIG_DIR_VAR: &str = "CLAUDE_CONFIG_DIR"; + +/// The configuration directory's name under `$HOME`, when the variable says nothing. +/// +/// Deliberately *not* shared with [`crate::flows::provision`]'s `CLAUDE_CONFIG_RELPATH`, +/// which spells the same string about the *container*. Two machines, two facts: that one +/// is Claude Code's default inside a container devlaunch did not build, this one is a +/// path on the host devlaunch is running on. A comment in each names the other, which is +/// the right amount of coupling for a coincidence of spelling. +const CONFIG_RELPATH: &str = ".claude"; + +/// The credential file's name inside whichever directory the above resolves to. +const CREDENTIALS_FILENAME: &str = ".credentials.json"; /// The key the OAuth credential sits under, and the field wanted from it. const OAUTH_KEY: &str = "claudeAiOauth"; @@ -115,6 +152,22 @@ pub(crate) struct HostEnv { /// what lets a `dl` running inside a workspace pass its own forwarded token /// further down — the same courtesy [`super::gh::HostEnv`] extends. pub(crate) token: Option, + /// `CLAUDE_CONFIG_DIR`, read *after* the exported token and *before* `$HOME`. + /// + /// Below the token because both are ambient, and the existing hatch above has to + /// keep winning so a `dl` launched from inside a workspace still forwards what it + /// was given. Above `$HOME` because it is what Claude Code itself prefers. + /// + /// An `OsString` and not a `String`, because this one is a **path** and the two + /// fields above are not. [`crate::osext::env_str`] decodes lossily on purpose, + /// which is right for a switch whose value is compared against words and wrong + /// here: a path is bytes, so a directory whose name is not valid UTF-8 would + /// arrive with U+FFFD where those bytes were, fail to open, and report + /// [`NoToken::NotLoggedIn`] on a host holding a perfectly good login. That is the + /// symptom this whole change removes, so reading it back in would be the same + /// defect wearing the fix's clothes. [`crate::domain::xdg`]'s `resolve` takes an + /// `OsString` for the same reason, and it is the rule this arm already cites. + pub(crate) config_dir: Option, } impl HostEnv { @@ -123,6 +176,8 @@ impl HostEnv { Self { disable: crate::osext::env_str(DISABLE_VAR), token: crate::osext::env_str(TOKEN_VAR), + // `var_os` rather than `osext::env_str`: see the field's own note. + config_dir: std::env::var_os(CONFIG_DIR_VAR), } } } @@ -155,7 +210,8 @@ pub(crate) enum TokenLookup { /// /// `home` is a parameter and not a read of `$HOME` so a test can state the host it /// means. No subprocess and no timing span, unlike [`super::gh::resolve_token`]: -/// this is one file read, and there is no CLI to ask. +/// this is one file read, and there is no CLI to ask. Which directory that read lands +/// in is [`config_dir`]'s decision. pub(crate) fn resolve_token(home: Option<&Path>, host: &HostEnv) -> TokenLookup { if forwarding_disabled(host.disable.as_deref()) { return TokenLookup::Missing(NoToken::OptedOut); @@ -163,10 +219,10 @@ pub(crate) fn resolve_token(home: Option<&Path>, host: &HostEnv) -> TokenLookup if let Some(token) = host.token.as_deref().and_then(Token::parse) { return TokenLookup::Found(token); } - let Some(home) = home else { + let Some(dir) = config_dir(home, host) else { return TokenLookup::Missing(NoToken::NotLoggedIn); }; - let path = home.join(CREDENTIALS_RELPATH); + let path = dir.join(CREDENTIALS_FILENAME); let Ok(text) = std::fs::read_to_string(&path) else { // Not distinguished from "no such file", because the two are the same // answer for the same reason: on macOS the credential is in the login @@ -180,6 +236,25 @@ pub(crate) fn resolve_token(home: Option<&Path>, host: &HostEnv) -> TokenLookup } } +/// The directory this host keeps its Claude configuration in, if it names one. +/// +/// `$CLAUDE_CONFIG_DIR` when it names something, `$HOME/.claude` otherwise, and +/// nothing at all on a machine with no home directory and no variable -- which is a +/// real state, since `dl` runs with `XDG_CACHE_HOME` set and no home. +/// +/// **The variable replaces the default rather than being tried before it.** Claude Code +/// does not fall back from `$CLAUDE_CONFIG_DIR` to `$HOME/.claude`, and a fallback here +/// would forward a credential out of a directory Claude Code is not reading -- the same +/// defect as ignoring the variable, and harder to see, because it only shows up on a +/// host with two logins. An empty value counts as unset, matching +/// [`crate::domain::xdg`]'s rule and what a shell exporting a bare variable means. +fn config_dir(home: Option<&Path>, host: &HostEnv) -> Option { + match host.config_dir.as_deref() { + Some(value) if !value.is_empty() => Some(PathBuf::from(value)), + _ => home.map(|home| home.join(CONFIG_RELPATH)), + } +} + /// Pull the access token out of a credential file's text. /// /// Deliberately not a `Deserialize` struct for the whole file. The file belongs to @@ -317,6 +392,174 @@ mod tests { } } + /// A credential file at an arbitrary directory, as `$CLAUDE_CONFIG_DIR` would name. + fn credential_dir_holding(token: &str) -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("a scratch config dir"); + std::fs::write( + dir.path().join(CREDENTIALS_FILENAME), + format!(r#"{{"claudeAiOauth":{{"accessToken":"{token}"}}}}"#), + ) + .expect("a credential"); + dir + } + + #[test] + fn the_host_honours_claude_config_dir() { + // The asymmetry this closes: the container-side probe has always read this + // variable and the host side did not, so a host that moved its configuration + // reported NotLoggedIn while sitting on a perfectly good login. + let moved = credential_dir_holding("not-a-real-moved-token"); + let host = HostEnv { + config_dir: Some(moved.path().into()), + ..HostEnv::default() + }; + assert_eq!( + resolve_token(None, &host), + TokenLookup::Found(Token("not-a-real-moved-token".to_owned())) + ); + } + + #[test] + fn the_config_dir_is_read_instead_of_home_rather_than_as_well() { + // Claude Code does not fall back from `$CLAUDE_CONFIG_DIR` to `$HOME/.claude`, + // and neither does this: falling back would forward a credential out of a + // directory Claude Code is not reading, which is the same defect as ignoring + // the variable, only harder to see. Two accounts on one host is exactly when + // it would bite. + let home = logged_in("not-a-real-home-token"); + let moved = credential_dir_holding("not-a-real-moved-token"); + let host = HostEnv { + config_dir: Some(moved.path().into()), + ..HostEnv::default() + }; + assert_eq!( + resolve_token(Some(home.path()), &host), + TokenLookup::Found(Token("not-a-real-moved-token".to_owned())) + ); + + // And an empty one names no directory at all, which is what the XDG rule + // already says elsewhere and what a shell exporting a bare variable means. + let empty = HostEnv { + config_dir: Some(OsString::new()), + ..HostEnv::default() + }; + assert_eq!( + resolve_token(Some(home.path()), &empty), + TokenLookup::Found(Token("not-a-real-home-token".to_owned())) + ); + } + + #[test] + fn a_config_dir_naming_nothing_is_not_logged_in_rather_than_a_fallback() { + // Ambient rather than typed, so its absence is the quiet arm and not a + // warning: the same reason a missing `$HOME/.claude` is NotLoggedIn. + let empty_dir = tempfile::tempdir().expect("a scratch config dir"); + let home = logged_in("not-a-real-home-token"); + let host = HostEnv { + config_dir: Some(empty_dir.path().into()), + ..HostEnv::default() + }; + assert_eq!( + resolve_token(Some(home.path()), &host), + TokenLookup::Missing(NoToken::NotLoggedIn) + ); + } + + #[test] + fn an_inherited_token_beats_the_config_dir() { + // Both are ambient, and the exported token keeps winning so that a `dl` + // launched from inside a workspace forwards the one it was handed. + let moved = credential_dir_holding("not-a-real-moved-token"); + let host = HostEnv { + token: Some("not-a-real-exported-token".to_owned()), + config_dir: Some(moved.path().into()), + ..HostEnv::default() + }; + assert_eq!( + resolve_token(None, &host), + TokenLookup::Found(Token("not-a-real-exported-token".to_owned())) + ); + } + + #[test] + fn the_opt_out_is_read_before_the_config_dir_is() { + // Set, therefore meant. A machine that has opted out has no account to choose. + let moved = credential_dir_holding("not-a-real-moved-token"); + let host = HostEnv { + disable: Some("1".to_owned()), + config_dir: Some(moved.path().into()), + ..HostEnv::default() + }; + assert_eq!( + resolve_token(None, &host), + TokenLookup::Missing(NoToken::OptedOut) + ); + } + + #[test] + fn the_refresh_token_still_does_not_travel_from_a_config_dir() { + // The module's whole reason for reading the file rather than shipping it, + // asserted on the new path as well as the old one. + let dir = tempfile::tempdir().expect("a scratch config dir"); + std::fs::write( + dir.path().join(CREDENTIALS_FILENAME), + r#"{"claudeAiOauth":{"accessToken":"not-a-real-access-token", + "refreshToken":"not-a-real-refresh-token"}}"#, + ) + .expect("a credential"); + let host = HostEnv { + config_dir: Some(dir.path().into()), + ..HostEnv::default() + }; + let TokenLookup::Found(token) = resolve_token(None, &host) else { + panic!("the credential should have been read"); + }; + assert!(!token.as_str().contains("refresh"), "{token:?}"); + } + + #[test] + #[cfg(unix)] + fn a_config_dir_whose_bytes_are_not_utf8_is_still_the_directory_it_names() { + // A path is bytes. Reading this variable through `osext::env_str`, which + // decodes lossily on purpose for the switches either side of it, put U+FFFD + // where the undecodable byte was and then failed to open the result -- so a + // host with a login reported NotLoggedIn, which is the exact symptom this + // change exists to remove. + use std::os::unix::ffi::OsStrExt as _; + + let parent = tempfile::tempdir().expect("a scratch parent"); + // `0xE9` is `é` in Latin-1: a legal byte in a Linux path and not valid UTF-8. + let mut name = std::ffi::OsString::from("cfg-caf"); + name.push(std::ffi::OsStr::from_bytes(&[0xE9])); + let moved = parent.path().join(name); + std::fs::create_dir(&moved).expect("a scratch config dir"); + std::fs::write( + moved.join(CREDENTIALS_FILENAME), + r#"{"claudeAiOauth":{"accessToken":"not-a-real-moved-token"}}"#, + ) + .expect("a credential"); + + let host = HostEnv { + config_dir: Some(moved.clone().into_os_string()), + ..HostEnv::default() + }; + assert_eq!( + resolve_token(None, &host), + TokenLookup::Found(Token("not-a-real-moved-token".to_owned())) + ); + + // And the lossy reading really would have missed it, so the test above is + // guarding something rather than restating that `PathBuf::from` is total. + let lossy = HostEnv { + config_dir: Some(moved.to_string_lossy().into_owned().into()), + ..HostEnv::default() + }; + assert_eq!( + resolve_token(None, &lossy), + TokenLookup::Missing(NoToken::NotLoggedIn) + ); + } + #[test] fn a_host_that_exported_one_itself_passes_it_further_down() { // A `dl` running inside a workspace has the variable and no credential file, @@ -360,7 +603,11 @@ mod tests { ] { let home = tempfile::tempdir().expect("a scratch home"); std::fs::create_dir_all(home.path().join(".claude")).expect("a config dir"); - std::fs::write(home.path().join(CREDENTIALS_RELPATH), text).expect("a file"); + std::fs::write( + home.path().join(CONFIG_RELPATH).join(CREDENTIALS_FILENAME), + text, + ) + .expect("a file"); assert!( matches!( resolve_token(Some(home.path()), &HostEnv::default()),