fix(setup): resolve path bugs and missing config extras

[Virtuoso633]

Description

This PR resolves dependency conflicts and improves the developer setup workflow by addressing all code review feedback.

Changes Made

1. Removed all extra to prevent dependency conflicts

Removed the all extra from pyproject.toml that was causing conflicts between pyspark and databricks-connect.
Updated documentation to guide users to install specific extras instead (e.g., -E aws -E pyspark).

2. Hardened setup.py for security and robustness

✅ Added shlex.split() for safer command parsing (handles quoted arguments correctly)
✅ Fixed unreachable error handling logic
✅ Removed automatic poetry install call to prevent overwriting user-specified extras
✅ Updated messaging to accurately reflect that setup.py only configures pre-commit hooks

3. Corrected CONTRIBUTING.md installation workflow

✅ Fixed installation order: dependencies installed before hooks configured
✅ Removed outdated comments about setup.py running poetry install
✅ Clear, logical workflow: poetry install -E <extras> → python setup.py → pytest

Testing

Verified end-to-end in a fresh environment:

poetry install -E aws -E pyspark  # ✅ Success
python setup.py                   # ✅ Success
poetry run pytest                 # ✅ All tests passed 

Addresses Review Feedback

Copilot: Dependency conflict between pyspark/databricks
Copilot: setup.py string command handling safety
Copilot: Unreachable error handling code
Copilot: Misleading "Installing dependencies..." messages
Copilot: Backwards installation workflow in CONTRIBUTING.md
Maintainer: Remove all extra
Maintainer: Document specific extras usage

[greptile-apps]

Greptile Overview

Greptile Summary

This PR improves the setup workflow by removing automatic dependency installation from setup.py and correcting the installation order in documentation. The changes successfully address the maintainer's feedback about removing the all extra (already done in a previous commit) and documenting specific extras usage.

Key improvements:

• Hardened setup.py with shlex.split() for safer command parsing
• Removed automatic poetry install call to prevent overwriting user-specified extras
• Fixed CONTRIBUTING.md installation workflow (dependencies installed before hooks configured)
• Updated messaging to accurately reflect that setup.py only configures pre-commit hooks

Issues found:

• Docstring in setup.py still claims it "installs core dependencies" when it no longer does
• Error handling incomplete: FileNotFoundError (raised when executable not found) won't be caught by the current except CalledProcessError clause

Confidence Score: 4/5

• This PR is safe to merge with minor issues to address
• The changes improve security and correctness by using shlex.split and fixing the installation workflow. However, there are two minor issues: an outdated docstring and incomplete error handling that should be fixed before merge.
• Pay close attention to setup.py - fix the docstring and error handling before merging

Important Files Changed

Filename	Overview
setup.py	Improved security with shlex.split and removed auto poetry install; error handling incomplete for FileNotFoundError
CONTRIBUTING.md	Corrected installation workflow order (deps before hooks), improved clarity with better formatting and comments

[greptile-apps]

_{4 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (2)

pyproject.toml
Broken all extra mapping

[tool.poetry.extras].all is defined as ["pyspark", "databricks-connect", "boto3"], but extras are supposed to list optional dependency keys (e.g. databricks-connect, boto3) not other extras (and pyspark here is an extra name, while the optional dependency key is pyspark). This will make poetry install -E all fail to include PySpark (and can error depending on Poetry version). all should reference the optional dependency keys (e.g. include "pyspark" via the dependency key, and consider using the existing databricks/aws extras rather than raw dependency names).

Prompt To Fix With AI

This is a comment left during a code review.
Path: pyproject.toml
Line: 73:79

Comment:
**Broken `all` extra mapping**

`[tool.poetry.extras].all` is defined as `["pyspark", "databricks-connect", "boto3"]`, but extras are supposed to list *optional dependency keys* (e.g. `databricks-connect`, `boto3`) not other extras (and `pyspark` here is an extra name, while the optional dependency key is `pyspark`). This will make `poetry install -E all` fail to include PySpark (and can error depending on Poetry version). `all` should reference the optional dependency keys (e.g. include `"pyspark"` via the dependency key, and consider using the existing `databricks`/`aws` extras rather than raw dependency names).

How can I resolve this? If you propose a fix, please make it concise.

---

setup.py
Shell invocation in setup

run_command() uses subprocess.run(command, ..., shell=True) (setup.py:11-13) with a string. If any part of command ever becomes user-controlled (CLI args/env), this becomes a command-injection vector. Given this script’s purpose, it’s safer to avoid shell=True and pass args as a list (e.g. ['poetry','run','pre-commit','install']).

Prompt To Fix With AI

This is a comment left during a code review.
Path: setup.py
Line: 11:13

Comment:
**Shell invocation in setup**

`run_command()` uses `subprocess.run(command, ..., shell=True)` (setup.py:11-13) with a string. If any part of `command` ever becomes user-controlled (CLI args/env), this becomes a command-injection vector. Given this script’s purpose, it’s safer to avoid `shell=True` and pass args as a list (e.g. `['poetry','run','pre-commit','install']`).

How can I resolve this? If you propose a fix, please make it concise.

[Copilot]

Pull request overview

This PR focuses on making local development setup more reliable by routing tooling through Poetry, aligning Poetry extras with documented install options, and cleaning up contributor documentation.

Changes:

• Update setup.py to install pre-commit hooks via poetry run pre-commit install and add a module docstring.
• Add all, lightweight, and deep-learning extras to pyproject.toml and sync poetry.lock accordingly.
• Improve CONTRIBUTING.md setup/test instructions with clearer Poetry activation and poetry run pytest guidance.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File	Description
setup.py	Adds module docstring and uses Poetry to run pre-commit installation.
pyproject.toml	Introduces new extras (all, lightweight, deep-learning) to match setup/documentation.
poetry.lock	Regenerates lockfile to reflect new extras/markers and Poetry version changes.
CONTRIBUTING.md	Refines onboarding commands for Poetry env activation and test execution.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[marcellodebernardi]

Thanks for the PR! Looks good, except that installing pyspark and databricks-connect at the same time leads to unexpected behaviour, so I think we should avoid that.

I think let's remove the all extras, other than that LGTM 👍

[Copilot]

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[marcellodebernardi]

@greptileai review again with latest changes, all issues have been addressed

[greptile-apps]

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

setup.py
Setup ignores requested extras

main() prints guidance about installing extras (e.g. -E aws -E pyspark), but the script always runs poetry install with no extras (line 31) and then installs pre-commit hooks. If a contributor follows the docs/print output expecting extras to be installed, they’ll still end up with the default lightweight environment, which is particularly confusing given the docs now recommend extras for common workflows. Consider either making setup.py accept/pass through extras (e.g. via argv) or removing the extras messaging so the script behavior matches what it says.

Prompt To Fix With AI

This is a comment left during a code review.
Path: setup.py
Line: 24:35

Comment:
**Setup ignores requested extras**

`main()` prints guidance about installing extras (e.g. `-E aws -E pyspark`), but the script always runs `poetry install` with no extras (line 31) and then installs pre-commit hooks. If a contributor follows the docs/print output expecting extras to be installed, they’ll still end up with the default lightweight environment, which is particularly confusing given the docs now recommend extras for common workflows. Consider either making setup.py accept/pass through extras (e.g. via argv) or removing the extras messaging so the script behavior matches what it says.

How can I resolve this? If you propose a fix, please make it concise.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Virtuoso633]

Hi @marcellodebernardi ,

Thanks for the feedback again! I've addressed all your points:

✅ Removed all extra to prevent pyspark/databricks conflict
✅ Updated docs to recommend specific extras (poetry install -E aws -E pyspark)
✅ Hardened setup.py (safer parsing, no auto-install, clearer messages)
✅ Fixed CONTRIBUTING.md installation order

I have Tested end-to-end in fresh environment - all working as documented. Ready for review!

[marcellodebernardi]

LGTM, thanks for the contribution @Virtuoso633!

[marcellodebernardi]

@greptileai review again with latest changes, but this PR has already been approved and everything looks good

[greptile-apps]

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}