Repository navigation
feat(OMN-1783): Implement PostgresRepositoryRuntime for contract-driven database access - #225
Merged
Merged
Conversation
…en database access Adds a generic contract executor for ModelDbRepositoryContract from omnibase_core. Key Features: - Typed exception hierarchy (RepositoryConfigError, RepositoryExecutionError, etc.) - ModelRepositoryRuntimeConfig for runtime configuration - PostgresRepositoryRuntime core implementation with: - Read/write operation mode support - Determinism enforcement via ORDER BY injection - LIMIT injection for multi-row queries - Positional parameter binding ($1, $2, ...) - Timeout enforcement via asyncio.wait_for() Testing: - 24 unit tests covering all behaviors - 10 integration tests against real PostgreSQL Dependencies: - Bumps omnibase-core to ^0.12.0 for ModelDbRepositoryContract - Bumps version to 0.2.9
📝 WalkthroughWalkthroughAdds a Postgres repository runtime and runtime config, a repository-specific error hierarchy, validation constant updates and exemptions, an omnibase-core dependency bump, lint ignore entries, and extensive unit and integration tests. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Runtime as PostgresRepositoryRuntime
participant Contract as ModelDbRepositoryContract
participant SQL as SQLBuilder
participant DB as Database
participant Err as ErrorHandler
Client->>Runtime: call(op_name, *args)
Runtime->>Contract: fetch_operation(op_name)
Contract-->>Runtime: ModelDbOperation
Runtime->>Runtime: validate_mode(operation)
Runtime->>Runtime: validate_arg_count(*args)
Runtime->>SQL: ensure_order_by_and_limit(sql, config)
SQL-->>Runtime: sql_with_order_and_limit
Runtime->>DB: execute_with_timeout(sql, *args)
alt Timeout
DB-->>Err: TimeoutError
Err-->>Runtime: RepositoryTimeoutError
else Execution Error
DB-->>Err: DatabaseError
Err-->>Runtime: RepositoryExecutionError
else Success
DB-->>Runtime: rows
end
Runtime->>Runtime: shape_result(rows, operation)
Runtime-->>Client: list[dict] | dict | None
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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/db/__init__.py`:
- Around line 34-53: Update the docstring example to use the correct
ModelDbOperation API: change mode="select" to mode="read" (or "write" where
appropriate) and replace params=["user_id"] with a mapping of parameter names to
ModelDbParam instances (e.g., params={"user_id": ModelDbParam(...)}), keeping
ModelDbReturn as-is; ensure the example still demonstrates creating
ModelDbRepositoryContract, initializing PostgresRepositoryRuntime, and calling
runtime.call("find_by_id", 123) so it matches the actual constructors/types
(ModelDbRepositoryContract, ModelDbOperation, ModelDbParam, ModelDbReturn,
PostgresRepositoryRuntime.call).
In `@src/omnibase_infra/runtime/db/models/__init__.py`:
- Around line 9-15: Update the module docstring for ModelRepositoryRuntimeConfig
to reference the actual field names (allowed_modes and allow_write_operations)
instead of the nonexistent allow_raw_operations and allow_delete_operations;
adjust the brief descriptions to match the semantics of allowed_modes (operation
allowlisting) and allow_write_operations (write/delete permission flag) and
ensure any other listed fields (e.g., max_row_limit, timeout_ms,
primary_key_column, default_order_by, metrics config) remain accurate and
consistent with the ModelRepositoryRuntimeConfig definition.
In `@src/omnibase_infra/runtime/db/models/model_repository_runtime_config.py`:
- Around line 21-31: Update the module docstring example for
ModelRepositoryRuntimeConfig to use the actual model fields: replace uses of
allowed_ops with allowed_modes and allow_raw_operations with emit_metrics (or
set emit_metrics to appropriate boolean), and ensure any membership checks
reference valid modes (e.g., "read" or "write") rather than "delete"; keep
max_row_limit and timeout_ms as shown and show correct printed values for the
real fields so the example matches the ModelRepositoryRuntimeConfig class
definition.
In `@src/omnibase_infra/runtime/db/postgres_repository_runtime.py`:
- Around line 29-49: Update the docstring example to use the real API: change
ModelDbOperation's mode from "select" to "read" and supply params as a dict
rather than a list (e.g., use params={"user_id": ...}), and ensure the
ModelDbReturn and PostgresRepositoryRuntime usage (and the
runtime.call("find_by_id", ...)) match the actual signatures; adjust the example
invocation and parameter shape accordingly so it compiles with ModelDbOperation,
ModelDbReturn, and PostgresRepositoryRuntime.
In
`@tests/integration/runtime/db/test_postgres_repository_runtime_integration.py`:
- Around line 46-54: The get_dsn function currently embeds hardcoded sensitive
defaults (POSTGRES_HOST and POSTGRES_PASSWORD); change its default values to
non-sensitive placeholders (e.g., POSTGRES_HOST default to "localhost" or "" and
POSTGRES_PASSWORD default to an obvious placeholder like "REQUIRED_OVERRIDE" or
empty string) and update the docstring or comment to state that these
environment variables must be provided for tests; ensure you only read
environment variables POSTGRES_HOST, POSTGRES_PORT, POSTGRES_DATABASE,
POSTGRES_USER, POSTGRES_PASSWORD and construct the DSN the same way so CI/devs
must explicitly override the placeholders when running tests.
🧹 Nitpick comments (3)
tests/integration/runtime/db/test_postgres_repository_runtime_integration.py (1)
72-102: Test table not cleaned up after test run.The test table is created but never dropped, leaving test artifacts in the database. Consider adding a module-level teardown or
autousefixture to clean up.🧹 Proposed cleanup fixture
`@pytest.fixture`(scope="module", autouse=True) async def cleanup_test_table(db_pool: asyncpg.Pool): """Clean up test table after all tests in module complete.""" yield async with db_pool.acquire() as conn: await conn.execute(f"DROP TABLE IF EXISTS {_TEST_TABLE_NAME}")src/omnibase_infra/runtime/db/postgres_repository_runtime.py (2)
78-80: SQL regex patterns have edge-case limitations.The regex patterns won't correctly handle
ORDER BYorLIMITinside subqueries, string literals, or comments. Since contract SQL is developer-authored (not user input), this is acceptable, but worth documenting in the class docstring for awareness.
485-494: Docstring overstates fingerprint sanitization.The docstring says "removes potentially sensitive values" but the implementation only truncates. Since parameter values aren't in the SQL (uses $1 placeholders), this is fine, but the docstring should be clarified.
📝 Proposed docstring fix
def _fingerprint_sql(self, sql: str) -> str: - """Create a safe fingerprint of SQL for logging/errors. - - Truncates long SQL and removes potentially sensitive values. - """ + """Create a fingerprint of SQL for logging/errors. + + Truncates long SQL to a reasonable length for diagnostics. + Note: Parameter values are not in the SQL (uses $1 placeholders). + """
Comment on lines
+34
to
+53
| >>> contract = ModelDbRepositoryContract( | ||
| ... name="users", | ||
| ... database_ref="primary", | ||
| ... ops={ | ||
| ... "find_by_id": ModelDbOperation( | ||
| ... mode="select", | ||
| ... sql="SELECT * FROM users WHERE id = $1", | ||
| ... params=["user_id"], | ||
| ... returns=ModelDbReturn(many=False), | ||
| ... ), | ||
| ... }, | ||
| ... ) | ||
| >>> | ||
| >>> # Create runtime with pool | ||
| >>> pool = await asyncpg.create_pool(...) | ||
| >>> runtime = PostgresRepositoryRuntime(pool, contract) | ||
| >>> | ||
| >>> # Execute operation | ||
| >>> user = await runtime.call("find_by_id", 123) | ||
| """ |
There was a problem hiding this comment.
Docstring example uses incorrect API.
The example shows mode="select" and params=["user_id"], but the actual ModelDbOperation expects mode="read" (or "write") and params as a dict[str, ModelDbParam]. This will cause errors if users copy-paste this example.
📝 Proposed fix
>>> # Define contract
>>> contract = ModelDbRepositoryContract(
... name="users",
... database_ref="primary",
+ ... engine="postgres",
+ ... tables=["users"],
+ ... models={"User": "dict"},
... ops={
... "find_by_id": ModelDbOperation(
- ... mode="select",
+ ... mode="read",
... sql="SELECT * FROM users WHERE id = $1",
- ... params=["user_id"],
+ ... params={"user_id": ModelDbParam(name="user_id", param_type="integer")},
... returns=ModelDbReturn(many=False),
... ),
... },
... )📝 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
| >>> contract = ModelDbRepositoryContract( | |
| ... name="users", | |
| ... database_ref="primary", | |
| ... ops={ | |
| ... "find_by_id": ModelDbOperation( | |
| ... mode="select", | |
| ... sql="SELECT * FROM users WHERE id = $1", | |
| ... params=["user_id"], | |
| ... returns=ModelDbReturn(many=False), | |
| ... ), | |
| ... }, | |
| ... ) | |
| >>> | |
| >>> # Create runtime with pool | |
| >>> pool = await asyncpg.create_pool(...) | |
| >>> runtime = PostgresRepositoryRuntime(pool, contract) | |
| >>> | |
| >>> # Execute operation | |
| >>> user = await runtime.call("find_by_id", 123) | |
| """ | |
| >>> contract = ModelDbRepositoryContract( | |
| ... name="users", | |
| ... database_ref="primary", | |
| ... engine="postgres", | |
| ... tables=["users"], | |
| ... models={"User": "dict"}, | |
| ... ops={ | |
| ... "find_by_id": ModelDbOperation( | |
| ... mode="read", | |
| ... sql="SELECT * FROM users WHERE id = $1", | |
| ... params={"user_id": ModelDbParam(name="user_id", param_type="integer")}, | |
| ... returns=ModelDbReturn(many=False), | |
| ... ), | |
| ... }, | |
| ... ) | |
| >>> | |
| >>> # Create runtime with pool | |
| >>> pool = await asyncpg.create_pool(...) | |
| >>> runtime = PostgresRepositoryRuntime(pool, contract) | |
| >>> | |
| >>> # Execute operation | |
| >>> user = await runtime.call("find_by_id", 123) |
🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/db/__init__.py` around lines 34 - 53, Update the
docstring example to use the correct ModelDbOperation API: change mode="select"
to mode="read" (or "write" where appropriate) and replace params=["user_id"]
with a mapping of parameter names to ModelDbParam instances (e.g.,
params={"user_id": ModelDbParam(...)}), keeping ModelDbReturn as-is; ensure the
example still demonstrates creating ModelDbRepositoryContract, initializing
PostgresRepositoryRuntime, and calling runtime.call("find_by_id", 123) so it
matches the actual constructors/types (ModelDbRepositoryContract,
ModelDbOperation, ModelDbParam, ModelDbReturn, PostgresRepositoryRuntime.call).
Comment on lines
+9
to
+15
| Exports: | ||
| ModelRepositoryRuntimeConfig: Configuration for PostgresRepositoryRuntime | ||
| - Safety constraints (max_row_limit, timeout_ms) | ||
| - Operation allowlisting (select, insert, update, upsert) | ||
| - Feature flags (allow_raw_operations, allow_delete_operations) | ||
| - Determinism controls (primary_key_column, default_order_by) | ||
| - Metrics emission configuration |
There was a problem hiding this comment.
Docstring references non-existent fields.
Lines 13-14 mention allow_raw_operations and allow_delete_operations, but the actual ModelRepositoryRuntimeConfig has allowed_modes and allow_write_operations.
📝 Proposed fix
Exports:
ModelRepositoryRuntimeConfig: Configuration for PostgresRepositoryRuntime
- Safety constraints (max_row_limit, timeout_ms)
- - Operation allowlisting (select, insert, update, upsert)
- - Feature flags (allow_raw_operations, allow_delete_operations)
+ - Operation mode allowlisting (read, write)
+ - Feature flags (allow_write_operations)
- Determinism controls (primary_key_column, default_order_by)
- Metrics emission configuration📝 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
| Exports: | |
| ModelRepositoryRuntimeConfig: Configuration for PostgresRepositoryRuntime | |
| - Safety constraints (max_row_limit, timeout_ms) | |
| - Operation allowlisting (select, insert, update, upsert) | |
| - Feature flags (allow_raw_operations, allow_delete_operations) | |
| - Determinism controls (primary_key_column, default_order_by) | |
| - Metrics emission configuration | |
| Exports: | |
| ModelRepositoryRuntimeConfig: Configuration for PostgresRepositoryRuntime | |
| - Safety constraints (max_row_limit, timeout_ms) | |
| - Operation mode allowlisting (read, write) | |
| - Feature flags (allow_write_operations) | |
| - Determinism controls (primary_key_column, default_order_by) | |
| - Metrics emission configuration |
🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/db/models/__init__.py` around lines 9 - 15, Update
the module docstring for ModelRepositoryRuntimeConfig to reference the actual
field names (allowed_modes and allow_write_operations) instead of the
nonexistent allow_raw_operations and allow_delete_operations; adjust the brief
descriptions to match the semantics of allowed_modes (operation allowlisting)
and allow_write_operations (write/delete permission flag) and ensure any other
listed fields (e.g., max_row_limit, timeout_ms, primary_key_column,
default_order_by, metrics config) remain accurate and consistent with the
ModelRepositoryRuntimeConfig definition.
Comment on lines
+21
to
+31
| Example: | ||
| >>> config = ModelRepositoryRuntimeConfig( | ||
| ... max_row_limit=100, | ||
| ... timeout_ms=5000, | ||
| ... allowed_ops={"select", "insert"}, | ||
| ... ) | ||
| >>> print(config.allow_raw_operations) | ||
| False | ||
| >>> print("delete" in config.allowed_ops) | ||
| False | ||
| """ |
There was a problem hiding this comment.
Module docstring example references non-existent fields.
The example shows allowed_ops and allow_raw_operations, but the actual model defines allowed_modes and emit_metrics. This will confuse users.
📝 Proposed fix for the docstring example
Example:
- >>> config = ModelRepositoryRuntimeConfig(
- ... max_row_limit=100,
- ... timeout_ms=5000,
- ... allowed_ops={"select", "insert"},
- ... )
- >>> print(config.allow_raw_operations)
- False
- >>> print("delete" in config.allowed_ops)
- False
+ >>> config = ModelRepositoryRuntimeConfig(
+ ... max_row_limit=100,
+ ... timeout_ms=5000,
+ ... allowed_modes=frozenset({"read", "write"}),
+ ... )
+ >>> print(config.emit_metrics)
+ True
+ >>> print("write" in config.allowed_modes)
+ True📝 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
| Example: | |
| >>> config = ModelRepositoryRuntimeConfig( | |
| ... max_row_limit=100, | |
| ... timeout_ms=5000, | |
| ... allowed_ops={"select", "insert"}, | |
| ... ) | |
| >>> print(config.allow_raw_operations) | |
| False | |
| >>> print("delete" in config.allowed_ops) | |
| False | |
| """ | |
| Example: | |
| >>> config = ModelRepositoryRuntimeConfig( | |
| ... max_row_limit=100, | |
| ... timeout_ms=5000, | |
| ... allowed_modes=frozenset({"read", "write"}), | |
| ... ) | |
| >>> print(config.emit_metrics) | |
| True | |
| >>> print("write" in config.allowed_modes) | |
| True | |
| """ |
🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/db/models/model_repository_runtime_config.py`
around lines 21 - 31, Update the module docstring example for
ModelRepositoryRuntimeConfig to use the actual model fields: replace uses of
allowed_ops with allowed_modes and allow_raw_operations with emit_metrics (or
set emit_metrics to appropriate boolean), and ensure any membership checks
reference valid modes (e.g., "read" or "write") rather than "delete"; keep
max_row_limit and timeout_ms as shown and show correct printed values for the
real fields so the example matches the ModelRepositoryRuntimeConfig class
definition.
Comment on lines
+29
to
+49
| >>> # Create contract | ||
| >>> contract = ModelDbRepositoryContract( | ||
| ... name="users", | ||
| ... database_ref="primary", | ||
| ... ops={ | ||
| ... "find_by_id": ModelDbOperation( | ||
| ... mode="select", | ||
| ... sql="SELECT * FROM users WHERE id = $1", | ||
| ... params=["user_id"], | ||
| ... returns=ModelDbReturn(many=False), | ||
| ... ), | ||
| ... }, | ||
| ... ) | ||
| >>> | ||
| >>> # Create runtime (with pool) | ||
| >>> pool = await asyncpg.create_pool(...) | ||
| >>> runtime = PostgresRepositoryRuntime(pool, contract) | ||
| >>> | ||
| >>> # Execute operation | ||
| >>> user = await runtime.call("find_by_id", 123) | ||
| """ |
There was a problem hiding this comment.
Docstring example uses incorrect API.
Similar to the package __init__.py, this example shows mode="select" and params=["user_id"] which don't match the actual API (mode="read" and params as a dict).
📝 Proposed fix
>>> # Create contract
>>> contract = ModelDbRepositoryContract(
... name="users",
... database_ref="primary",
+ ... engine="postgres",
+ ... tables=["users"],
+ ... models={"User": "dict"},
... ops={
... "find_by_id": ModelDbOperation(
- ... mode="select",
+ ... mode="read",
... sql="SELECT * FROM users WHERE id = $1",
- ... params=["user_id"],
- ... returns=ModelDbReturn(many=False),
+ ... params={"user_id": ModelDbParam(name="user_id", param_type="integer")},
+ ... returns=ModelDbReturn(model_ref="User", many=False),
... ),
... },
... )📝 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
| >>> # Create contract | |
| >>> contract = ModelDbRepositoryContract( | |
| ... name="users", | |
| ... database_ref="primary", | |
| ... ops={ | |
| ... "find_by_id": ModelDbOperation( | |
| ... mode="select", | |
| ... sql="SELECT * FROM users WHERE id = $1", | |
| ... params=["user_id"], | |
| ... returns=ModelDbReturn(many=False), | |
| ... ), | |
| ... }, | |
| ... ) | |
| >>> | |
| >>> # Create runtime (with pool) | |
| >>> pool = await asyncpg.create_pool(...) | |
| >>> runtime = PostgresRepositoryRuntime(pool, contract) | |
| >>> | |
| >>> # Execute operation | |
| >>> user = await runtime.call("find_by_id", 123) | |
| """ | |
| >>> # Create contract | |
| >>> contract = ModelDbRepositoryContract( | |
| ... name="users", | |
| ... database_ref="primary", | |
| ... engine="postgres", | |
| ... tables=["users"], | |
| ... models={"User": "dict"}, | |
| ... ops={ | |
| ... "find_by_id": ModelDbOperation( | |
| ... mode="read", | |
| ... sql="SELECT * FROM users WHERE id = $1", | |
| ... params={"user_id": ModelDbParam(name="user_id", param_type="integer")}, | |
| ... returns=ModelDbReturn(model_ref="User", many=False), | |
| ... ), | |
| ... }, | |
| ... ) | |
| >>> | |
| >>> # Create runtime (with pool) | |
| >>> pool = await asyncpg.create_pool(...) | |
| >>> runtime = PostgresRepositoryRuntime(pool, contract) | |
| >>> | |
| >>> # Execute operation | |
| >>> user = await runtime.call("find_by_id", 123) | |
| """ |
🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/db/postgres_repository_runtime.py` around lines 29
- 49, Update the docstring example to use the real API: change
ModelDbOperation's mode from "select" to "read" and supply params as a dict
rather than a list (e.g., use params={"user_id": ...}), and ensure the
ModelDbReturn and PostgresRepositoryRuntime usage (and the
runtime.call("find_by_id", ...)) match the actual signatures; adjust the example
invocation and parameter shape accordingly so it compiles with ModelDbOperation,
ModelDbReturn, and PostgresRepositoryRuntime.
…RepositoryRuntime - Change primary_key_column default from "pattern_id" to None (fail-fast) - Change default_order_by default from "score DESC, pattern_id ASC" to None - Add cleanup fixture to drop test table after integration tests complete - Change db_pool fixture to module scope to support cleanup fixture
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tests/integration/runtime/db/test_postgres_repository_runtime_integration.py`:
- Around line 403-438: The test
TestLearnedPatternsTable.test_query_learned_patterns_empty should not assume the
learned_patterns table is empty; instead, modify the assertion to validate
invariants about the returned rows from runtime.call("find_validated") — e.g.,
assert results is a list and for each row (returned model type from
ModelDbReturn "LearnedPattern") that row["status"] == "validated" (and
optionally other invariants like keys/id types or length <=
config.max_row_limit); alternatively, seed or clean the table before running the
test using the db_pool fixture so the test has a deterministic state.
Comment on lines
+403
to
+438
| class TestLearnedPatternsTable: | ||
| """Test against the real learned_patterns table schema.""" | ||
|
|
||
| async def test_query_learned_patterns_empty(self, db_pool: asyncpg.Pool): | ||
| """Test querying the actual learned_patterns table.""" | ||
| contract = ModelDbRepositoryContract( | ||
| name="learned_patterns_repo", | ||
| engine="postgres", | ||
| database_ref="omninode_bridge", | ||
| tables=["learned_patterns"], | ||
| models={"LearnedPattern": "dict"}, | ||
| ops={ | ||
| "find_validated": ModelDbOperation( | ||
| mode="read", | ||
| sql=""" | ||
| SELECT id, pattern_signature, domain_id, confidence, status | ||
| FROM learned_patterns | ||
| WHERE status = $1 | ||
| ORDER BY confidence DESC, id ASC | ||
| """, | ||
| params={"status": ModelDbParam(name="status", param_type="string")}, | ||
| returns=ModelDbReturn(model_ref="LearnedPattern", many=True), | ||
| safety_policy=ModelDbSafetyPolicy(), | ||
| ), | ||
| }, | ||
| ) | ||
|
|
||
| config = ModelRepositoryRuntimeConfig(max_row_limit=10) | ||
| runtime = PostgresRepositoryRuntime(db_pool, contract, config) | ||
|
|
||
| # Query for validated patterns (table is empty, so expect empty list) | ||
| results = await runtime.call("find_validated", "validated") | ||
|
|
||
| assert isinstance(results, list) | ||
| # Table is empty based on our earlier check | ||
| assert len(results) == 0 |
There was a problem hiding this comment.
Avoid assuming learned_patterns is empty.
This assertion will fail in any environment where the table already has data. Prefer asserting invariants (e.g., all returned rows are status == "validated") or seed/cleanup the table in the test DB.
✅ Suggested assertion change
- # Table is empty based on our earlier check
- assert len(results) == 0
+ assert all(r["status"] == "validated" for r in results)📝 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
| class TestLearnedPatternsTable: | |
| """Test against the real learned_patterns table schema.""" | |
| async def test_query_learned_patterns_empty(self, db_pool: asyncpg.Pool): | |
| """Test querying the actual learned_patterns table.""" | |
| contract = ModelDbRepositoryContract( | |
| name="learned_patterns_repo", | |
| engine="postgres", | |
| database_ref="omninode_bridge", | |
| tables=["learned_patterns"], | |
| models={"LearnedPattern": "dict"}, | |
| ops={ | |
| "find_validated": ModelDbOperation( | |
| mode="read", | |
| sql=""" | |
| SELECT id, pattern_signature, domain_id, confidence, status | |
| FROM learned_patterns | |
| WHERE status = $1 | |
| ORDER BY confidence DESC, id ASC | |
| """, | |
| params={"status": ModelDbParam(name="status", param_type="string")}, | |
| returns=ModelDbReturn(model_ref="LearnedPattern", many=True), | |
| safety_policy=ModelDbSafetyPolicy(), | |
| ), | |
| }, | |
| ) | |
| config = ModelRepositoryRuntimeConfig(max_row_limit=10) | |
| runtime = PostgresRepositoryRuntime(db_pool, contract, config) | |
| # Query for validated patterns (table is empty, so expect empty list) | |
| results = await runtime.call("find_validated", "validated") | |
| assert isinstance(results, list) | |
| # Table is empty based on our earlier check | |
| assert len(results) == 0 | |
| class TestLearnedPatternsTable: | |
| """Test against the real learned_patterns table schema.""" | |
| async def test_query_learned_patterns_empty(self, db_pool: asyncpg.Pool): | |
| """Test querying the actual learned_patterns table.""" | |
| contract = ModelDbRepositoryContract( | |
| name="learned_patterns_repo", | |
| engine="postgres", | |
| database_ref="omninode_bridge", | |
| tables=["learned_patterns"], | |
| models={"LearnedPattern": "dict"}, | |
| ops={ | |
| "find_validated": ModelDbOperation( | |
| mode="read", | |
| sql=""" | |
| SELECT id, pattern_signature, domain_id, confidence, status | |
| FROM learned_patterns | |
| WHERE status = $1 | |
| ORDER BY confidence DESC, id ASC | |
| """, | |
| params={"status": ModelDbParam(name="status", param_type="string")}, | |
| returns=ModelDbReturn(model_ref="LearnedPattern", many=True), | |
| safety_policy=ModelDbSafetyPolicy(), | |
| ), | |
| }, | |
| ) | |
| config = ModelRepositoryRuntimeConfig(max_row_limit=10) | |
| runtime = PostgresRepositoryRuntime(db_pool, contract, config) | |
| # Query for validated patterns (table is empty, so expect empty list) | |
| results = await runtime.call("find_validated", "validated") | |
| assert isinstance(results, list) | |
| assert all(r["status"] == "validated" for r in results) |
🤖 Prompt for AI Agents
In `@tests/integration/runtime/db/test_postgres_repository_runtime_integration.py`
around lines 403 - 438, The test
TestLearnedPatternsTable.test_query_learned_patterns_empty should not assume the
learned_patterns table is empty; instead, modify the assertion to validate
invariants about the returned rows from runtime.call("find_validated") — e.g.,
assert results is a list and for each row (returned model type from
ModelDbReturn "LearnedPattern") that row["status"] == "validated" (and
optionally other invariants like keys/id types or length <=
config.max_row_limit); alternatively, seed or clean the table before running the
test using the db_pool fixture so the test has a deterministic state.
Remove all default values for database configuration including the production password that was exposed in source code. All PostgreSQL environment variables are now required with no fallback defaults. Changes: - Add _check_postgres_env_vars() helper for validation - Remove hardcoded host, port, database, user, and password defaults - Use os.environ[] (raises KeyError) instead of os.getenv() with defaults - Skip tests gracefully when env vars not configured
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@tests/integration/runtime/db/test_postgres_repository_runtime_integration.py`:
- Around line 249-250: Remove the explicit isinstance check for `results` in the
test; instead rely on duck typing by deleting the line `assert
isinstance(results, list)` so the existing `assert len(results) == 3` and
subsequent indexing of `results` in
test_postgres_repository_runtime_integration.py will naturally fail if `results`
doesn't behave like a sequence, keeping the test concise and according to the
coding guideline that forbids isinstance checks.
- Around line 26-29: Replace any direct ValueError raises from get_dsn() with
raising RepositoryConfigError and chain the original KeyError using "from e";
specifically locate get_dsn() in test_postgres_repository_runtime_integration.py
(and similar occurrences around lines 65-90) and change patterns like "raise
ValueError(...)" to "raise RepositoryConfigError(... ) from e" so the raised
exception is an OnexError subclass and preserves the causal context; ensure you
import/use RepositoryConfigError (instead of ValueError) and keep the original
exception variable (e) when chaining.
Comment on lines
+26
to
+29
| from omnibase_infra.errors.repository import ( | ||
| RepositoryContractError, | ||
| RepositoryValidationError, | ||
| ) |
There was a problem hiding this comment.
Use RepositoryConfigError with chaining instead of ValueError.
get_dsn() raises ValueError, which violates the OnexError-only policy and loses causal context. Prefer raising RepositoryConfigError chained from the underlying KeyError.
🔧 Suggested fix
from omnibase_infra.errors.repository import (
RepositoryContractError,
RepositoryValidationError,
+ RepositoryConfigError,
)
@@
- is_configured, error_msg = _check_postgres_env_vars()
- if not is_configured:
- raise ValueError(error_msg)
-
- host = os.environ["POSTGRES_HOST"]
- port = os.environ["POSTGRES_PORT"]
- database = os.environ["POSTGRES_DATABASE"]
- user = os.environ["POSTGRES_USER"]
- password = os.environ["POSTGRES_PASSWORD"]
+ try:
+ host = os.environ["POSTGRES_HOST"]
+ port = os.environ["POSTGRES_PORT"]
+ database = os.environ["POSTGRES_DATABASE"]
+ user = os.environ["POSTGRES_USER"]
+ password = os.environ["POSTGRES_PASSWORD"]
+ except KeyError as e:
+ _, error_msg = _check_postgres_env_vars()
+ raise RepositoryConfigError(error_msg) from e
return f"postgresql://{user}:{password}@{host}:{port}/{database}"As per coding guidelines, "Only raise OnexError or its subclasses - always use raise OnexError(...) from e pattern for error chaining".
Also applies to: 65-90
🤖 Prompt for AI Agents
In `@tests/integration/runtime/db/test_postgres_repository_runtime_integration.py`
around lines 26 - 29, Replace any direct ValueError raises from get_dsn() with
raising RepositoryConfigError and chain the original KeyError using "from e";
specifically locate get_dsn() in test_postgres_repository_runtime_integration.py
(and similar occurrences around lines 65-90) and change patterns like "raise
ValueError(...)" to "raise RepositoryConfigError(... ) from e" so the raised
exception is an OnexError subclass and preserves the causal context; ensure you
import/use RepositoryConfigError (instead of ValueError) and keep the original
exception variable (e) when chaining.
Comment on lines
+249
to
+250
| assert isinstance(results, list) | ||
| assert len(results) == 3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file and the specific lines mentioned
cat -n tests/integration/runtime/db/test_postgres_repository_runtime_integration.py | sed -n '240,260p'Repository: OmniNode-ai/omnibase_infra
Length of output: 986
🏁 Script executed:
# Also check the full test function to understand context
cat -n tests/integration/runtime/db/test_postgres_repository_runtime_integration.py | sed -n '230,270p'Repository: OmniNode-ai/omnibase_infra
Length of output: 2029
🏁 Script executed:
# Check if there are protocol definitions in omnibase_spi related to results/list
find . -path "*/omnibase_spi*" -name "*.py" -type f | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
# Search for similar isinstance checks in test files to understand the pattern
rg "isinstance.*list" tests/integration/runtime/db/test_postgres_repository_runtime_integration.py -B 2 -A 2Repository: OmniNode-ai/omnibase_infra
Length of output: 443
🏁 Script executed:
# Find the PostgresRepositoryRuntime class and its call method signature
rg "class PostgresRepositoryRuntime" -A 20 --type pyRepository: OmniNode-ai/omnibase_infra
Length of output: 2234
🏁 Script executed:
# Search for the call method in PostgresRepositoryRuntime
rg "def call\(" --type py -A 5 | grep -A 5 "PostgresRepositoryRuntime\|class\|def call"Repository: OmniNode-ai/omnibase_infra
Length of output: 682
🏁 Script executed:
# Check the return type of the call method
fd "\.py$" -type f | xargs grep -l "PostgresRepositoryRuntime" | head -5Repository: OmniNode-ai/omnibase_infra
Length of output: 240
🏁 Script executed:
# Get more context on what the call method returns
rg "async def call" --type py -B 2 -A 8 | head -100Repository: OmniNode-ai/omnibase_infra
Length of output: 4408
Remove the explicit isinstance check and rely on duck typing.
Per the coding guidelines, isinstance checks should not be used. The subsequent assertions (len(results) == 3 and list indexing) will fail naturally with appropriate errors if results is not a list, validating the contract without an explicit type check.
♻️ Suggested cleanup
- assert isinstance(results, list)
assert len(results) == 3🤖 Prompt for AI Agents
In `@tests/integration/runtime/db/test_postgres_repository_runtime_integration.py`
around lines 249 - 250, Remove the explicit isinstance check for `results` in
the test; instead rely on duck typing by deleting the line `assert
isinstance(results, list)` so the existing `assert len(results) == 3` and
subsequent indexing of `results` in
test_postgres_repository_runtime_integration.py will naturally fail if `results`
doesn't behave like a sequence, keeping the test concise and according to the
coding guideline that forbids isinstance checks.
…itations Security hardening and documentation improvements from code review: - Add Pydantic field validators for primary_key_column and default_order_by to ensure only safe SQL identifiers are accepted (prevents misconfiguration) - Document regex-based SQL parsing limitations in postgres_repository_runtime.py including known edge cases (string literals, subqueries, comments) - Add recommendations for handling complex queries in contracts
- Use explicit asyncio.TimeoutError instead of bare TimeoutError - Add S608 suppression documentation explaining SQL injection safety - Add return type annotations to MockAsyncRecord test helper methods
Resolve conflicts: - pyproject.toml: Keep omnibase-core ^0.12.0 (includes v0.11.0 features) - poetry.lock: Regenerated with updated dependencies
- Add module-scoped event_loop fixture to integration tests
- Fixes pytest-asyncio ScopeMismatch error when db_pool (module-scoped)
tries to use function-scoped event_loop
- Update INFRA_MAX_UNIONS test assertion from 115 to 117
- Reflects 2 new union types in PostgresRepositoryRuntime:
- call() return type: list[dict] | dict | None
- _execute_with_timeout() return type: same pattern
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/validation/test_validator_defaults.py`:
- Around line 100-102: The assertion for INFRA_MAX_UNIONS currently expects 117
but its failure message still mentions 115; update the assertion message
attached to the assert of INFRA_MAX_UNIONS to reflect the new expected value
(117) and keep the explanatory text (e.g., "INFRA_MAX_UNIONS should be 117
(non-optional unions only, X | None excluded)"), ensuring the assert for
INFRA_MAX_UNIONS and its message are consistent.
Comment on lines
+100
to
102
| assert INFRA_MAX_UNIONS == 117, ( | ||
| "INFRA_MAX_UNIONS should be 115 (non-optional unions only, X | None excluded)" | ||
| ) |
There was a problem hiding this comment.
Update failure message to match the new expected constant.
The assertion now expects 117, but the message still says 115, which will confuse failures.
🔧 Proposed fix
- assert INFRA_MAX_UNIONS == 117, (
- "INFRA_MAX_UNIONS should be 115 (non-optional unions only, X | None excluded)"
- )
+ assert INFRA_MAX_UNIONS == 117, (
+ "INFRA_MAX_UNIONS should be 117 (non-optional unions only, X | None excluded)"
+ )📝 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
| assert INFRA_MAX_UNIONS == 117, ( | |
| "INFRA_MAX_UNIONS should be 115 (non-optional unions only, X | None excluded)" | |
| ) | |
| assert INFRA_MAX_UNIONS == 117, ( | |
| "INFRA_MAX_UNIONS should be 117 (non-optional unions only, X | None excluded)" | |
| ) |
🤖 Prompt for AI Agents
In `@tests/unit/validation/test_validator_defaults.py` around lines 100 - 102, The
assertion for INFRA_MAX_UNIONS currently expects 117 but its failure message
still mentions 115; update the assertion message attached to the assert of
INFRA_MAX_UNIONS to reflect the new expected value (117) and keep the
explanatory text (e.g., "INFRA_MAX_UNIONS should be 117 (non-optional unions
only, X | None excluded)"), ensuring the assert for INFRA_MAX_UNIONS and its
message are consistent.
This was referenced Feb 2, 2026
Merged
This was referenced Feb 10, 2026
8 tasks done
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
Adds a generic contract executor for
ModelDbRepositoryContractfrom omnibase_core, enabling contract-driven database access with safety constraints and determinism guarantees.Key Features:
RepositoryConfigError,RepositoryExecutionError,RepositoryTimeoutError,RepositoryContractError,RepositoryValidationError)ModelRepositoryRuntimeConfigfor runtime configuration (timeouts, limits, operation modes)PostgresRepositoryRuntimecore implementation with:asyncio.wait_for()Files Changed:
src/omnibase_infra/errors/repository/- New typed exception hierarchysrc/omnibase_infra/runtime/db/- Runtime implementation and configtests/unit/runtime/db/test_postgres_repository_runtime.py- 24 unit teststests/integration/runtime/db/test_postgres_repository_runtime_integration.py- 10 integration testsDependencies:
omnibase-coreto^0.12.0forModelDbRepositoryContract0.2.9Linear Ticket
OMN-1783
Test Plan
pytest tests/unit/runtime/db/)pytest -m postgres tests/integration/runtime/db/)Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.