Fix #144: Check FOUNDRY_TOKEN/FOUNDRY_HOST env vars before keyring access

[anjor]

Problem

pltr stores credentials in macOS keyring. In headless environments, keyring access hangs and the process gets killed. The CLI supports FOUNDRY_TOKEN and FOUNDRY_HOST env vars in token.py, but AuthManager.__init__() always creates CredentialStorage() which imports keyring before env vars are checked.

Solution

This PR modifies src/pltr/auth/manager.py to:

1. Check environment variables FIRST in get_client() before accessing keyring
2. Create TokenAuthProvider directly when FOUNDRY_TOKEN and FOUNDRY_HOST are set
3. Implement lazy initialization of CredentialStorage to avoid keyring import during AuthManager initialization
4. Update validate_profile() to handle env vars similarly

Changes

• Modified AuthManager.get_client(): Added env var check at the beginning
• Modified AuthManager.validate_profile(): Added env var validation path
• Lazy CredentialStorage initialization: Only create storage when actually needed
• Comprehensive tests: Added 7 new test cases covering all env var scenarios
• Backward compatibility: All existing functionality preserved

Testing

• ✅ All existing tests continue to pass (1009 passed, 20 skipped)
• ✅ New tests verify env var authentication flow
• ✅ Tests verify fallback to profiles when env vars incomplete
• ✅ Tests verify lazy storage initialization

Result

When FOUNDRY_TOKEN and FOUNDRY_HOST are set, the CLI will now authenticate without ever touching the keyring, resolving the hanging issue in headless environments.

Fixes #144

[claude]

Code Review

Good fix for a real pain point - keyring hanging in headless environments is a common problem. The lazy initialization approach is clean. A few things worth discussing:

---

Medium: Explicit profile argument silently ignored when env vars are set

In get_client(), env vars are checked before the profile parameter is used. This means:

# User passes --profile staging, but FOUNDRY_TOKEN/FOUNDRY_HOST point elsewhere
result = manager.get_client(profile="staging")  # silently uses env vars, not staging

This could confuse users who have env vars set for one environment but explicitly pass a different profile. I'd suggest either:

• Letting an explicit profile argument take precedence over env vars, or
• Logging a warning when env vars override an explicit profile argument

The current behavior makes sense for the headless CI use case (where no profile is passed), but the interaction with explicit profiles is worth defining clearly.

---

Minor: validate_profile() has inconsistent return paths

The env var path calls provider.validate() (which can return True/False based on actual validation), while the profile path always returns True as long as get_client() doesn't raise:

# Env var path - actual validation
if foundry_token and foundry_host:
    provider = TokenAuthProvider(token=foundry_token, host=foundry_host)
    return provider.validate()  # real True/False

# Profile path - always True if no exception
self.get_client(profile)
# comment says validation happens later in verify command

This inconsistency is minor since the profile path's behavior is pre-existing and documented, but worth a comment explaining why the two paths behave differently.

---

Minor: Duplicated env var lookup

The same os.environ.get pattern is copied in both get_client() and validate_profile(). A small private helper would eliminate the duplication:

def _get_env_credentials(self):
    """Return (token, host) from env vars, or (None, None) if not fully set."""
    token = os.environ.get("FOUNDRY_TOKEN")
    host = os.environ.get("FOUNDRY_HOST")
    return (token, host) if (token and host) else (None, None)

---

Observation: test_validate_profile_with_env_vars doesn't mock ProfileManager

@patch.dict(os.environ, {"FOUNDRY_TOKEN": "env_token", "FOUNDRY_HOST": "..."})
@patch("pltr.auth.manager.TokenAuthProvider")
def test_validate_profile_with_env_vars(self, mock_token_provider_class):
    manager = AuthManager()  # real ProfileManager instantiated here

The real ProfileManager is created. This works now because it doesn't touch the filesystem in __init__, but it's a latent fragility. Consistent with other tests that patch both dependencies would be safer.

---

What's good

• Lazy initialization via property is the right pattern - clean and Pythonic
• Adding @patch.dict(os.environ, {}, clear=True) to existing tests is exactly the right fix to prevent env var leakage between tests
• The test_lazy_storage_initialization test is thorough and verifies the singleton behavior
• Backward compatibility is preserved - existing tests all pass

The core fix is correct and solves the stated problem well.

[claude]

PR Review: Fix #144 - Check env vars before keyring access

This is a clean, well-motivated fix. The lazy initialization approach and env-var-first logic directly solve the headless environment hang. Overall structure is good - a few issues worth addressing before merging.

---

Issues

1. test_get_client_with_partial_env_vars is not hermetic (potential CI flakiness)

The @patch.dict at line 291 is missing clear=True:

@patch.dict(os.environ, {"FOUNDRY_TOKEN": "env_token"})  # Missing FOUNDRY_HOST

If FOUNDRY_HOST happens to be set in the CI environment or a developer shell, both env vars are present and the env-var fast-path fires instead of falling back to profile. Assertions break silently. Fix:

@patch.dict(os.environ, {"FOUNDRY_TOKEN": "env_token"}, clear=True)

---

2. Duplicate test

test_validate_profile_without_env_vars (new) and test_validate_profile (existing, line 429) are functionally identical - same patch, same assertions, same profile name. The only difference is the new one explicitly clears env vars. The old test adds no incremental coverage and having two near-identical tests creates confusion. Recommend removing test_validate_profile or repurposing it to cover a distinct edge case.

---

3. test_validate_profile_with_env_vars creates a real ProfileManager

@patch("pltr.auth.manager.TokenAuthProvider")
def test_validate_profile_with_env_vars(self, mock_token_provider_class):
    manager = AuthManager()  # ProfileManager() called without a mock

AuthManager.init always calls ProfileManager() eagerly. Unlike all the other new env-var tests, this one does not patch ProfileManager, so a real instance is created that may read from the filesystem. For consistency and isolation, it should be patched:

@patch("pltr.auth.manager.ProfileManager")
@patch("pltr.auth.manager.TokenAuthProvider")
def test_validate_profile_with_env_vars(self, mock_token_provider_class, _mock_pm):

---

4. Silent profile override - UX concern

When env vars are set, an explicit --profile staging passed by the user is silently discarded. A user who has FOUNDRY_TOKEN/FOUNDRY_HOST set globally but wants to test a specific profile will get the wrong credentials with no feedback. A logging.debug message would help:

if foundry_token and foundry_host:
    # logging.debug("FOUNDRY_TOKEN/FOUNDRY_HOST set; ignoring profile argument")
    provider = TokenAuthProvider(token=foundry_token, host=foundry_host)
    return provider.get_client()

---

Minor observations

• validate() is a no-op validation - TokenAuthProvider.validate() just constructs a client object and returns True without making a network call (pre-existing TODO comment). Worth noting so future contributors understand that validate_profile in the env-var path does not actually verify credentials against the server.

• Thread safety of lazy storage property - The if self._storage is None pattern is not thread-safe. Fine for a CLI, but worth a brief comment noting the single-threaded assumption.

• @patch.dict(os.environ, {}, clear=True) on existing tests - Good defensive practice applied consistently; this was the right call.

---

Summary

Core fix	Correct and clean
Lazy initialization	Idiomatic property pattern
Test coverage	Good breadth
Flaky test risk	Partial env var test missing clear=True (issue 1)
Test isolation	Missing ProfileManager mock (issue 3)
Duplicate test	Should remove or differentiate (issue 2)
UX	Silent profile override (issue 4)

The critical fix before merging is issue 1 (missing clear=True) - it can cause non-deterministic CI failures. Issues 2 and 3 are cleanup that improve test quality.