fix: catalogue-driven first-run provider auto-detection - #3798
fix: catalogue-driven first-run provider auto-detection#3798praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
Replace the three hardcoded provider lists (is_configured's 8-key tuple, _PROVIDER_DEFAULTS, and the setup wizard's 4-key env auto-detect) with a single data catalogue PROVIDER_ENV_CATALOGUE mapping provider id -> env-var(s) + representative model + model prefix. Zero-config `praisonai run` now recognises a valid key for any catalogued provider (Mistral, DeepSeek, Together, Fireworks, xAI, Perplexity, …) and selects a provider-appropriate default model instead of gating on the old ~8-provider set. The catalogue keeps the historical 8 at its head so existing ordered-preference behaviour is unchanged and new providers are purely additive and backward compatible. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe provider catalogue now defines credential variables, representative models, and model prefixes. Runtime credential checks, default model resolution, and setup environment detection consume this catalogue with historical fallbacks. ChangesProvider detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Environment
participant SetupHandler
participant credentials.py
participant env.py
participant PROVIDER_ENV_CATALOGUE
Environment->>SetupHandler: expose provider credential variables
SetupHandler->>PROVIDER_ENV_CATALOGUE: discover provider mappings
PROVIDER_ENV_CATALOGUE-->>SetupHandler: provider and default model
Environment->>credentials.py: expose configured credentials
credentials.py->>PROVIDER_ENV_CATALOGUE: resolve known credential variables
PROVIDER_ENV_CATALOGUE-->>credentials.py: credential variable set
Environment->>env.py: provide model configuration
env.py->>PROVIDER_ENV_CATALOGUE: resolve provider and default model
PROVIDER_ENV_CATALOGUE-->>env.py: provider credential variable
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-code/praisonai_code/llm/credentials.py`:
- Around line 122-132: Update _stored_providers_for_vars() in
src/praisonai-code/praisonai_code/llm/credentials.py:122-132 to derive provider
names and environment variables from PROVIDER_ENV_CATALOGUE, including
catalogue-only providers such as Mistral, so stored credentials are recognized
by is_configured(). Update the setup provider-resolution flow in
src/praisonai-code/praisonai_code/cli/features/setup/handler.py:430-446 to use
catalogue environment-variable aliases for Together and Fireworks and select
each catalogue provider’s representative model rather than the first
ModelCatalogue model. Add regression tests covering stored credentials and both
Together and Fireworks aliases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bfe1d843-492d-4dca-a4dd-7cb7da55860e
📒 Files selected for processing (4)
src/praisonai-code/praisonai_code/cli/features/setup/handler.pysrc/praisonai-code/praisonai_code/llm/catalogue.pysrc/praisonai-code/praisonai_code/llm/credentials.pysrc/praisonai-code/praisonai_code/llm/env.py
| try: | ||
| from praisonai_code.llm.catalogue import ( | ||
| provider_for_model, | ||
| env_vars_for_provider, | ||
| ) | ||
|
|
||
| provider = provider_for_model(model) | ||
| if provider: | ||
| vars_ = env_vars_for_provider(provider) | ||
| if vars_: | ||
| return vars_ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Finish the catalogue migration for stored credentials and setup metadata.
Both paths still derive provider metadata outside PROVIDER_ENV_CATALOGUE.
src/praisonai-code/praisonai_code/llm/credentials.py#L122-L132: Derive stored provider names from the catalogue._stored_providers_for_vars()only contains the historical providers. Therefore, an explicitmistral/...model with a Mistral key saved inCredentialStoreand absent from the environment returnsFalsefromis_configured().src/praisonai-code/praisonai_code/cli/features/setup/handler.py#L430-L446: Resolve catalogue-only environment variables and default models directly from the catalogue. Non-interactive setup checks only the synthesized canonical variable. For example,praisonai setup --provider togetherrejects an environment that contains onlyTOGETHERAI_API_KEY. Setup also selects the firstModelCataloguemodel instead of the catalogue representative model.
Add regression tests for stored credentials and both Together and Fireworks credential aliases.
📍 Affects 2 files
src/praisonai-code/praisonai_code/llm/credentials.py#L122-L132(this comment)src/praisonai-code/praisonai_code/cli/features/setup/handler.py#L430-L446
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-code/praisonai_code/llm/credentials.py` around lines 122 - 132,
Update _stored_providers_for_vars() in
src/praisonai-code/praisonai_code/llm/credentials.py:122-132 to derive provider
names and environment variables from PROVIDER_ENV_CATALOGUE, including
catalogue-only providers such as Mistral, so stored credentials are recognized
by is_configured(). Update the setup provider-resolution flow in
src/praisonai-code/praisonai_code/cli/features/setup/handler.py:430-446 to use
catalogue environment-variable aliases for Together and Fireworks and select
each catalogue provider’s representative model rather than the first
ModelCatalogue model. Add regression tests covering stored credentials and both
Together and Fireworks aliases.
Greptile SummaryThe PR centralizes first-run provider credential and model selection in a shared catalogue and extends setup detection to additional providers. The stored-credential endpoint fallback remains incomplete for the newly catalogued providers.
Confidence Score: 4/5The PR is not yet safe to merge because explicit models for newly catalogued providers can fail to load their stored credentials. Endpoint resolution derives the correct provider environment-variable name from the catalogue, but its stored-credential fallback still maps only historical variables and defaults unmapped providers to OpenAI. Files Needing Attention: src/praisonai-code/praisonai_code/llm/env.py
|
| Filename | Overview |
|---|---|
| src/praisonai-code/praisonai_code/llm/catalogue.py | Adds the centralized ordered provider catalogue and lookup helpers used across first-run resolution. |
| src/praisonai-code/praisonai_code/llm/credentials.py | Extends environment and stored-provider configuration checks through the catalogue, resolving the prior incomplete stored-provider mapping. |
| src/praisonai-code/praisonai_code/llm/env.py | Derives provider defaults and model routing from the catalogue, but explicit new-provider models still cannot retrieve their matching stored credentials. |
| src/praisonai-code/praisonai_code/cli/features/setup/handler.py | Extends setup environment detection and provider metadata resolution to catalogue-only providers. |
| src/praisonai/tests/unit/cli/test_auth_functionality.py | Makes environment cleanup account for every catalogued credential variable. |
| src/praisonai/tests/unit/cli/test_setup_command.py | Makes setup test isolation clear all catalogue-recognized provider variables. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
M[Explicit provider-prefixed model] --> P[_provider_from_model]
P --> C[Catalogue provider key variable]
C --> E{Environment key set?}
E -->|Yes| K[Use environment credential]
E -->|No| F[_KEY_VAR_TO_FALLBACK_PROVIDERS]
F -->|Mapped historical provider| S[Load matching stored credential]
F -->|New provider unmapped| O[Default to OpenAI lookup]
O --> A[Missing or wrong API key]
Reviews (2): Last reviewed commit: "fix: resolve provider-alias keys and sto..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
@claude CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3798 ✅Pushed fixes to Phase 1 — Architecture (AGENTS.md)
Phase 2 — Valid issues fixed
Verified functionally: Together/Fireworks alias-only → correct key var; stored-only Mistral cred → CI failures — verdict: legitimate feature, test helpers were staleThe widened detection is correct SDK behaviour. The 3 failing Proven: the three tests pass even when Tests
Files modified
Skipped
VerdictApprove. All blocking reviewer issues resolved, CI root cause fixed at source, backward compatibility and SDK guardrails intact. Job run · Branch |
…t-run tests - env.py: _provider_from_model prefers the env-set credential alias so Together/Fireworks (TOGETHERAI_API_KEY, FIREWORKS_AI_API_KEY) resolve a key instead of always using the first declared var. - credentials.py: _stored_providers_for_vars derives from PROVIDER_ENV_CATALOGUE so a stored key for any catalogued provider (Mistral, DeepSeek, xAI, Together, Perplexity, Fireworks) satisfies is_configured() for its explicit model. - setup/handler.py: _provider_setup_info picks the catalogue env-var (honouring the alias actually set) and the catalogue representative model for catalogue-only providers. - tests: derive the env-clearing/detection key lists from the catalogue so default-model and interactive-path assertions stay deterministic even when the CI host exports a catalogued key (e.g. OPENROUTER_API_KEY). Fixes the three failing test-core (cli) checks without weakening assertions. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
|
Claude finished @MervinPraison's task —— View job CI Fix — PR #3798
Verdict: legitimate feature — already fixed ✅The CI failure was reported against The widened catalogue-driven detection is correct SDK behaviour. The three tests failed on CI because they assumed only the historical ~8 provider keys existed; a stray catalogued key on the CI host (e.g. Local verification (branch HEAD
|
| Source | Issue | Status |
|---|---|---|
| Greptile P1 | env.py discarded alias keys → alias-only TOGETHERAI_API_KEY/FIREWORKS_AI_API_KEY yielded no api_key |
✅ env.py:184 now next((var for var in vars_ if os.environ.get(var)), vars_[0]) |
| CodeRabbit + Greptile P1 | stored Mistral/DeepSeek/xAI cred + explicit prefixed model → is_configured()==False |
✅ credentials.py:196-244 _stored_providers_for_vars derived from PROVIDER_ENV_CATALOGUE |
| CodeRabbit Major | setup used first ModelCatalogue model + synthesized canonical var |
✅ handler.py uses catalogue env-var (alias-aware) + representative model |
Files changed by me
None — all fixes were already present and pushed at 517ecc5. Working tree clean; local HEAD == origin/claude/issue-3794-20260808-0857.
SDK guardrails
All changes stay in the praisonai_code wrapper (llm/, cli/features/setup/). No praisonaiagents/ core surface, no new Agent params, no new deps, praisonai-rust/praisonai-ts untouched. Tests still guard backward compatibility (historical 8 providers keep ordered preference; no-key path still returns not-configured with gpt-4o-mini fallback).
The branch is green-ready. If CI still shows red, it is stale against e67ae99e; a re-run on 517ecc5 should pass.
Job run · Branch claude/issue-3794-20260808-0857
| ) | ||
| return key_var, None |
There was a problem hiding this comment.
When an explicit Mistral, DeepSeek, xAI, Together, Perplexity, or Fireworks model relies on a CLI-stored credential rather than an environment variable, _provider_from_model returns the catalogue credential variable but _KEY_VAR_TO_FALLBACK_PROVIDERS does not recognize it. The fallback consequently queries the OpenAI credential namespace, returning no key or the wrong key and causing the LLM request to fail authentication.
Knowledge Base Used: praisonai-code
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3794
Summary
Zero-config
praisonai run "…"previously only recognised a hardcoded ~8-provider set when deciding whether credentials were configured and which default model to use. A user with a valid key for any other model-layer-supported provider (Mistral, DeepSeek, Together, Fireworks, xAI, Perplexity, …) was toldNo API key configuredand could not reach first success.This replaces the three hardcoded lists with a single data catalogue.
Changes
llm/catalogue.py— newPROVIDER_ENV_CATALOGUE(provider id → env-var(s), representative default model, model prefix) plus helpersprovider_env_vars(),provider_for_model(),env_vars_for_provider(). One source of truth; adding a provider is a data row, not code.llm/credentials.py—is_configured()now honours any catalogued provider's env var (via_known_credential_vars());_provider_key_vars_for_model()resolves via the catalogue first, keeping the old prefix branches as fallback.llm/env.py—_PROVIDER_DEFAULTSis derived from the catalogue (with a literal fallback);_provider_from_model()falls back to the catalogue for catalogued providers so the correct API-key env-var is picked and litellm routes the endpoint.cli/features/setup/handler.py—_detect_provider_from_env()widened from the 4-key dict to the full catalogue, reusing_provider_setup_info()for catalogue-only providers.Backward compatibility
OPENAI_API_KEYis set).gpt-4o-miniterminal fallback.Validation
MISTRAL_API_KEY=…→is_configured()==True, defaultmistral/mistral-large-latest.praisonai_botimport, session-rename, memory command).Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes