feature: Refactor setup-first installer flow - #933
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a new Changesmesh-llm setup and uninstall flow
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InstallScript as install.sh / install.ps1
participant CLI as mesh-llm setup
participant Runtime as Native runtime helpers
participant Service as Service installer
participant GitHub as gh CLI
User->>InstallScript: run install command
InstallScript->>InstallScript: download executable, update PATH
InstallScript->>CLI: run or print "mesh-llm setup"
CLI->>Runtime: install_and_prune_native_runtime_for_setup
Runtime-->>CLI: SetupNativeRuntimeOutcome
CLI->>Service: install_service (systemd/launchd)
Service-->>CLI: ServiceInstallReport
CLI->>GitHub: execute_github_star_plan
GitHub-->>CLI: SetupGitHubOutcome
CLI-->>User: print_setup_summary
sequenceDiagram
participant User
participant CLI as mesh-llm uninstall
participant Env as detect_uninstall_environment
participant Plan as plan_uninstall
participant Exec as execute_uninstall_plan
User->>CLI: mesh-llm uninstall [--dry-run|--yes]
CLI->>Env: detect platform, paths, binary
Env-->>CLI: UninstallEnvironment
CLI->>Plan: build steps from options
Plan-->>CLI: UninstallPlan
alt dry-run
CLI-->>User: render_plan
else confirmed
CLI->>Exec: run each UninstallStep
Exec-->>CLI: UninstallOutcome
CLI-->>User: render_outcome
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
crates/mesh-llm-cli/src/parser.rs (1)
1521-1567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the new setup parsing tests out of
parser.rs.
parser.rsis already past the 2,000-line limit, and adding more inline setup-specific tests keeps growing the largest CLI file instead of splitting by responsibility. Please peel these tests into a dedicated parser test module/file. As per coding guidelines,**/*.rs: "Do not add Rust source files over 2,000 lines. If a file is approaching that size, split it by responsibility into an owning module instead of adding more code to the oversized file."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-cli/src/parser.rs` around lines 1521 - 1567, The new setup parsing tests should be moved out of the oversized parser.rs test section into a dedicated parser test module/file. Extract the setup_command_parses_without_plugin_fallback, setup_command_rejects_conflicting_service_flags, and setup_command_rejects_skip_doctor_flag cases so parser.rs stops growing past the file-size limit, and keep the assertions and Cli::parse_from / Cli::try_parse_from coverage intact in the new test location.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-commands/src/setup/github.rs`:
- Around line 7-8: The GITHUB_STAR_PROMPT string in github.rs currently
hardcodes a yes/no suffix that duplicates what
dialoguer::Confirm::with_prompt(...).default(true) already adds. Update the
prompt constant used by the GitHub setup flow to remove the trailing [Y/n], or
change the Confirm configuration if custom suffix handling is required, so the
prompt does not render twice.
In `@crates/mesh-llm-commands/src/setup/prompt.rs`:
- Around line 35-46: The confirm flow in confirm_yes_no currently collapses both
Esc/cancel and dialoguer errors into None, which SetupPromptDefault::resolve
then treats as approval. Update confirm_yes_no so that an aborted or failed
Confirm::interact_opt() is preserved as a non-approval outcome (or explicitly
returned as cancel/skip), and adjust SetupPromptDefault::resolve to treat that
outcome as a отказ/skip instead of falling back to the default true behavior.
In `@crates/mesh-llm-commands/src/setup/service_templates.rs`:
- Around line 17-23: The Bash template in render_service_runner embeds
binary_path and env_file directly into quoted shell assignments, so paths
containing special characters can break the generated runner or alter runtime
values. Update the template generation to shell-escape both Path values before
inserting them into the BIN and ENV_FILE assignments, and keep the rest of the
script unchanged so launchd can safely start the service from any valid path.
- Around line 42-55: render_launchd_plist currently injects dynamic values
directly into the XML text nodes, which can break the plist when paths contain
XML-reserved characters. Update render_launchd_plist to XML-escape the values
for service_runner, home_dir, stdout_log, and stderr_log before formatting the
plist string, keeping SERVICE_LABEL unchanged if it is already safe. Use the
existing render_launchd_plist function and its local string interpolation points
to locate the fix.
In `@install.sh`:
- Around line 320-340: The libcudart version detection in detect_cuda_major() is
using an over-escaped regex that will miss real shared library names. Update
both grep patterns in the nvcc/libcudart fallback probes to match
libcudart.so.<version> correctly, so detect_cuda_major can extract the CUDA
major version from versioned libcudart libraries instead of returning empty and
falling back unnecessarily.
In `@website/src/docs/pages/CLI.md`:
- Around line 37-49: The CLI docs contain a duplicated “Finish a fresh install”
introduction in the section around the mesh-llm setup command. Update the
content in CLI.md by either removing the standalone introductory block before
the numbered list or changing the list structure so the first numbered item does
not repeat the same heading; use the duplicated “Finish a fresh install” text
and the mesh-llm setup example as the anchors when editing.
---
Nitpick comments:
In `@crates/mesh-llm-cli/src/parser.rs`:
- Around line 1521-1567: The new setup parsing tests should be moved out of the
oversized parser.rs test section into a dedicated parser test module/file.
Extract the setup_command_parses_without_plugin_fallback,
setup_command_rejects_conflicting_service_flags, and
setup_command_rejects_skip_doctor_flag cases so parser.rs stops growing past the
file-size limit, and keep the assertions and Cli::parse_from /
Cli::try_parse_from coverage intact in the new test location.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82660c3f-4d83-4194-8332-c6a2d874d9df
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
README.mdcrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-commands/Cargo.tomlcrates/mesh-llm-commands/src/lib.rscrates/mesh-llm-commands/src/runtime_native.rscrates/mesh-llm-commands/src/runtime_native/setup_helpers.rscrates/mesh-llm-commands/src/setup/actions.rscrates/mesh-llm-commands/src/setup/cli_actions_tests.rscrates/mesh-llm-commands/src/setup/command.rscrates/mesh-llm-commands/src/setup/command_tests.rscrates/mesh-llm-commands/src/setup/environment.rscrates/mesh-llm-commands/src/setup/github.rscrates/mesh-llm-commands/src/setup/github_runner.rscrates/mesh-llm-commands/src/setup/github_tests.rscrates/mesh-llm-commands/src/setup/mod.rscrates/mesh-llm-commands/src/setup/orchestration_tests.rscrates/mesh-llm-commands/src/setup/plan.rscrates/mesh-llm-commands/src/setup/planner.rscrates/mesh-llm-commands/src/setup/prompt.rscrates/mesh-llm-commands/src/setup/service.rscrates/mesh-llm-commands/src/setup/service_files.rscrates/mesh-llm-commands/src/setup/service_paths.rscrates/mesh-llm-commands/src/setup/service_runner.rscrates/mesh-llm-commands/src/setup/service_templates.rscrates/mesh-llm-commands/src/setup/service_tests.rscrates/mesh-llm-commands/src/setup/summary.rscrates/mesh-llm-commands/src/setup/test_support.rscrates/mesh-llm-commands/src/setup/tests.rscrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/src/commands/runtime.rscrates/mesh-llm/src/commands/setup.rsdocs/CLI.mddocs/README.mddocs/USAGE.mddocs/specs/mesh-setup-installer.mdinstall.ps1install.shscripts/tests/test_generate_native_runtime_release_manifest.pyscripts/tests/test_install_ps1.pyscripts/tests/test_install_sh.pywebsite/src/_includes/components/cta-terminal.njkwebsite/src/_includes/sections/hero.njkwebsite/src/docs/pages/CLI.mdwebsite/src/docs/pages/installing-linux.mdwebsite/src/docs/pages/installing-macos.mdwebsite/src/docs/pages/installing-mesh.mdwebsite/src/docs/pages/installing-windows.mdwebsite/src/docs/pages/quickstart.mdwebsite/src/docs/pages/troubleshooting.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-commands/src/uninstall.rs`:
- Around line 293-295: The remove_binary helper is routing executable deletion
through remove_recursively, which can delete directories and still fails for
self-uninstall on Windows because the running exe is locked. Update
remove_binary to validate that the target is a file or symlink rather than a
directory, and use a Windows-specific deferred-delete path for
current_exe/self-removal instead of deleting in-process. Keep the fix centered
on remove_binary and the uninstall flow that determines the binary path.
In `@docs/specs/mesh-setup-installer.md`:
- Around line 148-149: Clarify the `--purge-config` and `--keep-config` behavior
in the installer spec so the conflict is explicit: state that the flags are
mutually exclusive or, if both are provided, that `--keep-config` takes
precedence as in the current `mesh-setup-installer` behavior. Update the option
descriptions in the spec around the supported flags to match this rule and
ensure the wording is unambiguous for future CLI parsing and implementations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07e59e93-37e2-40b5-aae4-3f93bb7a12e1
📒 Files selected for processing (14)
README.mdcrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-cli/src/parser/uninstall_tests.rscrates/mesh-llm-commands/src/lib.rscrates/mesh-llm-commands/src/uninstall.rscrates/mesh-llm/src/commands/mod.rsdocs/CLI.mddocs/specs/mesh-setup-installer.mdwebsite/src/docs/pages/CLI.mdwebsite/src/docs/pages/hardware-support.mdwebsite/src/docs/pages/installing-linux.mdwebsite/src/docs/pages/installing-macos.mdwebsite/src/docs/pages/installing-mesh.mdwebsite/src/docs/pages/installing-windows.md
✅ Files skipped from review due to trivial changes (9)
- website/src/docs/pages/hardware-support.md
- README.md
- crates/mesh-llm-cli/src/parser/uninstall_tests.rs
- website/src/docs/pages/installing-mesh.md
- website/src/docs/pages/installing-windows.md
- website/src/docs/pages/CLI.md
- website/src/docs/pages/installing-linux.md
- docs/CLI.md
- website/src/docs/pages/installing-macos.md
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/mesh-llm-commands/src/lib.rs
- crates/mesh-llm/src/commands/mod.rs
- crates/mesh-llm-cli/src/parser.rs
ndizazzo
changed the title
ndizazzo
force-pushed
the
feature/revise-install-paths
branch
3 times, most recently
from
June 30, 2026 18:38
48fb272 to
96d43cb
Compare
Contributor
|
This pull request has not been updated in at least 5 days. It will be closed after 7 days of inactivity to keep the active review queue current. Please update it within 2 days if the changes are still moving forward. |
Collaborator
Author
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
crates/mesh-llm-commands/src/setup/service_templates.rs (1)
25-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
EnvironmentFile=path is not escaped for systemd specifiers.
render_systemd_unitescapesbinary_pathin theExecStart=line viasystemd_quote_token, butservice_env_fileis written raw via.display()intoEnvironmentFile=-{service_env_file}(Line 37). Systemd specifier expansion (%h,%%, etc.) applies toEnvironmentFile=just as it does toExecStart=; ifservice_env_file(derived fromdirs::config_dir()/home dir) ever contains a literal%, the generated unit will misinterpret it. This is the same escaping-gap class already fixed forrender_service_runnerandrender_launchd_plistin this file per prior review comments.Proposed fix
format!( "# mesh-llm serve (startup models come from {mesh_config_file})\n[Unit]\nDescription=Mesh LLM user service\nAfter=network-online.target\nWants=network-online.target\n\n[Service]\nType=simple\nEnvironmentFile=-{service_env_file}\n\n{exec_line}\nWorkingDirectory=%h\nRestart=on-failure\nRestartSec=5\n\n[Install]\nWantedBy=default.target\n", mesh_config_file = mesh_config_file.display(), - service_env_file = service_env_file.display(), + service_env_file = systemd_quote_token(&service_env_file.to_string_lossy()).trim_matches('"'), exec_line = exec_line, )Alternatively, escape only the
%character (systemd's specifier escape) sinceEnvironmentFile=doesn't require full shell quoting, just specifier-safety.Please confirm systemd specifier expansion applies to
EnvironmentFile=values (not justExecStart=):Does systemd expand % specifiers in EnvironmentFile= directive🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-commands/src/setup/service_templates.rs` around lines 25 - 40, The generated systemd unit in render_systemd_unit leaves service_env_file unescaped in EnvironmentFile=, so systemd specifier expansion can misread literals like %. Update render_systemd_unit to apply the same systemd-safe escaping used elsewhere in this file (for example the escaping pattern already used by render_service_runner/render_launchd_plist) before writing EnvironmentFile=-{service_env_file}, while keeping ExecStart handling unchanged.
🧹 Nitpick comments (9)
crates/mesh-llm-commands/src/setup/service_files.rs (1)
8-22: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTOCTOU on env file existence check.
ensure_service_env_filechecksexists()then writes; a concurrent setup invocation could race between the check and the write. Low practical risk given setup is a manual interactive/one-shot command, but worth noting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-commands/src/setup/service_files.rs` around lines 8 - 22, ensure_service_env_file has a TOCTOU race because it checks Path::exists() before writing, so concurrent invocations can both pass the check and then overwrite or race on the same env file. Update ensure_service_env_file to make file creation atomic by using a create-if-missing approach or handling the already-exists case at write time, while keeping the parent-directory setup and render_service_env_file flow intact.crates/mesh-llm-commands/src/setup/github_runner.rs (1)
113-138: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMinor race between timeout kill and process exit.
If
childexits right as the timeout threshold is reached,child.kill()can fail (process already reaped), yielding aKillFailederror even though the command actually completed. Not critical, but consider a finaltry_wait()check inside the timeout branch before callingkill().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-commands/src/setup/github_runner.rs` around lines 113 - 138, The timeout handling in the GitHub runner loop can misclassify a process that exits right as the deadline is hit. In the `run`/polling loop around `child.try_wait()`, add a final `try_wait()` check inside the `started_at.elapsed() >= self.timeout` branch before calling `child.kill()`, and only kill if the child is still running; this should keep `GhCommandError::TimedOut`, `KillFailed`, and the `GhCommandOutput` flow aligned with the actual process state.crates/mesh-llm-commands/src/setup/service_tests.rs (1)
66-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test coverage for
EnvironmentFile=escaping.Given the escaping gap flagged in
service_templates.rs(unescapedservice_env_fileinEnvironmentFile=), consider adding a test analogous tolaunchd_plist_escapes_xml_specials_in_paths/launchd_runner_escapes_shell_specials_in_pathsthat exercises aservice_env_filepath containing%to lock in correct systemd-specifier-safe behavior once fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-commands/src/setup/service_tests.rs` around lines 66 - 73, Add coverage in service_tests around render_systemd_unit to exercise a service_env_file path containing % so we lock in systemd-specifier-safe escaping for EnvironmentFile=. Mirror the style of the existing launchd escape tests, but target render_systemd_unit and verify the generated unit contains the properly escaped EnvironmentFile value rather than the raw path.crates/mesh-llm-commands/src/setup/command.rs (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify line 54 is within rustfmt's width limit.
This line (with 4-space indent) appears to run past the typical 100-column rustfmt default. As per coding guidelines, Rust files touched by this change must be formatted before committing.
♻️ Suggested formatting
- let mut actions = CliSetupActions::new(args.environment, args.configured, args.options.verbose); + let mut actions = + CliSetupActions::new(args.environment, args.configured, args.options.verbose);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-commands/src/setup/command.rs` at line 54, The initialization of CliSetupActions in command.rs appears to exceed rustfmt’s preferred width. Reformat the statement in the setup command so it stays within the standard Rust formatting limits, and then run rustfmt on the touched Rust code to ensure the command module remains consistently formatted.Source: Coding guidelines
crates/mesh-llm-commands/src/setup/summary.rs (2)
166-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompact GitHub summary silently omits actionable states.
CliUnavailableandNotAuthenticatedare dropped from the compact "GitHub" line (fall to_ => None), unlikeservice_brief, which always surfaces a status (even "not recorded"). Users who hit these states get no compact-mode signal that GitHub star setup didn't run, only visible with--verbose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-commands/src/setup/summary.rs` around lines 166 - 176, The compact GitHub summary in github_brief is dropping CliUnavailable and NotAuthenticated into the default None case, so those actionable states never appear in the non-verbose output. Update github_brief to handle these SetupGitHubOutcome variants explicitly and return a visible status string, matching the pattern used by service_brief so the compact "GitHub" line always reports that setup was not performed or not recorded.
143-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile string-literal comparison for service status.
report.summary == "installed and started"couples UI status coloring to a display string owned byservice.rs. A future wording change there would silently downgrade a successfully running service to a "start manually" warning with no compiler signal.Consider adding an explicit status field/variant on
ServiceInstallReport(e.g.,started: boolor a small enum) and matching on that instead of the rendered summary text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-commands/src/setup/summary.rs` around lines 143 - 164, The service status logic in service_brief is relying on the display string report.summary == "installed and started", which is brittle and can silently misclassify a running service if the wording changes. Update ServiceInstallReport to carry an explicit status field or enum (for example a started flag), set it where the report is produced, and change service_brief to match on that structured value instead of comparing the summary text.crates/mesh-llm-cli/src/parser/uninstall_tests.rs (1)
34-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing coverage for
--purge-config.Neither test exercises
--purge-config; consider adding an assertion for it alongside the other automation flags inuninstall_accepts_automation_flags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-cli/src/parser/uninstall_tests.rs` around lines 34 - 77, The uninstall parser test is missing coverage for the `--purge-config` flag, so update `uninstall_accepts_automation_flags` to include that option in the `Cli::parse_from` inputs and assert the `purge_config` field from `Command::Uninstall` is set correctly alongside the existing flags. Use the `Cli::parse_from` and `Command::Uninstall` destructuring in this test to locate the change.crates/mesh-llm-commands/src/uninstall.rs (1)
176-181: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
--purge-configcombined with--keep-configsilently no-ops.When both flags are set, the config/identity removal step is simply omitted with no warning, which may surprise a user who explicitly asked to purge. A safe default is fine, but consider warning when the flags conflict.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-commands/src/uninstall.rs` around lines 176 - 181, In uninstall.rs, the UninstallStep::RemovePath branch inside the purge-config handling silently skips removal when options.purge_config and options.keep_config are both set. Update the uninstall flow to detect this flag conflict in the main option-processing logic and emit a warning using the existing logging/reporting path before continuing with the safe default, so users of --purge-config and --keep-config get explicit feedback instead of a silent no-op.install.ps1 (1)
196-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePowerShell will silently drop the backtick-quoting around
mesh-llm.exe setup.In
Write-FlavorCompatibilityWarning(and the matching text inShow-Usage), the message uses`mesh-llm.exe setup`inside a double-quoted string / here-string. A backtick followed by an unrecognized escape character (heremand a space) is not a documented PowerShell escape sequence, so the backtick is dropped rather than rendering literally — the intended "code-style" quoting around the command will not appear in the actual warning/help text.✏️ Proposed fix
- Write-Warning "Ignoring legacy -Flavor '$legacyFlavor'. The Windows installer now always installs the x64 host binary; run `mesh-llm.exe setup` to choose the recommended runtime." + Write-Warning "Ignoring legacy -Flavor '$legacyFlavor'. The Windows installer now always installs the x64 host binary; run ``mesh-llm.exe setup`` to choose the recommended runtime."Use doubled backticks (
``) to emit a literal backtick, or switch to single quotes around the phrase.Also applies to: 44-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install.ps1` around lines 196 - 207, The warning/help text in Write-FlavorCompatibilityWarning and Show-Usage is losing the intended literal backticks around mesh-llm.exe setup because PowerShell treats the single backtick as an escape. Update those message strings to use literal backticks correctly, either by doubling the backticks or by changing the surrounding quoting so the command renders exactly as code-style text in the final output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-commands/src/terminal.rs`:
- Around line 31-54: The confirmation prompt in the terminal input loop
currently treats EOF as an empty reply, which can incorrectly accept the default
choice. Update the reply handling in the prompt function that reads from stdin
so EOF from read_line is detected before matching the trimmed input, and return
a decline/cancel result instead of calling default.empty_reply(). Keep the
existing y/yes and n/no behavior unchanged.
In `@crates/mesh-llm-commands/src/uninstall.rs`:
- Around line 359-377: In schedule_windows_deferred_binary_delete, the
PowerShell invocation is not actually passing the target path through $args
because the trailing -- and path are being folded into the command string. Fix
the Command::new("powershell.exe") call so the binary path is embedded safely
into the PowerShell script itself, or switch to a script file approach, and
ensure Remove-Item -LiteralPath targets the actual exe rather than $args[0].
In `@crates/mesh-llm/src/commands/setup.rs`:
- Around line 50-57: The current_setup_platform function silently maps every
unknown OS to SetupPlatform::Linux, which can send unsupported hosts down the
wrong setup path. Update current_setup_platform to fail fast for
non-linux/macos/windows targets by returning an explicit unsupported-OS error
instead of defaulting to Linux, and adjust the callers in setup.rs to handle
that error path cleanly.
In `@docs/specs/mesh-setup-installer.md`:
- Around line 27-35: Update the installer spec in the section describing
`install.sh` and `install.ps1` to explicitly document checksum fallback
behavior: when a `.sha256` sidecar is missing, the default behavior should be to
warn and continue, and the install should fail closed only if
`MESH_LLM_REQUIRE_CHECKSUM=1` is set. Keep the wording alongside the existing
checksum verification and archive download bullets so the behavior is clear in
the installer flow.
In `@install.sh`:
- Around line 221-231: The platform_asset_name() helper is currently unused, so
either remove it if it is dead code or connect it to the install flow where the
asset name is needed. Check the install.sh logic around platform_id and any
download/extraction path to see whether platform_asset_name() should be invoked
there. If it is intended for a separate path, add a brief comment and the call
site; otherwise delete the function to avoid unused code.
In `@website/src/_includes/components/cta-terminal.njk`:
- Around line 19-29: The CTA copy in cta-terminal.njk overstates Windows support
because the visible install/setup flow only shows shell commands. Update the
platform wording in the terminal card to scope it to macOS/Linux, or add a
Windows-specific PowerShell option in the existing install templates and display
logic so the claim matches the actual commands shown.
---
Duplicate comments:
In `@crates/mesh-llm-commands/src/setup/service_templates.rs`:
- Around line 25-40: The generated systemd unit in render_systemd_unit leaves
service_env_file unescaped in EnvironmentFile=, so systemd specifier expansion
can misread literals like %. Update render_systemd_unit to apply the same
systemd-safe escaping used elsewhere in this file (for example the escaping
pattern already used by render_service_runner/render_launchd_plist) before
writing EnvironmentFile=-{service_env_file}, while keeping ExecStart handling
unchanged.
---
Nitpick comments:
In `@crates/mesh-llm-cli/src/parser/uninstall_tests.rs`:
- Around line 34-77: The uninstall parser test is missing coverage for the
`--purge-config` flag, so update `uninstall_accepts_automation_flags` to include
that option in the `Cli::parse_from` inputs and assert the `purge_config` field
from `Command::Uninstall` is set correctly alongside the existing flags. Use the
`Cli::parse_from` and `Command::Uninstall` destructuring in this test to locate
the change.
In `@crates/mesh-llm-commands/src/setup/command.rs`:
- Line 54: The initialization of CliSetupActions in command.rs appears to exceed
rustfmt’s preferred width. Reformat the statement in the setup command so it
stays within the standard Rust formatting limits, and then run rustfmt on the
touched Rust code to ensure the command module remains consistently formatted.
In `@crates/mesh-llm-commands/src/setup/github_runner.rs`:
- Around line 113-138: The timeout handling in the GitHub runner loop can
misclassify a process that exits right as the deadline is hit. In the
`run`/polling loop around `child.try_wait()`, add a final `try_wait()` check
inside the `started_at.elapsed() >= self.timeout` branch before calling
`child.kill()`, and only kill if the child is still running; this should keep
`GhCommandError::TimedOut`, `KillFailed`, and the `GhCommandOutput` flow aligned
with the actual process state.
In `@crates/mesh-llm-commands/src/setup/service_files.rs`:
- Around line 8-22: ensure_service_env_file has a TOCTOU race because it checks
Path::exists() before writing, so concurrent invocations can both pass the check
and then overwrite or race on the same env file. Update ensure_service_env_file
to make file creation atomic by using a create-if-missing approach or handling
the already-exists case at write time, while keeping the parent-directory setup
and render_service_env_file flow intact.
In `@crates/mesh-llm-commands/src/setup/service_tests.rs`:
- Around line 66-73: Add coverage in service_tests around render_systemd_unit to
exercise a service_env_file path containing % so we lock in
systemd-specifier-safe escaping for EnvironmentFile=. Mirror the style of the
existing launchd escape tests, but target render_systemd_unit and verify the
generated unit contains the properly escaped EnvironmentFile value rather than
the raw path.
In `@crates/mesh-llm-commands/src/setup/summary.rs`:
- Around line 166-176: The compact GitHub summary in github_brief is dropping
CliUnavailable and NotAuthenticated into the default None case, so those
actionable states never appear in the non-verbose output. Update github_brief to
handle these SetupGitHubOutcome variants explicitly and return a visible status
string, matching the pattern used by service_brief so the compact "GitHub" line
always reports that setup was not performed or not recorded.
- Around line 143-164: The service status logic in service_brief is relying on
the display string report.summary == "installed and started", which is brittle
and can silently misclassify a running service if the wording changes. Update
ServiceInstallReport to carry an explicit status field or enum (for example a
started flag), set it where the report is produced, and change service_brief to
match on that structured value instead of comparing the summary text.
In `@crates/mesh-llm-commands/src/uninstall.rs`:
- Around line 176-181: In uninstall.rs, the UninstallStep::RemovePath branch
inside the purge-config handling silently skips removal when
options.purge_config and options.keep_config are both set. Update the uninstall
flow to detect this flag conflict in the main option-processing logic and emit a
warning using the existing logging/reporting path before continuing with the
safe default, so users of --purge-config and --keep-config get explicit feedback
instead of a silent no-op.
In `@install.ps1`:
- Around line 196-207: The warning/help text in Write-FlavorCompatibilityWarning
and Show-Usage is losing the intended literal backticks around mesh-llm.exe
setup because PowerShell treats the single backtick as an escape. Update those
message strings to use literal backticks correctly, either by doubling the
backticks or by changing the surrounding quoting so the command renders exactly
as code-style text in the final output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07546f7b-7cff-43a1-825d-71847bcc839e
📒 Files selected for processing (54)
README.mdcrates/mesh-llm-cli/src/parser.rscrates/mesh-llm-cli/src/parser/setup_tests.rscrates/mesh-llm-cli/src/parser/uninstall_tests.rscrates/mesh-llm-commands/src/lib.rscrates/mesh-llm-commands/src/runtime_native.rscrates/mesh-llm-commands/src/runtime_native/setup_helpers.rscrates/mesh-llm-commands/src/setup/actions.rscrates/mesh-llm-commands/src/setup/cli_actions_tests.rscrates/mesh-llm-commands/src/setup/command.rscrates/mesh-llm-commands/src/setup/command_tests.rscrates/mesh-llm-commands/src/setup/environment.rscrates/mesh-llm-commands/src/setup/github.rscrates/mesh-llm-commands/src/setup/github_runner.rscrates/mesh-llm-commands/src/setup/github_tests.rscrates/mesh-llm-commands/src/setup/mod.rscrates/mesh-llm-commands/src/setup/orchestration_tests.rscrates/mesh-llm-commands/src/setup/plan.rscrates/mesh-llm-commands/src/setup/planner.rscrates/mesh-llm-commands/src/setup/prompt.rscrates/mesh-llm-commands/src/setup/service.rscrates/mesh-llm-commands/src/setup/service_files.rscrates/mesh-llm-commands/src/setup/service_paths.rscrates/mesh-llm-commands/src/setup/service_runner.rscrates/mesh-llm-commands/src/setup/service_templates.rscrates/mesh-llm-commands/src/setup/service_tests.rscrates/mesh-llm-commands/src/setup/summary.rscrates/mesh-llm-commands/src/setup/test_support.rscrates/mesh-llm-commands/src/setup/tests.rscrates/mesh-llm-commands/src/terminal.rscrates/mesh-llm-commands/src/uninstall.rscrates/mesh-llm/src/commands/discover.rscrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/src/commands/runtime.rscrates/mesh-llm/src/commands/setup.rsdocs/CLI.mddocs/README.mddocs/USAGE.mddocs/specs/mesh-setup-installer.mdinstall.ps1install.shscripts/tests/test_generate_native_runtime_release_manifest.pyscripts/tests/test_install_ps1.pyscripts/tests/test_install_sh.pywebsite/src/_includes/components/cta-terminal.njkwebsite/src/_includes/sections/hero.njkwebsite/src/docs/pages/CLI.mdwebsite/src/docs/pages/hardware-support.mdwebsite/src/docs/pages/installing-linux.mdwebsite/src/docs/pages/installing-macos.mdwebsite/src/docs/pages/installing-mesh.mdwebsite/src/docs/pages/installing-windows.mdwebsite/src/docs/pages/quickstart.mdwebsite/src/docs/pages/troubleshooting.md
ndizazzo
force-pushed
the
feature/revise-install-paths
branch
from
July 10, 2026 10:50
9f0435b to
61441b7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements the setup-first installer flow from
.omo/plans/mesh-setup-installer.mdand promotes the behavior spec todocs/specs/mesh-setup-installer.md.Key changes:
mesh-llm setupas the owner of native-runtime install/prune, optional service setup, and optional GitHub starring.mesh-llm setup.MESH_LLM_INSTALL_URL_BASEfor Unix installer branch/deployment tests, matching the existing PowerShell test override.Example
Screenshot
Review Findings Addressed
During review against the plan/spec I found and fixed:
mesh_setup_spec.mdwas stale and untracked. Replaced it with canonicaldocs/specs/mesh-setup-installer.mdand linked it from the docs indexes.MESH_LLM_INSTALL_URL_BASEand Bash coverage so branch release assets can be tested without unpublished GitHub release artifacts.Local Validation
cargo fmt --allcargo fmt --all --checkcargo test -p mesh-llm-cli --libcargo test -p mesh-llm-commands --libcargo test -p mesh-llm --libpython3 -m unittest scripts.tests.test_install_shpython3 -m unittest scripts.tests.test_install_ps1(PowerShell tests skipped wherepwshis unavailable locally)python3 -m unittest scripts.tests.test_generate_native_runtime_release_manifestcargo check -p mesh-llmcargo clippy -p mesh-llm --all-targets -- -D warningsjust website-buildjust build./target/debug/mesh-llm setup --skip-runtime --no-service --no-interactiveRemote Deployment Validation
Validated on both
mesh1.patio51.comandmesh2.patio51.com:feature/revise-install-paths.~/.local/bin/mesh-llminstall.just release-buildsuccessfully on each ARM host.mesh-llm-aarch64-unknown-linux-gnu.tar.gztest asset with SHA256 sidecar.MESH_LLM_INSTALL_URL_BASE=file:///tmp/mesh-setup-install-test/assets.~/.local/bin.mesh-llm 0.68.0.scripts/package-native-runtime.sh --build --backend cpu --target aarch64-unknown-linux-gnu.native-runtimes.json, served it over localhost HTTP, and ran:MESH_LLM_NATIVE_RUNTIME_MANIFEST_URL=http://127.0.0.1:8123/native-runtimes.json ~/.local/bin/mesh-llm setup --no-service --no-interactivemeshllm-native-runtime-linux-aarch64-cpu, pruned/checked cache, and summarized cleanly.already installed; cache already cleanbehavior.Note: the default setup manifest URL for this unreleased branch points at GitHub release
v0.68.0, which does not exist yet, so remote validation used the supported native-runtime manifest override with locally packaged branch runtimes.Summary by CodeRabbit
New Features
mesh-llm setupflow after installation, including optional background service setup and an optional GitHub star prompt.mesh-llm uninstallwith dry-run, confirmation, and retention options.Documentation
Bug Fixes