Skip to content

Refactor rules to remove some false positives#48

Merged
vvcb merged 4 commits into
mainfrom
fix/ss-join
Jun 5, 2026
Merged

Refactor rules to remove some false positives#48
vvcb merged 4 commits into
mainfrom
fix/ss-join

Conversation

@sshenzha

Copy link
Copy Markdown
Collaborator
  • Audit-driven false-positive sweep across joins/, data_quality/, anti_patterns/, and concept_standardization/, plus one new rule and a small UI MCP improvement (a pill that shows if the MCP is on/off).
image

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR focuses on reducing audit-identified false positives across multiple SQL validation rules by making table/alias resolution more scope-aware (especially around CTE shadowing), adds a new anti-pattern rule for CTE names that collide with OMOP tables, and includes a small UI enhancement showing MCP mount status.

Changes:

  • Add anti_patterns.cte_shadows_omop_table and supporting helper collect_cte_names, plus tests and changelog updates.
  • Refine multiple rules to avoid false positives: CTE-shadow-aware has_table_reference, scope-aware alias resolution (schema validation / OMOP_149), LIMIT scalar-aggregate carve-out, visit-occurrence join-key gating, and IN-subquery CTE bridging for join-path validation.
  • UI: add an MCP on/off “pill” backed by the app’s live mcp_mounted state.

Reviewed changes

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

Show a summary per file
File Description
tests/test_rules.py Adds regression tests for CTE shadowing, alias scoping, scalar-aggregate LIMIT carve-out, visit join-key gating, and new rule coverage.
tests/test_patch.py Extends canonical suggested-fix verbs to include RENAME:.
tests/test_helpers_cte.py Adds focused tests for new CTE-aware helper behavior.
src/fastssv/rules/joins/visit_occurrence_inner_join_validation.py Gates OMOP_043 warnings on whether visit_occurrence_id is actually used in the join criteria.
src/fastssv/rules/joins/left_join_then_where_on_right_table.py Refactors OMOP_149 to evaluate LEFT-join/right-table filters per-SELECT scope to avoid cross-CTE leakage.
src/fastssv/rules/joins/join_path_validation.py Suppresses join-path warnings for canonical IN (SELECT ... FROM <vocab_cte>) cohort-builder bridging.
src/fastssv/rules/data_quality/comprehensive_schema_validation.py Makes column resolution scope-aware and skips sqlglot time-unit keywords parsed as Column nodes.
src/fastssv/rules/concept_standardization/standard_concept_enforcement.py Makes suggested-fix messaging CTE-shadow-aware (schema-qualified fix under shadow).
src/fastssv/rules/anti_patterns/limit_without_order_by.py Adds scalar-aggregate detection to suppress LIMIT-without-ORDER-BY warnings for single-row aggregates.
src/fastssv/rules/anti_patterns/cte_shadows_omop_table.py New anti-pattern rule warning on CTE names that shadow OMOP CDM tables.
src/fastssv/rules/anti_patterns/init.py Exposes the new anti-pattern rule via package imports/__all__.
src/fastssv/core/helpers.py Adds collect_cte_names and makes has_table_reference CTE-shadow-aware.
src/fastssv/api/ui.py Adds MCP mount-status fields to the UI template context.
src/fastssv/api/templates/index.html Renders an MCP on/off status pill.
src/fastssv/api/static/style.css Styles the MCP status pill.
CHANGELOG.md Documents the new rule and multiple false-positive reductions/fixes.

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

Comment thread src/fastssv/core/helpers.py
Comment thread src/fastssv/rules/anti_patterns/limit_without_order_by.py
Comment thread src/fastssv/rules/joins/visit_occurrence_inner_join_validation.py Outdated
@sshenzha sshenzha requested a review from vvcb May 11, 2026 14:02

@vvcb vvcb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this apply to sub queries as well as CTEs?

@sshenzha

Copy link
Copy Markdown
Collaborator Author

Should this apply to sub queries as well as CTEs?

yes, good catch. I have extended it

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread src/fastssv/rules/data_quality/comprehensive_schema_validation.py
@sshenzha sshenzha requested a review from vvcb May 12, 2026 09:33
@vvcb vvcb merged commit 17b475b into main Jun 5, 2026
13 checks passed
@vvcb vvcb deleted the fix/ss-join branch June 5, 2026 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants