[P2.19] Provider-aware pre-flight key checks + Think-phase provider resolution (#768)

[frankbria]

Summary

Implements #768: provider-selection gaps — pre-flight key check and Think-phase ignored --llm-provider.

• New codeframe/core/llm_resolution.py: single source of truth for the effective-provider chain (--llm-provider flag → CODEFRAME_LLM_PROVIDER → .codeframe/config.yaml llm: → anthropic) and the provider → required-key-env mapping (anthropic → ANTHROPIC_API_KEY, openai → OPENAI_API_KEY, local providers → none).
• runtime.py and ui/routers/prd_v2.py deduped onto the shared helper; runtime pre-flight now also fails loudly on a missing OPENAI_API_KEY for the openai provider.
• cf work start / cf work batch run: builtin-engine key check validates the key matching the resolved provider instead of always ANTHROPIC_API_KEY; cf work retry resolves env → config → default.
• cf prd stress-test and cf tasks generate: new --llm-provider / --llm-model flags; both route through the shared resolution instead of hardcoding Anthropic; provider threaded through generate_from_prd.
• Cross-family review fixes: --recursive --no-llm is now rejected explicitly, and a model override for the anthropic provider is honored via a pinned ModelSelector (previously silently dropped).

Acceptance Criteria

• A shared helper resolves the effective provider (flag→env→config→default) and validates the matching key
• work start/batch/retry pre-flight honors the resolved provider (non-anthropic no longer demands ANTHROPIC_API_KEY; openai demands OPENAI_API_KEY)
• prd stress-test and tasks generate route through the same provider resolution

Test Plan

• Unit tests written (TDD approach) — new tests/core/test_llm_resolution.py, +9 CLI integration tests
• All tests passing — full CI-equivalent gate: 4206 passed, 12 skipped
• Diff coverage 100% on changed lines (threshold 85%)
• Linting clean (ruff check .)
• Internal code review (advisory) — spawned; no findings reported at PR time (see limitations)
• Cross-family review pass: codex (2 Major findings, both fixed). opencode/GLM was attempted first but modified the working tree instead of staying read-only, so it was terminated and codex used as fallback.
• Test mutation sanity check completed (precedence-chain and openai-dispatch mutations both made the new tests fail)

Known Limitations / Intentionally Deferred

• cf work retry gets no --llm-provider/--llm-model flags — it re-executes via execute_agent, which already honors env/config; only its pre-flight became provider-aware.
• generate_from_prd(provider=None) (core default path, no CLI in play) still falls back to plain get_provider() (anthropic) rather than the full env/config chain — existing test seams patch the zero-arg get_provider(); all CLI/server paths pass an explicitly resolved provider.
• base_url remains config → OPENAI_BASE_URL only (no dedicated CLI flag), matching pre-existing behavior.

Implementation Notes

Plan was self-authored (issue had no implementation plan). Two runtime behavior improvements ride along by design: missing OPENAI_API_KEY for the openai provider now fails loudly at execution pre-flight, and model overrides now reach AnthropicProvider via a purpose-pinned ModelSelector.

Closes #768

[github-actions]

Claude finished @frankbria's task in 7m 12s —— View job

---

✅ GLM precision review: no defects found

Reviewed the full diff for #768 (provider-aware pre-flight key checks + Think-phase provider resolution) against the checked-out source and their callers/callees. Scope: concrete defects only (logic errors, null/unhandled crash paths, security, data loss, breaking API changes, resource leaks) — not style/architecture/coverage.

No confirmed findings. Key things verified:

• adapters/llm/__init__.py (anthropic branch): AnthropicProvider(api_key=kwargs.get("api_key"), model_selector=...) — api_key=None is equivalent to the old AnthropicProvider() (constructor falls back to os.getenv("ANTHROPIC_API_KEY")). ModelSelector(...) is constructed with exactly the 5 fields the dataclass defines (base.py:67-71), so no TypeError. No get_provider(...) caller passes model_selector/api_key kwargs, so honoring them is a safe latent fix, not a regression.
• core/llm_resolution.py: precedence chains (flag → env → config → default for provider/model; config → env for base_url) are byte-for-byte equivalent to the old inline logic in runtime.py and prd_v2.py — pure dedup. provider_kwargs() only emits set values; required-key map covers all valid provider types (anthropic/openai keyed, ollama/vllm/compatible/mock → none, matching get_provider's "not-required" fallback).
• cli/app.py tasks_generate: the --recursive --no-llm rejection fires before provider is used, so generate_task_tree(None) is unreachable; provider is set iff not no_llm and threaded to both the recursive and LLM branches. generate_from_prd's new provider=None param is backward-compatible (only other caller, discovery_v2.py:393, passes no provider).
• runtime.py / prd_v2.py: key check correctly broadened from anthropic-only to required_key_env-driven; external engines still skip the check and get provider=None. Consistent with the CLI pre-flight.
• work retry: pre-flight resolves env → config → default (no flag), matching the execute_agent(...) call at app.py:3084 which also passes no flags — no pre-flight/execution mismatch.

Investigated and ruled out (non-fatal): the batch SupervisorResolver.llm (conductor.py:125) still hardcodes zero-arg get_provider() (anthropic). Since this PR's batch pre-flight no longer guarantees ANTHROPIC_API_KEY for non-anthropic providers, cf work batch run --llm-provider openai (with OPENAI_API_KEY but no ANTHROPIC_API_KEY) would make the supervisor's lazy AnthropicProvider() raise on first blocker classification — but that access sits inside try/except Exception (conductor.py:216-228) and degrades to the existing pattern-matching fallback (logged warning), so it does not crash or lose data. Flagging here for awareness; not a blocking defect.

[frankbria]

Cross-family review — codex (OpenAI family, fallback reviewer)

opencode/GLM was attempted first per the reviewer chain but modified the working tree instead of staying read-only; it was terminated and its changes reverted. codex was used for both the pre-PR and post-PR passes.

Pre-PR pass (commit 1853f53): 2 Major (P2) findings, both verified and fixed in f8dfe00:

1. tasks generate --recursive --no-llm crashed with AttributeError on a None provider → now rejected explicitly with a clear error.
2. get_provider('anthropic', model=...) silently dropped the model override → now honored via a purpose-pinned ModelSelector.

Post-PR pass (f8dfe00, full branch diff vs main):

I did not identify any new, actionable correctness issues in the diff. The provider resolution changes are consistently wired through the CLI, runtime, task generation, and PRD router paths.

Verdict: APPROVE

[claude]

Review: [P2.19] Provider-aware pre-flight key checks + Think-phase provider resolution (#768)

No prior reviews on this PR to reconcile with — this is the first pass. Overall this is a clean, well-scoped refactor: codeframe/core/llm_resolution.py is a solid single source of truth for the flag → env → config → default chain, and the dedup of runtime.py / prd_v2.py onto it removes real duplication. Tests are thorough (precedence matrix, provider-specific key checks, the --recursive --no-llm edge case, the anthropic model-override regression). Nice adherence to CLAUDE.md's core-must-stay-headless rule — llm_resolution.py has no FastAPI/UI imports.

One real gap: POST /api/v2/discovery/generate-tasks still bypasses the chain

codeframe/ui/routers/discovery_v2.py:393 calls tasks.generate_from_prd(workspace, prd_record, use_llm=use_llm) with no provider argument. That falls through to _generate_tasks_with_llm(prd_content, provider=None) → get_provider() (codeframe/core/tasks.py:1005-1006), which hardcodes Anthropic and only ever reads ANTHROPIC_API_KEY directly — it never consults CODEFRAME_LLM_PROVIDER or .codeframe/config.yaml's llm: block.

This endpoint is live and wired to the web UI (web-ui/src/lib/api.ts:724 generateTasks()), and it's a Think-phase task-generation-from-PRD flow — exactly the kind of surface this PR is fixing for prd_v2.py's stress-test endpoint via _resolve_llm_provider. Right now a user with .codeframe/config.yaml set to llm: provider: ollama (or CODEFRAME_LLM_PROVIDER=openai) will have the CLI (cf tasks generate) and the PRD-stress-test web flow honor that config, but the web UI's "generate tasks from PRD" button will still silently try to build an AnthropicProvider and fail (or worse, succeed unexpectedly with a stale Anthropic key) — inconsistent with its sibling endpoint in the same router family.

This also makes the PR description's "Known Limitations" claim slightly inaccurate: "existing test seams patch the zero-arg get_provider(); all CLI/server paths pass an explicitly resolved provider." That's true for prd_v2.py and the CLI, but not for discovery_v2.py:generate-tasks.

Suggested fix, mirroring what's already done in prd_v2.py:

from codeframe.core.llm_resolution import create_provider, resolve_llm_settings

settings = resolve_llm_settings(workspace.repo_path)
provider = create_provider(settings) if use_llm else None
generated_tasks = tasks.generate_from_prd(workspace, prd_record, use_llm=use_llm, provider=provider)

(plus the same missing-key → user-facing-error handling _resolve_llm_provider already does, since right now a missing key here just surfaces as a generic 500 from the outer except.)

Not necessarily a blocker for merging the CLI/prd_v2.py fix itself, but worth a fast follow-up (or folding in now, since it's a small change reusing code this PR already introduced) — otherwise #768's "Think-phase ignored --llm-provider" will still be true for one of the three Think-phase surfaces.

Minor / non-blocking

• runtime.py::execute_agent's docstring (Raises: ValueError: If ANTHROPIC_API_KEY is not set...) wasn't updated to reflect that the required key is now provider-dependent (could be OPENAI_API_KEY). Cosmetic only.
• The base_url resolution order (config → OPENAI_BASE_URL, config wins) is inverted relative to the provider/model chains (flag → env → config, env wins) — resolve_llm_settings's docstring does call this out explicitly, and it matches the pre-existing runtime.py behavior this PR is deduping, so it's not a regression, just a slightly surprising asymmetry if anyone touches this again later.

Security / correctness

No concerns — pre-flight checks fail closed (raise/exit) rather than silently falling back, and the get_provider("anthropic", api_key=kwargs.get("api_key")) change is a no-op vs. the old AnthropicProvider() call since provider_kwargs() never sets api_key, so it still falls back to AnthropicProvider's own env lookup. --recursive --no-llm now fails with a clear message instead of the AttributeError-on-None-provider that cross-family review caught.

[frankbria]

Feedback triage (as of 263046e)

claude-review (Major): discovery_v2.py generate-tasks bypasses the provider chain — CONFIRMED and fixed in 263046e. The endpoint now resolves via resolve_llm_settings/create_provider and threads the provider into generate_from_prd, with router regression tests (tests/ui/test_discovery_generate_tasks.py). This also makes the PR body's Known-Limitations claim accurate again.

Internal review (advisory, Claude family):

• Critical (--recursive --no-llm crash) — already fixed in f8dfe00, reviewer re-ran their repro and confirmed clean.
• Major (discovery_v2 gap) — same finding as claude-review, fixed in 263046e.
• Suggestion (4 remaining direct provider constructions: conductor SupervisorResolver, dependency_analyzer, prd_discovery, streaming_chat) — verified real, out of scope for [P2.19] Provider-selection gaps: pre-flight key check and Think-phase ignore --llm-provider #768's three criteria → spun off as [P2.27] Route remaining direct provider constructions through the shared llm_resolution chain #861 (P2.27).
• Nitpick (stale --execute help text mentioning ANTHROPIC_API_KEY) — fixed in 263046e.

CI Backend Unit Tests failure on f8dfe00 — root-caused: test_tasks_generate_without_recursive_uses_existing_behavior patched no provider-construction seam; locally a CredentialManager key masked it, in CI provider construction failed before generate_from_prd. Fixed by patching the get_provider factory seam (same pattern as its sibling test).

Related: the anthropic model-override fix in f8dfe00 resolves the model half of #780 (P3.9); base_url for anthropic remains open there.

[github-actions]

Precision review in progress

Bug-hunting pass on #768 (provider-aware pre-flight + Think-phase provider resolution). Scope: concrete defects only — logic errors, null/unhandled crash paths, security, data loss, breaking API changes, resource leaks. Not style/architecture/coverage (a general review covers those).

• Gather PR context (diff vs origin/main, prior review comments to avoid repeats)
• Read changed source + callers/callees, verify each suspected defect against real code
• Post confirmed findings as inline review comments
• Post summary (table or "no defects found")

View job run

[github-actions]

Claude finished @frankbria's task in 6m 6s —— View job

---

✅ GLM precision review: no defects found

Reviewed the full branch diff for #768 (provider-aware pre-flight key checks + Think-phase provider resolution) against the checked-out source and callers/callees. Scope: concrete defects only (logic errors, null/unhandled crash paths, security, data loss, breaking API changes, resource leaks). High precision over recall — no finding I couldn't attach a concrete failure scenario to.

No confirmed findings. Key high-risk areas verified:

• Provider-threading consistency through the batch path (highest-risk area): cf work batch run --llm-provider openai is internally consistent end-to-end. Pre-flight (app.py:3893-3901) resolves the provider and requires OPENAI_API_KEY; batch_run forwards llm_provider/llm_model to conductor.start_batch (app.py:3922-3923), the conductor persists them and passes batch.llm_provider to every execute_agent call (conductor.py:1116/1314/1546/...), and runtime.execute_agent re-resolves the same provider for actual execution. No pre-flight/execution key mismatch. (work_start threads the same pair at app.py:2544; work_retry deliberately resolves no-flag env→config→default on both pre-flight app.py:3040 and execution app.py:3084, so those match too.)
• adapters/llm/__init__.py (anthropic branch): ModelSelector is imported (__init__.py:27) and constructed with exactly the 5 fields the dataclass defines (base.py:67-71), all truthy so __post_init__ won't override — no TypeError. AnthropicProvider(api_key=kwargs.get("api_key"), ...) with api_key=None is equivalent to the old AnthropicProvider(): the constructor falls back to os.getenv("ANTHROPIC_API_KEY") and raises only if that's also unset (anthropic.py:53-67). provider_kwargs() never sets api_key, so the new kwarg is a safe latent path, not a regression.
• core/llm_resolution.py: precedence chains (flag → env → config → default for provider/model; config → env for base_url) are byte-for-byte equivalent to the old inline logic in runtime.py/prd_v2.py — pure dedup. provider_kwargs() emits only model/base_url, both consumed by get_provider's branches; nothing reaches a constructor as an unexpected kwarg. load_environment_config returns Optional[EnvironmentConfig] and env_cfg.llm is Optional[LLMConfig] with .provider/.model/.base_url — the (env_cfg and env_cfg.llm) guard matches the old code.
• cli/app.py tasks_generate: the --recursive --no-llm rejection (app.py:1882) fires before provider is used, so generate_task_tree(None) is unreachable; provider is set iff not no_llm and threaded to both the recursive (app.py:1910) and LLM (app.py:1923) branches. Key validation runs pre-try, matching old behavior.
• runtime.py: the two old if not is_external_engine(engine) blocks merge into one with identical control flow; the only behavior change is the intended one — missing OPENAI_API_KEY for the openai provider now raises loudly (runtime.py:665-672). create_provider(llm_settings) threads both model and base_url via provider_kwargs, equivalent to the old explicit kwargs.
• tasks.py: generate_from_prd(provider=None) / _generate_tasks_with_llm(provider=None) are backward-compatible additive params; all 4 in-repo callers verified (CLI use_llm=False/=True+provider, discovery_v2, tests) — no positional caller breaks, and provider is None still falls back to zero-arg get_provider().
• prd_v2.py _resolve_llm_provider: os is imported (prd_v2.py:20); the key check is correctly broadened from anthropic-only to required_key_env-driven, so openai now requires OPENAI_API_KEY. Mirrors runtime.py.

Investigated and ruled out (non-fatal): conductor.py's SupervisorResolver.llm (conductor.py:120-126) still hardcodes zero-arg get_provider() (anthropic). Because this PR's batch pre-flight no longer guarantees ANTHROPIC_API_KEY for non-anthropic providers, cf work batch run --llm-provider openai (with OPENAI_API_KEY but no ANTHROPIC_API_KEY) makes the supervisor's lazy AnthropicProvider() raise on first blocker classification — but that access sits inside try/except Exception (conductor.py:216-228) and degrades to the existing pattern-matching fallback (logged warning). No crash, no data loss. This was already flagged as an awareness point in the prior GLM pass and is pre-existing code not modified here; re-flagging would be a repeat. (Minor inefficiency: since a raising get_provider() never assigns self._llm, each subsequent blocker re-attempts construction — bounded by blocker count, not unbounded growth; out of scope.)

Verdict: APPROVE (no action requested).

• Gather PR context (diff vs origin/main, prior review comments to avoid repeats)
• Read changed source + callers/callees, verify each suspected defect against real code
• Post confirmed findings as inline review comments — none to post
• Post summary (no defects found)

[claude]

Review (at 263046e)

Re-reviewed after the discovery_v2.py fix landed. This is the fourth review pass on this PR (GLM, codex, my own prior pass, and this one) and the codebase is in good shape — I only have one small, non-blocking item to add.

Confirms prior findings are resolved

• discovery_v2.py provider-chain gap (my prior Major finding): fixed correctly. generate_tasks_from_prd now resolves via resolve_llm_settings/create_provider and threads provider into generate_from_prd, gated on use_llm, with real regression tests (tests/ui/test_discovery_generate_tasks.py) covering both the resolved-provider and the use_llm=False skip path.
• --recursive --no-llm now rejected explicitly before provider is touched — no more AttributeError on None.
• The Anthropic model-override fix (get_provider building a purpose-pinned ModelSelector) is correct: all 5 ModelSelector fields (planning/execution/generation/correction/supervision) are covered, so no field is silently left at its env/default value when an override is requested.
• codeframe/core/llm_resolution.py stays headless (no FastAPI/UI imports) — consistent with CLAUDE.md's Architecture Rule Add Claude Code GitHub Workflow #1.

One small gap left over from my last review

I'd flagged that discovery_v2.py's missing-key path should mirror prd_v2.py's _resolve_llm_provider, which pre-checks required_key_env and raises a clean, targeted error before constructing the provider. The triage comment addressed the core "bypasses the provider chain" finding but this secondary part wasn't part of that fix — generate_tasks_from_prd still calls create_provider(resolve_llm_settings(...)) directly with no key pre-check:

provider = None
if use_llm:
    ...
    provider = create_provider(resolve_llm_settings(workspace.repo_path))

So a missing ANTHROPIC_API_KEY still raises from deep inside AnthropicProvider.__init__, gets caught by the route's generic except Exception, and surfaces as a 500 with the raw exception text embedded in detail — rather than a clean, consistent error the way the stress-test SSE endpoint handles it (catches ValueError from _resolve_llm_provider and turns it into a friendly in-stream error event). Functionally the user still sees why it failed, just via a less consistent path (wrong status code, no dedicated test). Given this was called out as a "plus" in my last review rather than the blocking part of that finding, I don't consider it a merge blocker — just worth a fast follow-up (could ride along with the already-planned #861 spin-off) rather than leaving it silently unaddressed.

Other spot checks (no issues found)

• Precedence chain (flag → env → config → default) in resolve_llm_settings is consistent across all five call sites (prd stress-test, tasks generate, work start, work batch run, work retry) and matches the documented order in both the PR description and llm_resolution.py's docstring.
• require_api_key_for_provider correctly no-ops for ollama/vllm/compatible/mock and delegates to the existing require_anthropic_api_key/require_openai_api_key validators for the two keyed providers — no duplicated key-lookup logic.
• Removed import os from prd_stress_test's CLI handler and get_provider() import cleanups look correct — no leftover dead imports from the refactor.
• Test coverage is solid: precedence matrix, provider-specific pre-flight (CLI + batch + retry), the --recursive --no-llm regression, and the anthropic model-override regression are all directly tested.

No security concerns — pre-flight checks fail closed, and none of the resolved settings (provider/model/base_url) are logged or echoed in a way that would leak a key value.

Nice work tightening this up across three review rounds — looks ready to merge, with the discovery_v2 missing-key polish as an optional fast-follow.

[claude]

Review: [P2.19] Provider-aware pre-flight key checks + Think-phase provider resolution (#768)

Follow-up pass on top of the existing review thread (GLM precision review: no findings; codex cross-family: 2 Major fixed, approved; my earlier pass: the discovery_v2.py gap, confirmed fixed in 263046e). I re-checked the current head (6a41b41) directly rather than repeating what's already settled.

Confirmed fixed, matches the triage comment:

• discovery_v2.py:generate_tasks_from_prd now resolves via resolve_llm_settings/create_provider and threads the provider into generate_from_prd, with regression tests in tests/ui/test_discovery_generate_tasks.py. I checked its error path too: a missing key surfaces via the generic except Exception → 500, which is actually consistent with this file's existing convention for NoApiKeyError elsewhere in the same router (also 500) — just via a slightly different message. Not the prd_v2.py 400/503 pattern, but that's fine since it matches its own file's neighbors, not that router. Not a finding.
• --recursive --no-llm now rejects explicitly before provider is used — no reachable AttributeError-on-None path.
• The anthropic model-override is honored via the pinned ModelSelector, and codeframe/adapters/llm/__init__.py's AnthropicProvider(api_key=kwargs.get("api_key"), ...) change is a no-op vs. the old bare AnthropicProvider() (no caller passes api_key yet, and the constructor still falls back to ANTHROPIC_API_KEY from env).

Still open, both already flagged and explicitly accepted as non-blocking (no change since last pass):

• runtime.py::execute_agent's docstring (line 649) still says ValueError: If ANTHROPIC_API_KEY is not set... — cosmetic, the key is now provider-dependent.
• conductor.py's SupervisorResolver still hardcodes zero-arg get_provider() (anthropic) for blocker classification — correctly spun off into [P2.27] Route remaining direct provider constructions through the shared llm_resolution chain #861 per the triage comment rather than folded into this PR.

New spot-checks that came back clean:

• All four pre-flight call sites (work start, work batch run, work retry, runtime.execute_agent) now key off LLMSettings.required_key_env consistently — no site left checking ANTHROPIC_API_KEY unconditionally.
• generate_from_prd(provider=None) / _generate_tasks_with_llm(prd_content, provider=None) signature changes are backward compatible — grepped for all callers, only the two now-threaded call sites (cli/app.py, discovery_v2.py) and the internal default-None fallback exist.
• resolve_llm_settings is called twice per execute_agent invocation's config load (once internally, once again later for hooks via load_environment_config) — pre-existing double-load pattern from before this PR, not a regression.

Test coverage: thorough — precedence matrix in test_llm_resolution.py, provider-specific pre-flight cases across all four CLI call sites, the --recursive/--no-llm edge case, and the anthropic model-override regression. Nothing further to ask for.

No new blocking issues. This looks ready to merge; the two open items above are cosmetic/already-scoped-out and shouldn't hold it up.

[github-actions]

Claude finished @frankbria's task in 5m 12s —— View job

---

✅ GLM precision review: no defects found

Independently re-verified the full branch diff for #768 (provider-aware pre-flight key checks + Think-phase provider resolution) against the checked-out source and callers/callees. Scope: concrete defects only (logic errors, null/unhandled crash paths, security, data loss, breaking API/contract changes, resource leaks) — not style/architecture/coverage. High precision over recall.

No confirmed findings. Highest-risk areas verified against real code:

• core/llm_resolution.py: precedence chains (flag → CODEFRAME_LLM_PROVIDER → config → "anthropic" for provider; flag → CODEFRAME_LLM_MODEL → config for model; config → OPENAI_BASE_URL for base_url) are byte-for-byte equivalent to the old inline logic in runtime.py/prd_v2.py — pure dedup. provider_kwargs() emits only model/base_url, both consumed by get_provider's branches; nothing reaches a constructor as an unexpected kwarg. The (env_cfg and env_cfg.llm) guard matches the old code.
• cli/validators.py::require_api_key_for_provider: correctly dispatches to require_anthropic_api_key/require_openai_api_key for the two keyed providers and returns None for ollama/vllm/compatible/mock — no duplicated key-lookup logic, no KeyError on unknown types (dict .get). str | None annotation is safe — repo targets Python ≥3.11 (pyproject.toml:10).
• runtime.py::execute_agent: the two old if not is_external_engine(engine) blocks merge into one with identical control flow for every branch; the only behavior change is the intended one — a missing OPENAI_API_KEY for the openai provider now raises loudly (runtime.py:665-672) instead of deferring into OpenAIProvider. External engines still skip the check and get provider=None. os import retained and still used.
• adapters/llm/__init__.py (anthropic branch): ModelSelector constructed with exactly the 5 fields the dataclass defines (base.py:67-71), all truthy so __post_init__ won't override — no TypeError. AnthropicProvider(api_key=kwargs.get("api_key"), ...) with api_key=None is a no-op vs. the old bare AnthropicProvider() (constructor still falls back to ANTHROPIC_API_KEY from env); provider_kwargs() never sets api_key, so the new kwarg is a safe latent path.
• cli/app.py (all five surfaces): prd stress-test, tasks generate, work start, work batch run, and work retry each resolve via resolve_llm_settings and key-check via require_api_key_for_provider(settings.provider_type) consistently. work start/batch run thread the same llm_provider/llm_model into execute_agent/conductor.start_batch that the pre-flight checked (app.py:2544, 3922-3923); work retry deliberately resolves no-flag env→config→default on both pre-flight (app.py:3040) and execution (execute_agent via app.py:3084), so the two match. get_provider is fully removed from app.py (grep: 0 hits) — no dangling NameError. --recursive --no-llm is rejected before provider is touched, so generate_task_tree(None) is unreachable; provider is set iff not no_llm and threaded to both the recursive and LLM branches.
• tasks.py: generate_from_prd(provider=None) / _generate_tasks_with_llm(provider=None) are backward-compatible additive params; provider is None still falls back to zero-arg get_provider(). Verified all callers (CLI, discovery_v2.py, tests) — no positional caller breaks.
• ui/routers/prd_v2.py::_resolve_llm_provider: os still imported (prd_v2.py:20); key check correctly broadened from anthropic-only to required_key_env-driven, so openai now requires OPENAI_API_KEY. discovery_v2.py threads the resolved provider the same way; its missing-key path surfaces via the route's existing generic except → 500, matching the file's own NoApiKeyError convention — not a defect.

Investigated and ruled out (non-fatal, pre-existing / out-of-scope — not re-flagged): conductor.py's SupervisorResolver.llm still hardcodes zero-arg get_provider() (anthropic). Since batch pre-flight no longer guarantees ANTHROPIC_API_KEY for non-anthropic providers, cf work batch run --llm-provider openai (with OPENAI_API_KEY but no ANTHROPIC_API_KEY) makes the supervisor's lazy AnthropicProvider() raise on first blocker classification — but that access sits inside try/except Exception (conductor.py) and degrades to the existing pattern-matching fallback (logged warning). No crash, no data loss. Already noted by prior passes and scoped to #861; re-flagging would be a repeat.

Verdict: APPROVE (no action requested).

• Gather PR context (diff vs origin/main, prior review comments to avoid repeats)
• Read changed source + callers/callees, verify each suspected defect against real code
• Post confirmed findings as inline review comments — none to post
• Post summary (no defects found)

[claude]

Review (at b81fbd4, latest commit)

This is my fourth pass on this PR; the codebase has settled after GLM's two precision passes, codex's cross-family review, and my three prior comments. I re-read the full diff against current main independently rather than re-litigating settled points.

Confirms the last open item is now closed:

• The execute_agent docstring nitpick I flagged twice (Raises: ValueError: If ANTHROPIC_API_KEY is not set...) is fixed in b81fbd4: it now reads "If the API key matching the resolved LLM provider is not set (for builtin engines) or engine is invalid" — accurate now that the required key is provider-dependent.

Independent spot-checks (no new issues):

• codeframe/core/llm_resolution.py stays headless (no FastAPI/UI imports) per CLAUDE.md Architecture Rule Add Claude Code GitHub Workflow #1.
• The flag → env → config → default precedence is identical across all five call sites (work start, work batch run, work retry, prd stress-test, tasks generate) and matches llm_resolution.py's own docstring and tests.
• get_provider("anthropic", api_key=kwargs.get("api_key"), model_selector=...) is a safe no-op vs. the old bare AnthropicProvider() — no caller passes api_key, and the constructor still falls back to ANTHROPIC_API_KEY from env. The ModelSelector override covers all 5 purpose fields, so a --llm-model override for anthropic no longer gets silently dropped.
• runtime.py/prd_v2.py's direct os.getenv(key_env) checks (no .env-file fallback) match their pre-existing behavior before this PR — the CLI's require_api_key_for_provider (which does load .env and back-fills os.environ) already runs upstream of execute_agent in every CLI path, so this isn't a new gap.
• str | None return type in validators.py is fine — pyproject.toml pins requires-python = ">=3.11".
• discovery_v2.py's provider-chain gap (my Major finding from the second pass) is correctly fixed and tested (tests/ui/test_discovery_generate_tasks.py), including the use_llm=false skip-resolution case.
• Test coverage remains solid: precedence matrix, all four CLI pre-flight call sites, the --recursive --no-llm regression, and the anthropic model-override regression are all directly exercised.

Still open, already scoped out on purpose (no change needed here):

• conductor.py's SupervisorResolver still hardcodes zero-arg get_provider() — correctly spun off to [P2.27] Route remaining direct provider constructions through the shared llm_resolution chain #861 rather than folded into this PR's scope.
• discovery_v2.py's missing-key path surfaces via the router's generic except Exception → 500 rather than a targeted 400/503 like prd_v2.py's SSE endpoint — cosmetic/consistency-only, already noted as optional fast-follow, not a functional bug (the user still sees why it failed).

No new security, correctness, or test-coverage concerns. This is ready to merge.