fix: add auth-aware first-run onboarding to prevent cryptic failures#2126
fix: add auth-aware first-run onboarding to prevent cryptic failures#2126praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…ixes #2124) - Added is_configured() helper in llm/credentials.py to detect if credentials are available - Added credential check in cli/app.py before launching TUI for no-arg invocation - Added credential check in cli/commands/run.py before executing run commands - Both checks offer to launch setup wizard in interactive mode - Non-interactive/CI contexts get clear actionable error messages - Configured users see no change in behavior or startup latency This ensures new users get proper guidance instead of cryptic mid-run failures when no API keys are configured.
|
@coderabbitai review |
|
/review |
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? |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a ChangesAuth-aware onboarding and credential gating
Sequence Diagram(s)sequenceDiagram
participant User
participant CLIEntry as cli/app.py or run_main
participant is_configured as credentials.is_configured()
participant SetupWizard as _run_setup
participant Runtime as AsyncTUI / agent run
User->>CLIEntry: praisonai [run ...]
CLIEntry->>is_configured: is_configured(model)
alt Credentials present
is_configured-->>CLIEntry: True
CLIEntry->>Runtime: proceed normally
else Non-interactive / --quiet
is_configured-->>CLIEntry: False
CLIEntry-->>User: "Run praisonai setup", exit 1
else Interactive, user accepts setup
is_configured-->>CLIEntry: False
CLIEntry->>User: prompt to run setup wizard
User-->>CLIEntry: yes
CLIEntry->>SetupWizard: _run_setup()
SetupWizard-->>CLIEntry: return code
CLIEntry->>is_configured: re-check credentials
alt Still missing
CLIEntry-->>User: setup failed, exit nonzero
else Now configured
CLIEntry->>Runtime: proceed normally
end
else Interactive, user declines
CLIEntry-->>User: print instructions, exit 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR adds auth-aware first-run onboarding to the CLI: before launching the TUI or executing a
Confidence Score: 4/5Safe to merge with one fix: the model-specific credential check in run.py can strand cross-provider users in a confusing dead-end after setup completes. The src/praisonai/praisonai/cli/commands/run.py — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[praisonai / praisonai run target] --> B{target provided?}
B -- No --> Z[Skip credential check]
B -- Yes --> C[is_configured model?]
C --> D{Result}
D -- True --> E[Proceed with run]
D -- False --> F{sys.stdin.isatty\nand not json_mode?}
F -- No CI/pipe --> G[print_error + Exit 1]
F -- Yes interactive --> H[typer.confirm setup?]
H -- No --> I[print instructions + Exit 0]
H -- Yes --> J[_run_setup]
J --> K{exit_code == 0?}
K -- No --> L[Setup failed + Exit exit_code]
K -- Yes --> M[Re-check is_configured model?]
M -- True --> E
M -- False --> N[Setup completed but\ncreds not detected + Exit 1]
style N fill:#f66,color:#fff
style G fill:#f66,color:#fff
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[praisonai / praisonai run target] --> B{target provided?}
B -- No --> Z[Skip credential check]
B -- Yes --> C[is_configured model?]
C --> D{Result}
D -- True --> E[Proceed with run]
D -- False --> F{sys.stdin.isatty\nand not json_mode?}
F -- No CI/pipe --> G[print_error + Exit 1]
F -- Yes interactive --> H[typer.confirm setup?]
H -- No --> I[print instructions + Exit 0]
H -- Yes --> J[_run_setup]
J --> K{exit_code == 0?}
K -- No --> L[Setup failed + Exit exit_code]
K -- Yes --> M[Re-check is_configured model?]
M -- True --> E
M -- False --> N[Setup completed but\ncreds not detected + Exit 1]
style N fill:#f66,color:#fff
style G fill:#f66,color:#fff
Reviews (2): Last reviewed commit: "fix: address reviewer feedback on auth-a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Finally, check if we can resolve an endpoint with credentials | ||
| try: | ||
| endpoint = resolve_llm_endpoint_with_credentials() | ||
| return bool(endpoint.api_key) | ||
| except Exception: | ||
| pass | ||
|
|
||
| return False No newline at end of file |
There was a problem hiding this comment.
is_configured function.
| # Finally, check if we can resolve an endpoint with credentials | |
| try: | |
| endpoint = resolve_llm_endpoint_with_credentials() | |
| return bool(endpoint.api_key) | |
| except Exception: | |
| pass | |
| return False | |
| # Finally, check if we can resolve an endpoint with credentials | |
| try: | |
| endpoint = resolve_llm_endpoint_with_credentials() | |
| return bool(endpoint.api_key) | |
| except Exception: | |
| pass | |
| return False |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai/praisonai/cli/commands/run.py (1)
18-58:⚠️ Potential issue | 🟠 MajorRemove the dead code function
_check_api_key_available().The function is no longer called anywhere in the codebase. After the API key checks were centralized in
run_main()and removed from_run_from_file()and_run_prompt(), this helper function became unused. Remove lines 18-58 to reduce maintenance burden and improve code clarity.🤖 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/praisonai/cli/commands/run.py` around lines 18 - 58, The function `_check_api_key_available()` is dead code that is no longer called anywhere in the codebase and should be removed to reduce maintenance burden. Delete the entire function definition including its docstring and all logic from line 18 through line 58.
🤖 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/praisonai/cli/app.py`:
- Around line 668-673: After the setup wizard completes successfully (when
exit_code equals 0), add credential re-validation before continuing to the TUI
launch, similar to the implementation in run.py lines 137-140. This validation
should verify that the credentials configured by the user (including non-OpenAI
providers) are functional before the interactive mode starts. Insert this
credential check between the successful setup confirmation and the echo message
that says "Setup complete! Starting interactive mode...", ensuring the TUI
doesn't launch with invalid or misconfigured credentials.
In `@src/praisonai/praisonai/cli/commands/run.py`:
- Around line 684-695: The session existence check using
`project_store.session_exists(session)` only handles the true case where the
session exists, but lacks an else clause to handle when the session does not
exist, causing the code to silently continue without setting `session_id`. Add
an else block after the `if project_store.session_exists(session)` condition
that emits an error message stating the session was not found and exits
execution, matching the error handling pattern used in `_run_from_file` (lines
400-401). Additionally, ensure the `--continue` flag handling also includes
appropriate warning or error logging consistent with the pattern seen at lines
390-391 when no previous session is found.
- Around line 872-883: The session existence check in the elif session block is
missing error handling for when the specified session does not exist. The code
checks if project_store.session_exists(session) is true but has no corresponding
else clause to handle the false case, causing silent failures. Add proper error
handling (similar to how the non-profiled _run_prompt method handles this at
lines 498-506) by adding an else clause after the session_exists check that
raises an appropriate exception with a clear error message when the session
cannot be found, ensuring users are informed when their specified session does
not exist.
In `@src/praisonai/praisonai/llm/credentials.py`:
- Around line 126-160: The is_configured function is too permissive when a
specific model is requested - it returns True if any unrelated credential exists
(e.g., OLLAMA_HOST when checking for gpt-4o-mini). To fix this, modify the
initial environment variable check to filter known_keys based on the requested
model when model is specified, only checking relevant credentials for that model
prefix. Additionally, at the final return statement after listing providers, do
not return True unconditionally for any stored provider when model is specified
- instead, only return True if a matching provider for the requested model was
actually found in the earlier model prefix checks. This ensures that requesting
a specific model requires the appropriate credential type rather than any
unrelated credential.
---
Outside diff comments:
In `@src/praisonai/praisonai/cli/commands/run.py`:
- Around line 18-58: The function `_check_api_key_available()` is dead code that
is no longer called anywhere in the codebase and should be removed to reduce
maintenance burden. Delete the entire function definition including its
docstring and all logic from line 18 through line 58.
🪄 Autofix (Beta)
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
Run ID: e35d2762-3cf3-4ffc-aa9e-64e30c2e0d6a
📒 Files selected for processing (3)
src/praisonai/praisonai/cli/app.pysrc/praisonai/praisonai/cli/commands/run.pysrc/praisonai/praisonai/llm/credentials.py
| if exit_code != 0: | ||
| typer.echo("Setup failed. Exiting.", err=True) | ||
| raise typer.Exit(exit_code) | ||
|
|
||
| # After successful setup, continue to TUI | ||
| typer.echo("\nSetup complete! Starting interactive mode...\n") |
There was a problem hiding this comment.
Missing credential re-check after setup completion.
Unlike the parallel implementation in run.py (lines 137-140), this code path does not re-validate credentials after the setup wizard completes. If the user configures a provider other than OpenAI (which the hardcoded gpt-4o-mini check requires), the TUI will launch and fail at the first LLM call.
Proposed fix to add post-setup verification
if exit_code != 0:
typer.echo("Setup failed. Exiting.", err=True)
raise typer.Exit(exit_code)
- # After successful setup, continue to TUI
- typer.echo("\nSetup complete! Starting interactive mode...\n")
+ # Re-check credentials after setup
+ if not is_configured(model="gpt-4o-mini"):
+ typer.echo("Setup completed but credentials for gpt-4o-mini not detected.", err=True)
+ raise typer.Exit(1)
+
+ typer.echo("\nSetup complete! Starting interactive mode...\n")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if exit_code != 0: | |
| typer.echo("Setup failed. Exiting.", err=True) | |
| raise typer.Exit(exit_code) | |
| # After successful setup, continue to TUI | |
| typer.echo("\nSetup complete! Starting interactive mode...\n") | |
| if exit_code != 0: | |
| typer.echo("Setup failed. Exiting.", err=True) | |
| raise typer.Exit(exit_code) | |
| # Re-check credentials after setup | |
| if not is_configured(model="gpt-4o-mini"): | |
| typer.echo("Setup completed but credentials for gpt-4o-mini not detected.", err=True) | |
| raise typer.Exit(1) | |
| typer.echo("\nSetup complete! Starting interactive mode...\n") |
🤖 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/praisonai/cli/app.py` around lines 668 - 673, After the setup
wizard completes successfully (when exit_code equals 0), add credential
re-validation before continuing to the TUI launch, similar to the implementation
in run.py lines 137-140. This validation should verify that the credentials
configured by the user (including non-OpenAI providers) are functional before
the interactive mode starts. Insert this credential check between the successful
setup confirmation and the echo message that says "Setup complete! Starting
interactive mode...", ensuring the TUI doesn't launch with invalid or
misconfigured credentials.
| elif session: | ||
| project_store = get_project_session_store() | ||
| if project_store.session_exists(session): | ||
| session_id = session | ||
|
|
||
| if fork: | ||
| from praisonaiagents.session.hierarchy import HierarchicalSessionStore | ||
| from ..utils.project import get_project_sessions_dir | ||
|
|
||
| hierarchical_store = HierarchicalSessionStore(str(get_project_sessions_dir())) | ||
| forked_session_id = hierarchical_store.fork_session(session_id) | ||
| session_id = forked_session_id |
There was a problem hiding this comment.
Silent failure when specified session does not exist.
When --session is provided but the session doesn't exist, this profiled path silently continues without setting session_id, while the non-profiled _run_from_file (lines 400-401) properly emits an error and exits. Similarly, when --continue finds no previous session, no warning is logged here (contrast with lines 390-391).
This inconsistency could cause confusing behavior where profiled runs silently ignore session flags.
Proposed fix to add error handling
if continue_session or session or fork:
from ..state.project_sessions import get_project_session_store, find_last_session
if continue_session:
session_id = find_last_session()
+ if not session_id:
+ typer.echo("Warning: No previous sessions found. Starting new session.", err=True)
elif session:
project_store = get_project_session_store()
if project_store.session_exists(session):
session_id = session
if fork:
from praisonaiagents.session.hierarchy import HierarchicalSessionStore
from ..utils.project import get_project_sessions_dir
hierarchical_store = HierarchicalSessionStore(str(get_project_sessions_dir()))
forked_session_id = hierarchical_store.fork_session(session_id)
session_id = forked_session_id
+ else:
+ typer.echo(f"Error: Session not found: {session}", err=True)
+ raise typer.Exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif session: | |
| project_store = get_project_session_store() | |
| if project_store.session_exists(session): | |
| session_id = session | |
| if fork: | |
| from praisonaiagents.session.hierarchy import HierarchicalSessionStore | |
| from ..utils.project import get_project_sessions_dir | |
| hierarchical_store = HierarchicalSessionStore(str(get_project_sessions_dir())) | |
| forked_session_id = hierarchical_store.fork_session(session_id) | |
| session_id = forked_session_id | |
| if continue_session or session or fork: | |
| from ..state.project_sessions import get_project_session_store, find_last_session | |
| if continue_session: | |
| session_id = find_last_session() | |
| if not session_id: | |
| typer.echo("Warning: No previous sessions found. Starting new session.", err=True) | |
| elif session: | |
| project_store = get_project_session_store() | |
| if project_store.session_exists(session): | |
| session_id = session | |
| if fork: | |
| from praisonaiagents.session.hierarchy import HierarchicalSessionStore | |
| from ..utils.project import get_project_sessions_dir | |
| hierarchical_store = HierarchicalSessionStore(str(get_project_sessions_dir())) | |
| forked_session_id = hierarchical_store.fork_session(session_id) | |
| session_id = forked_session_id | |
| else: | |
| typer.echo(f"Error: Session not found: {session}", err=True) | |
| raise typer.Exit(1) |
🤖 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/praisonai/cli/commands/run.py` around lines 684 - 695, The
session existence check using `project_store.session_exists(session)` only
handles the true case where the session exists, but lacks an else clause to
handle when the session does not exist, causing the code to silently continue
without setting `session_id`. Add an else block after the `if
project_store.session_exists(session)` condition that emits an error message
stating the session was not found and exits execution, matching the error
handling pattern used in `_run_from_file` (lines 400-401). Additionally, ensure
the `--continue` flag handling also includes appropriate warning or error
logging consistent with the pattern seen at lines 390-391 when no previous
session is found.
| elif session: | ||
| project_store = get_project_session_store() | ||
| if project_store.session_exists(session): | ||
| session_id = session | ||
|
|
||
| if fork: | ||
| from praisonaiagents.session.hierarchy import HierarchicalSessionStore | ||
| from ..utils.project import get_project_sessions_dir | ||
|
|
||
| hierarchical_store = HierarchicalSessionStore(str(get_project_sessions_dir())) | ||
| forked_session_id = hierarchical_store.fork_session(session_id) | ||
| session_id = forked_session_id |
There was a problem hiding this comment.
Same silent failure issue as _run_from_file_profiled.
This code has the identical missing error handling when --session specifies a non-existent session or when --continue finds no previous sessions. The non-profiled _run_prompt (lines 498-506) properly handles these cases.
Proposed fix to add error handling
if continue_session or session or fork:
from ..state.project_sessions import get_project_session_store, find_last_session
if continue_session:
session_id = find_last_session()
+ if not session_id:
+ typer.echo("Warning: No previous sessions found. Starting new session.", err=True)
elif session:
project_store = get_project_session_store()
if project_store.session_exists(session):
session_id = session
if fork:
from praisonaiagents.session.hierarchy import HierarchicalSessionStore
from ..utils.project import get_project_sessions_dir
hierarchical_store = HierarchicalSessionStore(str(get_project_sessions_dir()))
forked_session_id = hierarchical_store.fork_session(session_id)
session_id = forked_session_id
+ else:
+ typer.echo(f"Error: Session not found: {session}", err=True)
+ raise typer.Exit(1)🤖 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/praisonai/cli/commands/run.py` around lines 872 - 883, The
session existence check in the elif session block is missing error handling for
when the specified session does not exist. The code checks if
project_store.session_exists(session) is true but has no corresponding else
clause to handle the false case, causing silent failures. Add proper error
handling (similar to how the non-profiled _run_prompt method handles this at
lines 498-506) by adding an else clause after the session_exists check that
raises an appropriate exception with a clear error message when the session
cannot be found, ensuring users are informed when their specified session does
not exist.
| # Check environment variables first | ||
| known_keys = ( | ||
| "OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GOOGLE_API_KEY", | ||
| "GEMINI_API_KEY", "GROQ_API_KEY", "COHERE_API_KEY", | ||
| "OLLAMA_HOST", # Ollama doesn't need an API key | ||
| ) | ||
|
|
||
| # If any env var is set, consider it configured | ||
| if any(os.environ.get(k) for k in known_keys): | ||
| return True | ||
|
|
||
| # Check stored credentials | ||
| try: | ||
| store = CredentialStore() | ||
| providers = store.list_providers() | ||
|
|
||
| # If we have any stored credentials, we're configured | ||
| if providers: | ||
| # If model is specified, check for that specific provider | ||
| if model: | ||
| # Map model prefixes to providers | ||
| model_lower = model.lower() | ||
| if model_lower.startswith("gpt"): | ||
| return "openai" in [p.lower() for p in providers] | ||
| elif model_lower.startswith("claude"): | ||
| return "anthropic" in [p.lower() for p in providers] | ||
| elif model_lower.startswith("gemini"): | ||
| return "google" in [p.lower() for p in providers] or "gemini" in [p.lower() for p in providers] | ||
| elif model_lower.startswith("llama") or model_lower.startswith("mistral"): | ||
| # Could be Ollama or Groq | ||
| return "ollama" in [p.lower() for p in providers] or "groq" in [p.lower() for p in providers] | ||
|
|
||
| # Any stored credential means we're configured | ||
| return True | ||
|
|
There was a problem hiding this comment.
Model-specific gate is bypassed by unrelated credentials.
Line 134 treats any credential signal (including OLLAMA_HOST) as configured, and Line 159 returns True for any stored provider when the model prefix is unknown. With downstream is_configured(model="gpt-4o-mini"), this can skip onboarding even when no OpenAI-compatible credential exists, leading back to first-call failures.
Suggested fix (model-aware credential checks)
def is_configured(model: Optional[str] = None) -> bool:
import os
+ provider_env = {
+ "openai": ("OPENAI_API_KEY",),
+ "anthropic": ("ANTHROPIC_API_KEY",),
+ "google": ("GOOGLE_API_KEY", "GEMINI_API_KEY"),
+ "gemini": ("GOOGLE_API_KEY", "GEMINI_API_KEY"),
+ "groq": ("GROQ_API_KEY",),
+ "cohere": ("COHERE_API_KEY",),
+ "ollama": ("OLLAMA_HOST",),
+ }
+
+ def providers_for_model(m: Optional[str]) -> Optional[set[str]]:
+ if not m:
+ return None
+ ml = m.lower()
+ if ml.startswith("gpt"):
+ return {"openai"}
+ if ml.startswith("claude"):
+ return {"anthropic"}
+ if ml.startswith("gemini"):
+ return {"google", "gemini"}
+ if ml.startswith("llama") or ml.startswith("mistral"):
+ return {"ollama", "groq"}
+ return set() # unknown: don't auto-pass; defer to resolver fallback
+
+ target = providers_for_model(model)
- known_keys = (
- "OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GOOGLE_API_KEY",
- "GEMINI_API_KEY", "GROQ_API_KEY", "COHERE_API_KEY",
- "OLLAMA_HOST",
- )
- if any(os.environ.get(k) for k in known_keys):
- return True
+ if target is None:
+ known_keys = tuple(k for keys in provider_env.values() for k in keys)
+ if any(os.environ.get(k) for k in known_keys):
+ return True
+ elif target:
+ target_keys = tuple(k for p in target for k in provider_env[p])
+ if any(os.environ.get(k) for k in target_keys):
+ return True
try:
store = CredentialStore()
- providers = store.list_providers()
- if providers:
- ...
- return True
+ providers = {p.lower() for p in store.list_providers()}
+ if target is None:
+ return any((p == "ollama" and os.environ.get("OLLAMA_HOST")) or store.has_credential(p) for p in providers)
+ if target:
+ return any((p == "ollama" and os.environ.get("OLLAMA_HOST")) or store.has_credential(p) for p in (providers & target))
+ # unknown model: continue to resolver fallback
except Exception:
pass🤖 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/praisonai/llm/credentials.py` around lines 126 - 160, The
is_configured function is too permissive when a specific model is requested - it
returns True if any unrelated credential exists (e.g., OLLAMA_HOST when checking
for gpt-4o-mini). To fix this, modify the initial environment variable check to
filter known_keys based on the requested model when model is specified, only
checking relevant credentials for that model prefix. Additionally, at the final
return statement after listing providers, do not return True unconditionally for
any stored provider when model is specified - instead, only return True if a
matching provider for the requested model was actually found in the earlier
model prefix checks. This ensures that requesting a specific model requires the
appropriate credential type rather than any unrelated credential.
|
@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 |
|
Claude finished @MervinPraison's task —— View job Architecture Review & Fixes ✅Review Checklist
Phase 1: Architecture ReviewThe PR changes follow AGENTS.md principles:
Phase 2: Fixed IssuesSuccessfully addressed all valid issues from reviewers:
Files Modified
What I SkippedDid not modify Phase 3: Final Verdict✅ APPROVED - All architectural issues resolved. The PR now correctly:
The implementation is minimal, focused, and backward-compatible. Ready to merge. |
- Remove hardcoded gpt-4o-mini model in app.py credential check - Add credential re-verification after setup wizard completes - Add session error handling in profiled execution paths - Remove dead code _check_api_key_available function These fixes ensure the credential gate works for all providers, not just OpenAI, and properly handles edge cases with session management and post-setup verification. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
| if not is_configured(model): | ||
| # In non-interactive mode, show clear error | ||
| if not sys.stdin.isatty() or output.is_json_mode: | ||
| output.print_error( | ||
| "No API key configured. Run: praisonai setup\n" | ||
| "or set environment variables like OPENAI_API_KEY" | ||
| ) | ||
| raise typer.Exit(1) | ||
|
|
||
| # In interactive mode, offer to run setup | ||
| typer.echo(f"No API key configured{f' for model {model}' if model else ''}.") | ||
| run_setup = typer.confirm("Would you like to run the setup wizard now?") | ||
|
|
||
| if run_setup: | ||
| from ..commands.setup import _run_setup | ||
| exit_code = _run_setup( | ||
| non_interactive=False, | ||
| provider=None, | ||
| api_key=None, | ||
| model=None | ||
| ) | ||
| if exit_code != 0: | ||
| output.print_error("Setup failed. Exiting.") | ||
| raise typer.Exit(exit_code) | ||
|
|
||
| output.print_success("Setup complete! Continuing with your run...") | ||
| # Re-check after setup | ||
| if not is_configured(model): | ||
| output.print_error("Setup completed but credentials still not detected.") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
is_configured triggers false "not configured" for cross-provider users
When a user passes --model gpt-4o-mini but has only Anthropic credentials stored (and no OPENAI_API_KEY env var), is_configured("gpt-4o-mini") returns False because the GPT-prefix branch checks "openai" in providers and finds nothing. This user is fully configured — just not for that specific model — but they get dropped into the onboarding flow as if they were a fresh install.
The failure scenario is even worse on the re-check at line 96: if the user runs setup and stores an Anthropic credential again (or any non-OpenAI provider), is_configured("gpt-4o-mini") still returns False, and they receive "Setup completed but credentials still not detected" with an Exit(1) — a deeply confusing dead-end for a user who has working credentials.
app.py already fixed the equivalent issue by calling is_configured() with no argument. The same fix is needed here: drop the model argument from the check so it gates only on "any credential configured," which is the intent of fresh-install detection. Model/provider compatibility is a separate validation concern that belongs closer to the LLM call.
1 participant
Summary
Implements auth-aware first-run onboarding so fresh installations reach first success without cryptic failures when no credentials are configured.
Problem
Solution
Testing
Fixes #2124
Summary by CodeRabbit
Release Notes
New Features
Improvements