feat: detect API keys from env vars and pflow settings

[spinje]

Summary

Enhances LLM provider detection to check multiple sources for API keys, not just the llm CLI. This makes model suggestions and actual LLM calls work reliably for all users regardless of how they configured their API keys.

Fixes #7

Changes

Environment Injection

• Add inject_settings_env_vars() to inject API keys from ~/.pflow/settings.json into os.environ at startup
• Called early in CLI (main.py) and MCP server (mcp_server/main.py) before any LLM operations
• User's actual environment variables are never overwritten (priority preserved)

Multi-Source Detection

• Add _has_provider_key() to check multiple sources in order:
  1. Environment variables (os.environ)
  2. pflow settings (settings.json env section)
  3. llm CLI keys (existing _has_llm_key() fallback)
• Add PROVIDER_ENV_VARS mapping for each provider's environment variable names

Error Message Improvements

• Unknown model errors now suggest a working model based on detected API keys
• Generic LLM errors now include the original exception details
• Fix trace to correctly show success=false when node returns "error" action

Files Changed

File	Changes
src/pflow/core/llm_config.py	Add injection + multi-source detection functions
src/pflow/cli/main.py	Call inject_settings_env_vars() at startup
src/pflow/mcp_server/main.py	Call inject_settings_env_vars() at startup
src/pflow/nodes/llm/llm.py	Enhanced error messages with model suggestions
tests/test_core/test_llm_config_provider_detection.py	17 new tests
.taskmaster/bugfix/bugfix-log.md	Document the fixes

Created Docs

• scratchpads/robust-provider-detection/implementation-spec.md - Comprehensive implementation spec

Testing

# Run new provider detection tests
uv run pytest tests/test_core/test_llm_config_provider_detection.py -v

# Run all LLM config tests
uv run pytest tests/test_core/test_llm_config*.py -v

# Full test suite
make test

Manual Verification

# With env var
export ANTHROPIC_API_KEY="test-key"
uv run pflow /tmp/test-workflow.json
# Shows: "Tip: Your API key supports 'anthropic/claude-sonnet-4-5'"

# With pflow settings
uv run pflow settings set-env OPENAI_API_KEY "test-key"
uv run pflow /tmp/test-workflow.json
# Shows: "Tip: Your API key supports 'gpt-5.2'"

---

[claude]

Code Review: Robust LLM Provider Detection

This PR implements multi-source API key detection to improve model suggestions and enable API keys stored in pflow settings. Overall, the implementation is solid and well-tested. Here's my detailed feedback:

---

✅ Critical — must fix before merge

None identified. The implementation follows security best practices and handles edge cases well.

---

⚠️ Warnings — should be addressed

1. Module-level import in llm_config.py introduces tight coupling

Location: src/pflow/core/llm_config.py:13

from pflow.core.settings import SettingsManager

Issue: The original implementation used lazy imports to avoid circular dependencies. This module-level import creates tight coupling between llm_config and settings, which could cause issues if settings ever needs to import from llm_config.

Current usage in _has_provider_key() (line 136):

# 2. Check pflow settings (uses module-level import)
try:
    manager = SettingsManager()

Recommendation: Revert to lazy import pattern for consistency and future-proofing:

-from pflow.core.settings import SettingsManager
+# SettingsManager imported lazily in _has_provider_key() to avoid circular imports

 def _has_provider_key(provider: str) -> bool:
     # ...
     # 2. Check pflow settings
     try:
+        from pflow.core.settings import SettingsManager
         manager = SettingsManager()

The comment in the docstring already mentions "uses module-level import" which suggests this was a conscious choice, but the previous pattern (lazy import in inject_settings_env_vars line 268) was more defensive.

2. Test isolation issue: Environment pollution between tests

Location: tests/test_core/test_llm_config_provider_detection.py:30

def test_injects_keys_from_settings(self, monkeypatch):
    # ...
    inject_settings_env_vars()

assert os.environ.get("TEST_API_KEY") == "injected-value"

Issue: The test modifies os.environ but doesn't clean up afterward. While monkeypatch.delenv() is used at the start, if the test fails mid-execution, the environment variable remains.

Recommendation: Use monkeypatch.setenv() pattern or add explicit cleanup:

def test_injects_keys_from_settings(self, monkeypatch):
    monkeypatch.delenv("TEST_API_KEY", raising=False)
    monkeypatch.delenv("PYTEST_CURRENT_TEST", raising=False)
    
    mock_manager = MagicMock()
    mock_manager.list_env.return_value = {"TEST_API_KEY": "injected-value"}
    
    with patch("pflow.core.llm_config.SettingsManager", return_value=mock_manager):
        from pflow.core.llm_config import inject_settings_env_vars
        inject_settings_env_vars()
    
    try:
        assert os.environ.get("TEST_API_KEY") == "injected-value"
    finally:
        # Explicit cleanup
        os.environ.pop("TEST_API_KEY", None)

Or better yet, use monkeypatch to verify injection happened without checking os.environ directly.

3. Missing test for test environment guard

Location: tests/test_core/test_llm_config_provider_detection.py

Issue: The inject_settings_env_vars() function has a guard for test environments (line 263-265):

if os.environ.get("PYTEST_CURRENT_TEST"):
    logger.debug("Skipping env injection in test environment")
    return

But there's no test verifying this behavior. The test suite explicitly removes PYTEST_CURRENT_TEST to test the injection, but we should also verify that injection is properly skipped when it IS set.

Recommendation: Add test case:

def test_skipped_in_pytest_environment(self, monkeypatch):
    """Injection is skipped when PYTEST_CURRENT_TEST is set."""
    monkeypatch.setenv("PYTEST_CURRENT_TEST", "test_module::test_name")
    
    mock_manager = MagicMock()
    mock_manager.list_env.return_value = {"SHOULD_NOT_INJECT": "value"}
    
    with patch("pflow.core.llm_config.SettingsManager", return_value=mock_manager):
        from pflow.core.llm_config import inject_settings_env_vars
        inject_settings_env_vars()
    
    # Manager should never be called
    mock_manager.list_env.assert_not_called()
    assert "SHOULD_NOT_INJECT" not in os.environ

---

💡 Suggestions — optional improvements

1. Consider adding a helper for repeated test setup pattern

Location: tests/test_core/test_llm_config_provider_detection.py (multiple test methods)

Pattern repeated across tests:

monkeypatch.delenv("PYTEST_CURRENT_TEST", raising=False)

Suggestion: Add a fixture to reduce boilerplate:

import pytest

@pytest.fixture
def non_test_env(monkeypatch):
    """Fixture that removes PYTEST_CURRENT_TEST to simulate production."""
    monkeypatch.delenv("PYTEST_CURRENT_TEST", raising=False)

class TestInjectSettingsEnvVars:
    def test_injects_keys_from_settings(self, monkeypatch, non_test_env):
        # No need to delenv PYTEST_CURRENT_TEST manually
        ...

This makes the intent clearer and reduces repetition.

2. Documentation: Clarify why injection happens at two different points

Location: src/pflow/cli/main.py:3561 and src/pflow/mcp_server/main.py:43

Both call inject_settings_env_vars() but at slightly different times in their initialization sequence. The CLI calls it before context initialization (line 3561), while MCP server calls it with an explicit guard check (line 40).

Suggestion: Add a comment explaining the timing requirements:

# CLI (main.py line ~3560)
# Inject API keys from pflow settings into environment
# MUST happen before any LLM operations (planner, validation, etc.)
_inject_settings_env_vars()

# MCP Server (main.py line ~38)
# Inject API keys from pflow settings into environment
# MUST happen before installing Anthropic model and registering tools
if not os.environ.get("PYTEST_CURRENT_TEST"):
    from pflow.core.llm_config import inject_settings_env_vars
    inject_settings_env_vars()

The MCP server version has a redundant check (the function itself guards against test environments), which could be confusing.

3. Error message could mention pflow settings path

Location: src/pflow/nodes/llm/llm.py:256-261

When suggesting a model based on detected API keys, the error message could be more helpful by mentioning that the key might be from pflow settings:

if detected_model:
    error_detail = (
        f"Unknown model: {prep_res['model']}. "
        f"Tip: Your API key supports '{detected_model}' "
        f"(detected from environment or pflow settings). "
        f"Run 'llm models' to see all available models."
    )

This helps users understand where the detection came from.

4. Consider logging which source provided the key

Location: src/pflow/core/llm_config.py:_has_provider_key()

The function logs when a key is found, but at INFO level this could be noisy. The existing debug logs are good, but for troubleshooting it might be useful to return not just bool but also source info (for logging at higher levels if needed).

This is optional and may be over-engineering for current needs.

---

Summary Assessment

Code Quality: ✅ Excellent

• Clean separation of concerns
• Comprehensive test coverage (17 tests)
• Follows existing patterns (security validation, error handling)
• Good documentation and docstrings

Security: ✅ Strong

• Uses existing ALLOWED_PROVIDERS allowlist
• No key values logged (only key existence)
• Respects environment variable priority
• Graceful degradation on errors

Test Quality: ✅ Good (with minor improvements needed)

• Tests cover happy paths, edge cases, and error conditions
• Good use of mocking to isolate units
• Missing: test for PYTEST_CURRENT_TEST guard behavior
• Minor: environment cleanup could be more robust

Performance: ✅ Optimal

• Multi-source detection stops at first found key (short-circuit)
• Caching already exists for model detection
• Injection is idempotent and lightweight

Maintainability: ✅ Strong

• Clear function names and responsibilities
• Well-commented for future developers
• Follows "Write code optimized for change" principle

---

Verdict

Recommend: Approve with minor fixes

The implementation is solid and ready for merge after addressing the Warnings section (particularly the module-level import and test environment guard test). The Suggestions are truly optional quality-of-life improvements.

Great work on comprehensive testing and documentation! The multi-source detection significantly improves UX for users who store API keys in environment variables or pflow settings. 🎯