Enable ruff and pre-commit#11
Conversation
|
Warning Review limit reached
More reviews will be available in 23 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR introduces development infrastructure (pre-commit workflow, Ruff linting configuration, dev dependencies), updates the default server binding address from ChangesCode Quality and Configuration
Sequence DiagramsequenceDiagram
participant Client
participant MyOpenStackShell
participant MyCommandManager
participant RejectedEntryPoint
participant SHELL_run
participant EXECUTOR
Client->>MyOpenStackShell: run(mcp_argv, user_argv)
MyOpenStackShell->>EXECUTOR: run_function(run_shell_cmd, combined_argv)
EXECUTOR->>SHELL_run: execute command
SHELL_run-->>EXECUTOR: stdout/stderr or SystemExit
alt SystemExit raised
EXECUTOR-->>MyOpenStackShell: capture exit code and message
MyOpenStackShell-->>Client: ToolError with captured output
else Success
EXECUTOR-->>MyOpenStackShell: return stdout/stderr
MyOpenStackShell->>MyCommandManager: load_commands() via initialize
MyCommandManager->>RejectedEntryPoint: wrap disallowed commands
RejectedEntryPoint-->>MyCommandManager: store rejection metadata
MyCommandManager-->>MyOpenStackShell: command list with rejections
MyOpenStackShell-->>Client: execution result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rhos_ls_mcps/osc.py (1)
335-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a second initialization guard after lock acquisition.
Line 335 pre-check alone is racy. Two concurrent first calls can both run initialization logic sequentially. Re-check after acquiring the lock to guarantee one-time initialization.
Suggested fix
async def _initialize_parser( self, mcp_argv: list[str], user_argv: list[str] ) -> None: if self.initialized: return await self.lock.acquire() try: + if self.initialized: + return await self._initialize_api_versions(mcp_argv) await self._initialize_global_args(user_argv) MyOpenStackShell.initialized = True finally: self.lock.release()🤖 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 `@src/rhos_ls_mcps/osc.py` around lines 335 - 344, Add a second initialization guard immediately after acquiring self.lock to avoid the race where two coroutines both pass the initial if self.initialized check: after await self.lock.acquire() (or as the first statement in the try block) re-check the initialization flag (self.initialized or MyOpenStackShell.initialized) and return early if set; otherwise proceed to call _initialize_api_versions and _initialize_global_args and set MyOpenStackShell.initialized = True as before, ensuring the lock is still released in the finally block.
🤖 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 @.github/workflows/pre-commit.yaml:
- Around line 22-30: Replace the mutable action tags in
.github/workflows/pre-commit.yaml with the provided commit SHAs to pin versions:
change uses: actions/checkout@v6 to uses:
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd, change uses:
actions/setup-python@v6 to uses:
actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405, and change uses:
pre-commit/action@v3.0.1 to uses:
pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd; you may optionally
keep the original tags as comments for readability but ensure the active uses:
lines reference the SHAs.
In `@pyproject.toml`:
- Around line 21-26: The package currently lists "cliff" only under the dev
dependency group but src/rhos_ls_mcps/osc.py imports "from cliff import
interactive" at runtime; move "cliff>=4.14.0" from the [dependency-groups].dev
list into the main dependencies section of pyproject.toml so it's installed for
runtime use (preserve the version constraint), and ensure the dependencies list
includes the same entry; you can remove it from the dev group afterward.
In `@scripts/allow-deny-list.py`:
- Around line 43-46: Remove the duplicate "cleanup" entry from the
REJECT_COMMANDS collection: find the REJECT_COMMANDS definition (variable name
REJECT_COMMANDS) and delete the redundant "cleanup" element so it only appears
once; no behavior change needed since Python deduplicates sets, but this removes
the static-analysis warning and cleans up the list.
In `@src/rhos_ls_mcps/osc.py`:
- Around line 443-445: The run() function currently catches SystemExit as e and
raises a ToolError using the message built from e.code and stdout/stderr, but it
drops the original exception context; change the re-raise to preserve chaining
by raising ToolError(...) from e (i.e., use "raise ToolError(... ) from e") so
the original SystemExit is kept as the __cause__; update the raise in the except
SystemExit block where ToolError is constructed to include "from e".
- Around line 418-423: The _do_run function currently returns from inside the
finally block (return return_code, stdout, stderr), which can swallow
exceptions; change the flow so finally only performs cleanup (call
self._clean_stds() and capture stdout/stderr into locals) and do not return
there. Instead assign stdout/stderr to local variables (e.g., stdout =
self.stdout.getvalue(); stderr = self.stderr.getvalue()), perform
self._clean_stds() in finally, and move the actual return (return return_code,
stdout, stderr) to the end of _do_run (after the try/except/finally) so any
exceptions propagate normally; reference symbols: _do_run,
self.stdout.getvalue(), self.stderr.getvalue(), self._clean_stds(), return_code.
---
Outside diff comments:
In `@src/rhos_ls_mcps/osc.py`:
- Around line 335-344: Add a second initialization guard immediately after
acquiring self.lock to avoid the race where two coroutines both pass the initial
if self.initialized check: after await self.lock.acquire() (or as the first
statement in the try block) re-check the initialization flag (self.initialized
or MyOpenStackShell.initialized) and return early if set; otherwise proceed to
call _initialize_api_versions and _initialize_global_args and set
MyOpenStackShell.initialized = True as before, ensuring the lock is still
released in the finally block.
🪄 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: 640f9a4b-d607-4552-8604-22b995daa636
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.github/workflows/pre-commit.yaml.pre-commit-config.yamlContainerfileREADME.mdpyproject.tomlscripts/allow-deny-list.pyscripts/diff-allow-deny.pysrc/rhos_ls_mcps/auth.pysrc/rhos_ls_mcps/logging.pysrc/rhos_ls_mcps/mcp_base.pysrc/rhos_ls_mcps/oc.pysrc/rhos_ls_mcps/osc.pysrc/rhos_ls_mcps/settings.pysrc/rhos_ls_mcps/utils.py
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Thanks Lukas! It looks good. Left a small comment. But overall it looks pretty good
|
|
||
| class Settings(BaseSettings): | ||
| ip: str = Field(default="0.0.0.0", description="IP address to bind to") | ||
| ip: str = Field(default="127.0.0.1", description="IP address to bind to") |
There was a problem hiding this comment.
Just a comment. I guess we had to update this default because it's "safer" if you run outside the container ?
in the container we need to run with 0.0.0.0 otherwise it won't be reached from outside .
That said, should we also update the sample file [0] ?
[0]
Line 1 in 45549e3
There was a problem hiding this comment.
I guess we had to update this default because it's "safer" if you run outside the container ?
Yes:)
That said, should we also update the sample file [0] ?
Yes 💯 . Thanks, done!:)
There was a problem hiding this comment.
-1: Unrelated change. And I'm also against changing this default.
Introduce .pre-commit-config.yaml with ruff-format and ruff-check hooks and add ruff and pre-commit to dev dependencies. Add a GitHub Actions workflow that runs pre-commit on pushes to main and on pull requests targeting main. Configure ruff to extend the default linters with Bandit (S) security rules via `extend-select` (preserving all default rules). Apply ruff-format across the entire codebase for consistent formatting. Fix S104: default bind address to 127.0.0.1 instead of 0.0.0.0. The Containerfile entrypoint now explicitly passes --ip 0.0.0.0 so container networking continues to work. Fix TCH violation in osc.py by moving the cliff import behind TYPE_CHECKING.
lpiwowar
force-pushed
the
lpiwowar/ruff-security
branch
from
a5be2f7 to
6ded951
Compare
|
|
||
| class Settings(BaseSettings): | ||
| ip: str = Field(default="0.0.0.0", description="IP address to bind to") | ||
| ip: str = Field(default="127.0.0.1", description="IP address to bind to") |
There was a problem hiding this comment.
-1: Unrelated change. And I'm also against changing this default.
| @@ -1,4 +1,4 @@ | |||
| ip: 0.0.0.0 | |||
| ip: 127.0.0.1 | |||
There was a problem hiding this comment.
-1: Unrelated change to this PR. I'm ok changing the sample in another PR though.
|
|
||
| ## General | ||
| - `ip`: IP address the server will bind to. Default `0.0.0.0`. | ||
| - `ip`: IP address the server will bind to. Default `127.0.0.1`. |
There was a problem hiding this comment.
-1: No changing defaults on this PR.
3 participants
Introduce
.pre-commit-config.yamlwithruff-formatandruff-checkhooks and addruffandpre-committo dev dependencies. Add a GitHub Actions workflow that runspre-commiton pushes tomainand on pull requests targetingmain.Configure
ruffto extend the default linters with Bandit (S) security rules viaextend-select(preserving all default rules). Applyruff-formatacross the entire codebase for consistent formatting.Fix
S104: default bind address to127.0.0.1instead of0.0.0.0. TheContainerfileentrypoint now explicitly passes--ip 0.0.0.0so container networking continues to work.Fix
TCHviolation inosc.pyby moving thecliffimport behindTYPE_CHECKING.Summary by CodeRabbit
Configuration
127.0.0.1) for enhanced security instead of binding to all interfaces.Chores