feat: unify LLM usage via Simon Willison's llm library (Task 95)

[spinje]

Summary

Fix critical bug where ALL Claude models were silently redirected to a hardcoded model (claude-sonnet-4-20250514), ignoring user's model choice. Migrate discovery and filtering features to use standard llm library with configurable models.

Changes

Bug Fix (Phase 1) - Critical

• Move monkey-patch from workflow_command() entry to planner-only path
• File/saved workflows now use standard llm library
• User's model choice is now respected
• Invalid models now properly error (not silently succeed)

Discovery Migration (Phase 2)

• Remove install_anthropic_model() from registry discover
• Remove install_anthropic_model() from workflow discover
• Use get_model_for_feature("discovery") for configurable model

Smart Filter + Settings (Phase 3+4)

• Add LLMSettings class with default_model, discovery_model, filtering_model
• Add get_model_for_feature() helper with resolution chain
• Smart filter uses configurable model instead of hardcoded Haiku

Default Model Configuration

• LLM nodes now require explicit model configuration
• Resolution order: IR params → settings → llm CLI → CompilationError
• Clear error messages with setup instructions

Model Updates

• Add GPT-5.2, Gemini 3 Flash to pricing
• Update default detection to use latest models
• Bump llm-gemini>=0.28.1, llm-anthropic==0.23

Files Changed (24 files)

Core Implementation:

• src/pflow/cli/main.py - Move monkey-patch to planner path
• src/pflow/cli/registry.py - Use configurable discovery model
• src/pflow/cli/commands/workflow.py - Use configurable discovery model
• src/pflow/core/llm_config.py - Add model resolution helpers
• src/pflow/core/settings.py - Add LLMSettings class
• src/pflow/core/smart_filter.py - Use configurable filtering model
• src/pflow/runtime/compiler.py - Add LLM model injection

Tests (21 new tests):

• tests/test_core/test_llm_config_workflow_model.py (15 tests)
• tests/test_runtime/test_compiler_llm_model.py (6 tests)

Task Documentation

• .taskmaster/tasks/task_95/implementation/progress-log.md
• .taskmaster/tasks/task_95/research/llm-usage-findings.md

Testing

All 3388 tests pass:

make test  → 3388 passed, 7 skipped
make check → All checks pass

Manual verification completed for all phases (see progress-log.md).

[claude]

Code Review: Task 95 - Unify LLM Usage via Simon Willison's llm Library

Overview

This PR successfully addresses a critical bug where all Claude models were being silently redirected to a hardcoded model, while also migrating the codebase to use a configurable LLM system based on Simon Willison's llm library. The implementation is well-structured, thoroughly tested (3388 passing tests), and includes comprehensive documentation.

---

✅ Strengths

Architecture & Design

• Excellent separation of concerns: The bug fix (Phase 1) is cleanly separated from feature additions (Phases 2-5)
• Proper error handling: Compile-time failures with helpful error messages instead of silent runtime failures
• Provider agnostic: Works with any LLM provider supported by the llm library (Anthropic, Gemini, OpenAI, OpenRouter, Ollama, etc.)
• Clear resolution chain: IR params → settings → llm CLI default → error (well documented and tested)

Code Quality

• Defensive programming: Input validation on provider names (allowlist), subprocess security (no shell injection), proper timeout handling
• Immutability preserved: params = {**params, "model": default_model} creates new dict instead of mutating IR
• Test coverage: 21 new tests covering all resolution paths, edge cases, and error conditions
• Documentation: Excellent docstrings with resolution order, examples, and clear explanations

Security

• Subprocess safety: Uses shutil.which() for path validation, stdin=subprocess.DEVNULL, timeouts, no shell expansion
• Provider allowlist: ALLOWED_PROVIDERS prevents arbitrary command injection
• No credentials in logs: Sensitive data properly handled

---

⚠️ Warnings — Should Be Addressed

1. Import Location in Compiler (compiler.py:605)

The import is placed inside the function, which is executed for every LLM node compilation:

if node_type == "llm" and "model" not in params:
    from pflow.core.llm_config import get_default_workflow_model, get_model_not_configured_help  # ← Inside function

Issue: Repeated imports on every LLM node compilation add unnecessary overhead.

Fix: Move to top-level imports:

# At top of compiler.py
from pflow.core.llm_config import get_default_workflow_model, get_model_not_configured_help

# In function
if node_type == "llm" and "model" not in params:
    default_model = get_default_workflow_model()
    # ...

Rationale: Follows Python best practices and the project's "boring and obvious" guideline. The circular import concern doesn't apply here since llm_config doesn't import from compiler.

---

2. Patch Import Pattern in llm_config.py (llm_config.py:243)

from pflow.core.settings import SettingsManager  # ← Inside function to avoid circular import

Issue: This pattern is used in multiple functions (get_model_for_feature, get_default_workflow_model), suggesting a deeper architectural issue.

Concern: If this is truly a circular import, it indicates tight coupling. If not, the comment is misleading.

Recommendation:

1. Verify if the circular import actually exists by testing top-level import
2. If it does exist, consider refactoring to break the cycle (e.g., extract shared utilities)
3. If it doesn't exist, move to top-level imports and remove misleading comment

---

3. Global State Caching (llm_config.py:16-18)

_cached_default_model: Optional[str] = None
_detection_complete: bool = False

Issue: Global mutable state makes testing harder and introduces potential bugs in concurrent scenarios.

Current mitigation: PYTEST_CURRENT_TEST check skips caching in tests.

Better approach:

• Use a singleton pattern with explicit cache management
• Or: Accept the cost of re-detection and remove caching entirely (subprocess calls are already fast with 1-2s timeouts)
• Or: Document thread-safety assumptions clearly

Why it matters: The current approach works but is fragile. If pflow adds multi-threading in the future, this could cause race conditions.

---

4. CI Debug Logging (llm_config.py:74-102)

if os.environ.get("CI"):
    print(f"[DEBUG CI] About to run subprocess: {command}", file=sys.stderr, flush=True)
    # ... more debug prints

Issue: This debug code is still present in production. It was likely added to diagnose CI issues but should be removed before merge.

Recommendation: Remove all [DEBUG CI] print statements or move to proper logging:

logger.debug(f"Running subprocess: {command}")  # Uses logging, controlled by log level

---

5. Test Mock Pattern Inconsistency (test_llm_config_workflow_model.py:37-39)

with patch.object(
    __import__("pflow.core.llm_config", fromlist=["get_llm_cli_default_model"]),
    "get_llm_cli_default_model",
    return_value="claude-3-sonnet",
):

Issue: This is unnecessarily complex. Compare to the cleaner approach in other tests:

with patch("pflow.core.llm_config.get_llm_cli_default_model") as mock_cli:
    mock_cli.return_value = "claude-3-sonnet"

Recommendation: Simplify the patching approach for consistency and readability.

---

💡 Suggestions — Optional Improvements

1. Error Message Formatting

The error message in get_model_not_configured_help() is excellent, but the JSON formatting could be improved for readability:

Current:

{"id": "my-llm", "type": "llm", "params": {"model": "gpt-5.2", "prompt": "..."}}

Suggested (multi-line JSON):

{
  "id": "my-llm",
  "type": "llm",
  "params": {
    "model": "gpt-5.2",
    "prompt": "..."
  }
}

This is more readable but not critical.

---

2. Type Hints in Test Fixtures

@pytest.fixture
def mock_registry(self):  # ← Missing return type
    """Create mock registry with LLM node."""
    registry = MagicMock()
    # ...
    return registry

Suggested:

@pytest.fixture
def mock_registry(self) -> MagicMock:
    """Create mock registry with LLM node."""

This improves IDE support and type checking, though not strictly necessary for test code.

---

3. Consolidate Model Resolution Logic

There's some duplication in model resolution between:

• get_default_workflow_model() (settings → llm CLI → None)
• get_model_for_feature() (settings → auto-detect → fallback)

Potential refactoring:

def _resolve_model(
    settings_getter: Callable[[LLMSettings], Optional[str]],
    allow_fallback: bool = False
) -> Optional[str]:
    """Generic model resolution with configurable fallback."""
    # Common logic here

This is a minor optimization and may not be worth the added abstraction at this stage.

---

🔍 Specific Code Observations

Good Practices Observed

1. Subprocess security: Excellent use of stdin=subprocess.DEVNULL, timeouts, and validated paths
2. Error message quality: The get_model_not_configured_help() message is a model for user-friendly errors
3. Test coverage: Edge cases well-covered (settings load failure, timeout, invalid provider, etc.)
4. Immutability: IR data structures are never mutated (new dict created in compiler)
5. Logging levels: Proper use of logger.debug() for internal operations and logger.info() for user-visible actions

Areas for Improvement

1. Inline imports: Should be moved to top-level where possible
2. Global state: Consider alternatives to module-level caching
3. Debug code: Remove CI-specific print statements
4. Test mocking: Use simpler patch() syntax consistently

---

📋 Test Quality Assessment

Excellent Coverage

• Resolution chain tested: All three sources (IR, settings, llm CLI) verified
• Error paths tested: Missing config, load failures, timeouts, invalid providers
• Edge cases tested: Empty stdout, subprocess failures, PYTEST_CURRENT_TEST skipping
• Integration verified: Manual testing confirms file workflows work correctly

Test Quality

• Tests are focused and clear - each tests one behavior
• Good use of fixtures - mock_registry reused appropriately
• Proper assertions - verifies both return values and function calls
• No flaky tests - deterministic mocking, no timing dependencies

Minor Improvements

• Some tests could be more concise (the patch.object(__import__()) pattern)
• Consider parameterized tests for similar scenarios (e.g., different providers)

---

🎯 Breaking Changes

The PR correctly identifies this as a breaking change for users who:

1. Don't specify model in workflows
2. Don't have settings configured
3. Don't have llm models default set

Mitigation: The error message provides three clear solutions with examples. This is the right approach - fail fast with helpful guidance.

---

📊 Overall Assessment

Criterion	Rating	Notes
Correctness	✅ Excellent	Bug fixed, new features work as intended
Code Quality	⚠️ Good	Minor issues (imports, debug code) prevent "Excellent"
Security	✅ Excellent	Subprocess handling, input validation exemplary
Test Coverage	✅ Excellent	Comprehensive, well-structured tests
Documentation	✅ Excellent	Clear docstrings, progress log, error messages
Architecture	✅ Excellent	Clean separation, proper abstractions
User Experience	✅ Excellent	Helpful errors, clear resolution chain

---

🏁 Final Recommendation

Approve with minor fixes - This is high-quality work that significantly improves the codebase. The warnings above are non-critical but should be addressed before merge:

Required Before Merge

1. ✅ Remove CI debug print statements (or convert to proper logging)
2. ✅ Move imports to top-level in compiler.py (avoid repeated imports)

Recommended Before Merge

3. Investigate and resolve/document circular import in llm_config.py
4. Simplify test mocking patterns for consistency

Optional (Future Tasks)

5. Consider alternatives to global state caching
6. Format error message JSON for better readability

---

💬 Additional Comments

This PR demonstrates excellent engineering practice:

• Clear problem identification and incremental solution (5 phases)
• Thorough manual and automated verification
• Comprehensive documentation (progress log is exemplary)
• User-centric design (error messages, configuration options)

The implementation follows the project's "boring and obvious" principle while solving a complex problem elegantly. Great work! 🎉

---

Reviewed by: Claude Code
Date: 2025-12-19
Test Status: ✅ 3388 passed, 7 skipped
Code Quality: ✅ All checks pass