fix：Harden SQL safety handling and align architecture - #7
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR centralizes unsafe-SQL detection (f-strings, Jinja, runtime placeholders), moves block detection/reporting into the application layer, updates Extract/Format flows to warn-and-skip unsafe blocks (with reason), improves Redshift-aware formatting, adds a CLI --version flag, and updates tests/docs/config to reflect the layered architecture. ChangesSafety-first Block Handling & Layered Architecture
Sequence Diagram(s)sequenceDiagram
participant CLI_User
participant CLI_App
participant Application
participant Domain
participant Infrastructure
CLI_User->>CLI_App: py-sql-cleaner format <file>
CLI_App->>Application: format_source(source_file, source, ...)
Application->>Domain: detect_sql_blocks(source)
Domain-->>Application: list[SqlBlock]
Application->>Domain: unsafe_reason(block)
Domain-->>Application: reason or None
alt reason present or ALWAYS_SKIP
Application->>Application: append "Skipped unsafe SQL block ... reason=<reason>" warning
else
Application->>Infrastructure: formatter(sql, dialect, backend)
Infrastructure-->>Application: formatted_sql
Application->>Application: replace SQL in source
end
Application-->>CLI_App: FormatSourceResult(blocks, warnings, errors)
CLI_App-->>CLI_User: display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ca9c6d9e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+100
to
+103
| return previous_token not in {"", ",", "SELECT", "AS"} and next_token not in { | ||
| "", | ||
| ",", | ||
| "FROM", |
There was a problem hiding this comment.
The new _looks_like_table_option heuristic treats any keyword followed by , as non-Redshift (next_token not in {"", ",", "FROM"}), which misses valid Redshift column attributes like DISTKEY, SORTKEY, or ENCODE when they appear on a non-final column definition (e.g., col1 INT DISTKEY, col2 INT). This is a regression from the previous behavior and causes Redshift-specific SQL to slip past explicit dialect detection under the default dialect, leading to incorrect formatting/parsing paths instead of the expected Redshift-only handling.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli/test_format_command.py (1)
12-12: ⚡ Quick winAvoid hardcoding the version string in the test.
The test hardcodes
"py-sql-cleaner 0.1.0", which will require manual updates on every version bump.Consider using dynamic version retrieval or a pattern match instead.
♻️ Proposed fix using dynamic version
+import importlib.metadata + from typer.testing import CliRunner from py_sql_cleaner.cli import appdef test_version_option_prints_package_version() -> None: result = runner.invoke(app, ["--version"]) assert result.exit_code == 0, result.output - assert "py-sql-cleaner 0.1.0" in result.output + expected_version = importlib.metadata.version("py-sql-cleaner") + assert f"py-sql-cleaner {expected_version}" in result.outputAlternatively, use a pattern if the exact format is less important:
def test_version_option_prints_package_version() -> None: result = runner.invoke(app, ["--version"]) assert result.exit_code == 0, result.output - assert "py-sql-cleaner 0.1.0" in result.output + assert "py-sql-cleaner" in result.output + assert result.output.strip().split()[-1][0].isdigit() # ends with version number🤖 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 `@tests/cli/test_format_command.py` at line 12, The test currently hardcodes the version string ("py-sql-cleaner 0.1.0") which will break on version bumps; update the assertion in tests/cli/test_format_command.py to either (a) import the package version (e.g., from py_sql_cleaner import __version__ or the package metadata) and assert f"py-sql-cleaner {__version__}" is in result.output, or (b) use a regex/pattern match against result.output like "py-sql-cleaner \\d+\\.\\d+\\.\\d+" so the test no longer depends on a literal version string.
🤖 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 `@tests/cli/test_format_command.py`:
- Line 12: The test currently hardcodes the version string ("py-sql-cleaner
0.1.0") which will break on version bumps; update the assertion in
tests/cli/test_format_command.py to either (a) import the package version (e.g.,
from py_sql_cleaner import __version__ or the package metadata) and assert
f"py-sql-cleaner {__version__}" is in result.output, or (b) use a regex/pattern
match against result.output like "py-sql-cleaner \\d+\\.\\d+\\.\\d+" so the test
no longer depends on a literal version string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fa09c62-9107-44b4-be3d-01c9d8583f78
📒 Files selected for processing (14)
README.mdpy_sql_cleaner/adapters/sqlglot_formatter.pypy_sql_cleaner/application/extract_sql.pypy_sql_cleaner/application/format_source.pypy_sql_cleaner/core/detector.pypy_sql_cleaner/domain/models.pytests/adapters/test_sqlglot_formatter.pytests/cli/test_extract_command.pytests/cli/test_format_command.pytests/core/test_detector.pywebsite/docs/intro.mdwebsite/docs/project/safety.mdwebsite/docs/reference/commands.mdwebsite/docs/reference/supported-input.md
✅ Files skipped from review due to trivial changes (4)
- website/docs/intro.md
- README.md
- website/docs/reference/commands.md
- website/docs/reference/supported-input.md
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 (1)
py_sql_cleaner/cli/app.py (1)
108-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't report every detected block as unformatted.
result.blocksnow contains all detected SQL blocks, including skipped unsafe blocks and blocks that were already formatted. In--checkmode this can point users at unrelated locations whenever any one block changes.Proposed minimal fix
if not write: if changed: - console.print("Found unformatted embedded SQL:") - for block in result.blocks: - console.print( - f"- {block.file_path}:{block.start_line}-{block.end_line} " - f"variable={block.variable_name or '-'}" - ) + console.print("Found unformatted embedded SQL.") raise typer.Exit(1) raise typer.Exit(0)🤖 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 `@py_sql_cleaner/cli/app.py` around lines 108 - 113, Report only truly unformatted, non-skipped blocks: when iterating result.blocks in the CLI (the loop that prints "Found unformatted embedded SQL:"), filter out blocks that were skipped or already formatted by checking block.skipped and block.was_formatted (or the equivalent flags on the Block object) and only print blocks where skipped is false and was_formatted is false; update the loop that references result.blocks to use this filtered list so --check only points to real failures.
🤖 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 `@py_sql_cleaner/cli/app.py`:
- Around line 108-113: Report only truly unformatted, non-skipped blocks: when
iterating result.blocks in the CLI (the loop that prints "Found unformatted
embedded SQL:"), filter out blocks that were skipped or already formatted by
checking block.skipped and block.was_formatted (or the equivalent flags on the
Block object) and only print blocks where skipped is false and was_formatted is
false; update the loop that references result.blocks to use this filtered list
so --check only points to real failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 249e17e9-eda4-4334-9896-46f50bd5f1da
📒 Files selected for processing (18)
AGENTS.mddocs/architecture/README.mdpy_sql_cleaner/adapters/__init__.pypy_sql_cleaner/application/extract_sql.pypy_sql_cleaner/application/format_source.pypy_sql_cleaner/application/inspect_sql.pypy_sql_cleaner/cli/app.pypy_sql_cleaner/core/__init__.pypy_sql_cleaner/domain/detector.pypy_sql_cleaner/domain/extractor.pypy_sql_cleaner/domain/rewriter.pypy_sql_cleaner/domain/safety.pypy_sql_cleaner/infrastructure/__init__.pypy_sql_cleaner/infrastructure/sqlglot_formatter.pypyproject.tomltests/README.mdtests/domain/test_detector.pytests/infrastructure/test_sqlglot_formatter.py
💤 Files with no reviewable changes (4)
- py_sql_cleaner/adapters/init.py
- py_sql_cleaner/domain/detector.py
- py_sql_cleaner/core/init.py
- py_sql_cleaner/infrastructure/sqlglot_formatter.py
✅ Files skipped from review due to trivial changes (2)
- py_sql_cleaner/infrastructure/init.py
- docs/architecture/README.md
enumura1
changed the title
Merged
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
py-sql-cleaner --versionfor installed CLI version checks.formatandextract, even when--include-unsafeis provided.Why
The release smoke test found three issues before wider sharing: no CLI version command, overly broad Redshift keyword detection, and unsafe formatting of f-string SQL when explicitly included. These changes keep the CLI easier to verify and avoid rewriting SQL-like strings that are not complete SQL at rest.
Validation
scripts/check111/111 passedagainst the fixed CLISummary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests