feat: Wire up bot onboarding + gateway daemon install + Windows support#1455
feat: Wire up bot onboarding + gateway daemon install + Windows support#1455MervinPraison merged 4 commits intomainfrom
Conversation
Wire existing OnboardWizard (254 lines) to CLI and extend gateway commands for cross-platform daemon management. Adds Windows Scheduled Task support. New Commands: - praisonai onboard - messaging bot setup wizard - praisonai gateway install/uninstall/logs - daemon management - praisonai bot install-daemon - convenience alias Platform Support: - Linux systemd user units - macOS LaunchAgent - Windows Scheduled Tasks (with Startup folder fallback) Enhanced install.sh: - Offers bot onboarding after LLM setup - Detects existing messaging tokens - Respects NO_ONBOARD flag Fixes #1454 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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 a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Onboard CLI
participant Wizard as Onboard Wizard
participant Dispatcher as Daemon Dispatcher
participant Platform as Platform Handler<br/>(systemd/launchd/windows)
participant OS as OS Service Manager
User->>CLI: praisonai onboard
CLI->>Wizard: run_onboard()
Wizard->>Wizard: Collect bot tokens & settings
Wizard->>Wizard: Offer daemon setup?
alt Daemon Install Requested
Wizard->>Dispatcher: install_daemon(config_path)
Dispatcher->>Dispatcher: _detect_platform()
Dispatcher->>Platform: install(config_path)
Platform->>OS: Create/Register Service
OS-->>Platform: Success/Failure
Platform-->>Dispatcher: {ok, installed, running}
Dispatcher-->>Wizard: Status result
Wizard->>Wizard: Display post-install instructions
end
Wizard-->>User: Onboarding complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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:
|
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/56b7de0e-b6b5-4139-a3fd-d3a47e5d7ce7 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
src/praisonai/tests/integration/test_install_sh_bot_onboarding.py (1)
59-66: Usebash -ninstead of bracket counting for shell syntax.The current checks can both miss real shell syntax errors and fail on harmless braces/brackets in strings or comments.
🧪 Proposed fix
- # Check for balanced brackets/braces - assert content.count('{') == content.count('}'), "Unmatched braces in shell script" - assert content.count('[') >= content.count(']'), "Unmatched brackets in shell script" - - # Check that functions are properly closed - function_starts = content.count('() {') - # Each function should have at least one closing brace - assert content.count('}') >= function_starts, "Functions not properly closed" + result = subprocess.run( + ["bash", "-n", str(script_path)], + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/integration/test_install_sh_bot_onboarding.py` around lines 59 - 66, Replace the brittle bracket-count checks (the asserts using content.count('{'), content.count(']'), and the function_starts logic) with a real shell syntax validation using "bash -n": write the script text in variable content to a temporary file or pass it to bash -n via stdin, run subprocess with "bash -n" and assert the exit code is zero (or raise an assertion including bash's stderr output) so actual shell syntax errors are detected reliably; remove the old count-based assertions (references: content, function_starts) and ensure any failure message includes the captured bash -n diagnostics.
🤖 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/praisonai/cli/commands/bot.py`:
- Around line 457-475: The installation block currently mixes exception handling
with result-based control, calls a non-existent output.print_warn, and never
forwards the start flag to install_daemon; fix by 1) move the call to
install_daemon(config_path=config, start=start) inside a minimal try that only
captures real exceptions and let result-based success/failure handling occur
outside that try so typer.Exit(1) isn't swallowed, 2) replace
output.print_warn(...) with the correct output.print_warning(...) in the
failure-to-start branch, and 3) keep the try/except narrowly scoped to catching
actual install_daemon exceptions and re-raise/exit immediately on exceptions
while handling result.get("ok") logic after the call (refer to install_daemon,
output.print_warning, typer.Exit, and the local variable start).
In `@src/praisonai/praisonai/cli/commands/gateway.py`:
- Around line 152-177: The CLI option --start/--no-start is not wired to the
daemon installer so --no-start may be ignored; either remove the start parameter
from the gateway command signature and the follow-up status check, or thread the
value through to the installer: modify the call to install_daemon(...) to accept
start=start (i.e. install_daemon(config_path=config, start=start) and update the
praisonai.daemon.install_daemon signature and each backend implementation to
honor the start flag, then keep the follow-up get_daemon_status() check only
when start is True.
- Around line 178-184: The broad except Exception blocks are catching deliberate
typer.Exit exceptions and causing duplicate/misleading error output; update each
try/except (the block handling result/error and the surrounding excepts in
gateway.py, including the other similar blocks around the code that raise
typer.Exit) to immediately re-raise typer.Exit before handling other exceptions
— e.g., add an explicit except typer.Exit: raise above the general except
Exception as e: so that typer.Exit from the earlier raise is not swallowed, and
keep the existing output.print_error(f"Installation error: {str(e)}") only in
the general Exception handler.
In `@src/praisonai/praisonai/cli/commands/onboard.py`:
- Around line 20-22: The except KeyboardInterrupt block currently re-raises
typer.Exit inside the handler which causes exception chaining noise; change the
raise to suppress chaining by raising typer.Exit(130) from None (i.e., update
the except KeyboardInterrupt that calls
get_output_controller().print_info("Cancelled.") so it does raise
typer.Exit(130) from None) to ensure only the intended cancellation message and
exit code are shown.
In `@src/praisonai/praisonai/daemon/windows.py`:
- Around line 201-205: The get_logs function returns a static message but uses
an unnecessary f-string which triggers Ruff F541; edit the get_logs function to
return a plain string literal instead of an f-string (update the return in
get_logs to "Log viewing not yet implemented for Windows. Check Event Viewer for
system logs.").
- Around line 22-24: The _python_executable function currently prefers PATH
python before the running interpreter which can install a daemon using a
different Python than the one running praisoni; change the selection to prefer
sys.executable first, then fallback to shutil.which("python3") and
shutil.which("python"), and ensure a non-empty string is returned (raise or
default to "python" only if all lookups fail). Update the return logic in
_python_executable to use sys.executable as the primary candidate and use the
which() fallbacks to locate a system binary only if sys.executable is not
available.
- Around line 55-68: The schtasks command assembly incorrectly uses the /SD flag
to pass working_dir (in the cmd list in the schtasks /Create block around
TASK_NAME and /TR), but /SD is a start-date flag and will break task creation;
remove the conditional that extends cmd with ["/SD", working_dir] so the
working_dir is not added to cmd (leave the existing /TR using the absolute
config path abs_config and python invocation untouched).
- Around line 187-193: Replace the unreliable tasklist CSV subprocess approach
(the cmd variable, subprocess.run call and result stdout checks that set
status["running"]) with a psutil-based iteration: import psutil and use
psutil.process_iter(['pid','name','cmdline']) to inspect proc.info['cmdline']
for any argument containing "praisonai", set status["running"]=True and break
when found, and handle psutil.NoSuchProcess, psutil.AccessDenied and
psutil.ZombieProcess per-process while wrapping the whole loop in a safe
try/except to avoid crashing the daemon.
In `@src/praisonai/scripts/install.sh`:
- Around line 667-698: The install script early-returns when "$env_file" is
missing which skips the bot onboarding prompt on fresh installs; instead remove
the "|| return 0" so the flow continues with has_token left as 0, ensure the
token-detection loop guards grep against a missing file (e.g., only run grep if
[[ -f "$env_file" ]]) and keep has_token default 0, and change the interactive
prompt/echo to write to /dev/tty (and already read from /dev/tty) so the prompt
is visible on interactive terminals; locate symbols env_file, has_token,
venv_dir, py, and the praisonai onboard invocation to make the changes.
In `@src/praisonai/tests/integration/test_install_sh_bot_onboarding.py`:
- Line 16: The test currently hardcodes script_path = "/home/runner/work/..."
which breaks outside that CI layout; update the test to compute the install.sh
path relative to the test file using Python's pathlib (e.g.
Path(__file__).resolve().parent / ".." / "scripts" / "install.sh" or similar) so
script_path is derived dynamically; replace occurrences of the hardcoded
script_path variable in the test (and any duplicates) with this computed Path
(and call str(...) if a string is required).
- Around line 47-48: The test currently checks for the symbol
maybe_offer_bot_onboarding in the file which matches the function definition;
update the assertion to ensure an actual invocation is present by asserting the
source contains a call pattern such as "maybe_offer_bot_onboarding()" or using a
regex like r"(?<!def\s)maybe_offer_bot_onboarding\s*\(" to exclude the
definition; locate the assertion in test_install_sh_bot_onboarding.py and
replace the loose substring check with a check for the invocation of
maybe_offer_bot_onboarding in main.
In `@src/praisonai/tests/unit/cli/test_onboard_command.py`:
- Around line 15-20: The test test_onboard_command_registered currently invokes
the onboard sub-app directly, so update it to invoke the top-level CLI app to
assert that register_commands() actually registered the "onboard" command: use
CliRunner().invoke(...) on the main CLI application object (the same app that
calls register_commands(), referenced by register_commands() and the top-level
app variable used across the CLI) and then assert result.exit_code == 0 and that
"onboard" or "wizard" appears in result.output; this ensures the test exercises
the central registration path rather than the onboard sub-app directly.
- Around line 23-43: The tests patch the wrong target: replace the `@patch` target
"praisonai.cli.commands.onboard.run_onboard" with the actual provider
"praisonai.cli.features.onboard.run_onboard" (since run_onboard is imported
inside onboard_callback), so update both tests (test_onboard_calls_wizard and
test_onboard_handles_keyboard_interrupt) to patch
"praisonai.cli.features.onboard.run_onboard" to avoid AttributeError and
correctly simulate KeyboardInterrupt and normal call behavior.
---
Nitpick comments:
In `@src/praisonai/tests/integration/test_install_sh_bot_onboarding.py`:
- Around line 59-66: Replace the brittle bracket-count checks (the asserts using
content.count('{'), content.count(']'), and the function_starts logic) with a
real shell syntax validation using "bash -n": write the script text in variable
content to a temporary file or pass it to bash -n via stdin, run subprocess with
"bash -n" and assert the exit code is zero (or raise an assertion including
bash's stderr output) so actual shell syntax errors are detected reliably;
remove the old count-based assertions (references: content, function_starts) and
ensure any failure message includes the captured bash -n diagnostics.
🪄 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: 8b3541e3-c62f-4fbd-a18b-5449cd0f98e1
📒 Files selected for processing (12)
src/praisonai/praisonai/cli/app.pysrc/praisonai/praisonai/cli/commands/__init__.pysrc/praisonai/praisonai/cli/commands/bot.pysrc/praisonai/praisonai/cli/commands/gateway.pysrc/praisonai/praisonai/cli/commands/onboard.pysrc/praisonai/praisonai/daemon/__init__.pysrc/praisonai/praisonai/daemon/windows.pysrc/praisonai/scripts/install.shsrc/praisonai/tests/integration/test_install_sh_bot_onboarding.pysrc/praisonai/tests/unit/cli/test_gateway_install.pysrc/praisonai/tests/unit/cli/test_onboard_command.pysrc/praisonai/tests/unit/daemon/test_daemon_dispatch.py
| try: | ||
| result = install_daemon(config_path=config) | ||
| if result.get("ok"): | ||
| output.print_success(result.get("message", "Service installed successfully")) | ||
| if start: | ||
| output.print_info("Starting the service...") | ||
| from praisonai.daemon import get_daemon_status | ||
| status = get_daemon_status() | ||
| if status.get("running"): | ||
| output.print_success("Service is now running") | ||
| else: | ||
| output.print_warn("Service installed but not running. Check system logs.") | ||
| else: | ||
| error = result.get("error", "Installation failed") | ||
| output.print_error(f"Installation failed: {error}") | ||
| raise typer.Exit(1) | ||
| except Exception as e: | ||
| output.print_error(f"Installation error: {str(e)}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'def install_daemon|def install\(|class OutputController|def print_warn|def print_warning' --iglob '*.py'Repository: MervinPraison/PraisonAI
Length of output: 4107
🏁 Script executed:
#!/bin/bash
# Check the full install_daemon implementation
echo "=== install_daemon implementation ==="
sed -n '44,65p' src/praisonai/praisonai/daemon/__init__.py
# Check the actual bot.py code at lines 457-475
echo -e "\n=== bot.py lines 440-480 ==="
sed -n '440,480p' src/praisonai/praisonai/cli/commands/bot.py
# Check if print_warn exists
echo -e "\n=== Search for print_warn ==="
rg 'def print_warn' --iglob '*.py'
# Check the full OutputController methods
echo -e "\n=== OutputController methods ==="
sed -n '91,250p' src/praisonai/praisonai/cli/output/console.py | head -100Repository: MervinPraison/PraisonAI
Length of output: 6152
Fix critical print_warn bug and honor --no-start flag.
The code has three issues:
-
Critical: Line 472 calls
output.print_warn(), butOutputControlleronly providesprint_warning(). This will crash withAttributeErrorwhen the daemon installs but fails to start. -
Major: The
startparameter is accepted but never forwarded toinstall_daemon(). Passstart=startto honor the--no-startflag. -
Major: The
typer.Exit(1)raised in the failure path (line 473) is caught by the broadexcept, producing misleading error messages. Move the success/failure logic outside the try block.
Proposed fix
output = get_output_controller()
try:
- result = install_daemon(config_path=config)
- if result.get("ok"):
- output.print_success(result.get("message", "Service installed successfully"))
- if start:
- output.print_info("Starting the service...")
- from praisonai.daemon import get_daemon_status
- status = get_daemon_status()
- if status.get("running"):
- output.print_success("Service is now running")
- else:
- output.print_warn("Service installed but not running. Check system logs.")
- else:
- error = result.get("error", "Installation failed")
- output.print_error(f"Installation failed: {error}")
- raise typer.Exit(1)
+ result = install_daemon(config_path=config, start=start)
except Exception as e:
- output.print_error(f"Installation error: {str(e)}")
- raise typer.Exit(1)
+ output.print_error(f"Installation error: {e!s}")
+ raise typer.Exit(1) from e
+
+ if result.get("ok"):
+ output.print_success(result.get("message", "Service installed successfully"))
+ if start:
+ output.print_info("Checking service status...")
+ from praisonai.daemon import get_daemon_status
+ status = get_daemon_status()
+ if status.get("running"):
+ output.print_success("Service is now running")
+ else:
+ output.print_warning("Service installed but not running. Check system logs.")
+ return
+
+ error = result.get("error", "Installation failed")
+ output.print_error(f"Installation failed: {error}")
+ raise typer.Exit(1)🧰 Tools
🪛 Ruff (0.15.10)
[warning] 473-473: Do not catch blind exception: Exception
(BLE001)
[warning] 474-474: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 475-475: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/cli/commands/bot.py` around lines 457 - 475, The
installation block currently mixes exception handling with result-based control,
calls a non-existent output.print_warn, and never forwards the start flag to
install_daemon; fix by 1) move the call to install_daemon(config_path=config,
start=start) inside a minimal try that only captures real exceptions and let
result-based success/failure handling occur outside that try so typer.Exit(1)
isn't swallowed, 2) replace output.print_warn(...) with the correct
output.print_warning(...) in the failure-to-start branch, and 3) keep the
try/except narrowly scoped to catching actual install_daemon exceptions and
re-raise/exit immediately on exceptions while handling result.get("ok") logic
after the call (refer to install_daemon, output.print_warning, typer.Exit, and
the local variable start).
| config: str = typer.Option("bot.yaml", "--config", help="Path to bot.yaml"), | ||
| start: bool = typer.Option(True, "--start/--no-start", help="Start after install"), | ||
| ): | ||
| """Install the gateway as an OS daemon (LaunchAgent / systemd). | ||
|
|
||
| Examples: | ||
| praisonai gateway install | ||
| praisonai gateway install --config my-bot.yaml --no-start | ||
| """ | ||
| from praisonai.daemon import install_daemon | ||
| from ..output.console import get_output_controller | ||
|
|
||
| output = get_output_controller() | ||
|
|
||
| try: | ||
| result = install_daemon(config_path=config) | ||
| if result.get("ok"): | ||
| output.print_success(result.get("message", "Service installed successfully")) | ||
| if start: | ||
| output.print_info("Starting the service...") | ||
| from praisonai.daemon import get_daemon_status | ||
| status = get_daemon_status() | ||
| if status.get("running"): | ||
| output.print_success("Service is now running") | ||
| else: | ||
| output.print_warn("Service installed but not running. Check system logs.") |
There was a problem hiding this comment.
Wire up --start/--no-start or remove the option.
start only controls the follow-up status check; it is not passed to install_daemon, and existing backends may start unconditionally. As written, praisonai gateway install --no-start can still start the service.
Either remove the option for now, or thread start=start through install_daemon() and make each backend honor it before documenting --no-start.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/cli/commands/gateway.py` around lines 152 - 177, The
CLI option --start/--no-start is not wired to the daemon installer so --no-start
may be ignored; either remove the start parameter from the gateway command
signature and the follow-up status check, or thread the value through to the
installer: modify the call to install_daemon(...) to accept start=start (i.e.
install_daemon(config_path=config, start=start) and update the
praisonai.daemon.install_daemon signature and each backend implementation to
honor the start flag, then keep the follow-up get_daemon_status() check only
when start is True.
| else: | ||
| error = result.get("error", "Installation failed") | ||
| output.print_error(f"Installation failed: {error}") | ||
| raise typer.Exit(1) | ||
| except Exception as e: | ||
| output.print_error(f"Installation error: {str(e)}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether typer.Exit is caught by a broad Exception handler in this environment.
python - <<'PY'
import typer
print(typer.Exit.__mro__)
print(issubclass(typer.Exit, Exception))
PYRepository: MervinPraison/PraisonAI
Length of output: 184
🏁 Script executed:
# First, let's examine the actual code at the specified lines
cat -n src/praisonai/praisonai/cli/commands/gateway.py | head -260 | tail -90Repository: MervinPraison/PraisonAI
Length of output: 3673
🌐 Web query:
typer.Exit exception inheritance chain Python
💡 Result:
typer.Exit is a custom exception class in the Typer library used to terminate CLI program execution gracefully with an optional exit code. It is imported directly from the typer module (import typer; raise typer.Exit) and is handled specially by Typer's internal machinery, which catches it and calls sys.exit with the specified code (default 0). Its exact inheritance chain is not explicitly documented in the official Typer documentation or visible source code snippets, but given that Typer is built on Click and Click uses click.exceptions.Exit (which inherits from Exception), typer.Exit is almost certainly a subclass of Exception. This allows it to be caught by except Exception blocks, unlike SystemExit which inherits directly from BaseException to avoid accidental catching. To verify the precise inheritance locally: import typer print(typer.Exit.mro) This would show the full Method Resolution Order (inheritance chain), e.g., (<class 'typer.Exit'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>). typer.Abort is a similar special exception that prints "Aborted!" before exiting with code 1.
Citations:
- 1: https://typer.tiangolo.com/tutorial/terminating/
- 2: https://typer.tiangolo.com/tutorial/terminating
- 3: [QUESTION] How to handle exception globally fastapi/typer#310
- 4: https://github.com/tiangolo/typer/blob/master/typer/main.py
- 5: https://github.com/tiangolo/typer/blob/master/typer/core.py
- 6: https://docs.python.org/3/library/exceptions.html
Do not catch deliberate typer.Exit failures.
These broad exception handlers catch the typer.Exit(1) raised just above them, causing duplicate/misleading error output on normal failure paths. Since typer.Exit is a subclass of Exception, it will be caught by the blanket except Exception clause.
🐛 Proposed fix
else:
error = result.get("error", "Installation failed")
output.print_error(f"Installation failed: {error}")
raise typer.Exit(1)
+ except typer.Exit:
+ raise
except Exception as e:
output.print_error(f"Installation error: {str(e)}")
raise typer.Exit(1)
@@
else:
error = result.get("error", "Uninstallation failed")
output.print_error(f"Uninstallation failed: {error}")
raise typer.Exit(1)
+ except typer.Exit:
+ raise
except Exception as e:
output.print_error(f"Uninstallation error: {str(e)}")
raise typer.Exit(1)
@@
else:
output.print_error(f"Unsupported platform: {plat}")
raise typer.Exit(1)
+ except typer.Exit:
+ raise
except Exception as e:
output.print_error(f"Error reading logs: {str(e)}")
raise typer.Exit(1)Also applies to: 203-209, 252-257
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 182-182: Do not catch blind exception: Exception
(BLE001)
[warning] 183-183: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 184-184: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/cli/commands/gateway.py` around lines 178 - 184, The
broad except Exception blocks are catching deliberate typer.Exit exceptions and
causing duplicate/misleading error output; update each try/except (the block
handling result/error and the surrounding excepts in gateway.py, including the
other similar blocks around the code that raise typer.Exit) to immediately
re-raise typer.Exit before handling other exceptions — e.g., add an explicit
except typer.Exit: raise above the general except Exception as e: so that
typer.Exit from the earlier raise is not swallowed, and keep the existing
output.print_error(f"Installation error: {str(e)}") only in the general
Exception handler.
| except KeyboardInterrupt: | ||
| get_output_controller().print_info("Cancelled.") | ||
| raise typer.Exit(130) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/praisonai/praisonai/cli/commands/onboard.py | head -30Repository: MervinPraison/PraisonAI
Length of output: 827
🌐 Web query:
Ruff B904 rule exception handling
💡 Result:
The Ruff B904 rule (raise-without-from-inside-except) detects raise statements inside except clauses that lack a from clause. It recommends using raise ... from exc for chaining or raise ... from None to suppress chaining, to clarify whether the raised exception is intended or an error during handling. To handle exceptions to this rule (i.e., suppress or ignore B904 violations): 1. Globally ignore via configuration (pyproject.toml or ruff.toml): [tool.ruff.lint] ignore = ["B904"] 2. Per-file ignores: [tool.ruff.lint.per-file-ignores] "somefile.py" = ["B904"] "/*.py" = ["B904"] # all Python files 3. Inline suppression on the specific line: raise SomeError("msg") # noqa: B904 B904 is from flake8-bugbear (B rules), not enabled by default in Ruff (requires select = ["B"] or ["ALL"]). Some users ignore it due to debates on implicit vs explicit chaining (GitHub issues #22541, #8736). No special "rule exceptions" like allowlists exist; use standard Ruff ignore mechanisms. Examples from docs: Bad: try: ... except FileNotFoundError: raise RuntimeError("...") Good: try: ... except FileNotFoundError as exc: raise RuntimeError("...") from exc or from None if not chaining.
Citations:
- 1: https://docs.astral.sh/ruff/rules/raise-without-from-inside-except/
- 2: https://docs.astral.sh/ruff/rules/
- 3: https://beta.ruff.rs/docs/configuration/
- 4: https://docs.astral.sh/ruff/configuration/
- 5: https://stackoverflow.com/questions/76341244/ruff-ignore-inline-or-function-rule-check
Suppress the handled KeyboardInterrupt context.
Ruff B904 flags this because typer.Exit is raised from inside an except block without explicit intent. Use from None to suppress exception chaining, so the user sees only the intended cancellation message and exit code without exception-chaining noise.
Proposed fix
except KeyboardInterrupt:
get_output_controller().print_info("Cancelled.")
- raise typer.Exit(130)
+ raise typer.Exit(130) from None📝 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.
| except KeyboardInterrupt: | |
| get_output_controller().print_info("Cancelled.") | |
| raise typer.Exit(130) | |
| except KeyboardInterrupt: | |
| get_output_controller().print_info("Cancelled.") | |
| raise typer.Exit(130) from None |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 22-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/cli/commands/onboard.py` around lines 20 - 22, The
except KeyboardInterrupt block currently re-raises typer.Exit inside the handler
which causes exception chaining noise; change the raise to suppress chaining by
raising typer.Exit(130) from None (i.e., update the except KeyboardInterrupt
that calls get_output_controller().print_info("Cancelled.") so it does raise
typer.Exit(130) from None) to ensure only the intended cancellation message and
exit code are shown.
| def _python_executable() -> str: | ||
| """Get the Python executable path.""" | ||
| return shutil.which("python3") or shutil.which("python") or sys.executable |
There was a problem hiding this comment.
Prefer the current interpreter for the daemon command.
Using PATH python before sys.executable can install a task that runs a different interpreter than the one where praisonai is installed, especially under venv/pipx/conda.
🐛 Proposed fix
def _python_executable() -> str:
"""Get the Python executable path."""
- return shutil.which("python3") or shutil.which("python") or sys.executable
+ return sys.executable or shutil.which("python3") or shutil.which("python")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/daemon/windows.py` around lines 22 - 24, The
_python_executable function currently prefers PATH python before the running
interpreter which can install a daemon using a different Python than the one
running praisoni; change the selection to prefer sys.executable first, then
fallback to shutil.which("python3") and shutil.which("python"), and ensure a
non-empty string is returned (raise or default to "python" only if all lookups
fail). Update the return logic in _python_executable to use sys.executable as
the primary candidate and use the which() fallbacks to locate a system binary
only if sys.executable is not available.
| local env_file="$HOME/.praisonai/.env" | ||
| [[ -f "$env_file" ]] || return 0 | ||
|
|
||
| # Only if any messaging token already present (user set up bots elsewhere) | ||
| # OR if user opts in interactively | ||
| local has_token=0 | ||
| for tok in TELEGRAM_BOT_TOKEN DISCORD_BOT_TOKEN SLACK_BOT_TOKEN WHATSAPP_ACCESS_TOKEN; do | ||
| grep -qE "^${tok}=..+" "$env_file" 2>/dev/null && has_token=1 && break | ||
| done | ||
|
|
||
| # 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 | ||
|
|
||
| if [[ "$has_token" == "1" ]]; then | ||
| log_info "Detected a messaging token — launching bot onboarding..." | ||
| if "$py" -m praisonai onboard < /dev/tty > /dev/tty 2> /dev/tty; then | ||
| log_success "Bot onboarding completed!" | ||
| else | ||
| log_warn "Bot onboarding skipped or failed." | ||
| fi | ||
| else | ||
| # Offer it; default NO so quiet installs stay quiet | ||
| echo "" | ||
| echo -ne "${CYAN}Set up a messaging bot now? [y/N] ${NC}" | ||
| read -r yn < /dev/tty || yn="" |
There was a problem hiding this comment.
Don’t skip the bot prompt when .env is missing.
Fresh installs can have no ~/.praisonai/.env, but this returns before offering [y/N], so the new onboarding hook is silently skipped. Also write the prompt to /dev/tty since input is read there.
Proposed fix
local env_file="$HOME/.praisonai/.env"
- [[ -f "$env_file" ]] || return 0
# Only if any messaging token already present (user set up bots elsewhere)
# OR if user opts in interactively
local has_token=0
- for tok in TELEGRAM_BOT_TOKEN DISCORD_BOT_TOKEN SLACK_BOT_TOKEN WHATSAPP_ACCESS_TOKEN; do
- grep -qE "^${tok}=..+" "$env_file" 2>/dev/null && has_token=1 && break
- done
+ if [[ -f "$env_file" ]]; then
+ for tok in TELEGRAM_BOT_TOKEN DISCORD_BOT_TOKEN SLACK_BOT_TOKEN WHATSAPP_ACCESS_TOKEN; do
+ grep -qE "^${tok}=..+" "$env_file" 2>/dev/null && has_token=1 && break
+ done
+ fi
@@
# Offer it; default NO so quiet installs stay quiet
echo ""
- echo -ne "${CYAN}Set up a messaging bot now? [y/N] ${NC}"
+ printf "%b" "${CYAN}Set up a messaging bot now? [y/N] ${NC}" > /dev/tty
read -r yn < /dev/tty || yn=""🤖 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 667 - 698, The install script
early-returns when "$env_file" is missing which skips the bot onboarding prompt
on fresh installs; instead remove the "|| return 0" so the flow continues with
has_token left as 0, ensure the token-detection loop guards grep against a
missing file (e.g., only run grep if [[ -f "$env_file" ]]) and keep has_token
default 0, and change the interactive prompt/echo to write to /dev/tty (and
already read from /dev/tty) so the prompt is visible on interactive terminals;
locate symbols env_file, has_token, venv_dir, py, and the praisonai onboard
invocation to make the changes.
|
|
||
| def test_install_sh_mentions_bot_onboarding(): | ||
| """Test that install.sh mentions bot onboarding in dry-run mode.""" | ||
| script_path = "/home/runner/work/PraisonAI/PraisonAI/src/praisonai/scripts/install.sh" |
There was a problem hiding this comment.
Derive install.sh from the test file instead of hardcoding the CI workspace.
This path only works for one GitHub Actions checkout layout and will fail locally or in other CI runners.
🐛 Proposed fix
+from pathlib import Path
+
+
+INSTALL_SH = Path(__file__).resolve().parents[2] / "scripts" / "install.sh"
+
def test_install_sh_mentions_bot_onboarding():
"""Test that install.sh mentions bot onboarding in dry-run mode."""
- script_path = "/home/runner/work/PraisonAI/PraisonAI/src/praisonai/scripts/install.sh"
+ script_path = INSTALL_SH
@@
def test_bot_onboarding_function_logic():
"""Test the logic of the bot onboarding function."""
- script_path = "/home/runner/work/PraisonAI/PraisonAI/src/praisonai/scripts/install.sh"
+ script_path = INSTALL_SH
@@
def test_install_sh_has_no_syntax_errors():
"""Test that install.sh has no basic syntax errors."""
- script_path = "/home/runner/work/PraisonAI/PraisonAI/src/praisonai/scripts/install.sh"
+ script_path = INSTALL_SHAlso applies to: 30-30, 53-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/integration/test_install_sh_bot_onboarding.py` at line
16, The test currently hardcodes script_path = "/home/runner/work/..." which
breaks outside that CI layout; update the test to compute the install.sh path
relative to the test file using Python's pathlib (e.g.
Path(__file__).resolve().parent / ".." / "scripts" / "install.sh" or similar) so
script_path is derived dynamically; replace occurrences of the hardcoded
script_path variable in the test (and any duplicates) with this computed Path
(and call str(...) if a string is required).
| # Check the function is called from main | ||
| assert "maybe_offer_bot_onboarding" in content |
There was a problem hiding this comment.
Assert an actual onboarding function call, not just the definition.
The current substring check is satisfied by maybe_offer_bot_onboarding() in the function definition, so the test can pass even if the main flow never calls it.
🧪 Proposed fix
+ import re
+
# Check the function is called from main
- assert "maybe_offer_bot_onboarding" in content
+ assert re.search(r"^\s*maybe_offer_bot_onboarding\s*(?:#.*)?$", content, re.MULTILINE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/integration/test_install_sh_bot_onboarding.py` around
lines 47 - 48, The test currently checks for the symbol
maybe_offer_bot_onboarding in the file which matches the function definition;
update the assertion to ensure an actual invocation is present by asserting the
source contains a call pattern such as "maybe_offer_bot_onboarding()" or using a
regex like r"(?<!def\s)maybe_offer_bot_onboarding\s*\(" to exclude the
definition; locate the assertion in test_install_sh_bot_onboarding.py and
replace the loose substring check with a check for the invocation of
maybe_offer_bot_onboarding in main.
| def test_onboard_command_registered(): | ||
| """Test that onboard command is registered.""" | ||
| runner = CliRunner() | ||
| result = runner.invoke(app, ["--help"]) | ||
| assert result.exit_code == 0 | ||
| assert "onboard" in result.output or "wizard" in result.output |
There was a problem hiding this comment.
Assert registration through the main CLI app.
This invokes the onboard sub-app directly, so it would still pass if register_commands() stopped adding praisonai onboard. Use the main CLI app for this test to cover the PR objective.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/unit/cli/test_onboard_command.py` around lines 15 - 20,
The test test_onboard_command_registered currently invokes the onboard sub-app
directly, so update it to invoke the top-level CLI app to assert that
register_commands() actually registered the "onboard" command: use
CliRunner().invoke(...) on the main CLI application object (the same app that
calls register_commands(), referenced by register_commands() and the top-level
app variable used across the CLI) and then assert result.exit_code == 0 and that
"onboard" or "wizard" appears in result.output; this ensures the test exercises
the central registration path rather than the onboard sub-app directly.
| @patch("praisonai.cli.commands.onboard.run_onboard") | ||
| def test_onboard_calls_wizard(mock_run_onboard): | ||
| """Test that onboard command calls run_onboard.""" | ||
| runner = CliRunner() | ||
| result = runner.invoke(app, []) | ||
|
|
||
| # Should call the wizard | ||
| mock_run_onboard.assert_called_once() | ||
| assert result.exit_code == 0 | ||
|
|
||
|
|
||
| @patch("praisonai.cli.commands.onboard.run_onboard") | ||
| def test_onboard_handles_keyboard_interrupt(mock_run_onboard): | ||
| """Test that onboard handles KeyboardInterrupt gracefully.""" | ||
| mock_run_onboard.side_effect = KeyboardInterrupt() | ||
|
|
||
| runner = CliRunner() | ||
| result = runner.invoke(app, []) | ||
|
|
||
| # Should exit with code 130 (standard SIGINT exit code) | ||
| assert result.exit_code == 130 No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the relevant files
find . -type f -name "*.py" | grep -E "(onboard|test_onboard)" | head -20Repository: MervinPraison/PraisonAI
Length of output: 289
🏁 Script executed:
# Let's examine the test file to see the current state
cat -n src/praisonai/tests/unit/cli/test_onboard_command.py | head -60Repository: MervinPraison/PraisonAI
Length of output: 1637
🏁 Script executed:
# Now let's look at the onboard command module to see how run_onboard is imported
cat -n src/praisonai/cli/commands/onboard.pyRepository: MervinPraison/PraisonAI
Length of output: 136
🏁 Script executed:
# Let's also check the features.onboard module to verify run_onboard exists there
cat -n src/praisonai/cli/features/onboard.py | head -80Repository: MervinPraison/PraisonAI
Length of output: 136
🏁 Script executed:
# Using correct paths found in the repository
cat -n ./src/praisonai/praisonai/cli/commands/onboard.pyRepository: MervinPraison/PraisonAI
Length of output: 827
🏁 Script executed:
# Check the features/onboard module
cat -n ./src/praisonai/praisonai/cli/features/onboard.py | head -80Repository: MervinPraison/PraisonAI
Length of output: 3115
Patch the module that actually provides run_onboard.
run_onboard is imported inside onboard_callback from praisonai.cli.features.onboard (line 17), so praisonai.cli.commands.onboard.run_onboard does not exist at decoration time and these tests will fail with an AttributeError before invoking the CLI.
Proposed fix
-@patch("praisonai.cli.commands.onboard.run_onboard")
+@patch("praisonai.cli.features.onboard.run_onboard")
def test_onboard_calls_wizard(mock_run_onboard):
@@
-@patch("praisonai.cli.commands.onboard.run_onboard")
+@patch("praisonai.cli.features.onboard.run_onboard")
def test_onboard_handles_keyboard_interrupt(mock_run_onboard):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/unit/cli/test_onboard_command.py` around lines 23 - 43,
The tests patch the wrong target: replace the `@patch` target
"praisonai.cli.commands.onboard.run_onboard" with the actual provider
"praisonai.cli.features.onboard.run_onboard" (since run_onboard is imported
inside onboard_callback), so update both tests (test_onboard_calls_wizard and
test_onboard_handles_keyboard_interrupt) to patch
"praisonai.cli.features.onboard.run_onboard" to avoid AttributeError and
correctly simulate KeyboardInterrupt and normal call behavior.
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/56b7de0e-b6b5-4139-a3fd-d3a47e5d7ce7 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
… input Brings bot onboarding to parity with hermes/openclaw reference CLIs: - Persist tokens + allowlist + home channel atomically to ~/.praisonai/.env (chmod 600, tmp-then-rename). No more ephemeral os.environ-only state. - Password-hidden token input via Rich password prompt (rich path) and getpass (plain fallback). Tokens no longer echo to terminal. - Per-platform ALLOWED_USERS allowlist prompt with security warning when skipped (anyone who finds the bot can use it). - Per-platform HOME_CHANNEL prompt for proactive/cron delivery, defaulting to first allowed user ID. - Platform-specific user-ID discovery hints (@userinfobot for Telegram, Developer Mode for Discord, member ID for Slack, E.164 for WhatsApp). - bot.yaml now references ${..._ALLOWED_USERS} and ${..._HOME_CHANNEL} for every channel, keeping secrets out of the yaml file. Also fixes: - gateway.py: print_warn() -> print_warning() (correct method name on OutputController). Prevents AttributeError on daemon status/install/logs. - test_onboard_command.py: patch the actual source of run_onboard (features.onboard) since the commands module imports it lazily. Refs #1454 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.
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 pass and shipped focused fixes in Findings + actions:
If you want, I can also post this as a structured PR review comment with severity tags. |
|
@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 Complete ✅After thorough analysis of the merged PR #1455 and reviewer feedback, here are my findings: Phase 1: Architecture Review
Phase 2: Issue Analysis
🔍 Key Findings
|
Fixes #1454
Summary
This PR wires up the existing bot onboarding functionality and implements cross-platform daemon management as specified in issue #1454. The implementation follows AGENTS.md guidelines and delivers both Phase 1 (wiring existing code) and Phase 2 (Windows support).
Key Changes
New CLI Commands
Cross-Platform Daemon Support
install.sh Integration
User Journey
Install with bot setup:
curl -fsSL https://praison.ai/install.sh | bash
Manual bot setup anytime:
praisonai onboard
praisonai gateway install
praisonai gateway status
Implementation Notes
The implementation delivers the complete bot onboarding and daemon installation workflow, bridging the gap between LLM setup and running 24/7 messaging bots.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
onboardCLI command for interactive messaging bot setup wizardgateway: install, uninstall, and logsbotcommandTests