Fix S001 implicit-cross-join false negatives (aliases, schema-qual, multi-line) (#46)

* Fix S001 false negatives on aliased / schema-qualified / multi-line FROM

The previous detection regex `\bFROM\s+(\w+\s*,\s*\w+)` only matched
when the comma came immediately after the first word after FROM. Most
real-world comma-joins missed it: aliased tables, schema-qualified
names, three-or-more-way joins, and multi-line layouts all slipped
through silently. Reproductions on main:

  FROM orders o, customers c WHERE ...                  -> not flagged
  FROM raw.orders, raw.customers WHERE ...              -> not flagged
  FROM o1, o2, o3 WHERE ...                             -> not flagged
  FROM orders o,\n     customers c\nWHERE ...            -> not flagged

Replaces the regex with a paren-depth-aware scan: from each FROM
keyword, walk forward tracking parenthesis depth, stop at WHERE /
GROUP BY / ORDER BY / HAVING / LIMIT / explicit JOIN / UNION / EXCEPT
/ INTERSECT, and flag the first depth-0 comma. Strings and comments
are stripped first so commas inside them do not register.

Two suppression cases added:

  - DELETE FROM is skipped (single-target DELETE; bare comma after
    DELETE FROM is invalid syntax in most dialects)
  - Comma followed by LATERAL is treated as a legitimate
    Snowflake/Postgres lateral join, not flagged

This addresses #41 (the W027 comma-join request) — the rule already
existed as S001 but was not catching the patterns it claimed to
cover. Improving S001 in place avoids a duplicate W027.

* Add S001 regression tests for the false-negative cases

14 new tests under TestImplicitCrossJoin: aliases, schema-qualified
names, three-or-more-way joins, multi-line layout, comma-joins inside
CTEs, plus the suppression cases (function-call commas, subquery
commas, VALUES rows, LATERAL after comma, LEFT OUTER JOIN, string
literals with commas, DELETE FROM, comments between tables). Each
case has a one-line comment explaining what it pins down.

Total tests in this class go from 3 to 17. Full suite stays at 226
passing.

* Document S001 fix in CHANGELOG under [Unreleased]

* style: pre-commit auto-fixes

[pre-commit.ci] auto-applied fixes from configured hooks

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: Pawansingh3889

CHANGELOG.md

@@ -23,6 +23,22 @@ a deprecation window (see `GOVERNANCE.md` § Scope discipline).
   ([#39](https://github.com/Pawansingh3889/sql-guard/pull/39)).
   Resolves #3.
 
+### Fixed
+
+- **S001 `implicit-cross-join`** - the previous regex only matched a
+  comma immediately after the first word after `FROM`. Most real-world
+  comma-joins slipped through silently: aliased tables
+  (`FROM orders o, customers c`), schema-qualified names
+  (`FROM raw.orders, raw.customers`), three-or-more-way joins, and
+  multi-line layouts. Replaced with a paren-depth-aware scan that
+  walks from each `FROM` keyword to the next `WHERE` / `GROUP BY` /
+  `ORDER BY` / `HAVING` / `LIMIT` / explicit `JOIN` / `UNION` /
+  `EXCEPT` / `INTERSECT`, flagging the first depth-0 comma. Strings
+  and comments are stripped first. `DELETE FROM` is skipped.
+  `, LATERAL ...` is recognised as a legitimate
+  Snowflake/Postgres lateral join and not flagged. Resolves #41
+  (the W027 comma-join request was a duplicate of S001).
+
 ## [0.7.0] - 2026-05-02
 
 Headline release: schema-aware linting via the new **Contracts pack**, three community-contributed rules (W014 / W015 / W022), and the `schema-snapshot` and `validate-contract` subcommands. Core registry is 43 rules (10 errors, 28 warnings, 5 Python-source). With `--contract` enabled the registry grows to 48 rules (12 errors, 31 warnings, 5 Python-source). Without `--contract` behaviour is identical to v0.6.x and there are no breaking changes.
@@ -219,7 +235,7 @@ Headline release: schema-aware linting via the new **Contracts pack**, three com
 - `test_duration_tracked` no longer fails on fast hardware where
   `time.perf_counter` resolution is coarser than the scan duration.
   Assertion relaxed from `> 0` to `>= 0` with an explicit float type check.
-- Added W014: warn on OVER() without ORDER BY / PARTITION BY to flag non-deterministic window 
+- Added W014: warn on OVER() without ORDER BY / PARTITION BY to flag non-deterministic window
   functions.
 
 ## [0.4.1] - 2026-04-19

scripts/scaffold_rule.py

@@ -76,7 +76,7 @@ def check_line(self, line: str, line_number: int, file: str):
 '''
 
 
-TEST_TEMPLATE = '''\
+TEST_TEMPLATE = """\
 def test_{code_lower}_fires_on_bad_sql():
     rule = {klass}()
     finding = _line(rule, {bad!r})
@@ -87,7 +87,7 @@ def test_{code_lower}_fires_on_bad_sql():
 def test_{code_lower}_passes_on_safe_sql():
     rule = {klass}()
     assert _line(rule, {good!r}) is None
-'''
+"""
 
 
 def main() -> int:
@@ -96,8 +96,12 @@ def main() -> int:
         formatter_class=argparse.RawDescriptionHelpFormatter,
         epilog=__doc__.split("Example:")[1] if "Example:" in __doc__ else "",
     )
-    parser.add_argument("--code", required=True, help="Rule code, e.g. W024 (must match [EWTSP]NNN)")
-    parser.add_argument("--name", required=True, help="Rule name in kebab-case, e.g. negate-of-equality")
+    parser.add_argument(
+        "--code", required=True, help="Rule code, e.g. W024 (must match [EWTSP]NNN)"
+    )
+    parser.add_argument(
+        "--name", required=True, help="Rule name in kebab-case, e.g. negate-of-equality"
+    )
     parser.add_argument("--description", required=True, help="One-line description for the rule")
     parser.add_argument("--regex", default="YOUR_PATTERN_HERE", help="Regex pattern to match")
     parser.add_argument("--message", help="Finding message (defaults to description)")
@@ -106,8 +110,12 @@ def main() -> int:
         default="Refactor for clarity and performance",
         help="Suggested fix shown alongside the finding",
     )
-    parser.add_argument("--bad", default="-- TODO: bad SQL example", help="SQL example that should fire")
-    parser.add_argument("--good", default="-- TODO: safe SQL example", help="SQL example that should not fire")
+    parser.add_argument(
+        "--bad", default="-- TODO: bad SQL example", help="SQL example that should fire"
+    )
+    parser.add_argument(
+        "--good", default="-- TODO: safe SQL example", help="SQL example that should not fire"
+    )
     args = parser.parse_args()
 
     code = args.code.upper()

sql_guard/checker.py

@@ -127,29 +127,35 @@ def check_file(
         try:
             content = path.read_text(encoding="latin-1")
         except Exception as e:
-            return [Finding(
+            return [
+                Finding(
+                    rule_id="SYS",
+                    severity="error",
+                    file=file_str,
+                    line=0,
+                    message=f"Cannot read file: {e}",
+                )
+            ]
+    except PermissionError:
+        return [
+            Finding(
                 rule_id="SYS",
                 severity="error",
                 file=file_str,
                 line=0,
-                message=f"Cannot read file: {e}",
-            )]
-    except PermissionError:
-        return [Finding(
-            rule_id="SYS",
-            severity="error",
-            file=file_str,
-            line=0,
-            message="Permission denied",
-        )]
+                message="Permission denied",
+            )
+        ]
     except OSError as e:
-        return [Finding(
-            rule_id="SYS",
-            severity="error",
-            file=file_str,
-            line=0,
-            message=f"Cannot read file: {e}",
-        )]
+        return [
+            Finding(
+                rule_id="SYS",
+                severity="error",
+                file=file_str,
+                line=0,
+                message=f"Cannot read file: {e}",
+            )
+        ]
 
     single_pass_rules = [r for r in rules if not r.multiline]
     multi_line_rules = [r for r in rules if r.multiline]

sql_guard/cli.py

@@ -30,9 +30,13 @@
 @app.command("check")
 def check_cmd(
     paths: list[str] = typer.Argument(default=None, help="Files or directories to check."),
-    severity: str = typer.Option("warning", "--severity", "-s", help="Minimum severity: error or warning."),
+    severity: str = typer.Option(
+        "warning", "--severity", "-s", help="Minimum severity: error or warning."
+    ),
     fail_fast: bool = typer.Option(False, "--fail-fast", help="Stop after first error."),
-    disable: Optional[list[str]] = typer.Option(None, "--disable", "-d", help="Rule IDs to disable."),
+    disable: Optional[list[str]] = typer.Option(
+        None, "--disable", "-d", help="Rule IDs to disable."
+    ),
     include_python: bool = typer.Option(
         False,
         "--include-python",
@@ -96,14 +100,10 @@ def check_cmd(
         try:
             contract = Contract.from_file(effective_contract_path)
         except FileNotFoundError:
-            console.print(
-                f"[red]Contract file not found:[/red] {effective_contract_path}"
-            )
+            console.print(f"[red]Contract file not found:[/red] {effective_contract_path}")
             raise typer.Exit(code=2)
         except Exception as exc:
-            console.print(
-                f"[red]Failed to load contract {effective_contract_path}:[/red] {exc}"
-            )
+            console.print(f"[red]Failed to load contract {effective_contract_path}:[/red] {exc}")
             raise typer.Exit(code=2)
 
     if changed_only:
@@ -200,12 +200,7 @@ def validate_contract_cmd(
 
     column_count = sum(len(t.columns) for t in contract.tables.values())
     pk_count = sum(len(t.primary_keys) for t in contract.tables.values())
-    fk_count = sum(
-        1
-        for t in contract.tables.values()
-        for c in t.columns.values()
-        if c.foreign_key
-    )
+    fk_count = sum(1 for t in contract.tables.values() for c in t.columns.values() if c.foreign_key)
 
     console.print(
         f"[green]OK[/green] {contract_path}: {table_count} tables, "
@@ -251,9 +246,7 @@ def schema_snapshot_cmd(
     from sql_guard import snapshot as snapshot_mod
 
     try:
-        data = snapshot_mod.introspect(
-            dsn=dsn, schema=schema, include_tables=include_table
-        )
+        data = snapshot_mod.introspect(dsn=dsn, schema=schema, include_tables=include_table)
     except snapshot_mod.SnapshotError as exc:
         console.print(f"[red]{exc}[/red]")
         raise typer.Exit(code=2)
@@ -264,9 +257,7 @@ def schema_snapshot_cmd(
     snapshot_mod.write_snapshot(data, output_path)
 
     table_count = len(data.get("tables") or {})
-    console.print(
-        f"[green]OK[/green] wrote {output_path} with {table_count} tables."
-    )
+    console.print(f"[green]OK[/green] wrote {output_path} with {table_count} tables.")
 
 
 @app.command()

sql_guard/fluent.py

@@ -67,10 +67,7 @@ def summary(self) -> str:
         if not parts:
             return f"no issues in {self.files_checked} file{'s' if self.files_checked != 1 else ''}"
         count_str = ", ".join(parts)
-        return (
-            f"{count_str} in "
-            f"{self.files_checked} file{'s' if self.files_checked != 1 else ''}"
-        )
+        return f"{count_str} in {self.files_checked} file{'s' if self.files_checked != 1 else ''}"
 
     def __len__(self) -> int:
         return len(self.findings)
@@ -123,7 +120,10 @@ def scan(self, sql_string: str) -> ScanResult:
         rules = get_rules(enabled_ids=self._enabled, disabled_ids=self._disabled)
 
         with tempfile.NamedTemporaryFile(
-            mode="w", suffix=".sql", delete=False, encoding="utf-8",
+            mode="w",
+            suffix=".sql",
+            delete=False,
+            encoding="utf-8",
         ) as tmp:
             tmp.write(sql_string)
             tmp_path = Path(tmp.name)

sql_guard/git_filter.py

@@ -54,9 +54,7 @@ def changed_files(base: str | None = None) -> set[Path] | None:
     return out
 
 
-def filter_to_changed(
-    discovered: list[Path], base: str | None = None
-) -> tuple[list[Path], bool]:
+def filter_to_changed(discovered: list[Path], base: str | None = None) -> tuple[list[Path], bool]:
     """Filter ``discovered`` to only the files git reports as changed.
 
     Returns ``(filtered_paths, used_git)``. When git isn't available the

sql_guard/python_scanner.py

@@ -175,7 +175,9 @@ def visit_Call(self, node: "cst.Call") -> None:  # noqa: N802 - libcst API
         if target is None:
             return
         if not (
-            isinstance(target, (cst.SimpleString, cst.ConcatenatedString, cst.FormattedString, cst.Name))
+            isinstance(
+                target, (cst.SimpleString, cst.ConcatenatedString, cst.FormattedString, cst.Name)
+            )
             or isinstance(target, cst.BinaryOperation)
             or _is_dot_format(target)
         ):

sql_guard/reporters/sarif.py

@@ -17,8 +17,7 @@
 
 INFO_URI = "https://github.com/Pawansingh3889/sql-guard"
 SARIF_SCHEMA = (
-    "https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/"
-    "sarif-schema-2.1.0.json"
+    "https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json"
 )
 
 
@@ -31,14 +30,16 @@ def _all_rule_descriptors() -> list[dict]:
     """Build the SARIF rule catalogue."""
     descriptors: list[dict] = []
     for rule in [*ALL_RULES, *PYTHON_RULES]:
-        descriptors.append({
-            "id": rule.id,
-            "name": rule.name,
-            "shortDescription": {"text": rule.description},
-            "fullDescription": {"text": rule.description},
-            "defaultConfiguration": {"level": _level(rule.severity)},
-            "helpUri": INFO_URI,
-        })
+        descriptors.append(
+            {
+                "id": rule.id,
+                "name": rule.name,
+                "shortDescription": {"text": rule.description},
+                "fullDescription": {"text": rule.description},
+                "defaultConfiguration": {"level": _level(rule.severity)},
+                "helpUri": INFO_URI,
+            }
+        )
     return descriptors
 
 
@@ -50,14 +51,16 @@ def build(result: CheckResult) -> dict:
             "ruleId": finding.rule_id,
             "level": _level(finding.severity),
             "message": {"text": finding.message},
-            "locations": [{
-                "physicalLocation": {
-                    "artifactLocation": {
-                        "uri": str(Path(finding.file).as_posix()),
+            "locations": [
+                {
+                    "physicalLocation": {
+                        "artifactLocation": {
+                            "uri": str(Path(finding.file).as_posix()),
+                        },
+                        "region": {"startLine": max(1, finding.line)},
                     },
-                    "region": {"startLine": max(1, finding.line)},
-                },
-            }],
+                }
+            ],
         }
         if finding.suggestion:
             sarif_result["message"]["text"] += f" -- {finding.suggestion}"
@@ -66,17 +69,19 @@ def build(result: CheckResult) -> dict:
     return {
         "$schema": SARIF_SCHEMA,
         "version": "2.1.0",
-        "runs": [{
-            "tool": {
-                "driver": {
-                    "name": "sql-guard",
-                    "version": __version__,
-                    "informationUri": INFO_URI,
-                    "rules": _all_rule_descriptors(),
+        "runs": [
+            {
+                "tool": {
+                    "driver": {
+                        "name": "sql-guard",
+                        "version": __version__,
+                        "informationUri": INFO_URI,
+                        "rules": _all_rule_descriptors(),
+                    },
                 },
-            },
-            "results": results,
-        }],
+                "results": results,
+            }
+        ],
     }
 
 

sql_guard/rules/contracts.py

@@ -84,8 +84,7 @@ def check_statement(self, statement: str, start_line: int, file: str) -> Finding
                         f"for table '{table_name}'"
                     ),
                     suggestion=(
-                        f"Either add '{ref_col}' to the contract or correct "
-                        f"the column name"
+                        f"Either add '{ref_col}' to the contract or correct the column name"
                     ),
                 )
         return None
@@ -162,9 +161,7 @@ def check_statement(self, statement: str, start_line: int, file: str) -> Finding
         if table is None:
             return None
 
-        listed_cols = {
-            c.strip().strip("[]`\"").lower() for c in m.group(2).split(",")
-        }
+        listed_cols = {c.strip().strip('[]`"').lower() for c in m.group(2).split(",")}
         missing = [c for c in table.required_columns if c not in listed_cols]
 
         if missing:
@@ -216,9 +213,7 @@ def check_statement(self, statement: str, start_line: int, file: str) -> Finding
         if table is None or not table.primary_keys:
             return None
 
-        listed_cols = {
-            c.strip().strip("[]`\"").lower() for c in m.group(2).split(",")
-        }
+        listed_cols = {c.strip().strip('[]`"').lower() for c in m.group(2).split(",")}
         missing_pks = [
             pk
             for pk in table.primary_keys
@@ -274,8 +269,11 @@ class UnmappedForeignKey(ContractRule):
     )
 
     def _fk_resolves(
-        self, source_table_name: str, source_col: str,
-        target_table_name: str, target_col: str,
+        self,
+        source_table_name: str,
+        source_col: str,
+        target_table_name: str,
+        target_col: str,
     ) -> bool:
         if self.contract is None:
             return False
@@ -287,8 +285,7 @@ def _fk_resolves(
             return False
         ref_table, _, ref_col = col.foreign_key.partition(".")
         return (
-            ref_table.lower() == target_table_name.lower()
-            and ref_col.lower() == target_col.lower()
+            ref_table.lower() == target_table_name.lower() and ref_col.lower() == target_col.lower()
         )
 
     def check_statement(self, statement: str, start_line: int, file: str) -> Finding | None: