Repository navigation
feat(contracts): add contract-driven DB repository schema and validat… - #463
Merged
jonahgabriel merged 9 commits intoFeb 1, 2026
Conversation
…ors (OMN-1782) Implement v1 of the contract-driven database repository system: Contract Models: - ModelDbParam: Parameter definition with type-based constraints - ModelDbReturn: Return type (model_ref + many cardinality) - ModelDbSafetyPolicy: Opt-in flags for dangerous operations - ModelDbOperation: SQL operation with mode, params, returns, safety_policy - ModelDbRepositoryContract: Full repository contract with table access control Validators (src/omnibase_core/validation/db/): - validator_db_structural: Required fields, valid identifiers, single statements - validator_db_sql_safety: Mode/verb matching, DDL blocking, WHERE requirements - validator_db_table_access: Tables in SQL ⊆ declared tables (fail-closed) - validator_db_deterministic: ORDER BY for many=true and LIMIT/OFFSET - validator_db_params: Named params only (:param), no positional ($N) Key design decisions: - Named parameters only (positional deferred to OMN-1789) - No return.fields for v1 (projection validation deferred to OMN-1790) - Regex-based SQL parsing with fail-closed on CTEs/subqueries - Safety policy opt-ins for DELETE/UPDATE without WHERE Includes 40 unit tests and example YAML contract.
Contributor
📝 WalkthroughWalkthroughAdds typed DB repository contract models, SQL preprocessing utilities, five DB validators with consolidated exports, example contracts and docs, and comprehensive unit tests for validators and SQL utils. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as FileRegistry / Loader
participant Contract as ModelDbRepositoryContract
participant SqlUtils as sql_utils
participant Validators as DB Validators
participant Reporter as ModelValidationResult
Loader->>Contract: load YAML -> instantiate ModelDbRepositoryContract
Contract->>SqlUtils: normalize_sql / strip_sql_strings (per op)
Validators->>SqlUtils: call utilities for parsing/stripping
Contract->>Validators: pass contract to each validator
Validators->>Validator: structural, params, sql_safety, table_access, deterministic
Validators->>Reporter: aggregate errors/summaries -> return ModelValidationResult
Reporter-->>Loader: validation result (valid/invalid + messages)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@examples/contracts/README.md`:
- Line 38: Replace the malformed Markdown table separator
"|------|-------------|" (and the similar occurrences at the other noted lines)
with properly spaced Markdown table separator rows that include at least three
dashes per column and a single space padding around pipes, e.g. change to "| ---
| --- |" (or match column widths like "| ----- | ------------- |") so the table
conforms to MD060 spacing rules.
In `@src/omnibase_core/validation/db/validator_db_deterministic.py`:
- Around line 106-121: _strip_sql_strings currently uses naive regexes that
break on escaped quotes (e.g., O''Reilly) and may leak keywords; update the
regexes in _strip_sql_strings to match SQL-escaped quotes inside literals (for
single quotes use a pattern that allows doubled single-quotes like '' as
escaping, and for double-quoted identifiers allow doubled double-quotes) so
string/identifier contents are consumed entirely before replacement; keep using
re.sub to remove matched literals but replace the simple r"'[^']*'" and
r'"[^"]*"' with patterns that accept escaped quotes.
In `@src/omnibase_core/validation/db/validator_db_params.py`:
- Around line 22-23: The named-parameter regex (_NAMED_PARAM_PATTERN)
incorrectly matches the second colon in PostgreSQL casts like ::int; update the
pattern to ignore colons that are immediately preceded by another colon by using
a negative lookbehind (e.g., require the leading ":" not be preceded by ":") so
casts are not treated as parameters; update the _NAMED_PARAM_PATTERN definition
accordingly and ensure any place relying on _NAMED_PARAM_PATTERN (and helper
_strip_sql_strings) continues to work with the new regex.
In `@src/omnibase_core/validation/db/validator_db_sql_safety.py`:
- Around line 110-116: The current multi-statement detection uses a simple "if
';' in sql_without_strings" which wrongly flags single statements with a
trailing semicolon; replace that check in validator_db_sql_safety.py (around the
code using sql_without_strings, op.safety_policy.allow_multi_statement,
errors.append and op_name) and in validator_db_structural.py (the analogous
check around sql_without_strings/op_name/errors.append) with a regex that treats
only internal semicolons as multi-statement delimiters and ignores a single
trailing semicolon (e.g. trim trailing semicolons or use a pattern that looks
for semicolons followed by non-whitespace); ensure behavior still respects
op.safety_policy.allow_multi_statement and append the same error message only
when a true multi-statement is detected.
In `@src/omnibase_core/validation/db/validator_db_structural.py`:
- Around line 82-121: The multi-statement detection can be fooled by semicolons
in comments or in string literals with escaped quotes; update the logic used by
the multi-statement check (the block that checks
op.safety_policy.allow_multi_statement and calls _strip_sql_strings) to first
remove SQL comments and then use a more robust string literal stripper: modify
_strip_sql_strings to strip single-line (--) and block (/* ... */) comments
before removing strings, and update the single-quoted and double-quoted regexes
to handle escaped quotes (e.g., use patterns that allow backslash or
doubled-quote escapes) so semicolons inside comments or escaped quotes won't
trigger the multiple-statement error.
In `@src/omnibase_core/validation/db/validator_db_table_access.py`:
- Around line 82-95: The current logic uses table.split(".")[-1] which strips
schema and allows schema-qualified names to bypass the allowlist; update the
check in the loop that iterates referenced_tables (from _extract_tables) to not
drop schema: normalize the full referenced table to lowercase (e.g.,
referenced_table = table.lower()) and compare that full name against
allowed_tables (instead of using table_name), and if the referenced table
contains a schema qualifier (contains ".") but the allowlist only contains
unqualified names, treat that as disallowed (append to errors with op_name and
contract.tables) or require explicit schema-qualified entries in allowed_tables
— fail closed on any schema-qualified reference unless it exactly matches an
allowed schema-qualified entry.
- Around line 143-159: The current _strip_sql_strings function removes
double-quoted tokens which strips quoted identifiers like "users" or
"public"."users" and allows table references to evade validation; change
_strip_sql_strings to only strip single-quoted string literals (leave
double-quoted tokens intact) and add detection for any remaining double-quoted
identifiers so the validator fails closed: update _strip_sql_strings to stop
removing double-quoted content (preserve or replace with a safe placeholder) and
add a check in the validator that scans the returned SQL for double-quoted
identifiers (e.g., presence of " or patterns like "schema"."table") and
raise/return an error when found so quoted identifiers are either handled
explicitly by the extractor or cause validation to fail.
🧹 Nitpick comments (3)
src/omnibase_core/models/contracts/model_db_return.py (1)
1-8: Shorten the module docstring to stay within 3–6 lines.Suggested change
-"""Database return type definition for repository contracts. - -Declares shape tightly enough that: -- Infra can deserialize deterministically -- Domains can bind to a stable interface - -v1: Simple model_ref + many. No fields validation (deferred to OMN-1790). -""" +"""Repository contract return type definition. + +Defines model_ref + many for v1 contracts. +No return.fields validation until OMN-1790. +"""As per coding guidelines, Module docstrings must be concise (3-6 lines max for simple modules).
src/omnibase_core/models/contracts/model_db_param.py (1)
1-8: Shorten the module docstring to stay within 3–6 lines.Suggested change
-"""Database parameter definition for repository contracts. - -Fully specifies each parameter so validators can prove: -- Every placeholder has a definition -- Every definition is actually used -- Constraints are type-consistent -- Defaults are legal -""" +"""Repository contract parameter definition. + +Specifies constraints and defaults for validator checks. +"""As per coding guidelines, Module docstrings must be concise (3-6 lines max for simple modules).
tests/unit/validation/db/test_db_validators.py (1)
114-149: Add@pytest.mark.unitmarkers to test classes.Per coding guidelines, test files must use pytest markers. All test classes in this file are missing the
@pytest.mark.unitmarker.♻️ Proposed fix for TestGoldenContract (apply similar pattern to other classes)
+@pytest.mark.unit class TestGoldenContract: """Test that the golden contract passes all validators."""Apply the same marker to all other test classes:
TestValidatorDbStructural,TestValidatorDbSqlSafety,TestValidatorDbTableAccess,TestValidatorDbDeterministic,TestValidatorDbParams, andTestEdgeCases.
| ## Other Contract Examples | ||
|
|
||
| | File | Description | | ||
| |------|-------------| |
Contributor
There was a problem hiding this comment.
Fix markdownlint MD060 table spacing.
Suggested change
-|------|-------------|
+| ------ | ------------- |
@@
-|------|-------------|-------------|
+| ------ | ------------- | ------------- |
@@
-|-------|-------|-----|
+| ------- | ------- | ----- |Also applies to: 89-89, 100-100
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 38-38: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 38-38: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
[warning] 38-38: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 38-38: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@examples/contracts/README.md` at line 38, Replace the malformed Markdown
table separator "|------|-------------|" (and the similar occurrences at the
other noted lines) with properly spaced Markdown table separator rows that
include at least three dashes per column and a single space padding around
pipes, e.g. change to "| --- | --- |" (or match column widths like "| ----- |
------------- |") so the table conforms to MD060 spacing rules.
Comment on lines
+82
to
+121
| # Validate SQL is single statement unless allow_multi_statement is True | ||
| if not op.safety_policy.allow_multi_statement: | ||
| sql_without_strings = _strip_sql_strings(op.sql) | ||
| if ";" in sql_without_strings: | ||
| errors.append( | ||
| f"Operation '{op_name}': SQL contains multiple statements. " | ||
| "Use single statement per operation or set " | ||
| "safety_policy.allow_multi_statement=True." | ||
| ) | ||
|
|
||
| if errors: | ||
| return ModelValidationResult.create_invalid( | ||
| errors=errors, | ||
| summary=f"Structural validation failed with {len(errors)} error(s)", | ||
| ) | ||
|
|
||
| return ModelValidationResult.create_valid( | ||
| summary=( | ||
| f"Structural validation passed for contract '{contract.name}' " | ||
| f"with {len(contract.ops)} operation(s)" | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def _strip_sql_strings(sql: str) -> str: | ||
| """Remove string literals from SQL to avoid false positives in pattern matching. | ||
|
|
||
| Strips both single-quoted string literals and double-quoted identifiers | ||
| to prevent semicolons within strings from triggering multi-statement detection. | ||
|
|
||
| Args: | ||
| sql: The SQL string to process. | ||
|
|
||
| Returns: | ||
| SQL with all string literals removed. | ||
| """ | ||
| # Remove single-quoted strings (handles escaped quotes via non-greedy match) | ||
| sql = re.sub(r"'[^']*'", "", sql) | ||
| # Remove double-quoted identifiers | ||
| sql = re.sub(r'"[^"]*"', "", sql) |
Contributor
There was a problem hiding this comment.
Normalize SQL before the multi-statement check.
Semicolons in comments (or in strings with escaped quotes) will be misdetected as multi-statement. Strip comments before checking and use a more robust string-literal regex.
🛠️ Suggested fix
- if not op.safety_policy.allow_multi_statement:
- sql_without_strings = _strip_sql_strings(op.sql)
+ if not op.safety_policy.allow_multi_statement:
+ normalized_sql = _normalize_sql(op.sql)
+ sql_without_strings = _strip_sql_strings(normalized_sql)
if ";" in sql_without_strings:
errors.append(
f"Operation '{op_name}': SQL contains multiple statements. "
"Use single statement per operation or set "
"safety_policy.allow_multi_statement=True."
)
@@
def _strip_sql_strings(sql: str) -> str:
@@
- # Remove single-quoted strings (handles escaped quotes via non-greedy match)
- sql = re.sub(r"'[^']*'", "", sql)
- # Remove double-quoted identifiers
- sql = re.sub(r'"[^"]*"', "", sql)
+ # Remove single-quoted strings (handles escaped quotes via doubled '')
+ sql = re.sub(r"'(?:[^']|'')*'", "", sql)
+ # Remove double-quoted identifiers (handles escaped quotes via doubled "")
+ sql = re.sub(r'"(?:[^"]|"")*"', "", sql)
return sql
+
+
+def _normalize_sql(sql: str) -> str:
+ """Normalize SQL by stripping comments and collapsing whitespace."""
+ sql = re.sub(r"--.*$", "", sql, flags=re.MULTILINE)
+ sql = re.sub(r"/\*.*?\*/", "", sql, flags=re.DOTALL)
+ sql = " ".join(sql.split())
+ return sql.strip()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Validate SQL is single statement unless allow_multi_statement is True | |
| if not op.safety_policy.allow_multi_statement: | |
| sql_without_strings = _strip_sql_strings(op.sql) | |
| if ";" in sql_without_strings: | |
| errors.append( | |
| f"Operation '{op_name}': SQL contains multiple statements. " | |
| "Use single statement per operation or set " | |
| "safety_policy.allow_multi_statement=True." | |
| ) | |
| if errors: | |
| return ModelValidationResult.create_invalid( | |
| errors=errors, | |
| summary=f"Structural validation failed with {len(errors)} error(s)", | |
| ) | |
| return ModelValidationResult.create_valid( | |
| summary=( | |
| f"Structural validation passed for contract '{contract.name}' " | |
| f"with {len(contract.ops)} operation(s)" | |
| ), | |
| ) | |
| def _strip_sql_strings(sql: str) -> str: | |
| """Remove string literals from SQL to avoid false positives in pattern matching. | |
| Strips both single-quoted string literals and double-quoted identifiers | |
| to prevent semicolons within strings from triggering multi-statement detection. | |
| Args: | |
| sql: The SQL string to process. | |
| Returns: | |
| SQL with all string literals removed. | |
| """ | |
| # Remove single-quoted strings (handles escaped quotes via non-greedy match) | |
| sql = re.sub(r"'[^']*'", "", sql) | |
| # Remove double-quoted identifiers | |
| sql = re.sub(r'"[^"]*"', "", sql) | |
| # Validate SQL is single statement unless allow_multi_statement is True | |
| if not op.safety_policy.allow_multi_statement: | |
| normalized_sql = _normalize_sql(op.sql) | |
| sql_without_strings = _strip_sql_strings(normalized_sql) | |
| if ";" in sql_without_strings: | |
| errors.append( | |
| f"Operation '{op_name}': SQL contains multiple statements. " | |
| "Use single statement per operation or set " | |
| "safety_policy.allow_multi_statement=True." | |
| ) | |
| if errors: | |
| return ModelValidationResult.create_invalid( | |
| errors=errors, | |
| summary=f"Structural validation failed with {len(errors)} error(s)", | |
| ) | |
| return ModelValidationResult.create_valid( | |
| summary=( | |
| f"Structural validation passed for contract '{contract.name}' " | |
| f"with {len(contract.ops)} operation(s)" | |
| ), | |
| ) | |
| def _strip_sql_strings(sql: str) -> str: | |
| """Remove string literals from SQL to avoid false positives in pattern matching. | |
| Strips both single-quoted string literals and double-quoted identifiers | |
| to prevent semicolons within strings from triggering multi-statement detection. | |
| Args: | |
| sql: The SQL string to process. | |
| Returns: | |
| SQL with all string literals removed. | |
| """ | |
| # Remove single-quoted strings (handles escaped quotes via doubled '') | |
| sql = re.sub(r"'(?:[^']|'')*'", "", sql) | |
| # Remove double-quoted identifiers (handles escaped quotes via doubled "") | |
| sql = re.sub(r'"(?:[^"]|"")*"', "", sql) | |
| return sql | |
| def _normalize_sql(sql: str) -> str: | |
| """Normalize SQL by stripping comments and collapsing whitespace.""" | |
| sql = re.sub(r"--.*$", "", sql, flags=re.MULTILINE) | |
| sql = re.sub(r"/\*.*?\*/", "", sql, flags=re.DOTALL) | |
| sql = " ".join(sql.split()) | |
| return sql.strip() |
🤖 Prompt for AI Agents
In `@src/omnibase_core/validation/db/validator_db_structural.py` around lines 82 -
121, The multi-statement detection can be fooled by semicolons in comments or in
string literals with escaped quotes; update the logic used by the
multi-statement check (the block that checks
op.safety_policy.allow_multi_statement and calls _strip_sql_strings) to first
remove SQL comments and then use a more robust string literal stripper: modify
_strip_sql_strings to strip single-line (--) and block (/* ... */) comments
before removing strings, and update the single-quoted and double-quoted regexes
to handle escaped quotes (e.g., use patterns that allow backslash or
doubled-quote escapes) so semicolons inside comments or escaped quotes won't
trigger the multiple-statement error.
…ts (OMN-1782) - Create shared _sql_utils.py module with normalize_sql() and strip_sql_strings() - Standardize escaped quote handling (supports both \' and '' conventions) - Remove duplicate helper functions from 5 validators (~150 lines) - Add __all__ exports to validator_db_deterministic, validator_db_sql_safety, and validator_db_structural - Add 3 new test cases for escaped quote edge cases Code review follow-up: addresses duplicate code and inconsistent string literal handling identified in PR #463 review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_core/validation/db/_sql_utils.py`:
- Around line 25-55: normalize_sql currently uses re.sub(r"--.*$", ...) which is
not quote-aware and strips text when `--` appears inside string literals;
replace that regex-based single-line comment removal with a quote-aware scanner
inside normalize_sql that iterates the SQL string, tracks entering/exiting
single- and double-quoted strings (and escaped quotes), and only treats `--` as
the start of a comment when not inside any string; apply the same approach (or
ensure) for block comments `/* ... */` so they are ignored inside strings, then
continue to collapse whitespace as before; reference the normalize_sql function
and remove the unsafe re.sub usages for single- and multi-line comment
stripping.
🧹 Nitpick comments (7)
src/omnibase_core/validation/db/_sql_utils.py (1)
1-18: Trim the module docstring to 3–6 lines.This module is small (two utilities), so the long docstring + example exceeds the concise-docstring guideline; consider moving examples into function docstrings.
As per coding guidelines: Module docstrings must be concise (3-6 lines max for simple modules); follow PEP 257 with module docstring first, then from future import annotations.💡 Example trim
-"""Shared SQL utility functions for database repository validators. - -This module provides common SQL processing utilities used across multiple -database validators. These functions handle SQL normalization and string -literal removal to enable accurate pattern matching in SQL validation. - -Functions: - normalize_sql: Remove comments and collapse whitespace from SQL. - strip_sql_strings: Remove string literals to prevent false positives. - -Example: - >>> from omnibase_core.validation.db._sql_utils import normalize_sql, strip_sql_strings - >>> sql = "SELECT * FROM users -- get all users" - >>> normalized = normalize_sql(sql) - >>> "SELECT * FROM users" - >>> clean = strip_sql_strings("SELECT * FROM t WHERE name = 'DROP TABLE'") - >>> # Returns: "SELECT * FROM t WHERE name = " -""" +"""Shared SQL utilities for validator normalization and string stripping."""src/omnibase_core/validation/db/validator_db_structural.py (1)
1-10: Keep the module docstring to 3–6 lines.This file is a small validator module; consider moving the longer explanation into function docstrings or docs.
As per coding guidelines: Module docstrings must be concise (3-6 lines max for simple modules); follow PEP 257 with module docstring first, then from future import annotations.💡 Example trim
-"""Structural validation for DB repository contracts. - -Validates: -- Table names are valid SQL identifiers -- Operation names are valid identifiers -- SQL statements are single statements (unless allow_multi_statement=True) - -Note: Basic field constraints (name, engine, database_ref, mode) are enforced -by Pydantic model validation. This validator adds semantic validation rules. -""" +"""Structural validation for DB repository contracts (identifiers, single-statement SQL)."""src/omnibase_core/validation/db/validator_db_table_access.py (1)
1-8: Keep the module docstring to 3–6 lines.This is a small validator module; consider shortening the header docstring to meet the concise-docstring guideline.
As per coding guidelines: Module docstrings must be concise (3-6 lines max for simple modules); follow PEP 257 with module docstring first, then from future import annotations.💡 Example trim
-"""Table access validation for DB repository contracts. - -Validates: -- Tables referenced in SQL are subset of declared tables -- Fail-closed on unrecognized SQL patterns (CTEs, subqueries) - -Uses regex-based extraction with conservative matching. -""" +"""Table access validation for DB repository contracts (fail-closed regex extraction)."""src/omnibase_core/validation/db/validator_db_deterministic.py (1)
1-9: Shorten the module docstring to 3–6 lines.Consider condensing the header docstring and leaving detailed rationale in the function docstring.
As per coding guidelines: Module docstrings must be concise (3-6 lines max for simple modules); follow PEP 257 with module docstring first, then from future import annotations.💡 Example trim
-"""Deterministic ordering validator for DB repository contracts. - -Validates: -- Queries returning many=True have ORDER BY clause -- Queries using LIMIT/OFFSET have ORDER BY clause - -This prevents non-deterministic result ordering that would -make tests flaky and behavior unpredictable. -""" +"""Deterministic ordering validator for DB repository contracts."""src/omnibase_core/validation/db/validator_db_sql_safety.py (1)
1-9: Shorten the module docstring to 3–6 lines.This header is longer than the concise-docstring guideline; consider keeping only the essentials.
As per coding guidelines: Module docstrings must be concise (3-6 lines max for simple modules); follow PEP 257 with module docstring first, then from future import annotations.💡 Example trim
-"""SQL safety validation for DB repository contracts. - -Validates: -- read operations use SELECT only -- write operations use INSERT/UPDATE/DELETE -- DDL statements (CREATE, DROP, ALTER, TRUNCATE, GRANT, REVOKE) are forbidden -- DELETE/UPDATE without WHERE requires safety policy opt-in -- Multi-statement SQL requires safety policy opt-in -""" +"""SQL safety validation for DB repository contracts."""tests/unit/validation/db/test_db_validators.py (2)
1-13: Trim the module docstring to 3–6 lines.The current header docstring is longer than the concise-docstring guideline; consider moving details into section comments.
As per coding guidelines: Module docstrings must be concise (3-6 lines max for simple modules); follow PEP 257 with module docstring first, then from future import annotations.💡 Example trim
-"""Unit tests for DB repository contract validators. - -This module tests all five DB repository validators: -- validate_db_structural: Required fields, valid identifiers, single statements -- validate_db_sql_safety: read->SELECT, forbid DDL, safety policy -- validate_db_table_access: Tables in SQL <= declared tables -- validate_db_deterministic: many=True -> ORDER BY required -- validate_db_params: Named params validation - -Each validator has: -1. A golden contract test showing it passes -2. One or more failure cases showing it correctly rejects invalid contracts -""" +"""Unit tests for DB repository contract validators."""
15-33: Add pytest markers for this test module.Tests under
tests/**should carry markers; a module‑level marker keeps it simple and consistent.
As per coding guidelines: tests/**/*.py: Maintain 60%+ test coverage and use 12,000+ tests with markers (@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow,@pytest.mark.smoke,@pytest.mark.performance).🛠️ Suggested change
import pytest @@ from omnibase_core.validation.db import ( validate_db_deterministic, validate_db_params, validate_db_sql_safety, validate_db_structural, validate_db_table_access, ) + +pytestmark = pytest.mark.unit
Add .venv, venv, .tox, and site-packages to skip list in validate-file-locations.py to prevent scanning third-party packages.
…ators (OMN-1782) Code review fixes: - Remove duplicate multi-statement check from validator_db_structural (now only in validator_db_sql_safety to avoid duplicate errors) - Fix named param regex to exclude PostgreSQL type casts (::integer) using negative lookbehind (?<!:) - Rename _sql_utils.py to sql_utils.py (public API, not internal) - Add documentation about case-insensitive table matching in README - Add test for type cast exclusion in parameter validation - Move multi-statement tests to TestValidatorDbSqlSafety class
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/validation/db/test_db_validators.py`:
- Around line 885-903: The test is calling the wrong validator: replace the call
to validate_db_structural with validate_db_sql_safety (and update the test name
if desired) so the semicolon-in-string-literal behavior is checked by the
SQL-safety validator; locate the test function
test_semicolon_in_string_literal_ignored and change the validation call to
validate_db_sql_safety(contract), keeping the same contract setup and assertion
on result.is_valid.
🧹 Nitpick comments (1)
tests/unit/validation/db/test_db_validators.py (1)
1018-1039: Test assumes backslash escape handling that may not be implemented.Standard PostgreSQL uses doubled single quotes (
'') for escaping within strings, not backslash escapes. Backslash escapes only work with theE'...'syntax or whenstandard_conforming_stringsis OFF (deprecated since PostgreSQL 9.1).The SQL
'foo\'bar'would either be a syntax error or treat\as a literal character in standard PostgreSQL. This test passes, but likely because the string stripping doesn't treat\'as an escape sequence—the validators may be parsing this incorrectly but not failing visibly.Consider either:
- Removing this test if backslash escapes aren't intended to be supported
- Using the PostgreSQL-standard E-string syntax if testing escape handling:
E'foo\'bar'- Adding a comment clarifying this tests robustness against non-standard input
Merge origin/main into feature branch, combining: - DB repository contract models (OMN-1782) - this branch - Handler routing models (OMN-1295) - from main - Virtual environment exclusions in validation script
MAJOR fixes: - sql_utils.py: Rewrite normalize_sql() to extract strings FIRST, then strip comments, preventing false positives on comment-like patterns inside string literals - validator_db_sql_safety.py: Strip trailing semicolons before multi-statement check to allow single statements ending with `;` - validator_db_table_access.py: Add quoted identifier extraction and schema-qualified table matching (both simple and exact match modes) - validator_db_deterministic.py: Add 5 tests for escaped quote handling MINOR fixes: - test_db_validators.py: Fix test calling wrong validator after multi-statement check was moved to sql_safety - Add @pytest.mark.unit markers to all test classes NITPICK fixes: - Shorten all module docstrings to 4 lines (within 3-6 line guideline) Test coverage: - Added 36 new tests (89 total, was 53) - New test_sql_utils.py with 27 tests for SQL normalization - All tests pass, mypy strict passes
- Add @pytest.mark.unit markers to test_sql_utils.py test classes - Add regex length limit (128 chars) for defense-in-depth against ReDoS - Document quoted identifier case sensitivity in contracts README - Document future engine extension path in ModelDbRepositoryContract
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_core/validation/db/validator_db_table_access.py`:
- Around line 95-148: The code currently lowercases all table names via
_is_table_allowed so quoted (double-quoted) identifiers become case-insensitive;
change the check to preserve case for quoted identifiers and keep
case-insensitive behavior for unquoted ones: when building
allowed_exact/allowed_simple continue to store unquoted/simple names lowercased,
but do NOT lowercase names returned from _extract_quoted_tables (leave them
as-is) and modify _is_table_allowed (or overload it) to accept a flag (e.g.,
quoted: bool) so that if quoted==True it compares the quoted identifier against
allowed_exact/allowed_simple using exact (case-sensitive) matching, and if
quoted==False it lowercases the input and does case-insensitive matching against
allowed_simple/allowed_exact; update the loop that iterates referenced_tables
(or call sites for quoted_tables vs unquoted_tables) to call _is_table_allowed
with the correct quoted flag for quoted_tables vs unquoted_tables.
🧹 Nitpick comments (2)
src/omnibase_core/models/contracts/model_db_repository_contract.py (1)
1-12: Shorten the module docstring to 3–6 lines.This module docstring exceeds the allowed length for simple modules; please condense it.
As per coding guidelines: Module docstrings must be concise (3-6 lines max for simple modules).
✂️ Example condensation
-"""Database repository contract definition. - -Defines a complete repository contract including: -- Repository identification -- Database connection reference -- Allowed tables (enforced by validators) -- Model mappings -- Named operations - -This eliminates per-domain Postgres adapters by making -data access a first-class, governed contract. -""" +"""Database repository contract definition. + +Defines the repository identity, allowed tables, model mappings, +and named operations for governed data access. +"""src/omnibase_core/validation/db/validator_db_table_access.py (1)
1-13: Shorten the module docstring to 3–6 lines.This module docstring exceeds the allowed length for simple modules.
As per coding guidelines: Module docstrings must be concise (3-6 lines max for simple modules).
Comment on lines
+95
to
+148
| # Build two sets for different matching strategies | ||
| # Schema-qualified tables require exact match, simple tables match any schema prefix | ||
| allowed_exact: set[str] = set() # Schema-qualified (e.g., "public.users") | ||
| allowed_simple: set[str] = set() # Simple names (e.g., "users") | ||
|
|
||
| for table in contract.tables: | ||
| table_lower = table.lower() | ||
| if "." in table: | ||
| allowed_exact.add(table_lower) | ||
| else: | ||
| allowed_simple.add(table_lower) | ||
|
|
||
| for op_name, op in contract.ops.items(): | ||
| normalized_sql = normalize_sql(op.sql) | ||
|
|
||
| # Strip all strings (single and double quoted) for unsupported pattern checks | ||
| sql_without_all_strings = strip_sql_strings(normalized_sql) | ||
|
|
||
| # Check for unsupported patterns (fail closed) | ||
| unsupported_found = False | ||
| for pattern, description in _UNSUPPORTED_PATTERNS: | ||
| if pattern.search(sql_without_all_strings): | ||
| errors.append( | ||
| f"Operation '{op_name}': SQL contains {description} which cannot be " | ||
| "reliably validated. Use simple table references or implement in v2 (OMN-1791)." | ||
| ) | ||
| unsupported_found = True | ||
| break # Only report first unsupported pattern per operation | ||
|
|
||
| if unsupported_found: | ||
| continue | ||
|
|
||
| # Extract quoted identifiers from normalized SQL (before any stripping) | ||
| quoted_tables = _extract_quoted_tables(normalized_sql) | ||
|
|
||
| # Strip only single-quoted strings for unquoted table extraction | ||
| # This preserves double-quoted identifiers so aliases after them | ||
| # aren't mistakenly matched as table names | ||
| sql_without_string_literals = _strip_single_quoted_strings(normalized_sql) | ||
|
|
||
| # Extract unquoted table references | ||
| unquoted_tables = _extract_tables(sql_without_string_literals) | ||
|
|
||
| # Combine all referenced tables | ||
| referenced_tables = unquoted_tables | quoted_tables | ||
|
|
||
| # Check each table against allowed list | ||
| for table in referenced_tables: | ||
| if not _is_table_allowed(table, allowed_exact, allowed_simple): | ||
| errors.append( | ||
| f"Operation '{op_name}': Table '{table}' is not in allowed tables list. " | ||
| f"Allowed: {sorted(contract.tables)}" | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Quoted identifiers are matched case-insensitively (bypasses allowlist semantics).
All table comparisons are lowercased in _is_table_allowed, so "Users" and "users" are treated the same. In Postgres, quoted identifiers are case-sensitive, and the README documents exact matching. This broadens access beyond the declared table names.
Please keep case-sensitive comparisons for quoted identifiers and case-insensitive comparisons for unquoted identifiers.
🛠️ Proposed fix (case-sensitive for quoted identifiers)
@@
- allowed_exact: set[str] = set() # Schema-qualified (e.g., "public.users")
- allowed_simple: set[str] = set() # Simple names (e.g., "users")
+ allowed_exact: set[str] = set() # Schema-qualified (case-sensitive)
+ allowed_simple: set[str] = set() # Simple names (case-sensitive)
+ allowed_exact_lower: set[str] = set() # Schema-qualified (lowercased)
+ allowed_simple_lower: set[str] = set() # Simple names (lowercased)
@@
- table_lower = table.lower()
- if "." in table:
- allowed_exact.add(table_lower)
- else:
- allowed_simple.add(table_lower)
+ table_lower = table.lower()
+ if "." in table:
+ allowed_exact.add(table)
+ allowed_exact_lower.add(table_lower)
+ else:
+ allowed_simple.add(table)
+ allowed_simple_lower.add(table_lower)
@@
- referenced_tables = unquoted_tables | quoted_tables
-
- # Check each table against allowed list
- for table in referenced_tables:
- if not _is_table_allowed(table, allowed_exact, allowed_simple):
- errors.append(
- f"Operation '{op_name}': Table '{table}' is not in allowed tables list. "
- f"Allowed: {sorted(contract.tables)}"
- )
+ # Check quoted tables (case-sensitive exact match)
+ for table in quoted_tables:
+ if not _is_quoted_table_allowed(table, allowed_exact, allowed_simple):
+ errors.append(
+ f"Operation '{op_name}': Table '{table}' is not in allowed tables list. "
+ f"Allowed: {sorted(contract.tables)}"
+ )
+
+ # Check unquoted tables (case-insensitive)
+ for table in unquoted_tables:
+ if not _is_table_allowed(table, allowed_exact_lower, allowed_simple_lower):
+ errors.append(
+ f"Operation '{op_name}': Table '{table}' is not in allowed tables list. "
+ f"Allowed: {sorted(contract.tables)}"
+ )
@@
-def _is_table_allowed(
+def _is_table_allowed(
table: str,
- allowed_exact: set[str],
- allowed_simple: set[str],
+ allowed_exact: set[str],
+ allowed_simple: set[str],
) -> bool:
@@
table_lower = table.lower()
@@
else:
@@
return False
+
+
+def _is_quoted_table_allowed(
+ table: str,
+ allowed_exact: set[str],
+ allowed_simple: set[str],
+) -> bool:
+ """Quoted identifiers must match declared table names exactly (case-sensitive)."""
+ return table in allowed_exact or table in allowed_simpleAlso applies to: 246-289
🤖 Prompt for AI Agents
In `@src/omnibase_core/validation/db/validator_db_table_access.py` around lines 95
- 148, The code currently lowercases all table names via _is_table_allowed so
quoted (double-quoted) identifiers become case-insensitive; change the check to
preserve case for quoted identifiers and keep case-insensitive behavior for
unquoted ones: when building allowed_exact/allowed_simple continue to store
unquoted/simple names lowercased, but do NOT lowercase names returned from
_extract_quoted_tables (leave them as-is) and modify _is_table_allowed (or
overload it) to accept a flag (e.g., quoted: bool) so that if quoted==True it
compares the quoted identifier against allowed_exact/allowed_simple using exact
(case-sensitive) matching, and if quoted==False it lowercases the input and does
case-insensitive matching against allowed_simple/allowed_exact; update the loop
that iterates referenced_tables (or call sites for quoted_tables vs
unquoted_tables) to call _is_table_allowed with the correct quoted flag for
quoted_tables vs unquoted_tables.
…-1782) Replace Literal["postgres"] with EnumDatabaseEngine enum supporting postgres, mysql, and sqlite database engines. - Add EnumDatabaseEngine with helper methods (supports_schemas, supports_returning, get_param_style) - Update ModelDbRepositoryContract to use the new enum - Update all 62 tests to use EnumDatabaseEngine.POSTGRES - Update README to document supported engines
…alidator (OMN-1782) - Update _IDENTIFIER_PATTERN to accept schema.table format (e.g., public.users) - Add tests for schema-qualified names, edge cases (.users, users., a.b.c) - Add test coverage for table aliases (FROM patterns p) - Add test coverage for self-joins (same table with different aliases) Addresses code review feedback from PR #463.
This was referenced Feb 5, 2026
jonahgabriel
deleted the
jonah/omn-1782-core-contract-driven-db-repository-schema-validators
branch
April 4, 2026 02:04
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.
…ors (OMN-1782)
Implement v1 of the contract-driven database repository system:
Contract Models:
Validators (src/omnibase_core/validation/db/):
Key design decisions:
Includes 40 unit tests and example YAML contract.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.