feat: add qlty and ast-grep installation to setup wizard#144
Open
udhaya10 wants to merge 2 commits intoparcadei:mainfrom
Open
feat: add qlty and ast-grep installation to setup wizard#144udhaya10 wants to merge 2 commits intoparcadei:mainfrom
udhaya10 wants to merge 2 commits intoparcadei:mainfrom
Conversation
- Add Step 11/15: qlty CLI installation (code quality, 70+ linters) - Add Step 12/15: ast-grep installation (AST-based code search) - Both follow existing TLDR install pattern (explain → confirm → install → verify) - Check if already installed before offering - Try brew first, then cargo for ast-grep - Update all step numbers from /13 to /15 - Update README wizard steps table Closes parcadei#140
|
PR author is not in the allowed authors list. |
📝 WalkthroughWalkthroughThe changes expand the setup wizard to include two additional optional CLI tools (qlty and ast-grep) with interactive installation flows, documentation updates, and renumbered wizard steps. The wizard now totals 15 steps instead of 12. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+1099
to
+1104
| result = subprocess.run( | ||
| ["sh", "-c", "curl -fsSL https://qlty.sh | sh"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| ) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@opc/scripts/setup/wizard.py`:
- Line 631: The print call uses an unnecessary f-string and likely meant to
include the runtime variable; update the call to include runtime (e.g.,
interpolate runtime into the message) or remove the leading f to make it a
normal string. Locate the console.print invocation in wizard.py (the call to
console.print(... "Step 6/15: Container Stack (Sandbox Infrastructure)")) and
either add {runtime} in the formatted string where appropriate or change the
string to a plain literal to eliminate the unused f-string.
- Around line 1133-1164: The ast-grep install block (in wizard.py around the
ast-grep install logic) must verify the installation and avoid misleading
fallback text: after a successful subprocess.run that returns 0, run a
verification check (e.g., subprocess.run(["sg","--version"],
capture_output=True, text=True) or check shutil.which("sg")) and only print
"[green]OK[/green] ast-grep installed" if the verification succeeds; if
verification fails, treat it as an installation failure and print the
appropriate failure message. Also replace hardcoded "Install manually: brew
install ast-grep" in the failure and timeout branches with a generic instruction
or a context-aware suggestion (if shutil.which("brew") is truthy suggest brew,
elif shutil.which("cargo") suggest cargo, else print the upstream install URL
https://github.com/ast-grep/ast-grep#installation) so users without brew aren't
directed to use it.
- Around line 1098-1117: After a successful curl install (the branch where
subprocess.run returns 0), invoke the qlty initialization and a verification
command: call "qlty init" (run via subprocess.run similar to the existing call)
and then run a verification command such as "qlty --version" or another
appropriate qlty health/status check; handle non-zero return codes and
exceptions for both calls and log results via console.print (reuse the same
timeout/ capture_output/text pattern), printing an OK on success or WARN/ERROR
with actionable manual instructions on failure. Ensure you update the success
branch that currently prints "OK qlty installed" to perform these two
subprocess.run calls (referencing subprocess.run and result.returncode) and to
surface errors/exceptions just like the existing install block.
🧹 Nitpick comments (2)
opc/scripts/setup/wizard.py (2)
1084-1084: Internal comments use stale sub-step numbering (10b/10c/10d) that doesn't match displayed step headers (11/12/13).These inline comments appear to be leftover from an earlier numbering scheme and will cause confusion during future maintenance.
Proposed fix
- # Step 10b: Code Quality CLI - qlty (Optional) + # Step 11: Code Quality CLI - qlty (Optional)- # Step 10c: AST-Grep (Optional) + # Step 12: AST-Grep (Optional)- # Step 10d: Diagnostics Tools (Shift-Left Feedback) + # Step 13: Diagnostics Tools (Shift-Left Feedback)Also applies to: 1119-1119, 1169-1169
1099-1104: Piping curl tosh— consider adding--proto =httpsfor defense-in-depth.While
curl | shmatches the official qlty install instructions, adding--proto =https --tlsv1.2hardens against protocol downgrade. This is a common hardening pattern used by rustup, Homebrew, etc.Proposed fix
- ["sh", "-c", "curl -fsSL https://qlty.sh | sh"], + ["sh", "-c", "curl --proto =https --tlsv1.2 -fsSL https://qlty.sh | sh"],
opc/scripts/setup/wizard.py
Outdated
| # Step 5: Container stack (Sandbox Infrastructure) | ||
| runtime = prereqs.get("container_runtime", "docker") | ||
| console.print(f"\n[bold]Step 6/13: Container Stack (Sandbox Infrastructure)[/bold]") | ||
| console.print(f"\n[bold]Step 6/15: Container Stack (Sandbox Infrastructure)[/bold]") |
There was a problem hiding this comment.
f-string without placeholders (static analysis).
Line 631 uses an f-string but contains no interpolated variables. The runtime variable on line 630 is likely intended to be used here.
Proposed fix
- console.print(f"\n[bold]Step 6/15: Container Stack (Sandbox Infrastructure)[/bold]")
+ console.print("\n[bold]Step 6/15: Container Stack (Sandbox Infrastructure)[/bold]")📝 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.
Suggested change
| console.print(f"\n[bold]Step 6/15: Container Stack (Sandbox Infrastructure)[/bold]") | |
| console.print("\n[bold]Step 6/15: Container Stack (Sandbox Infrastructure)[/bold]") |
🧰 Tools
🪛 Ruff (0.15.0)
[error] 631-631: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In `@opc/scripts/setup/wizard.py` at line 631, The print call uses an unnecessary
f-string and likely meant to include the runtime variable; update the call to
include runtime (e.g., interpolate runtime into the message) or remove the
leading f to make it a normal string. Locate the console.print invocation in
wizard.py (the call to console.print(... "Step 6/15: Container Stack (Sandbox
Infrastructure)")) and either add {runtime} in the formatted string where
appropriate or change the string to a plain literal to eliminate the unused
f-string.
Comment on lines
+1098
to
+1117
| try: | ||
| result = subprocess.run( | ||
| ["sh", "-c", "curl -fsSL https://qlty.sh | sh"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| ) | ||
| if result.returncode == 0: | ||
| console.print(" [green]OK[/green] qlty installed") | ||
| else: | ||
| console.print(" [yellow]WARN[/yellow] Installation failed") | ||
| console.print(" Install manually: curl -fsSL https://qlty.sh | sh") | ||
| except subprocess.TimeoutExpired: | ||
| console.print(" [yellow]WARN[/yellow] Installation timed out") | ||
| console.print(" Install manually: curl -fsSL https://qlty.sh | sh") | ||
| except Exception as e: | ||
| console.print(f" [red]ERROR[/red] {e}") | ||
| else: | ||
| console.print(" Skipped qlty installation") | ||
| console.print(" [dim]Install later: curl -fsSL https://qlty.sh | sh[/dim]") |
There was a problem hiding this comment.
Missing qlty init and post-install verification per PR objectives.
The PR description states that qlty init should be called on successful install, and the issue requires following the "explain → confirm → install → verify" pattern. Currently, neither happens after qlty installation succeeds.
Proposed fix
if result.returncode == 0:
console.print(" [green]OK[/green] qlty installed")
+
+ # Verify installation
+ verify = subprocess.run(
+ ["qlty", "--version"],
+ capture_output=True,
+ text=True,
+ timeout=10,
+ )
+ if verify.returncode == 0:
+ console.print(f" [green]OK[/green] Verified: {verify.stdout.strip()}")
+ else:
+ console.print(" [yellow]WARN[/yellow] qlty installed but not on PATH")
+
+ # Initialize qlty in the project
+ console.print(" Running qlty init...")
+ init_result = subprocess.run(
+ ["qlty", "init"],
+ capture_output=True,
+ text=True,
+ timeout=60,
+ )
+ if init_result.returncode == 0:
+ console.print(" [green]OK[/green] qlty initialized")
+ else:
+ console.print(" [yellow]WARN[/yellow] qlty init failed (run manually: qlty init)")
else:🤖 Prompt for AI Agents
In `@opc/scripts/setup/wizard.py` around lines 1098 - 1117, After a successful
curl install (the branch where subprocess.run returns 0), invoke the qlty
initialization and a verification command: call "qlty init" (run via
subprocess.run similar to the existing call) and then run a verification command
such as "qlty --version" or another appropriate qlty health/status check; handle
non-zero return codes and exceptions for both calls and log results via
console.print (reuse the same timeout/ capture_output/text pattern), printing an
OK on success or WARN/ERROR with actionable manual instructions on failure.
Ensure you update the success branch that currently prints "OK qlty installed"
to perform these two subprocess.run calls (referencing subprocess.run and
result.returncode) and to surface errors/exceptions just like the existing
install block.
Comment on lines
+1133
to
+1164
| try: | ||
| # Try brew first (macOS), then cargo | ||
| if shutil.which("brew"): | ||
| result = subprocess.run( | ||
| ["brew", "install", "ast-grep"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| ) | ||
| elif shutil.which("cargo"): | ||
| console.print(" [dim]Using cargo (may take a few minutes)...[/dim]") | ||
| result = subprocess.run( | ||
| ["cargo", "install", "ast-grep", "--locked"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=300, | ||
| ) | ||
| else: | ||
| console.print(" [yellow]WARN[/yellow] Neither brew nor cargo found") | ||
| console.print(" Install manually: https://github.com/ast-grep/ast-grep#installation") | ||
| result = None | ||
|
|
||
| if result and result.returncode == 0: | ||
| console.print(" [green]OK[/green] ast-grep installed") | ||
| elif result: | ||
| console.print(" [yellow]WARN[/yellow] Installation failed") | ||
| console.print(" Install manually: brew install ast-grep") | ||
| except subprocess.TimeoutExpired: | ||
| console.print(" [yellow]WARN[/yellow] Installation timed out") | ||
| console.print(" Install manually: brew install ast-grep") | ||
| except Exception as e: | ||
| console.print(f" [red]ERROR[/red] {e}") |
There was a problem hiding this comment.
Missing post-install verification for ast-grep, and misleading fallback message.
Two issues:
-
Like qlty, ast-grep doesn't verify the install succeeded (e.g.,
sg --version), deviating from the "explain → confirm → install → verify" pattern required by issue#140. -
Lines 1158-1159 and 1161-1162: when cargo install fails or times out, the fallback message suggests
brew install ast-grep, which is confusing if the user doesn't have brew. Consider a more generic message.
Proposed fix
if result and result.returncode == 0:
console.print(" [green]OK[/green] ast-grep installed")
+
+ # Verify installation
+ sg_cmd = "sg" if shutil.which("sg") else "ast-grep"
+ verify = subprocess.run(
+ [sg_cmd, "--version"],
+ capture_output=True,
+ text=True,
+ timeout=10,
+ )
+ if verify.returncode == 0:
+ console.print(f" [green]OK[/green] Verified: {verify.stdout.strip()}")
+ else:
+ console.print(" [yellow]WARN[/yellow] ast-grep installed but not on PATH")
elif result:
console.print(" [yellow]WARN[/yellow] Installation failed")
- console.print(" Install manually: brew install ast-grep")
+ console.print(" Install manually: https://github.com/ast-grep/ast-grep#installation")
except subprocess.TimeoutExpired:
console.print(" [yellow]WARN[/yellow] Installation timed out")
- console.print(" Install manually: brew install ast-grep")
+ console.print(" Install manually: https://github.com/ast-grep/ast-grep#installation")🤖 Prompt for AI Agents
In `@opc/scripts/setup/wizard.py` around lines 1133 - 1164, The ast-grep install
block (in wizard.py around the ast-grep install logic) must verify the
installation and avoid misleading fallback text: after a successful
subprocess.run that returns 0, run a verification check (e.g.,
subprocess.run(["sg","--version"], capture_output=True, text=True) or check
shutil.which("sg")) and only print "[green]OK[/green] ast-grep installed" if the
verification succeeds; if verification fails, treat it as an installation
failure and print the appropriate failure message. Also replace hardcoded
"Install manually: brew install ast-grep" in the failure and timeout branches
with a generic instruction or a context-aware suggestion (if
shutil.which("brew") is truthy suggest brew, elif shutil.which("cargo") suggest
cargo, else print the upstream install URL
https://github.com/ast-grep/ast-grep#installation) so users without brew aren't
directed to use it.
- Remove spurious f-string prefix on line 631 (no placeholders) - Add qlty --version verification after successful install - Add sg --version verification after successful ast-grep install - Replace hardcoded "brew install ast-grep" fallback with generic GitHub install URL (works for cargo/npm/brew users alike) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| if result and result.returncode == 0: | ||
| console.print(" [green]OK[/green] ast-grep installed") | ||
| # Verify installation | ||
| sg_cmd = "sg" if shutil.which("sg") else "ast-grep" |
There was a problem hiding this comment.
Bug: The verification check for newly installed tools like ast-grep or qlty fails because the PATH environment variable is not updated within the running script.
Severity: MEDIUM
Suggested Fix
Instead of relying on the process's PATH variable, determine the absolute path to the newly installed binary. For cargo, this is typically ~/.cargo/bin/. Use this absolute path directly when calling subprocess.run() for verification, bypassing the need for shutil.which() or a PATH search.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: opc/scripts/setup/wizard.py#L1169
Potential issue: After installing a tool like `ast-grep` or `qlty` via `cargo` or
`brew`, the verification step fails. This occurs because the installation updates the
shell's `PATH` configuration but not the `PATH` of the currently running Python wizard
process. The script uses `shutil.which()` to find the executable, which searches the
outdated `PATH` and fails. Consequently, the script incorrectly reports that the tool is
"installed but not on PATH", even though the installation was successful and the tool
will be available in new shell sessions.
3 tasks
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
Why
The wizard already installs TLDR (Step 10) and Loogle (Step 14) using the same pattern, but skips these two free tools that unlock 5+ skills:
qlty-check,qlty-during-development,fix(deps scope)ast-grep-find,search-router,search-toolsWithout them, these skills silently fail or degrade with no user feedback.
Changes
opc/scripts/setup/wizard.py— 2 new install steps + step number updatesREADME.md— Updated wizard steps table (12 → 15 steps)Test plan
curl -fsSL https://qlty.sh | shrunsCloses #140
Summary by CodeRabbit
New Features
Documentation