Refine CTE-shadow rules and harden the API (rule isolation, limits, logging, release hygiene) - #49
Open
sshenzha wants to merge 6 commits into
Open
Refine CTE-shadow rules and harden the API (rule isolation, limits, logging, release hygiene)#49sshenzha wants to merge 6 commits into
sshenzha wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines OMOP validation semantics (notably CTE/table shadowing and concept-field enforcement) and hardens production operation of the FastSSV API/CLI through rule isolation, improved parsing/input-shape handling, stronger logging, and new operational limits.
Changes:
- Refines concept-standardization targeting (standard vs. source concept fields; default vs. strict mode) and adds schema-backed concept-field declarations.
- Hardens validation execution and API behavior (per-rule exception isolation, template short-circuiting, concurrency/rate-limit/proxy settings, CSP header, structured logging extras).
- Improves rule robustness against real-world OHDSI/Achilles SQL patterns (CTAS/DDL targets, ANALYZE, CAST-wrapped literals, comma-join connectivity, destructive-op shadow exemptions), with expanded tests/docs and release hygiene updates.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_schema_consistency.py | Adds CI guards for SOURCE_CONCEPT_FIELDS existence and disjointness from standard fields. |
| tests/test_rules.py | Updates/extends rule tests for new semantics (strict vs default), new rules, and multiple regressions. |
| tests/test_parse_sql.py | Adds template detection + short-circuit tests; expands valid SQL shapes (ANALYZE). |
| tests/test_logging.py | New unit tests ensuring JSONFormatter serializes all extra= fields. |
| tests/test_integration.py | Adds integration coverage for per-rule exception isolation surfacing meta violations. |
| tests/test_helpers_cte.py | Adds helper tests for CTE visibility and local-table collectors. |
| tests/test_cli.py | Ensures CLI rejects phantom categories and handles missing files without tracebacks. |
| tests/api/test_config.py | Adds coverage for new hardening settings defaults and env overrides. |
| tests/api/test_api.py | Adds CSP assertions, capacity 503 behavior, and health exemption from rate limiting. |
| src/fastssv/schemas/semantic_schema.py | Defines STANDARD_CONCEPT_FIELDS, SOURCE_CONCEPT_FIELDS, VOCABULARY_TABLES with updated contract docs. |
| src/fastssv/schemas/init.py | Re-exports new schema declarations (source fields + vocabulary tables). |
| src/fastssv/rules/joins/cost_table_domain_validation.py | Recognizes CAST/Paren-wrapped domain literals via unwrap_cast. |
| src/fastssv/rules/domain_specific/visit/visit_detail_visit_occurrence_reference.py | Removes old over-broad rule implementation (moved/narrowed). |
| src/fastssv/rules/domain_specific/visit/init.py | Drops old visit package import of the moved rule. |
| src/fastssv/rules/domain_specific/visit_detail/visit_detail_visit_occurrence_reference.py | New narrowed linkage-correctness version of CLIN_044 rule. |
| src/fastssv/rules/domain_specific/visit_detail/init.py | Exposes the moved/narrowed visit_detail linkage rule. |
| src/fastssv/rules/domain_specific/person/year_of_birth_age_arithmetic.py | Suppresses warnings under coarse age binning patterns. |
| src/fastssv/rules/domain_specific/cost/cost_event_id_polymorphic_resolution.py | Treats CAST/Paren-wrapped cost_domain_id literals as valid filters. |
| src/fastssv/rules/data_quality/comprehensive_schema_validation.py | Skips DDL/maintenance targets and supports cross-statement local tables. |
| src/fastssv/rules/concept_standardization/standard_concept_enforcement.py | Redesigns rule behavior around CDM v5.4 standard vs source concepts + vocabulary context. |
| src/fastssv/rules/anti_patterns/duplicate_column_alias.py | Suppresses NULL / CAST(NULL AS ...) placeholders in duplicate detection. |
| src/fastssv/rules/anti_patterns/destructive_operations_on_clinical_tables.py | Adds local-shadow exemption using context-provided local-unqualified tables. |
| src/fastssv/rules/anti_patterns/comma_separated_cross_join.py | Improves join connectivity detection (theta joins, function-wrapped cols, schema attribution). |
| src/fastssv/core/validation_context.py | Adds local table scoping to ValidationContext + context manager helper. |
| src/fastssv/core/logging.py | JSONFormatter now serializes all non-reserved extra= fields. |
| src/fastssv/core/helpers.py | Adds ANALYZE as valid statement, template detector, unwrap_cast, and local-table collectors. |
| src/fastssv/cli.py | Adds batch local-table scoping and tightens CLI behavior/arg choices. |
| src/fastssv/api/ui.py | Adds request_id logging + concurrency bounding for UI validation endpoint. |
| src/fastssv/api/routes.py | Passes request_id + semaphore into shared validation runner. |
| src/fastssv/api/config.py | Adds max-concurrent validations, rate-limit storage URI, and trusted proxy hosts settings. |
| src/fastssv/api/app.py | Adds CSP, rate-limit backend wiring, health exemption, proxy trust narrowing, and validation semaphore. |
| src/fastssv/api/_validation.py | Adds shared validation runner improvements (capacity gate, local-table context, request_id logging). |
| src/fastssv/init.py | Adds template short-circuiting and per-rule exception isolation; exports new rule IDs. |
| README.md | Updates rule count/version references. |
| pyproject.toml | Tightens ruff ignore list (removes F841 ignore). |
| docs/semantic_rules_guide.md | Updates registry counts. |
| docs/rules_reference.md | Adds new rule entry and updates rule docs/counts. |
| docs/plugin_architecture.md | Updates category and total rule counts. |
| docs/logging.md | Updates examples/counts and documents structured extra= emission. |
| docs/architecture.md | Updates category and total rule counts. |
| docs/api.md | Documents new API env vars and updates counts. |
| deploy/docker-compose.yml | Adds rate-limit backend env + concurrency limit + cgroup ceilings. |
| deploy/.env.example | Documents new env vars and container ceiling knobs. |
| CHANGELOG.md | Adds missing 0.3.0 section and documents new behavior changes. |
| AGENTS.md | Updates ruff ignore guidance. |
| .github/workflows/publish.yml | Makes wheel/sdist smoke test rule-count derive from source decorators. |
Comments suppressed due to low confidence (1)
src/fastssv/api/_validation.py:94
- The semaphore is used via
async with ...aroundasyncio.wait_for(...). Whenwait_fortimes out it cancels the awaitable but the underlyingto_threadwork continues; exiting theasync with semaphorereleases the permit immediately, so new validations can still be accepted while old timed-out parses keep pinning worker threads. Also,semaphore.locked()is race-prone for fail-fast behavior.
if semaphore is not None and semaphore.locked():
logger.warning(
"validation_capacity_exceeded",
extra={"sql_hash": _sql_hash(sql), "client": client, "request_id": request_id},
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
452
to
459
| __all__ = [ | ||
| # Main API | ||
| "validate_sql", | ||
| "validate_sql_structured", | ||
| "PARSE_ERROR_RULE_ID", | ||
| "NOT_SQL_RULE_ID", | ||
| "RULE_EXECUTION_ERROR_RULE_ID", | ||
| # Core classes |
Comment on lines
+200
to
+204
| # Same per-worker concurrency bound as /v1/validate: a timed-out parse | ||
| # keeps its thread busy, so refuse new work instead of pinning the pool. | ||
| semaphore = getattr(request.app.state, "validation_semaphore", None) | ||
| if semaphore is not None and semaphore.locked(): | ||
| logger.warning( |
Comment on lines
+94
to
+96
| if any( | ||
| isinstance(unwrap_cast(v), exp.Literal) and unwrap_cast(v).is_string for v in (node.expressions or []) | ||
| ): |
Comment on lines
+83
to
+88
| # Cross-statement scope: tables created elsewhere in the same submission | ||
| # are treated as known so per-statement schema validation doesn't flag | ||
| # intra-batch scratch tables as unknown OMOP tables. The unqualified | ||
| # subset separately gates the destructive-operations shadow exemption. | ||
| local_tables = collect_locally_defined_tables(sql, dialect) | ||
| local_unqualified = collect_locally_defined_unqualified_tables(sql, dialect) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
src/fastssv/api/ui.py:204
- The UI endpoint uses the same
semaphore.locked()pre-check pattern as the JSON API, which is racy (another request can consume the last permit between the check andasync with semaphore, causing this request to wait instead of returning 503). If the goal is load-shedding, acquire with a 0-timeout and release infinallyso saturation reliably fails fast.
# Same per-worker concurrency bound as /v1/validate: a timed-out parse
# keeps its thread busy, so refuse new work instead of pinning the pool.
semaphore = getattr(request.app.state, "validation_semaphore", None)
if semaphore is not None and semaphore.locked():
logger.warning(
src/fastssv/rules/domain_specific/cost/cost_event_id_polymorphic_resolution.py:96
- Minor inefficiency: in the
IN (...)branch,unwrap_cast(v)is evaluated twice per element, which is unnecessary and can add overhead on large IN lists. Compute the peeled value once per element (e.g. with an assignment expression) before checkingisinstance/is_string.
if any(
isinstance(unwrap_cast(v), exp.Literal) and unwrap_cast(v).is_string for v in (node.expressions or [])
):
src/fastssv/init.py:459
TEMPLATE_RULE_IDis introduced as a public constant (tests import it fromfastssv), but it isn’t included in__all__. If__all__is intended to define the supported public surface (as it already does forPARSE_ERROR_RULE_ID/NOT_SQL_RULE_ID), it should includeTEMPLATE_RULE_IDas well for consistency.
"validate_sql",
"validate_sql_structured",
"PARSE_ERROR_RULE_ID",
"NOT_SQL_RULE_ID",
"RULE_EXECUTION_ERROR_RULE_ID",
# Core classes
Comment on lines
+90
to
+94
| if semaphore is not None and semaphore.locked(): | ||
| logger.warning( | ||
| "validation_capacity_exceeded", | ||
| extra={"sql_hash": _sql_hash(sql), "client": client, "request_id": request_id}, | ||
| ) |
| if not standard_refs: | ||
| continue | ||
|
|
||
| vocab_in_scope = bool({normalize_name(t) for t in aliases.values() if t} & VOCABULARY_TABLES) |
Comment on lines
+297
to
+305
| # Skip DDL / maintenance targets — the table is being | ||
| # defined, dropped, altered, analyzed, or truncated here, | ||
| # not referenced. Patterns this covers: | ||
| # CREATE TABLE scratch.tmpach_0 AS SELECT … FROM cdm.person | ||
| # DROP TABLE scratch.tempResults_104 | ||
| # ANALYZE tempResults_104 | ||
| # OHDSI Achilles emits all three against a scratch namespace. | ||
| if isinstance(table.parent, _DDL_TARGET_PARENTS): | ||
| continue |
Comment on lines
+454
to
+472
| # Enforcement signals (shared across both branches). | ||
| has_standard_enforcement = _enforces_standard_concept(tree) | ||
| has_maps_to = _uses_maps_to_relationship(tree) | ||
| has_specific_filter = _has_specific_concept_id_filter(tree, aliases, standard_fields) | ||
| has_specific_filter = _has_specific_concept_id_filter(tree, aliases, all_concept_fields) | ||
| has_concept_ancestor_filter = _filters_via_concept_ancestor(tree, aliases, standard_fields) | ||
| has_concept_ancestor_join = _has_clinical_join_to_concept_ancestor(tree, aliases, standard_fields) | ||
| has_concept_ancestor_chain = _has_chained_join_to_concept_ancestor_via_concept( | ||
| tree, aliases, standard_fields | ||
| ) | ||
| any_concept_ancestor = ( | ||
| has_concept_ancestor_filter or has_concept_ancestor_join or has_concept_ancestor_chain | ||
| ) | ||
|
|
||
| # If no enforcement mechanism is present, warn | ||
| if ( | ||
| not has_standard_enforcement | ||
| and not has_maps_to | ||
| and not has_specific_filter | ||
| and not has_concept_ancestor_filter | ||
| and not has_concept_ancestor_join | ||
| and not has_concept_ancestor_chain | ||
| ): | ||
| # Check strict mode for severity escalation | ||
| from fastssv.core.validation_context import get_validation_context | ||
|
|
||
| ctx = get_validation_context() | ||
| severity = Severity.ERROR if ctx.should_escalate_rule(self.rule_id) else Severity.WARNING | ||
|
|
||
| message = "Query uses STANDARD concept fields without ensuring concepts are standard." | ||
| if severity == Severity.ERROR: | ||
| message += " (Strict mode: cohort definitions must use standard concepts)" | ||
|
|
||
| # CTE-shadow aware suggested fix: if the user has a CTE named | ||
| # `concept` (or `concept_relationship`) *at the top level of | ||
| # the statement*, the default `JOIN concept c ...` suggestion | ||
| # — applied at that same top level — would resolve to that | ||
| # CTE, which has no `standard_concept` column, and break at | ||
| # execution time. Switch to the schema-qualified form and | ||
| # flag the shadow so the user sees the actual root cause. | ||
| # | ||
| # Scope deliberately restricted to the *top-level* WITH: | ||
| # CTEs defined inside a nested subquery (e.g. inside an IN / | ||
| # EXISTS / FROM-derived) are lexically out of scope for a | ||
| # JOIN added at the outer SELECT, so the generic fix is | ||
| # already executable in that case. The broader tree-global | ||
| # "any matching CTE anywhere" signal is handled by the | ||
| # `anti_patterns.cte_shadows_omop_table` rule independently. | ||
| top_with = tree.args.get("with_") or tree.args.get("with") | ||
| top_cte_names: Set[str] = set() | ||
| if top_with is not None: | ||
| for top_cte in top_with.expressions or []: | ||
| if top_cte.alias: | ||
| top_cte_names.add(normalize_name(top_cte.alias)) | ||
| shadow = top_cte_names & {"concept", "concept_relationship"} | ||
| if shadow: | ||
| shadow_list = ", ".join(sorted(shadow)) | ||
| suggested_fix = ( | ||
| "ADD: `JOIN omop.concept c ON c.concept_id = <table>.<concept_id_col>` " | ||
| "AND `WHERE c.standard_concept = 'S'` to filter to standard concepts. " | ||
| f"NOTE: this query has a CTE named `{shadow_list}` which shadows the OMOP " | ||
| "vocabulary table — the JOIN must be schema-qualified (`omop.concept`) " | ||
| "or the CTE renamed, otherwise the JOIN would bind to the CTE and the " | ||
| "`standard_concept` column would not exist." | ||
| ) | ||
| else: | ||
| suggested_fix = ( | ||
| "ADD: `JOIN concept c ON c.concept_id = <table>.<concept_id_col>` " | ||
| "AND `WHERE c.standard_concept = 'S'` to filter to standard concepts." | ||
| ) | ||
|
|
||
| # --- Branch 1: source-concept fire (always-on, default + strict) --- | ||
| # Source concepts are CDM-defined as the pre-mapping layer. | ||
| # Analytical use without (a) ``Maps to`` mapping, (b) a specific | ||
| # literal filter, or (c) a concept_ancestor pattern feeding into | ||
| # it, mixes vocabulary layers silently. | ||
| if source_refs and not (has_maps_to or has_specific_filter or any_concept_ancestor): |
Comment on lines
203
to
207
| { | ||
| "status": "ok", | ||
| "version": "0.2.0", | ||
| "rules_loaded": 154 | ||
| "rules_loaded": 155 | ||
| } |
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
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.
1. CTE-shadow rule refinements (first 5 commits)
anti_patterns.cte_shadows_omop_table: warns when a CTE alias or derived-table subquery alias collides with an OMOP CDM table name, with scope-aware handling so nested/sibling scopes don't false-positive.2. Production-hardening sweep (last commit), from a full repo audit
meta.rule_execution_errorWARNING instead of aborting the batch (library/CLI) or 500ing the API. NewRULE_EXECUTION_ERROR_RULE_IDexport.JSONFormatternow serializes everyextra=field (was a 3-field allowlist that silently dropped).FASTSSV_API_MAX_CONCURRENT_VALIDATIONS(503 +Retry-Afterfail-fast; timed-out sqlglot parses can't be cancelled, so this stops thread-pool pinning),FASTSSV_API_RATE_LIMIT_STORAGE_URI(shared cross-worker rate-limit backend),FASTSSV_API_TRUSTED_PROXY_HOSTS(narrows the previous trust-any-peerX-Forwarded-Forhandling), aContent-Security-Policyheader,/v1/healthexempted from rate limiting, and theHTTPExceptionhandler now propagates attached headers.--categorieschoices (analytics,performance,schema) that matched zero rules and silently reported VALID.mem_limit/cpus/pids_limit) in compose, new env vars documented in.env.example.[0.3.0] - 2026-05-06changelog section (content verified identical to the tag).