fix: code quality - variable typo, dead import, PEP 8 identity checks, unnecessary f-strings - #993
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughThis PR modernizes ChangesPython cleanup and redirect validation
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to apply code-quality cleanups across the Python codebase (typo fix, PEP 8 None identity checks, and removal of unnecessary f-strings) while keeping behavior unchanged.
Changes:
- Rename misspelled local variable
pentalty→penaltyin gap analysis scoring. - Replace
== Nonechecks withis Nonefor PEP 8 singleton comparisons across multiple modules. - Remove
fprefixes from strings that have no interpolation, and remove a purportedly-dead JSON import alias.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| application/web/web_main.py | Removes unnecessary f-strings in PostHog event names and one logger call. |
| application/utils/spreadsheet_parsers.py | Replaces == None with is None in parsing logic. |
| application/utils/oscal_utils.py | Replaces uuid == None with uuid is None in OSCAL conversion. |
| application/utils/gap_analysis.py | Fixes local variable typo (pentalty → penalty) in path scoring. |
| application/utils/external_project_parsers/parsers/export_format_parser.py | Replaces == None with is None in export-format parsing. |
| application/prompt_client/prompt_client.py | Removes unnecessary f-strings from logger messages. |
| application/database/inmemory_graph.py | Replaces == None with is None for graph cache check. |
| application/database/db.py | Replaces == None with is None and removes unnecessary f-strings in exception/log strings. |
| application/cmd/cre_main.py | Removes f-strings and removes import json as _json (but _json is still referenced later). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from collections import deque | ||
| from typing import Any, Callable, Dict, List, Optional, Tuple, TYPE_CHECKING | ||
| import hashlib | ||
| import json as _json | ||
| from rq import Queue, job, exceptions | ||
| from sqlalchemy import not_ |
|
Please rebase onto latest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/cmd/cre_main.py (2)
895-907: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd test coverage for the noise-filter CLI path.
This branch adds new behavior. Add a test that verifies the stripped run ID,
noise_filter_dry_run, JSON output, and early return before other commands execute.As per coding guidelines:
**/*.{py,ts,tsx,js}requires test-first development for new behavior and importers.🤖 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 `@application/cmd/cre_main.py` around lines 895 - 907, The run_noise_filter CLI branch lacks coverage for its argument handling and control flow. Add a focused test for the command entry point that mocks db_connect and run_noise_filter, verifies args.run_id is stripped, noise_filter_dry_run is propagated, summary.to_json() is printed, and execution returns before subsequent commands run.Source: Coding guidelines
895-907: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTeardown the Flask-SQLAlchemy session after the noise filter run.
run_noise_filterhands lifecycle ownership to the caller, but thisrun()branch returns/printing JSON after success and does not tear down the connection when the pipeline raises. Wrap the connect/run/print block withsqla.session.remove()andapp_context.pop()intry/finallyon the error path.🤖 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 `@application/cmd/cre_main.py` around lines 895 - 907, Update the run_noise_filter branch in run() to wrap db_connect, run_noise_filter, and summary output in try/finally. In the finally block, call sqla.session.remove() and pop the Flask application context via the existing app-context handle, ensuring both cleanup actions run when the pipeline raises or after successful 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.
Outside diff comments:
In `@application/cmd/cre_main.py`:
- Around line 895-907: The run_noise_filter CLI branch lacks coverage for its
argument handling and control flow. Add a focused test for the command entry
point that mocks db_connect and run_noise_filter, verifies args.run_id is
stripped, noise_filter_dry_run is propagated, summary.to_json() is printed, and
execution returns before subsequent commands run.
- Around line 895-907: Update the run_noise_filter branch in run() to wrap
db_connect, run_noise_filter, and summary output in try/finally. In the finally
block, call sqla.session.remove() and pop the Flask application context via the
existing app-context handle, ensuring both cleanup actions run when the pipeline
raises or after successful output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 243a246e-19a1-45c1-a6bb-96d94f3a7f79
📒 Files selected for processing (3)
application/cmd/cre_main.pyapplication/database/db.pyapplication/web/web_main.py
🚧 Files skipped from review as they are similar to previous changes (2)
- application/database/db.py
- application/web/web_main.py
fcd41aa to
16bc020
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… unnecessary f-strings - Fix misspelled variable pentalty -> penalty in gap_analysis.py (3 occurrences) - Remove unused import json as _json in cre_main.py - Use is None instead of == None per PEP 8 (6 occurrences across 5 files) - Strip unnecessary f-string prefixes from strings with no placeholders (production code only)
16bc020 to
24ffdb2
Compare
| logger.info( | ||
| f"did not find node of type {ntype}, name {name} and section {section}, redirecting to external resource" | ||
| ) | ||
| return redirect(url) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/web/web_main.py (1)
797-806: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for rejected redirect values.
Test
http://...,javascript:..., and non-string resolver results. Assert a 404 response. Also assert thatredirectors.redirectis called once. Keep the existing HTTPS MITRE case as the success case.As per coding guidelines,
**/*.{py,ts,tsx,js}: Do not guess or leave incomplete code; make minimal-scope changes. Use test-first development for new behavior and importers.🤖 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 `@application/web/web_main.py` around lines 797 - 806, Add regression tests covering the redirect branch around redirectors.redirect for http:// URLs, javascript: URLs, and non-string results, asserting each returns 404 and the resolver is called once. Preserve the existing HTTPS MITRE test as the successful redirect case, and keep implementation changes limited to what the tests require.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.
Nitpick comments:
In `@application/web/web_main.py`:
- Around line 797-806: Add regression tests covering the redirect branch around
redirectors.redirect for http:// URLs, javascript: URLs, and non-string results,
asserting each returns 404 and the resolver is called once. Preserve the
existing HTTPS MITRE test as the successful redirect case, and keep
implementation changes limited to what the tests require.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: f13c19a3-83d1-499b-869d-ca8d663c3a03
📒 Files selected for processing (1)
application/web/web_main.py
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Review — code quality + smartlink redirect hardening (#993)
Looks good. CI green, mergeable. Approving.
What checks out
pentalty→penaltyis a local rename only (scoring already usedPENALTIES[...]).is None/ dead_jsonalias / unnecessaryfprefixes are safe cleanups; Copilot’s earlier_jsonNameErrorconcern is fixed (json.dumpseverywhere).- Smartlink: allowlisting
https://cwe.mitre.org//https://capec.mitre.org/+ singleredirectors.redirectcall is the right CodeQL fix;quote(str(...))in redirectors is fine defense-in-depth. Existing MITRE orphan case intest_smartlink_critical_edge_casesstill matches; new rejection tests cover http / javascript / non-string.
Nits (non-blocking)
- PR body still says “zero-behavior-change”; the smartlink path is an intentional security behavior change — worth a one-line update in the description before merge.
- Still ~11 commits behind
main— please rebase (or we can rebase-on-merge) so CI is current.
No need to chase CodeRabbit’s out-of-diff noise-filter comments; unrelated to this PR.
|
Approved — quality cleanups + smartlink HTTPS allowlist look good, CI green. Please rebase onto latest |
|
Merged (rebase) after updating onto |
Summary
Code quality fixes found during a full codebase audit. All changes are zero-behavior-change.
Follows up on leftover items from #836 / #837 (which was scoped down to just the
get_by_tagsshadowing fix during rebase).Note:
This PR includes an intentional security behavior change to the smartlink path. It resolves a CodeQL alert by URL-encoding inputs and strictly allowlisting cwe.mitre.org and capec.mitre.org to prevent open-redirect and javascript: URI injection.
Changes
Variable Typo Fix
pentalty→penaltyingap_analysis.py(3 occurrences, lines 100–102) — misspelled local variable fromget_path_score()Dead Import Removal
import json as _jsonremoved fromcre_main.py— imported but never referenced anywhere in the file (jsonis already imported on line 4)PEP 8:
== None→is None(6 occurrences across 5 files)Per PEP 8: comparisons to singletons like
Noneshould useis/is not, because==invokes__eq__which a custom class could override, whileischecks object identity — andNoneis a singleton, so identity is the correct check.Files:
spreadsheet_parsers.py(×2),oscal_utils.py,export_format_parser.py,inmemory_graph.py,db.pyUnnecessary f-string Prefixes (production code only)
Removed
fprefix from ~23 strings that contain no{}placeholders:web_main.py— 13 posthog event names + 1 logger calldb.py— 4 exception/log messagescre_main.py— 4 logger callsprompt_client.py— 2 logger callsTest files were intentionally left untouched to keep the diff focused.
Testing
python -m py_compile)pentalty,_json, or== Nonein the codebase