From 67567e4e7b5368db67002adadcc25bccb6546f03 Mon Sep 17 00:00:00 2001 From: stephenleo Date: Sun, 6 Sep 2026 17:14:56 +0800 Subject: [PATCH] fix(account): surface malformed CSHIP_ACCOUNT in explain hint, warn on empty Follow-up to PR #201's CSHIP_ACCOUNT account source: - explain.rs: the "account" hint arm now checks CSHIP_ACCOUNT first, so a set-but-unparseable value is reported as the cause instead of the misleading "no credential found" / re-authenticate hint - modules/account.rs: parse_account_env now warns on set-but-empty CSHIP_ACCOUNT (previously silent), matching the "absent data -> warn" convention already used for the malformed-JSON case - docs/configuration.md: qualify the two now-inaccurate unconditional statements about the OAuth fetch/cache and credential-store prerequisite with "unless CSHIP_ACCOUNT is set" Co-Authored-By: Claude Opus 5 (1M context) --- docs/configuration.md | 4 +-- src/explain.rs | 58 ++++++++++++++++++++++++++++++++++++++++++ src/modules/account.rs | 5 +++- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 610d904..2f13e11 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -507,7 +507,7 @@ specific diagnostics. ## `[cship.account]` — Authenticated Account -Displays which Anthropic account the active Claude Code session is signed in to — handy for telling work and personal accounts apart at a glance. Profile data is fetched once from the OAuth `/api/oauth/profile` endpoint and cached for 24 hours (the profile rarely changes). The OAuth token is held only for the duration of the fetch — never written to disk, cache, stdout, or stderr. +Displays which Anthropic account the active Claude Code session is signed in to — handy for telling work and personal accounts apart at a glance. Unless `CSHIP_ACCOUNT` is set (see below), profile data is fetched once from the OAuth `/api/oauth/profile` endpoint and cached for 24 hours (the profile rarely changes). The OAuth token is held only for the duration of the fetch — never written to disk, cache, stdout, or stderr. **Token:** `$cship.account` @@ -541,7 +541,7 @@ Because field values render verbatim, a launcher can also embed ANSI color in th CSHIP_ACCOUNT='{"organization_name":"Acme Corp","organization_tier":"Team","account_display_name":"work"}' ``` -**Prerequisites:** Requires an OAuth token in the OS credential store (the same credential used by `usage_limits`). On Linux/WSL2, install `libsecret-tools` and store your token with `secret-tool`. If the module renders nothing, run `cship explain cship.account` for a diagnosis (missing credential, expired token, or unreachable API). +**Prerequisites:** Unless `CSHIP_ACCOUNT` is set, requires an OAuth token in the OS credential store (the same credential used by `usage_limits`). On Linux/WSL2, install `libsecret-tools` and store your token with `secret-tool`. If the module renders nothing, run `cship explain cship.account` for a diagnosis (missing credential, expired token, unreachable API, or a malformed `CSHIP_ACCOUNT` value). ```toml [cship.account] diff --git a/src/explain.rs b/src/explain.rs index 1222f8b..9b5ab4b 100644 --- a/src/explain.rs +++ b/src/explain.rs @@ -307,6 +307,18 @@ fn error_hint_for( } } "account" => { + // CSHIP_ACCOUNT takes priority over the keychain in the render path + // (see modules::account::resolve_profile), so a set-but-unparseable + // value is the real root cause here, not a missing/expired credential. + if let Ok(raw) = std::env::var("CSHIP_ACCOUNT") + && !raw.trim().is_empty() + && crate::modules::account::parse_account_env(&raw).is_none() + { + return ( + "account returned no data — CSHIP_ACCOUNT is set but not valid JSON".into(), + "Check the CSHIP_ACCOUNT value passed by your launcher; it must be compact JSON matching the account profile shape (e.g. organization_name, account_display_name).".into(), + ); + } // The account module renders nothing when the OAuth credential is // missing/malformed, or when it is present but the profile fetch // failed (and no fingerprint-matching stale cache is available). @@ -764,4 +776,50 @@ mod tests { // a no-op — same trade-off as the sibling test above. let _ = asserted_at_least_once; } + + #[test] + fn test_error_hint_account_malformed_env_names_env_as_cause() { + // No other test in this file touches CSHIP_ACCOUNT, so set/unset here + // doesn't race with a sibling test's own use of the var. + // ponytail: unsafe because std::env::set_var is process-global; scoped + // tightly around the single assertion this test needs. + unsafe { + std::env::set_var("CSHIP_ACCOUNT", "{not json"); + } + let ctx = crate::context::Context::default(); + let cfg = crate::config::CshipConfig::default(); + let (error, remediation) = error_hint_for("account", &ctx, &cfg); + unsafe { + std::env::remove_var("CSHIP_ACCOUNT"); + } + + assert!( + error.contains("CSHIP_ACCOUNT"), + "expected CSHIP_ACCOUNT named as the cause, got: {error}" + ); + assert!( + remediation.contains("CSHIP_ACCOUNT"), + "expected remediation to mention CSHIP_ACCOUNT, got: {remediation}" + ); + } + + #[test] + fn test_error_hint_account_empty_env_falls_back_to_credential_probe() { + // Empty CSHIP_ACCOUNT must not trigger the malformed-env hint; it + // falls through to the normal credential probe. + unsafe { + std::env::set_var("CSHIP_ACCOUNT", " "); + } + let ctx = crate::context::Context::default(); + let cfg = crate::config::CshipConfig::default(); + let (error, _) = error_hint_for("account", &ctx, &cfg); + unsafe { + std::env::remove_var("CSHIP_ACCOUNT"); + } + + assert!( + !error.contains("CSHIP_ACCOUNT"), + "empty CSHIP_ACCOUNT should not be reported as malformed, got: {error}" + ); + } } diff --git a/src/modules/account.rs b/src/modules/account.rs index 634f4dd..2d1f437 100644 --- a/src/modules/account.rs +++ b/src/modules/account.rs @@ -70,8 +70,9 @@ fn account_from_env() -> Option { /// Parse the `CSHIP_ACCOUNT` payload. Split from env access so the JSON contract /// is unit-testable. `None` for empty or malformed input (→ keychain fallback). -fn parse_account_env(raw: &str) -> Option { +pub(crate) fn parse_account_env(raw: &str) -> Option { if raw.trim().is_empty() { + tracing::warn!("cship.account: {ACCOUNT_ENV_VAR} set but empty"); return None; } match serde_json::from_str::(raw) { @@ -209,6 +210,8 @@ mod tests { #[test] fn test_parse_account_env_rejects_empty_and_malformed() { + // Both cases return None (fall back to keychain); tracing::warn! fires for + // both since a set-but-unusable CSHIP_ACCOUNT is worth surfacing. assert!(parse_account_env("").is_none()); assert!(parse_account_env(" ").is_none()); assert!(parse_account_env("{not json").is_none());