fix(auth): honor config-defaulted --aspect-* flags, user-over-repo default, and migrate legacy credentials

crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags.axl

@@ -78,15 +78,23 @@ def deployment_flag_args() -> dict:
     }
 
 def _want(ctx, name: str, umbrella: bool) -> (bool, bool):
-    """Resolve capability arg `name` to `(on, explicit)`: an explicitly-set
-    granular flag wins (and is explicit); otherwise it follows the
-    `--aspect-remote` umbrella (not explicit).
-
-    The distinction drives error handling: an explicitly-requested capability the
+    """Resolve capability arg `name` to `(on, requested)`: a directly-set granular
+    flag wins over the `--aspect-remote` umbrella; otherwise the flag follows it.
+
+    A flag is "directly set" when it was given on the CLI (`is_explicit`) or turned
+    on via a `config.axl` default. The granular flags default off, so a resolved
+    `True` covers the config-on case that `is_explicit` (CLI-only) misses; a CLI
+    `--aspect-remote-*=false` is caught by `is_explicit` so it still opts that
+    capability out of the umbrella. (A config default of `False` reads the same as
+    the built-in default, so config cannot opt a capability out of the umbrella —
+    only the CLI can.)
+
+    `requested` drives error handling: a directly-requested capability the
     deployment can't serve is an error, whereas the umbrella silently skips a
     capability the deployment doesn't advertise ("use whatever it offers")."""
-    if ctx.args.is_explicit(name):
-        return (getattr(ctx.args, name), True)
+    value = getattr(ctx.args, name)
+    if ctx.args.is_explicit(name) or value:
+        return (value, True)
     return (umbrella, False)
 
 def deployment_endpoint_flags(ctx) -> DeploymentFlags:

crates/aspect-cli/src/builtins/aspect/private/lib/deployment_flags_test.axl

@@ -103,6 +103,17 @@ def _test_umbrella_reports_injected_capabilities(ctx):
         ("bes backend", "grpcs://bes.gcp"),
     ])
 
+def _test_config_default_granular_honored(ctx):
+    # A granular flag turned on via a config.axl default (value True, but NOT
+    # is_explicit — that's CLI-only) must be honored, not ignored in favor of the
+    # umbrella. Regression guard for the is_explicit-only gate.
+    got = deployment_endpoint_flags(_ctx(
+        _endpoints("gcp", cache = "remote.gcp", bes = "bes.gcp", exec = "remote.gcp"),
+        aspect_remote_cache = True,
+    ))
+    _eq("config-on cache → cache wired", got.base_flags, ["--remote_cache=grpcs://remote.gcp"])
+    _eq("config-on cache → no bes (umbrella not set)", got.bes_backends, [])
+
 def _test_join_and(ctx):
     _eq("empty", join_and([]), "")
     _eq("one", join_and(["remote cache"]), "remote cache")
@@ -116,6 +127,7 @@ _UNIT_TESTS = [
     _test_umbrella_skips_unadvertised_capability,
     _test_granular_cache_only,
     _test_explicit_false_overrides_umbrella,
+    _test_config_default_granular_honored,
     _test_umbrella_reports_injected_capabilities,
     _test_join_and,
 ]

crates/axl-runtime/src/engine/aspect/auth.rs

@@ -160,41 +160,41 @@ fn load_config_file(path: &PathBuf) -> anyhow::Result<Vec<Deployment>> {
 /// `.aspect/config.json` (when `$ASPECT_WORKSPACE` names one), overlaid by the
 /// user's `~/.aspect/config.json`. Overlay is by `name` (a later entry with the
 /// same name replaces an earlier one), so a user can override the seed or a
-/// repo-declared deployment. Exactly one entry stays `default` — [`select_deployment`]
-/// re-derives the default rather than trusting the flag on every entry.
+/// repo-declared deployment. Exactly one entry stays `default`, with a
+/// higher-precedence layer's default winning — see [`overlay_deployments`].
+/// [`select_deployment`] then re-derives the default rather than trusting the
+/// flag on every entry.
 fn load_deployments() -> anyhow::Result<Vec<Deployment>> {
     let mut merged: Vec<Deployment> = vec![default_deployment()];
-    let overlay = |merged: &mut Vec<Deployment>, entries: Vec<Deployment>| {
-        for entry in entries {
-            if let Some(existing) = merged.iter_mut().find(|d| d.name == entry.name) {
-                *existing = entry;
-            } else {
-                merged.push(entry);
-            }
-        }
-    };
     if let Some(repo_cfg) = repo_config_path() {
-        overlay(&mut merged, load_config_file(&repo_cfg)?);
+        overlay_deployments(&mut merged, load_config_file(&repo_cfg)?);
     }
-    overlay(&mut merged, load_config_file(&config_path()?)?);
-    reconcile_seed_default(&mut merged);
+    overlay_deployments(&mut merged, load_config_file(&config_path()?)?);
     Ok(merged)
 }
 
-/// The seed is re-created `default = true` on every load, but a configured
-/// deployment marked default must win. So clear the seed's default whenever any
-/// configured deployment claims it — leaving the seed as default only when
-/// nothing configured does (which is also the "logged out of the default" state).
-fn reconcile_seed_default(deployments: &mut [Deployment]) {
-    let configured_default = deployments
-        .iter()
-        .any(|d| d.default && d.name != DEFAULT_DEPLOYMENT_NAME);
-    if configured_default {
-        if let Some(seed) = deployments
-            .iter_mut()
-            .find(|d| d.name == DEFAULT_DEPLOYMENT_NAME)
-        {
-            seed.default = false;
+/// Overlay a config layer's `entries` onto `merged` (by `name`), preserving the
+/// "exactly one default" invariant with the incoming layer winning.
+///
+/// Each layer is written with at most one default (see [`upsert_deployment`]).
+/// The built-in seed starts `default = true`, and a repo layer may mark its own
+/// default, so without care a later user layer marking a different deployment
+/// would leave two defaults — and [`select_deployment`] would pick the earlier
+/// (repo) one, so the user could never override a repo default. So when this
+/// layer introduces a default, clear the flag on every prior entry first; a layer
+/// that declares no default leaves the existing one untouched.
+fn overlay_deployments(merged: &mut Vec<Deployment>, entries: Vec<Deployment>) {
+    let layer_has_default = entries.iter().any(|d| d.default);
+    if layer_has_default {
+        for d in merged.iter_mut() {
+            d.default = false;
+        }
+    }
+    for entry in entries {
+        if let Some(existing) = merged.iter_mut().find(|d| d.name == entry.name) {
+            *existing = entry;
+        } else {
+            merged.push(entry);
         }
     }
 }
@@ -2913,16 +2913,33 @@ mod tests {
     }
 
     #[test]
-    fn reconcile_seed_default_yields_to_configured_default() {
-        // Seed loads default=true; a configured default clears it.
-        let mut merged = vec![default_deployment(), dep("acme", true)];
-        reconcile_seed_default(&mut merged);
-        assert!(!merged[0].default, "seed yields to configured default");
-        assert!(merged[1].default);
-
-        // No configured default → the seed stays default (the fallback state).
-        let mut none_configured = vec![default_deployment(), dep("acme", false)];
-        reconcile_seed_default(&mut none_configured);
+    fn overlay_deployments_lets_a_layer_default_win() {
+        // Seed starts default=true; a layer that marks a configured default
+        // clears the seed's.
+        let mut merged = vec![default_deployment()];
+        overlay_deployments(&mut merged, vec![dep("acme", true)]);
+        assert!(!merged[0].default, "seed yields to a configured default");
+        assert!(merged.iter().find(|d| d.name == "acme").unwrap().default);
+
+        // A later layer marking a DIFFERENT deployment default must win over the
+        // earlier layer's default — the repo+user override case. Both must not
+        // remain default, else select_deployment picks the earlier (repo) one.
+        overlay_deployments(&mut merged, vec![dep("emca", true)]);
+        assert!(
+            !merged.iter().find(|d| d.name == "acme").unwrap().default,
+            "the earlier layer's default is cleared"
+        );
+        assert!(merged.iter().find(|d| d.name == "emca").unwrap().default);
+        assert_eq!(merged.iter().filter(|d| d.default).count(), 1);
+
+        // A layer that declares no default leaves the existing default untouched.
+        overlay_deployments(&mut merged, vec![dep("third", false)]);
+        assert!(merged.iter().find(|d| d.name == "emca").unwrap().default);
+        assert_eq!(merged.iter().filter(|d| d.default).count(), 1);
+
+        // No configured default anywhere → the seed stays the default fallback.
+        let mut none_configured = vec![default_deployment()];
+        overlay_deployments(&mut none_configured, vec![dep("acme", false)]);
         assert!(none_configured[0].default, "seed is the default fallback");
     }
 

crates/axl-runtime/src/engine/aspect/credential_store.rs

@@ -75,9 +75,21 @@ impl CredentialStore {
     /// read. The file backend additionally treats an *unparseable* file as empty
     /// (see `file_load_all`): a legacy/corrupt file means "re-run login", not a
     /// hard error on every command.
-    pub(crate) fn load_all<T: DeserializeOwned>(&self) -> anyhow::Result<HashMap<String, T>> {
+    ///
+    /// When the keyring is empty, credentials from the legacy `credentials.json`
+    /// (written by releases before the keyring backend) are migrated in, so
+    /// upgrading users stay logged in — see [`migrate_legacy_file`].
+    pub(crate) fn load_all<T: DeserializeOwned + Serialize>(
+        &self,
+    ) -> anyhow::Result<HashMap<String, T>> {
         match self {
-            Self::Keyring => keyring_load_all(),
+            Self::Keyring => {
+                let map = keyring_load_all()?;
+                if map.is_empty() {
+                    return migrate_legacy_file();
+                }
+                Ok(map)
+            }
             Self::File(path) => file_load_all(path),
         }
     }
@@ -123,6 +135,21 @@ fn keyring_load_all<T: DeserializeOwned>() -> anyhow::Result<HashMap<String, T>>
     }
 }
 
+/// One-time migration of credentials written by pre-keyring releases: read the
+/// legacy file at [`default_file_path`] via [`file_load_all`] and, if it holds
+/// any, write them into the keyring so subsequent reads are keyring-native.
+/// Returns whatever the file held (empty when there is nothing to migrate). The
+/// keyring write is best-effort — a write failure still returns the file's
+/// credentials so the user isn't spuriously logged out; the migration simply
+/// retries on the next load.
+fn migrate_legacy_file<T: DeserializeOwned + Serialize>() -> anyhow::Result<HashMap<String, T>> {
+    let map: HashMap<String, T> = file_load_all(&default_file_path()?)?;
+    if !map.is_empty() {
+        let _ = keyring_save_all(&map);
+    }
+    Ok(map)
+}
+
 fn keyring_save_all<T: Serialize>(map: &HashMap<String, T>) -> anyhow::Result<()> {
     let entry = keyring_entry()?;
     if map.is_empty() {
@@ -218,6 +245,29 @@ mod tests {
         let _ = fs::remove_dir_all(&dir);
     }
 
+    #[test]
+    fn legacy_file_is_read_for_migration() {
+        // `migrate_legacy_file` (keyring-empty fallback) reads the legacy file via
+        // `file_load_all`; this covers that read — a populated legacy file yields
+        // its credentials so an upgrading user with a reachable-but-empty keyring
+        // stays logged in, an absent one yields empty. The keyring write-through is
+        // best-effort and deliberately not exercised here (no hermetic keyring).
+        let dir = std::env::temp_dir().join(format!("aspect-cred-migrate-{}", std::process::id()));
+        let path = dir.join("credentials.json");
+
+        let empty: HashMap<String, String> = file_load_all(&path).unwrap();
+        assert!(empty.is_empty(), "absent legacy file → nothing to migrate");
+
+        let mut legacy = HashMap::new();
+        legacy.insert("default".to_string(), "tok-legacy".to_string());
+        CredentialStore::File(path.clone()).save_all(&legacy).unwrap();
+
+        let read: HashMap<String, String> = file_load_all(&path).unwrap();
+        assert_eq!(read, legacy);
+
+        let _ = fs::remove_dir_all(&dir);
+    }
+
     #[test]
     fn env_override_forces_file_backend() {
         // SAFETY: single-threaded test; var removed before returning.