Conversation
When a SCIP indexer binary isn't found, automatically install it using the language's native package manager (go install, gem install, dart pub, dotnet tool, rustup, coursier, npm). This removes a major accessibility barrier - users no longer need to manually install indexer tools. - Add generic SCIPToolInstaller with 7 install methods - Add install_config field to SCIPLanguageConfig for go, ruby, dart, java, scala, csharp, vb (c/cpp have no auto-install) - Integrate auto-install into ConfigurableSCIPIndexer._find_executable() - Add rust-analyzer auto-install via rustup in RustSCIPIndexer
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 1. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Summary of ChangesHello @wende, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust auto-installation system for SCIP indexer tools, significantly improving the developer experience by automatically resolving missing dependencies. By centralizing installation logic and integrating it into the core indexer framework, it streamlines the setup process for various programming languages, making the system more self-sufficient and user-friendly. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 2 other issues
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `cicada/languages/scip/installer.py:135-58` </location>
<code_context>
+ return None
+
+ @classmethod
+ def _install_gem(cls, config: InstallConfig, verbose: bool) -> str | None:
+ """Install via gem install."""
+ result = subprocess.run(
+ ["gem", "install", config.package, "--no-document"],
+ capture_output=True,
+ text=True,
+ timeout=180,
+ )
+
+ if result.returncode != 0:
+ if verbose:
+ print(f" gem install failed: {result.stderr}")
+ return None
+
+ path = shutil.which(config.executable)
</code_context>
<issue_to_address>
**suggestion:** Gem-installed binaries may not be found via PATH; consider probing standard gem bin directories.
Unlike the Go/Dart/Dotnet helpers, this relies only on `shutil.which(config.executable)` after `gem install`. If the gem bin dir isn’t on PATH (common on some distros/CI images), this will look like a failed install even when it succeeded. Consider checking standard gem bin locations (e.g., from `gem environment` or `$HOME/.gem/.../bin`) before falling back to `which` so installs work reliably across environments.
Suggested implementation:
```python
@classmethod
def _install_gem(cls, config: InstallConfig, verbose: bool) -> str | None:
"""Install via gem install."""
result = subprocess.run(
["gem", "install", config.package, "--no-document"],
capture_output=True,
text=True,
timeout=180,
)
if result.returncode != 0:
if verbose:
print(f" gem install failed: {result.stderr}")
return None
# Probe standard gem bin directories first, then fall back to PATH.
gem_bin_dirs = set()
# Prefer gem-reported directories for robustness across environments.
for key in ("gemdir", "user_gemdir"):
try:
env = subprocess.run(
["gem", "environment", key],
capture_output=True,
text=True,
timeout=30,
)
if env.returncode == 0:
value = env.stdout.strip()
if value:
gem_bin_dirs.add(Path(value) / "bin")
except Exception as exc: # pragma: no cover - defensive
if verbose:
print(f" failed to query gem environment {key}: {exc}")
# Common fallback: ~/.gem/.../bin
try:
home = Path.home()
gem_bin_dirs.add(home / ".gem" / "bin")
except Exception:
# If we can't resolve $HOME, just skip this fallback.
pass
for bin_dir in gem_bin_dirs:
candidate = bin_dir / config.executable
if candidate.is_file() and os.access(candidate, os.X_OK):
if verbose:
print(f" Installed {config.executable} to {candidate}")
return str(candidate)
path = shutil.which(config.executable)
if path:
return path
return None
```
This edit assumes `Path` from `pathlib` and `os` are already imported in `cicada/languages/scip/installer.py`. If they are not, add:
- `from pathlib import Path`
- `import os`
to the import section near the top of the file, following existing style and ordering.
</issue_to_address>
### Comment 2
<location> `cicada/languages/typescript/indexer.py:51` </location>
<code_context>
# Security audit: Command uses list-form arguments (not shell=True),
# so no command injection risk. All arguments are hardcoded strings.
- cmd = ["npx", "@sourcegraph/scip-typescript", "index"]
+ cmd = ["npx", "--yes", "@sourcegraph/scip-typescript", "index"]
scip_file = repo_path / "index.scip"
</code_context>
<issue_to_address>
**question:** Using `npx --yes` can break on older npm versions that don’t support this flag.
This flag avoids prompts but is unsupported in older npm/npx versions (e.g., npm < 7), causing the command to fail there. If this tool is expected to run on varied environments, consider either checking the npm/npx version before adding `--yes` or falling back to plain `npx` when the flag isn’t supported to avoid failures on older toolchains.
</issue_to_address>
### Comment 3
<location> `cicada/languages/scip/installer.py:243-248` </location>
<code_context>
result = subprocess.run(
[cs_cmd, "install", config.package],
capture_output=True,
text=True,
timeout=180,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if config.runtime_check and not shutil.which(config.runtime_check): | ||
| if verbose: | ||
| print(f" {config.runtime_check} not found - cannot install {config.executable}") | ||
| return None |
There was a problem hiding this comment.
suggestion: Gem-installed binaries may not be found via PATH; consider probing standard gem bin directories.
Unlike the Go/Dart/Dotnet helpers, this relies only on shutil.which(config.executable) after gem install. If the gem bin dir isn’t on PATH (common on some distros/CI images), this will look like a failed install even when it succeeded. Consider checking standard gem bin locations (e.g., from gem environment or $HOME/.gem/.../bin) before falling back to which so installs work reliably across environments.
Suggested implementation:
@classmethod
def _install_gem(cls, config: InstallConfig, verbose: bool) -> str | None:
"""Install via gem install."""
result = subprocess.run(
["gem", "install", config.package, "--no-document"],
capture_output=True,
text=True,
timeout=180,
)
if result.returncode != 0:
if verbose:
print(f" gem install failed: {result.stderr}")
return None
# Probe standard gem bin directories first, then fall back to PATH.
gem_bin_dirs = set()
# Prefer gem-reported directories for robustness across environments.
for key in ("gemdir", "user_gemdir"):
try:
env = subprocess.run(
["gem", "environment", key],
capture_output=True,
text=True,
timeout=30,
)
if env.returncode == 0:
value = env.stdout.strip()
if value:
gem_bin_dirs.add(Path(value) / "bin")
except Exception as exc: # pragma: no cover - defensive
if verbose:
print(f" failed to query gem environment {key}: {exc}")
# Common fallback: ~/.gem/.../bin
try:
home = Path.home()
gem_bin_dirs.add(home / ".gem" / "bin")
except Exception:
# If we can't resolve $HOME, just skip this fallback.
pass
for bin_dir in gem_bin_dirs:
candidate = bin_dir / config.executable
if candidate.is_file() and os.access(candidate, os.X_OK):
if verbose:
print(f" Installed {config.executable} to {candidate}")
return str(candidate)
path = shutil.which(config.executable)
if path:
return path
return NoneThis edit assumes Path from pathlib and os are already imported in cicada/languages/scip/installer.py. If they are not, add:
from pathlib import Pathimport os
to the import section near the top of the file, following existing style and ordering.
| # Security audit: Command uses list-form arguments (not shell=True), | ||
| # so no command injection risk. All arguments are hardcoded strings. | ||
| cmd = ["npx", "@sourcegraph/scip-typescript", "index"] | ||
| cmd = ["npx", "--yes", "@sourcegraph/scip-typescript", "index"] |
There was a problem hiding this comment.
question: Using npx --yes can break on older npm versions that don’t support this flag.
This flag avoids prompts but is unsupported in older npm/npx versions (e.g., npm < 7), causing the command to fail there. If this tool is expected to run on varied environments, consider either checking the npm/npx version before adding --yes or falling back to plain npx when the flag isn’t supported to avoid failures on older toolchains.
| result = subprocess.run( | ||
| [cs_cmd, "install", config.package], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=180, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent feature for automatically installing missing SCIP indexer tools, which will significantly improve the user experience. The implementation is well-structured, with a new generic SCIPToolInstaller that cleanly handles different package managers. The integration into the existing indexers is also well-done, and the new unit tests are comprehensive. I have a couple of minor suggestions to improve code consistency and efficiency, but overall this is a high-quality contribution.
| def _make_install_configs() -> dict[str, InstallConfig]: | ||
| """Build install configs lazily to avoid circular imports.""" | ||
| from cicada.languages.scip.installer import InstallConfig, InstallMethod | ||
|
|
||
| return { | ||
| "go": InstallConfig( | ||
| method=InstallMethod.GO, | ||
| package="github.com/sourcegraph/scip-go/cmd/scip-go@latest", | ||
| executable="scip-go", | ||
| runtime_check="go", | ||
| ), | ||
| "ruby": InstallConfig( | ||
| method=InstallMethod.GEM, | ||
| package="scip-ruby", | ||
| executable="scip-ruby", | ||
| runtime_check="gem", | ||
| ), | ||
| "dart": InstallConfig( | ||
| method=InstallMethod.DART_PUB, | ||
| package="scip_dart", | ||
| executable="scip_dart", | ||
| runtime_check="dart", | ||
| ), | ||
| "java": InstallConfig( | ||
| method=InstallMethod.COURSIER, | ||
| package="scip-java", | ||
| executable="scip-java", | ||
| ), | ||
| "scala": InstallConfig( | ||
| method=InstallMethod.COURSIER, | ||
| package="scip-java", | ||
| executable="scip-java", | ||
| ), | ||
| "csharp": InstallConfig( | ||
| method=InstallMethod.DOTNET, | ||
| package="scip-dotnet", | ||
| executable="scip-dotnet", | ||
| runtime_check="dotnet", | ||
| ), | ||
| "vb": InstallConfig( | ||
| method=InstallMethod.DOTNET, | ||
| package="scip-dotnet", | ||
| executable="scip-dotnet", | ||
| runtime_check="dotnet", | ||
| ), | ||
| } | ||
|
|
||
|
|
||
| def _get_install_config(language: str) -> InstallConfig | None: | ||
| """Get install config for a language, or None if not supported.""" | ||
| configs = _make_install_configs() | ||
| return configs.get(language) |
There was a problem hiding this comment.
The _make_install_configs() function is called every time _get_install_config() is invoked. Since _get_install_config() is called for each language config at module load time, this leads to the dictionary of install configs being created multiple times unnecessarily. It would be more efficient to create it once and cache it in a module-level variable.
_INSTALL_CONFIGS: dict[str, InstallConfig] | None = None
def def _make_install_configs() -> dict[str, InstallConfig]:
"""Build install configs lazily to avoid circular imports."""
from cicada.languages.scip.installer import InstallConfig, InstallMethod
return {
"go": InstallConfig(
method=InstallMethod.GO,
package="github.com/sourcegraph/scip-go/cmd/scip-go@latest",
executable="scip-go",
runtime_check="go",
),
"ruby": InstallConfig(
method=InstallMethod.GEM,
package="scip-ruby",
executable="scip-ruby",
runtime_check="gem",
),
"dart": InstallConfig(
method=InstallMethod.DART_PUB,
package="scip_dart",
executable="scip_dart",
runtime_check="dart",
),
"java": InstallConfig(
method=InstallMethod.COURSIER,
package="scip-java",
executable="scip-java",
),
"scala": InstallConfig(
method=InstallMethod.COURSIER,
package="scip-java",
executable="scip-java",
),
"csharp": InstallConfig(
method=InstallMethod.DOTNET,
package="scip-dotnet",
executable="scip-dotnet",
runtime_check="dotnet",
),
"vb": InstallConfig(
method=InstallMethod.DOTNET,
package="scip-dotnet",
executable="scip-dotnet",
runtime_check="dotnet",
),
}
def _get_install_config(language: str) -> InstallConfig | None:
"""Get install config for a language, or None if not supported."""
global _INSTALL_CONFIGS
if _INSTALL_CONFIGS is None:
_INSTALL_CONFIGS = _make_install_configs()
return _INSTALL_CONFIGS.get(language)| path = shutil.which(config.executable) | ||
| if path: | ||
| return path |
There was a problem hiding this comment.
The fallback to shutil.which() does not log a success message when verbose is true. This is inconsistent with other installer methods like _install_gem which do log a message. Adding a log message here would improve consistency and aid in debugging. This also applies to _install_dart_pub and _install_dotnet.
| path = shutil.which(config.executable) | |
| if path: | |
| return path | |
| path = shutil.which(config.executable) | |
| if path: | |
| if verbose: | |
| print(f" Installed {config.executable} at {path}") | |
| return path |
1 participant
Summary
SCIPToolInstallerclass with 7 install methods andInstallConfigdataclassConfigurableSCIPIndexer._find_executable()andRustSCIPIndexer._ensure_rust_analyzer_installed()Test plan
_find_executableauto-install path