feat: add frictionless onboarding with praisonai setup wizard and post-install hooks#1453
Conversation
- Add interactive setup command with provider selection (OpenAI, Anthropic, Google, Ollama) - Support both interactive and non-interactive modes - Secure .env file creation with 600 permissions - Idempotent operation (safe to re-run) - Create ~/.praisonai directory structure (logs, sessions) - Modify install.sh and install.ps1 for post-install onboarding - Add --no-onboard flag to skip wizard - Add PraisonAIConfigError for missing API keys - Update doctor command to suggest 'praisonai setup' - Include comprehensive unit and integration tests Fixes #1451 Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
praisonai-triage-agent[bot] has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@coderabbitai review |
|
/review |
✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughThis PR introduces an interactive setup/onboarding feature for PraisonAI, including a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as praisonai setup (CLI)
participant Handler as SetupHandler
participant FileSystem as Filesystem<br/>(.env, config.yaml)
User->>CLI: praisonai setup --provider openai --api-key XXX
CLI->>Handler: execute(non_interactive=True, provider="openai", api_key="XXX")
Handler->>Handler: Validate provider & API key
Handler->>Handler: Assign default model
Handler->>FileSystem: Write ~/.praisonai/.env
Handler->>FileSystem: Write ~/.praisonai/config.yaml
Handler->>FileSystem: Create logs/, sessions/ dirs
Handler-->>CLI: Return exit code 0
CLI-->>User: Setup complete
sequenceDiagram
participant User
participant CLI as praisonai setup (CLI)
participant Handler as SetupHandler
participant RichPrompt as rich.prompt
participant FileSystem as Filesystem
User->>CLI: praisonai setup (interactive)
CLI->>Handler: execute(non_interactive=False)
Handler->>RichPrompt: Prompt provider selection
User->>RichPrompt: Select provider (e.g., openai)
RichPrompt-->>Handler: Return provider
Handler->>RichPrompt: Prompt API key
User->>RichPrompt: Enter API key
RichPrompt-->>Handler: Return API key
Handler->>RichPrompt: Prompt model selection
User->>RichPrompt: Select model
RichPrompt-->>Handler: Return model
Handler->>RichPrompt: Prompt telemetry consent
User->>RichPrompt: Confirm telemetry
RichPrompt-->>Handler: Return telemetry choice
Handler->>FileSystem: Write .env, config.yaml, optionally agents.yaml
Handler-->>CLI: Return exit code 0
CLI-->>User: Setup wizard complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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:
|
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/8c683e28-66c2-42f7-abc2-08efd37b22c7 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
praisonai-triage-agent[bot] has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/8c683e28-66c2-42f7-abc2-08efd37b22c7 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
praisonai-triage-agent[bot] has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Completed a full PR review and applied targeted fixes in Key findings:
CI note: I checked workflow runs/logs via Actions API; the failing runs for this branch showed |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai-agents/praisonaiagents/errors.py (1)
363-376:⚠️ Potential issue | 🟡 MinorExport
PraisonAIConfigErrorfrompraisonaiagents.errors.The new public error is missing from
__all__, sofrom praisonaiagents.errors import *will omit it despite this list being the file’s “Export all error types” surface.Proposed fix
"HandoffError", "HandoffCycleError", "HandoffDepthError", - "HandoffTimeoutError" + "HandoffTimeoutError", + "PraisonAIConfigError" ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/errors.py` around lines 363 - 376, The __all__ export list in praisonaiagents.errors is missing the new public error class PraisonAIConfigError, so wildcard imports (from praisonaiagents.errors import *) will omit it; update the __all__ list to include "PraisonAIConfigError" alongside the existing symbols (e.g., "PraisonAIError", "ToolExecutionError", "LLMError", etc.) so the new class is exported publicly.
🧹 Nitpick comments (1)
src/praisonai/scripts/install.sh (1)
632-644: Python resolution can produce an empty$pywhenPYTHON_CMDis unset and venv is skipped.When
SKIP_VENV=1(orvenv_diris empty) andPYTHON_CMDis empty (the common case —find_pythonwrites to stdout but its result is never assigned toPYTHON_CMD),pybecomes"", then[[ ! -x "" ]]is true and it falls back topython3. That's fine, but it's fragile: if a user passed--python /custom/pythonearlier,PYTHON_CMDwill be set to that value and used here directly, bypassing the venv's interpreter even when one exists.Suggest flipping the precedence to prefer the venv interpreter first, then
PYTHON_CMD, thenpython3:Proposed fix
- # Determine python command - local py="$PYTHON_CMD" - if [[ -n "$venv_dir" ]] && [[ "$SKIP_VENV" != "1" ]]; then - py="$venv_dir/bin/python" - fi - - # Fallback to system python if venv python doesn't exist - if [[ ! -x "$py" ]]; then - py="python3" - fi + # Prefer venv python, then user-specified, then system python3 + local py="" + if [[ -n "$venv_dir" && "$SKIP_VENV" != "1" && -x "$venv_dir/bin/python" ]]; then + py="$venv_dir/bin/python" + elif [[ -n "$PYTHON_CMD" ]] && command -v "$PYTHON_CMD" >/dev/null 2>&1; then + py="$PYTHON_CMD" + else + py="python3" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/scripts/install.sh` around lines 632 - 644, The current python selection can end up using PYTHON_CMD even when a valid venv interpreter exists or produce an empty $py; change the selection in the block that sets local py so it prefers the venv interpreter first (when venv_dir is set and SKIP_VENV != "1"), then uses PYTHON_CMD only if non-empty and executable, and finally falls back to "python3"; implement this by setting py to the venv path when applicable, otherwise checking [[ -n "$PYTHON_CMD" && -x "$PYTHON_CMD" ]] to assign py="$PYTHON_CMD", and only if neither is available set py="python3" (use the existing variables py, venv_dir, SKIP_VENV, and PYTHON_CMD in the refactor).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/errors.py`:
- Around line 295-323: PraisonAIConfigError should carry a structured
remediation hint: add an optional parameter remediation_hint: Optional[str] =
None to the PraisonAIConfigError __init__, compute a sensible default (e.g.,
"Verify and set the {config_key} configuration" when config_key is present) when
remediation_hint is not provided, include this hint in the error's context
(context["remediation_hint"] = remediation_hint) and set self.remediation_hint =
remediation_hint, and ensure the call to super().__init__ still receives the
augmented context so downstream renderers and on_error hooks (error interception
points) can show consistent recovery steps; update references to
PraisonAIConfigError accordingly.
In `@src/praisonai/praisonai/cli/commands/setup.py`:
- Around line 152-164: The reset currently only unlinks ENV_FILE (deletes .env)
but must also remove the written config.yaml; update the reset flow in setup.py
so after checking/removing ENV_FILE it also checks for the config file (e.g.,
the CONFIG_FILE or CONFIG_YAML path used elsewhere in this module, or fallback
to "config.yaml") and unlinks it if it exists, handling errors the same way and
updating output.print_success to reflect both files removed (and still
respecting the force/confirm logic and try/except around file deletion).
In `@src/praisonai/praisonai/cli/features/setup/handler.py`:
- Around line 227-228: The starter YAML currently uses a hard-coded model in
AGENTS_TEMPLATE (gpt-4o-mini) causing mismatches; update the workflow so the
chosen model is threaded into YAML generation: change the caller that sets
create_starter (where create_starter is used) to pass the selected model to
_create_starter_yaml, update _create_starter_yaml to accept a model parameter
and substitute it into AGENTS_TEMPLATE rather than using the hard-coded value,
and ensure other code paths noted around the block at lines ~299-307 also pass
the selected model when generating the YAML so CLI, Python API, and YAML outputs
stay consistent.
- Around line 64-69: The directory creation in _create_directories currently
uses mkdir() without explicit permissions; change it to create ~/.praisonai,
logs, and sessions with owner-only perms by passing mode=0o700 to mkdir (for
praison_home, praison_home/"logs", and praison_home/"sessions") and then ensure
existing directories also get tightened by calling praison_home.chmod(0o700) and
likewise chmod(0o700) on the logs and sessions paths after creation; keep the
output.print_success line unchanged so the success message still prints.
- Line 127: Ruff flags F541 because there are several output.console.print calls
using unnecessary f-strings; locate each output.console.print(...) invocation
(e.g., the line with output.console.print(f'Try: [cyan]praisonai "Say hello in
one sentence"[/cyan]') and the other occurrences around the reported ranges) and
remove the leading f from the string literal so they become plain strings (e.g.,
change f'...' to '...') since no interpolation is performed; ensure you update
all occurrences referenced (lines ~127, 192, 202, 211, 231-238, 304-309) so no
placeholder-free f-strings remain.
- Around line 102-120: The current non-interactive setup allows passing secrets
via --api-key and can write model: null for custom/no-default providers; change
the logic in the handler to (1) prefer reading the secret from the environment
using env_key (os.getenv(env_key)) and only use the api_key CLI value as a
fallback with an explicit warning or reject it in non-interactive runs to avoid
exposing secrets in process args, (2) when env_key exists and env value is
present, populate env_vars[env_key] from that value (do not write raw
--api-key), (3) enforce that if default_model is None or provider == "custom"
and model is not provided then return an error asking for --model (do not set
model to None in config), and (4) ensure the final config dict uses the resolved
non-null model and includes provider, model, telemetry, and env_vars
appropriately (references: env_key, api_key, provider, provider_name, model,
default_model, env_vars, config).
- Around line 269-271: The current sequence calls
env_file.write_text(env_content) then env_file.chmod(0o600), which briefly
creates the .env with umask-controlled permissions; instead open and create the
file with the desired mode atomically so it is never writable/readable with
permissive bits: use os.open on the env_file path with flags
O_WRONLY|O_CREAT|O_TRUNC and mode 0o600, write env_content (as bytes) via the
returned file descriptor and close it (or wrap the fd in os.fdopen) rather than
calling write_text then chmod; update the code in handler.py where env_file and
env_content are used (replace write_text/chmod calls) to perform this
secure-create-and-write sequence.
In `@src/praisonai/praisonai/cli/features/setup/templates.py`:
- Around line 39-62: The AGENTS_TEMPLATE currently hardcodes llm: "gpt-4o-mini",
which breaks non-OpenAI providers; update AGENTS_TEMPLATE to accept a model
placeholder (e.g., "{model}" or leave llm empty) and change the
_create_starter_yaml function to substitute the selected provider/model from the
setup wizard into that placeholder (or omit the llm key so default
provider/model resolution applies) so the generated agents.yaml uses the
user-selected model instead of always gpt-4o-mini.
In `@src/praisonai/scripts/install.ps1`:
- Around line 426-453: Invoke-Onboarding currently only checks -NoOnboard and
-DryRun and can still run in non-interactive contexts; update Invoke-Onboarding
to also honor an explicit -NoPrompt switch and the environment variable
$env:PRAISONAI_NO_PROMPT, and add a TTY/user-interactive guard using
[Environment]::UserInteractive and [Console]::IsInputRedirected (or similar) so
the function returns early when running in CI, scheduled tasks, or piped install
scenarios; keep references to the existing Invoke-Onboarding function and the
NoOnboard/DryRun checks and ensure the new checks run before starting the
interactive setup wizard.
- Around line 455-468: The try/catch around running the setup wizard won't catch
non-terminating failures from the native python process; after invoking &
$pythonCmd -m praisonai setup, check $LASTEXITCODE and treat any non-zero value
as failure: move the success messages behind a conditional that verifies
$LASTEXITCODE -eq 0 and run the existing warning/help text when it's non-zero
(the same handling currently in the catch block); keep the catch for true
terminating errors (e.g., missing python) but ensure $pythonCmd and the
praisonai setup invocation are followed by an explicit $LASTEXITCODE check to
decide success vs failure.
In `@src/praisonai/scripts/install.sh`:
- Around line 644-655: The current invocation in install.sh that runs the setup
wizard (the if "$py" -m praisonai setup < /dev/tty; then ... in run_onboarding)
only re-attaches stdin to /dev/tty and can still leave stdout/stderr non-TTY
(causing Rich prompts to fall back and a misleading success message); update
run_onboarding to either (A) also attach stdout and stderr to the TTY when
launching the wizard (redirect >/dev/tty and 2>/dev/tty along with the existing
stdin redirection) or (B) perform an explicit TTY check before launching (e.g.
verify [ -t 0 ] && [ -t 1 ] && [ -t 2 ] or at minimum [ -t 1 ]) and skip or warn
if not all required streams are TTYs so the wizard is not invoked in a
broken/non-interactive environment.
In `@src/praisonai/tests/integration/test_setup_integration.py`:
- Around line 8-11: Tests currently use a hard-coded GitHub runner path and a
plain "python" invocation; change subprocess calls to use sys.executable and
compute the working directory relative to the test file using
Path(__file__).resolve() (e.g., package_dir =
Path(__file__).resolve().parents[1] or appropriate parent to reach the
src/praisonai package) and pass cwd=str(package_dir) to subprocess.run/ Popen;
update all occurrences referenced in the file (lines ~33-48, 65-73, 83-97,
121-149) and replace "python" with sys.executable to ensure the current test
interpreter is used, and where external state, timing or network calls are
involved wrap/replace them with mocks/fixtures (use unittest.mock or pytest
fixtures) so the subprocess tests are deterministic and runner-path independent.
In `@src/praisonai/tests/unit/cli/test_setup_command.py`:
- Around line 69-70: The permission assertion using
oct(env_file.stat().st_mode)[-3:] == "600" will fail on Windows; update the
test(s) that check file permissions (the assertion around
env_file.stat().st_mode and the similar block at lines 216-230) to skip on
Windows using pytest.mark.skipif(os.name == "nt" or
sys.platform.startswith("win"), reason="permission bits not reliable on
Windows") or alternatively replace the exact-mode assertion with a
platform-aware check (e.g., only assert presence of owner read/write on POSIX
via os.name == "posix" guard before checking st_mode). Ensure you reference the
existing assertion expression env_file.stat().st_mode when locating and
modifying the tests.
---
Outside diff comments:
In `@src/praisonai-agents/praisonaiagents/errors.py`:
- Around line 363-376: The __all__ export list in praisonaiagents.errors is
missing the new public error class PraisonAIConfigError, so wildcard imports
(from praisonaiagents.errors import *) will omit it; update the __all__ list to
include "PraisonAIConfigError" alongside the existing symbols (e.g.,
"PraisonAIError", "ToolExecutionError", "LLMError", etc.) so the new class is
exported publicly.
---
Nitpick comments:
In `@src/praisonai/scripts/install.sh`:
- Around line 632-644: The current python selection can end up using PYTHON_CMD
even when a valid venv interpreter exists or produce an empty $py; change the
selection in the block that sets local py so it prefers the venv interpreter
first (when venv_dir is set and SKIP_VENV != "1"), then uses PYTHON_CMD only if
non-empty and executable, and finally falls back to "python3"; implement this by
setting py to the venv path when applicable, otherwise checking [[ -n
"$PYTHON_CMD" && -x "$PYTHON_CMD" ]] to assign py="$PYTHON_CMD", and only if
neither is available set py="python3" (use the existing variables py, venv_dir,
SKIP_VENV, and PYTHON_CMD in the refactor).
🪄 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: 5ba87177-287a-48d2-83f2-519bb79c9f86
📒 Files selected for processing (13)
src/praisonai-agents/praisonaiagents/__init__.pysrc/praisonai-agents/praisonaiagents/errors.pysrc/praisonai/praisonai/cli/app.pysrc/praisonai/praisonai/cli/commands/__init__.pysrc/praisonai/praisonai/cli/commands/setup.pysrc/praisonai/praisonai/cli/features/doctor/checks/env_checks.pysrc/praisonai/praisonai/cli/features/setup/__init__.pysrc/praisonai/praisonai/cli/features/setup/handler.pysrc/praisonai/praisonai/cli/features/setup/templates.pysrc/praisonai/scripts/install.ps1src/praisonai/scripts/install.shsrc/praisonai/tests/integration/test_setup_integration.pysrc/praisonai/tests/unit/cli/test_setup_command.py
| class PraisonAIConfigError(PraisonAIError): | ||
| """ | ||
| Configuration error (missing API keys, invalid settings, etc). | ||
|
|
||
| Usually indicates setup issues that require user action. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| message: str, | ||
| config_key: Optional[str] = None, | ||
| agent_id: str = "unknown", | ||
| run_id: Optional[str] = None, | ||
| is_retryable: bool = False, # Config errors need user intervention | ||
| context: Optional[Dict[str, Any]] = None | ||
| ): | ||
| context = context or {} | ||
| if config_key: | ||
| context.update({"config_key": config_key}) | ||
|
|
||
| super().__init__( | ||
| message, | ||
| agent_id=agent_id, | ||
| run_id=run_id, | ||
| error_category="validation", # Configuration is a type of validation error | ||
| is_retryable=is_retryable, | ||
| context=context | ||
| ) | ||
| self.config_key = config_key |
There was a problem hiding this comment.
Add a structured remediation hint to config errors.
PraisonAIConfigError captures config_key, but the rendered exception still relies entirely on caller-supplied text for next steps. Add an optional/default remediation hint so setup failures consistently tell users how to recover. As per coding guidelines, "Error handling: Fail fast with clear error messages; include remediation hints in exceptions; propagate context (agent name, tool name, session ID); provide hook points for error interception (on_error events)".
Proposed refinement
class PraisonAIConfigError(PraisonAIError):
@@
def __init__(
self,
message: str,
config_key: Optional[str] = None,
agent_id: str = "unknown",
run_id: Optional[str] = None,
is_retryable: bool = False, # Config errors need user intervention
+ remediation_hint: Optional[str] = None,
context: Optional[Dict[str, Any]] = None
):
context = context or {}
if config_key:
context.update({"config_key": config_key})
+ if remediation_hint is None:
+ remediation_hint = f"Set {config_key} or run the setup wizard before retrying."
+ if remediation_hint:
+ context["remediation_hint"] = remediation_hint
+ message = f"{message} Remediation: {remediation_hint}"
super().__init__(
message,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/errors.py` around lines 295 - 323,
PraisonAIConfigError should carry a structured remediation hint: add an optional
parameter remediation_hint: Optional[str] = None to the PraisonAIConfigError
__init__, compute a sensible default (e.g., "Verify and set the {config_key}
configuration" when config_key is present) when remediation_hint is not
provided, include this hint in the error's context (context["remediation_hint"]
= remediation_hint) and set self.remediation_hint = remediation_hint, and ensure
the call to super().__init__ still receives the augmented context so downstream
renderers and on_error hooks (error interception points) can show consistent
recovery steps; update references to PraisonAIConfigError accordingly.
| if not ENV_FILE.exists(): | ||
| output.print_info("No setup configuration to reset.") | ||
| return | ||
|
|
||
| if not force: | ||
| confirm = typer.confirm(f"Reset configuration at {ENV_FILE}?") | ||
| if not confirm: | ||
| output.print_info("Reset cancelled.") | ||
| return | ||
|
|
||
| try: | ||
| ENV_FILE.unlink() | ||
| output.print_success("Configuration reset successfully.") |
There was a problem hiding this comment.
Reset should remove config.yaml as well as .env.
Setup writes both .env and config.yaml, but reset only deletes .env. A user can reset successfully and still retain stale provider/model/telemetry configuration.
Proposed fix
- if not ENV_FILE.exists():
+ praison_home = get_praison_home()
+ env_file = praison_home / ".env"
+ config_file = praison_home / "config.yaml"
+ files_to_remove = [path for path in (env_file, config_file) if path.exists()]
+
+ if not files_to_remove:
output.print_info("No setup configuration to reset.")
return
if not force:
- confirm = typer.confirm(f"Reset configuration at {ENV_FILE}?")
+ confirm = typer.confirm(f"Reset configuration at {praison_home}?")
if not confirm:
output.print_info("Reset cancelled.")
return
try:
- ENV_FILE.unlink()
+ for path in files_to_remove:
+ path.unlink()
output.print_success("Configuration reset successfully.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/cli/commands/setup.py` around lines 152 - 164, The
reset currently only unlinks ENV_FILE (deletes .env) but must also remove the
written config.yaml; update the reset flow in setup.py so after
checking/removing ENV_FILE it also checks for the config file (e.g., the
CONFIG_FILE or CONFIG_YAML path used elsewhere in this module, or fallback to
"config.yaml") and unlinks it if it exists, handling errors the same way and
updating output.print_success to reflect both files removed (and still
respecting the force/confirm logic and try/except around file deletion).
| def _create_directories(self, praison_home: Path, output) -> None: | ||
| """Create the ~/.praisonai directory structure.""" | ||
| praison_home.mkdir(parents=True, exist_ok=True) | ||
| (praison_home / "logs").mkdir(exist_ok=True) | ||
| (praison_home / "sessions").mkdir(exist_ok=True) | ||
| output.print_success(f"Created directory structure at {praison_home}") |
There was a problem hiding this comment.
Create PraisonAI state directories with private permissions.
mkdir() currently relies on the process umask, so ~/.praisonai, logs, and sessions may become 755. These directories can hold user prompts, outputs, and session metadata; default them to owner-only access.
Proposed fix
def _create_directories(self, praison_home: Path, output) -> None:
"""Create the ~/.praisonai directory structure."""
- praison_home.mkdir(parents=True, exist_ok=True)
- (praison_home / "logs").mkdir(exist_ok=True)
- (praison_home / "sessions").mkdir(exist_ok=True)
+ praison_home.mkdir(parents=True, exist_ok=True, mode=0o700)
+ (praison_home / "logs").mkdir(exist_ok=True, mode=0o700)
+ (praison_home / "sessions").mkdir(exist_ok=True, mode=0o700)
+ praison_home.chmod(0o700)
+ (praison_home / "logs").chmod(0o700)
+ (praison_home / "sessions").chmod(0o700)
output.print_success(f"Created directory structure at {praison_home}")📝 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.
| def _create_directories(self, praison_home: Path, output) -> None: | |
| """Create the ~/.praisonai directory structure.""" | |
| praison_home.mkdir(parents=True, exist_ok=True) | |
| (praison_home / "logs").mkdir(exist_ok=True) | |
| (praison_home / "sessions").mkdir(exist_ok=True) | |
| output.print_success(f"Created directory structure at {praison_home}") | |
| def _create_directories(self, praison_home: Path, output) -> None: | |
| """Create the ~/.praisonai directory structure.""" | |
| praison_home.mkdir(parents=True, exist_ok=True, mode=0o700) | |
| (praison_home / "logs").mkdir(exist_ok=True, mode=0o700) | |
| (praison_home / "sessions").mkdir(exist_ok=True, mode=0o700) | |
| praison_home.chmod(0o700) | |
| (praison_home / "logs").chmod(0o700) | |
| (praison_home / "sessions").chmod(0o700) | |
| output.print_success(f"Created directory structure at {praison_home}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/cli/features/setup/handler.py` around lines 64 - 69,
The directory creation in _create_directories currently uses mkdir() without
explicit permissions; change it to create ~/.praisonai, logs, and sessions with
owner-only perms by passing mode=0o700 to mkdir (for praison_home,
praison_home/"logs", and praison_home/"sessions") and then ensure existing
directories also get tightened by calling praison_home.chmod(0o700) and likewise
chmod(0o700) on the logs and sessions paths after creation; keep the
output.print_success line unchanged so the success message still prints.
| # Validate API key requirement | ||
| if env_key and not api_key: | ||
| output.print_error(f"API key is required for {provider_name}") | ||
| return 1 | ||
|
|
||
| # Use default model if not specified | ||
| if not model: | ||
| model = default_model | ||
|
|
||
| # Write configuration | ||
| env_vars = {} | ||
| if env_key and api_key: | ||
| env_vars[env_key] = api_key | ||
|
|
||
| config = { | ||
| "provider": provider, | ||
| "model": model, | ||
| "telemetry": True, # Default enabled in non-interactive mode | ||
| } |
There was a problem hiding this comment.
Tighten non-interactive validation for secrets and custom providers.
CI users currently have to pass API keys via --api-key, which exposes secrets through shell history/process args. Also, custom without --model writes model: null. Read the provider env var as a fallback and require a model when no default exists.
Proposed fix
provider_name = provider_info[0]
env_key = provider_info[1]
default_model = provider_info[2]
# Validate API key requirement
- if env_key and not api_key:
- output.print_error(f"API key is required for {provider_name}")
- return 1
+ if env_key:
+ api_key = api_key or os.getenv(env_key)
+ if not api_key:
+ output.print_error(
+ f"API key is required for {provider_name}; pass --api-key or set {env_key}"
+ )
+ return 1
# Use default model if not specified
if not model:
- model = default_model
+ if default_model:
+ model = default_model
+ else:
+ output.print_error("Model is required for custom provider")
+ return 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/cli/features/setup/handler.py` around lines 102 -
120, The current non-interactive setup allows passing secrets via --api-key and
can write model: null for custom/no-default providers; change the logic in the
handler to (1) prefer reading the secret from the environment using env_key
(os.getenv(env_key)) and only use the api_key CLI value as a fallback with an
explicit warning or reject it in non-interactive runs to avoid exposing secrets
in process args, (2) when env_key exists and env value is present, populate
env_vars[env_key] from that value (do not write raw --api-key), (3) enforce that
if default_model is None or provider == "custom" and model is not provided then
return an error asking for --model (do not set model to None in config), and (4)
ensure the final config dict uses the resolved non-null model and includes
provider, model, telemetry, and env_vars appropriately (references: env_key,
api_key, provider, provider_name, model, default_model, env_vars, config).
|
|
||
| output.print_success("Setup complete") | ||
| output.console.print(f"Configuration saved to {praison_home}") | ||
| output.console.print(f'Try: [cyan]praisonai "Say hello in one sentence"[/cyan]') |
There was a problem hiding this comment.
Remove placeholder-free f-strings flagged by Ruff.
These strings do not interpolate anything and will fail Ruff F541 in lint-enforced CI.
Proposed fix
- output.console.print(f'Try: [cyan]praisonai "Say hello in one sentence"[/cyan]')
+ output.console.print('Try: [cyan]praisonai "Say hello in one sentence"[/cyan]')
@@
- output.console.print(f"\n[bold]3. Choose default model:[/bold]")
+ output.console.print("\n[bold]3. Choose default model:[/bold]")
@@
- output.console.print(f"\n[bold]4. Telemetry settings:[/bold]")
+ output.console.print("\n[bold]4. Telemetry settings:[/bold]")
@@
- output.console.print(f"\n[bold]5. Create starter configuration:[/bold]")
+ output.console.print("\n[bold]5. Create starter configuration:[/bold]")
@@
- output.console.print(f"\n[bold green]✅ Setup complete![/bold green]")
+ output.console.print("\n[bold green]✅ Setup complete![/bold green]")
@@
- output.console.print(f"\n[bold]Next steps:[/bold]")
- output.console.print(f'• Test: [cyan]praisonai "Say hello in one sentence"[/cyan]')
- output.console.print(f'• Chat: [cyan]praisonai chat[/cyan]')
- output.console.print(f'• Help: [cyan]praisonai --help[/cyan]')
- output.console.print(f'• Docs: [cyan]praisonai doctor[/cyan]')
+ output.console.print("\n[bold]Next steps:[/bold]")
+ output.console.print('• Test: [cyan]praisonai "Say hello in one sentence"[/cyan]')
+ output.console.print("• Chat: [cyan]praisonai chat[/cyan]")
+ output.console.print("• Help: [cyan]praisonai --help[/cyan]")
+ output.console.print("• Docs: [cyan]praisonai doctor[/cyan]")
@@
- output.print_warning(f"agents.yaml already exists, skipping creation")
+ output.print_warning("agents.yaml already exists, skipping creation")
@@
- output.console.print(f"Run [cyan]praisonai workflow run --file agents.yaml[/cyan] to test it")
+ output.console.print("Run [cyan]praisonai workflow run --file agents.yaml[/cyan] to test it")Also applies to: 192-192, 202-202, 211-211, 231-238, 304-309
🧰 Tools
🪛 Ruff (0.15.10)
[error] 127-127: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/cli/features/setup/handler.py` at line 127, Ruff
flags F541 because there are several output.console.print calls using
unnecessary f-strings; locate each output.console.print(...) invocation (e.g.,
the line with output.console.print(f'Try: [cyan]praisonai "Say hello in one
sentence"[/cyan]') and the other occurrences around the reported ranges) and
remove the leading f from the string literal so they become plain strings (e.g.,
change f'...' to '...') since no interpolation is performed; ensure you update
all occurrences referenced (lines ~127, 192, 202, 211, 231-238, 304-309) so no
placeholder-free f-strings remain.
| # Run interactive onboarding after installation | ||
| function Invoke-Onboarding { | ||
| param([string]$VenvPath) | ||
|
|
||
| # Skip onboarding if requested | ||
| if ($NoOnboard) { | ||
| Write-Info "Skipping onboarding (-NoOnboard)" | ||
| return | ||
| } | ||
|
|
||
| # Skip onboarding in dry run mode | ||
| if ($DryRun) { | ||
| Write-Info "Dry run mode — skipping onboarding" | ||
| return | ||
| } | ||
|
|
||
| Write-Step "Starting interactive setup wizard..." | ||
|
|
||
| # Determine python command | ||
| $pythonCmd = "python" | ||
| if ($VenvPath -and -not $NoVenv) { | ||
| $pythonCmd = "$VenvPath\Scripts\python.exe" | ||
| } | ||
|
|
||
| # Fallback to system python if venv python doesn't exist | ||
| if (-not (Test-Path $pythonCmd)) { | ||
| $pythonCmd = "python" | ||
| } |
There was a problem hiding this comment.
Missing non-interactive/TTY guard — PowerShell counterpart of the shell script's /dev/tty check.
install.sh correctly skips onboarding when there is no TTY or when NO_PROMPT=1. Invoke-Onboarding skips only on -NoOnboard and -DryRun, so running iwr … | iex inside a CI job, a scheduled task, or a remoting session will still attempt to launch the interactive wizard and then hang or fail. Consider also honoring $env:PRAISONAI_NO_PROMPT/a -NoPrompt switch and checking [Environment]::UserInteractive / [Console]::IsInputRedirected before invoking the wizard.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'install.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/scripts/install.ps1` around lines 426 - 453, Invoke-Onboarding
currently only checks -NoOnboard and -DryRun and can still run in
non-interactive contexts; update Invoke-Onboarding to also honor an explicit
-NoPrompt switch and the environment variable $env:PRAISONAI_NO_PROMPT, and add
a TTY/user-interactive guard using [Environment]::UserInteractive and
[Console]::IsInputRedirected (or similar) so the function returns early when
running in CI, scheduled tasks, or piped install scenarios; keep references to
the existing Invoke-Onboarding function and the NoOnboard/DryRun checks and
ensure the new checks run before starting the interactive setup wizard.
| try { | ||
| # Run the setup wizard | ||
| & $pythonCmd -m praisonai setup | ||
| Write-Success "Setup wizard completed successfully!" | ||
| Write-Host "" | ||
| Write-Host "You're all set! 🎉" -ForegroundColor Green | ||
| Write-Host "" | ||
| } catch { | ||
| Write-Warning "Setup wizard failed or was cancelled." | ||
| Write-Host "" | ||
| Write-Host "Don't worry! You can run the setup wizard anytime with:" -ForegroundColor Yellow | ||
| Write-Host " praisonai setup" -ForegroundColor Cyan | ||
| Write-Host "" | ||
| } |
There was a problem hiding this comment.
try/catch won't catch a non-zero exit from python -m praisonai setup.
Native-command failures in PowerShell do not raise terminating errors by default, so this try/catch only fires on things like "python.exe not found". If the wizard exits with a non-zero status (user cancels with Ctrl-C, validation error, invalid provider, etc.), the catch block is skipped and the script prints "Setup wizard completed successfully! 🎉" — the opposite of what happened.
Check $LASTEXITCODE explicitly:
Proposed fix
- try {
- # Run the setup wizard
- & $pythonCmd -m praisonai setup
- Write-Success "Setup wizard completed successfully!"
- Write-Host ""
- Write-Host "You're all set! 🎉" -ForegroundColor Green
- Write-Host ""
- } catch {
- Write-Warning "Setup wizard failed or was cancelled."
- Write-Host ""
- Write-Host "Don't worry! You can run the setup wizard anytime with:" -ForegroundColor Yellow
- Write-Host " praisonai setup" -ForegroundColor Cyan
- Write-Host ""
- }
+ try {
+ & $pythonCmd -m praisonai setup
+ $wizardExit = $LASTEXITCODE
+ } catch {
+ $wizardExit = 1
+ }
+
+ if ($wizardExit -eq 0) {
+ Write-Success "Setup wizard completed successfully!"
+ Write-Host ""
+ Write-Host "You're all set! 🎉" -ForegroundColor Green
+ Write-Host ""
+ } else {
+ Write-Warning "Setup wizard failed or was cancelled."
+ Write-Host ""
+ Write-Host "Don't worry! You can run the setup wizard anytime with:" -ForegroundColor Yellow
+ Write-Host " praisonai setup" -ForegroundColor Cyan
+ Write-Host ""
+ }📝 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.
| try { | |
| # Run the setup wizard | |
| & $pythonCmd -m praisonai setup | |
| Write-Success "Setup wizard completed successfully!" | |
| Write-Host "" | |
| Write-Host "You're all set! 🎉" -ForegroundColor Green | |
| Write-Host "" | |
| } catch { | |
| Write-Warning "Setup wizard failed or was cancelled." | |
| Write-Host "" | |
| Write-Host "Don't worry! You can run the setup wizard anytime with:" -ForegroundColor Yellow | |
| Write-Host " praisonai setup" -ForegroundColor Cyan | |
| Write-Host "" | |
| } | |
| try { | |
| & $pythonCmd -m praisonai setup | |
| $wizardExit = $LASTEXITCODE | |
| } catch { | |
| $wizardExit = 1 | |
| } | |
| if ($wizardExit -eq 0) { | |
| Write-Success "Setup wizard completed successfully!" | |
| Write-Host "" | |
| Write-Host "You're all set! 🎉" -ForegroundColor Green | |
| Write-Host "" | |
| } else { | |
| Write-Warning "Setup wizard failed or was cancelled." | |
| Write-Host "" | |
| Write-Host "Don't worry! You can run the setup wizard anytime with:" -ForegroundColor Yellow | |
| Write-Host " praisonai setup" -ForegroundColor Cyan | |
| Write-Host "" | |
| } |
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'install.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/scripts/install.ps1` around lines 455 - 468, The try/catch
around running the setup wizard won't catch non-terminating failures from the
native python process; after invoking & $pythonCmd -m praisonai setup, check
$LASTEXITCODE and treat any non-zero value as failure: move the success messages
behind a conditional that verifies $LASTEXITCODE -eq 0 and run the existing
warning/help text when it's non-zero (the same handling currently in the catch
block); keep the catch for true terminating errors (e.g., missing python) but
ensure $pythonCmd and the praisonai setup invocation are followed by an explicit
$LASTEXITCODE check to decide success vs failure.
| if "$py" -m praisonai setup < /dev/tty; then | ||
| log_success "Setup wizard completed successfully!" | ||
| echo "" | ||
| echo -e "${BOLD}${GREEN}You're all set! 🎉${NC}" | ||
| echo "" | ||
| else | ||
| log_warn "Setup wizard failed or was cancelled." | ||
| echo "" | ||
| echo -e "${YELLOW}Don't worry! You can run the setup wizard anytime with:${NC}" | ||
| echo -e " ${CYAN}praisonai setup${NC}" | ||
| echo "" | ||
| fi |
There was a problem hiding this comment.
Only stdin is re-attached to the TTY; Rich prompts also need a TTY on stdout/stderr.
When the installer is run via curl … | bash, only stdin is a pipe — stdout/stderr typically stay attached to the terminal, so < /dev/tty is usually sufficient. But if a user does something like curl … | bash > install.log 2>&1, the Rich wizard will detect a non-TTY stdout and silently fall back to non-interactive defaults (or error out), yet run_onboarding will still report "Setup wizard completed successfully!" based on exit code.
Consider either (a) also redirecting stdout/stderr from /dev/tty when appropriate, or (b) explicitly checking [ -t 1 ] in addition to [ -e /dev/tty ] before invoking the wizard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/scripts/install.sh` around lines 644 - 655, The current
invocation in install.sh that runs the setup wizard (the if "$py" -m praisonai
setup < /dev/tty; then ... in run_onboarding) only re-attaches stdin to /dev/tty
and can still leave stdout/stderr non-TTY (causing Rich prompts to fall back and
a misleading success message); update run_onboarding to either (A) also attach
stdout and stderr to the TTY when launching the wizard (redirect >/dev/tty and
2>/dev/tty along with the existing stdin redirection) or (B) perform an explicit
TTY check before launching (e.g. verify [ -t 0 ] && [ -t 1 ] && [ -t 2 ] or at
minimum [ -t 1 ]) and skip or warn if not all required streams are TTYs so the
wizard is not invoked in a broken/non-interactive environment.
| import os | ||
| import subprocess | ||
| import tempfile | ||
| from pathlib import Path |
There was a problem hiding this comment.
Make the subprocess tests independent of the GitHub runner path.
The hard-coded cwd="/home/runner/work/PraisonAI/PraisonAI/src/praisonai" will fail outside that exact checkout location. Use a path derived from __file__, and invoke the current test interpreter.
Proposed fix
import os
import subprocess
+import sys
import tempfile
from pathlib import Path
import pytest
+PROJECT_ROOT = Path(__file__).resolve().parents[2]
+
`@pytest.fixture`
def temp_praison_home():
@@
cmd = [
- "python", "-m", "praisonai", "setup",
+ sys.executable, "-m", "praisonai", "setup",
@@
- cwd="/home/runner/work/PraisonAI/PraisonAI/src/praisonai"
+ cwd=PROJECT_ROOT,
)
@@
- cmd = ["python", "-m", "praisonai", "setup", "--help"]
+ cmd = [sys.executable, "-m", "praisonai", "setup", "--help"]
@@
- cwd="/home/runner/work/PraisonAI/PraisonAI/src/praisonai"
+ cwd=PROJECT_ROOT,
)
@@
cmd = [
- "python", "-m", "praisonai", "setup",
+ sys.executable, "-m", "praisonai", "setup",
@@
- cwd="/home/runner/work/PraisonAI/PraisonAI/src/praisonai"
+ cwd=PROJECT_ROOT,
)
@@
base_cmd = [
- "python", "-m", "praisonai", "setup",
+ sys.executable, "-m", "praisonai", "setup",
@@
- cwd="/home/runner/work/PraisonAI/PraisonAI/src/praisonai"
+ cwd=PROJECT_ROOT,
)
@@
- cwd="/home/runner/work/PraisonAI/PraisonAI/src/praisonai"
+ cwd=PROJECT_ROOT,
)Based on learnings, test files must not depend on timing, external state, or network calls; use mocks and fixtures for isolation; ensure all tests are deterministic.
Also applies to: 33-48, 65-73, 83-97, 121-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/integration/test_setup_integration.py` around lines 8 -
11, Tests currently use a hard-coded GitHub runner path and a plain "python"
invocation; change subprocess calls to use sys.executable and compute the
working directory relative to the test file using Path(__file__).resolve()
(e.g., package_dir = Path(__file__).resolve().parents[1] or appropriate parent
to reach the src/praisonai package) and pass cwd=str(package_dir) to
subprocess.run/ Popen; update all occurrences referenced in the file (lines
~33-48, 65-73, 83-97, 121-149) and replace "python" with sys.executable to
ensure the current test interpreter is used, and where external state, timing or
network calls are involved wrap/replace them with mocks/fixtures (use
unittest.mock or pytest fixtures) so the subprocess tests are deterministic and
runner-path independent.
| # Check file permissions (should be 600) | ||
| assert oct(env_file.stat().st_mode)[-3:] == "600" |
There was a problem hiding this comment.
Permission assertion will fail on Windows.
os.chmod(path, 0o600) on Windows only toggles the read-only bit, so oct(stat_mode)[-3:] typically evaluates to "666" / "444", not "600". If this test suite is intended to run on Windows CI, guard these assertions with pytest.mark.skipif(os.name == "nt", ...) or check sys.platform.
Also applies to: 216-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/unit/cli/test_setup_command.py` around lines 69 - 70, The
permission assertion using oct(env_file.stat().st_mode)[-3:] == "600" will fail
on Windows; update the test(s) that check file permissions (the assertion around
env_file.stat().st_mode and the similar block at lines 216-230) to skip on
Windows using pytest.mark.skipif(os.name == "nt" or
sys.platform.startswith("win"), reason="permission bits not reliable on
Windows") or alternatively replace the exact-mode assertion with a
platform-aware check (e.g., only assert presence of owner read/write on POSIX
via os.name == "posix" guard before checking st_mode). Ensure you reference the
existing assertion expression env_file.stat().st_mode when locating and
modifying the tests.
|
@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 Final Architecture Review & Fix Implementation ✅ COMPLETEPhase 1: Architecture Review per AGENTS.md
Phase 2: Fix Valid Issues from Reviewers
Phase 3: Final Verdict
Architecture Review Results✅ PROTOCOL-DRIVEN: Setup feature correctly uses optional dependencies and lazy imports Issues Fixed & Committed (SHA: 00806cd)Critical Security & Architecture Issues:
Platform Compatibility & Robustness:
Final Verdict: ✅ APPROVEDThis PR successfully implements frictionless onboarding following AGENTS.md principles:
The setup wizard provides excellent developer experience while maintaining the framework's architectural integrity. All critical issues from reviewers have been resolved. Recommendation: Merge this PR. The setup feature will significantly improve user onboarding experience. |
- Add remediation hints to PraisonAIConfigError for better guidance - Fix setup reset to remove both .env and config.yaml files - Create directories with secure 700 permissions - Enhance non-interactive validation (prefer env vars, require model) - Use atomic .env file creation with secure permissions - Fix starter YAML to use selected model instead of hardcoded gpt-4o-mini - Add TTY/interactive checks to PowerShell installer - Improve shell script python command precedence - Remove unnecessary f-string prefixes (ruff F541) - Make tests platform-independent and Windows-compatible - Export PraisonAIConfigError in errors.__all__ Addresses CodeRabbit security and architecture findings. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Mervin Praison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
praisonai-triage-agent[bot] has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Summary
Implements comprehensive frictionless onboarding system per Issue #1451
Key Features
Implementation Details
Files Changed
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
praisonai setupcommand for onboarding and provider configurationwizard(interactive),config(view/edit settings), andreset--no-onboardflag)Bug Fixes
praisonai setupfor API key configurationTests