feat: L9 graph integration pass#92
Open
cryptoxdog wants to merge 3 commits into
Open
Conversation
- engine/boot.py — boot sequence updates - engine/config/settings.py — settings refinements - engine/gds/community_export.py — new GDS community export module - engine/startup_wiring.py — startup wiring improvements - .env.template — updated env template - tests/integration/test_startup_integration.py — new startup integration tests 6 files, 887 insertions / 117 deletions
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
❌ PR Too Large 📋 Best Practices for Large Changes
🚫 This PR is blocked from merging until size limits are met. |
📝 WalkthroughWalkthroughAdds PostgreSQL-backed audit persistence and a Louvain community-export hook; hardens startup wiring to fail-closed on missing audit DSN and fail-fast on convergence patch import; updates settings/env defaults; and provides integration tests covering startup wiring, community export, and production secret validation. Changes
Sequence DiagramsequenceDiagram
participant Boot as Engine Boot
participant Wiring as Startup Wiring
participant PG as PostgreSQL
participant Neo4j as Neo4j
participant Loader as Domain Loader
participant GDS as GDS Scheduler
participant Rtn as ReturnChannel
Boot->>Wiring: startup()
Wiring->>Wiring: _assert_convergence_patch_importable()
Wiring->>Wiring: _apply_gap_fixes(l9_postgres_dsn, neo4j_driver, domain_loader)
Wiring->>PG: create audit pool (asyncpg) using l9_postgres_dsn
PG-->>Wiring: pool ready
Wiring->>Loader: load domain KB/rules
Loader-->>Wiring: domains & KB loaded
Wiring->>Rtn: initialize GraphToEnrichReturnChannel singleton
Rtn-->>Wiring: singleton ready
Wiring->>GDS: register_post_job_hook(export_community_labels_to_enrich)
GDS-->>Wiring: hook registered
par Louvain job flow
GDS->>Neo4j: run Louvain, label communities
Neo4j-->>GDS: community rows (entity_id, community_id)
GDS->>Rtn: export_community_labels_to_enrich(...)
Rtn-->>GDS: enqueue counts returned
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.template:
- Around line 39-43: The .env.template advertises TENANT_ALLOWLIST but
engine/config/settings.py has no tenant_allowlist field; either remove the
TENANT_ALLOWLIST line from .env.template or add a corresponding setting in
engine/config/settings.py (e.g., a tenant_allowlist config parsed from env
alongside tenant_auth_enabled, tenant_auth_bypass_key, and
capability_auth_enabled). If you add it, implement reading the env var,
validate/normalize it (e.g., comma-separated list or JSON), expose it as
tenant_allowlist in the settings module, and update any auth code that should
use tenant_allowlist.
In `@engine/boot.py`:
- Line 83: Remove the stale noqa by deleting the " # noqa: PLR0912" directive
on the async def startup(self) method in engine.boot (the startup function);
this will let Ruff enforce RUF100 correctly and avoid the CI failure — simply
remove the noqa comment so linters run against startup as intended.
In `@engine/config/settings.py`:
- Around line 50-55: Rename the settings field and env binding from postgres_dsn
to l9_postgres_dsn and switch the environment variable key to L9_POSTGRES_DSN so
the audit DSN follows the platform L9_ prefix convention; update the settings
default value only if needed (keep same DSN string), and adjust the config
loader/template that reads environment variables (the code that loads
postgres_dsn) to read L9_POSTGRES_DSN instead and update any operator/docs
references to the variable name so all places (settings field, loader/template,
and docs) consistently use l9_postgres_dsn / L9_POSTGRES_DSN.
In `@engine/gds/community_export.py`:
- Around line 32-45: The file has orphaned imports causing an F401 linter error
— remove unused imports hashlib, json, uuid and the unused type
GraphInferenceResultEnvelope from the import block; keep the needed imports like
build_graph_inference_result_envelope and GraphToEnrichReturnChannel and update
the top-level import statement accordingly in community_export.py, then run the
linter to confirm no other references to those symbols remain.
- Around line 65-73: The Neo4j queries in export_community_labels_to_enrich are
scoped to database=domain_id which is incorrect; change all calls that pass
database=domain_id to use database=tenant_id instead so the graph access stays
tenant-scoped (update the GraphDriver.query/execute calls inside
export_community_labels_to_enrich and the additional query block later in the
same function that currently uses database=domain_id — ensure every occurrence
using database=domain_id is replaced with database=tenant_id).
In `@engine/startup_wiring.py`:
- Around line 83-99: GDSScheduler currently lacks register_post_job_hook so
startup raises AttributeError; add a class-level API on GDSScheduler named
register_post_job_hook(job_type: str, hook: Callable[[tenant_id, domain_id],
Any]) that validates inputs and stores hooks in a class-level registry (e.g.,
dict mapping job_type -> list of callables, optionally protected by a lock), and
ensure the Louvain execution/completion path (the method that
finishes/dispatches louvain jobs in GDSScheduler, e.g.,
run_louvain/execute_job/_on_job_complete) invokes all registered hooks for
job_type="louvain" with (tenant_id, domain_id) so the imported
export_community_labels_to_enrich can be called; implement safe error handling
so hook exceptions are logged but do not crash the scheduler.
In `@tests/integration/test_startup_integration.py`:
- Around line 437-441: Remove the unused BaseSettings import in the failing
test: delete the "from pydantic_settings import BaseSettings" line and keep only
the existing import and instantiation of Settings (the Settings symbol and its
instantiation should remain so the test still raises ValidationError for
"postgres_dsn"); this resolves the F401 lint error while preserving test
behavior around Settings().
- Around line 23-29: The integration tests in
tests/integration/test_startup_integration.py currently mock Neo4j driver calls;
change them to use testcontainers-neo4j by starting a Neo4jContainer in the test
fixture (replace the driver-bound mock), point the code under test (the
startup/community-export path invoked by the test) to the container's bolt URL
and credentials, and keep only external IO mocks like asyncpg.create_pool; move
any driver-mocking cases into unit tests. Ensure the test fixture performs any
required DB schema/data setup against the real Neo4j instance before invoking
the code paths that call the real driver so the queries executed by functions
under test succeed.
- Around line 47-65: The _reset_return_channel fixture is a generator (uses
yield) but is annotated -> None and its decorator uses empty parentheses; update
its signature to return typing.Iterator[None] (or typing.Generator[None, None,
None]) and change the decorator from `@pytest.fixture`() to `@pytest.fixture`; apply
the same decorator change for other fixtures like mock_neo4j_driver and
mock_domain_loader (remove empty parens) and, if any other fixture uses yield,
ensure its return type is the appropriate Iterator/Generator and import
typing.Iterator (or Generator) at top as needed; keep the calls to
GraphToEnrichReturnChannel.reset_instance() and AsyncMock usage in
mock_neo4j_driver unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05789ac4-cbfd-47b6-9f93-51a7a4e90bf8
📒 Files selected for processing (6)
.env.templateengine/boot.pyengine/config/settings.pyengine/gds/community_export.pyengine/startup_wiring.pytests/integration/test_startup_integration.py
Comment on lines
+39
to
+43
| # --- Tenant Auth --- | ||
| TENANT_AUTH_ENABLED=true | ||
| TENANT_AUTH_BYPASS_KEY= | ||
| CAPABILITY_AUTH_ENABLED=true | ||
| TENANT_ALLOWLIST= |
There was a problem hiding this comment.
Remove or wire up TENANT_ALLOWLIST.
engine/config/settings.py defines tenant_auth_enabled, tenant_auth_bypass_key, and capability_auth_enabled, but no tenant_allowlist field. Leaving this in the template advertises a supported auth control that the engine never reads.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 41-41: [UnorderedKey] The TENANT_AUTH_BYPASS_KEY key should go before the TENANT_AUTH_ENABLED key
(UnorderedKey)
[warning] 42-42: [UnorderedKey] The CAPABILITY_AUTH_ENABLED key should go before the TENANT_AUTH_BYPASS_KEY key
(UnorderedKey)
[warning] 43-43: [UnorderedKey] The TENANT_ALLOWLIST key should go before the TENANT_AUTH_BYPASS_KEY key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.template around lines 39 - 43, The .env.template advertises
TENANT_ALLOWLIST but engine/config/settings.py has no tenant_allowlist field;
either remove the TENANT_ALLOWLIST line from .env.template or add a
corresponding setting in engine/config/settings.py (e.g., a tenant_allowlist
config parsed from env alongside tenant_auth_enabled, tenant_auth_bypass_key,
and capability_auth_enabled). If you add it, implement reading the env var,
validate/normalize it (e.g., comma-separated list or JSON), expose it as
tenant_allowlist in the settings module, and update any auth code that should
use tenant_allowlist.
Comment on lines
+50
to
+55
| # --- PostgreSQL (audit persistence) --- | ||
| # FIX(RULE-9 + GAP-5): postgres_dsn is REQUIRED for audit pool wiring. | ||
| # apply_all_gap_fixes() reads this at startup and raises RuntimeError if absent. | ||
| # Set POSTGRES_DSN in .env or environment. Production: use a secrets manager DSN. | ||
| # Format: postgresql://user:pass@host:port/dbname | ||
| postgres_dsn: str = "postgresql://l9:change-me-in-production@localhost:5432/l9_audit" |
There was a problem hiding this comment.
Don't introduce another unprefixed infrastructure variable.
This change hard-codes POSTGRES_DSN into the settings contract and operator docs, but the repo rules require infrastructure env vars to be L9_-prefixed. Please wire this as L9_POSTGRES_DSN and update the loader/template together so the new audit config follows the same contract as the rest of the platform.
As per coding guidelines, "HIGH: non-L9_ env var name for infrastructure variables → convention violation (ENV-001). All infra env vars must be prefixed L9_."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/config/settings.py` around lines 50 - 55, Rename the settings field
and env binding from postgres_dsn to l9_postgres_dsn and switch the environment
variable key to L9_POSTGRES_DSN so the audit DSN follows the platform L9_ prefix
convention; update the settings default value only if needed (keep same DSN
string), and adjust the config loader/template that reads environment variables
(the code that loads postgres_dsn) to read L9_POSTGRES_DSN instead and update
any operator/docs references to the variable name so all places (settings field,
loader/template, and docs) consistently use l9_postgres_dsn / L9_POSTGRES_DSN.
Comment on lines
+65
to
+73
| async def export_community_labels_to_enrich( | ||
| driver: GraphDriver, | ||
| tenant_id: str, | ||
| domain_id: str, | ||
| *, | ||
| node_label: str = "Facility", | ||
| community_property: str = "communityId", | ||
| min_community_size: int = _MIN_COMMUNITY_SIZE, | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
Scope this query to the tenant database, not domain_id.
tenant_id is already passed in, but the query is executed against database=domain_id. The engine contract says graph access must stay tenant-scoped, so this can read the wrong graph and enqueue cross-tenant enrichment targets whenever domain_id and tenant database names diverge.
Suggested fix
results = await driver.execute_query(
cypher=cypher,
parameters={"min_size": min_community_size},
- database=domain_id,
+ database=tenant_id,
)As per coding guidelines, "Tenant is resolved by chassis and passed as string argument. Engine NEVER resolves tenant. Every Neo4j query scopes to tenant database."
Also applies to: 110-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/gds/community_export.py` around lines 65 - 73, The Neo4j queries in
export_community_labels_to_enrich are scoped to database=domain_id which is
incorrect; change all calls that pass database=domain_id to use
database=tenant_id instead so the graph access stays tenant-scoped (update the
GraphDriver.query/execute calls inside export_community_labels_to_enrich and the
additional query block later in the same function that currently uses
database=domain_id — ensure every occurrence using database=domain_id is
replaced with database=tenant_id).
Comment on lines
+83
to
+99
| # ── Gap-6: Register community-export hook on GDSScheduler ──────────────── | ||
| # FIX(RULE-2 + RULE-9 + GAP-6): import path corrected from orphan `graph/` | ||
| # package to canonical `engine.gds.community_export`. GDSScheduler is | ||
| # imported from `engine.gds.scheduler` (the only registered scheduler). | ||
| # If register_post_job_hook raises AttributeError the method is not yet | ||
| # implemented on GDSScheduler — this surfaces at boot rather than silently | ||
| # skipping the hook registration. | ||
| from engine.gds.community_export import export_community_labels_to_enrich | ||
| from engine.gds.scheduler import GDSScheduler | ||
|
|
||
| GDSScheduler.register_post_job_hook( | ||
| job_type="louvain", | ||
| hook=lambda tenant_id, domain_id: export_community_labels_to_enrich( | ||
| neo4j_driver, tenant_id, domain_id | ||
| ), | ||
| ) | ||
| logger.info("startup: Gap-6 community export hook registered on GDSScheduler") |
There was a problem hiding this comment.
GDSScheduler.register_post_job_hook doesn't exist yet.
engine/gds/scheduler.py currently exposes no register_post_job_hook method, so startup will raise AttributeError here and the engine won't come up. This also breaks the new test that patches the same classmethod. Please add a real hook-registration API to GDSScheduler (and invoke it from the Louvain execution path) before wiring boot to call it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/startup_wiring.py` around lines 83 - 99, GDSScheduler currently lacks
register_post_job_hook so startup raises AttributeError; add a class-level API
on GDSScheduler named register_post_job_hook(job_type: str, hook:
Callable[[tenant_id, domain_id], Any]) that validates inputs and stores hooks in
a class-level registry (e.g., dict mapping job_type -> list of callables,
optionally protected by a lock), and ensure the Louvain execution/completion
path (the method that finishes/dispatches louvain jobs in GDSScheduler, e.g.,
run_louvain/execute_job/_on_job_complete) invokes all registered hooks for
job_type="louvain" with (tenant_id, domain_id) so the imported
export_community_labels_to_enrich can be called; implement safe error handling
so hook exceptions are logged but do not crash the scheduler.
Comment on lines
+23
to
+29
| Test philosophy: | ||
| - Tests use real asyncio event loops (no sync wrappers). | ||
| - Driver calls are mocked at the Neo4j driver boundary only. | ||
| - PostgreSQL pool is mocked via asyncpg.create_pool patch. | ||
| - No mocking of engine logic — only external I/O boundaries. | ||
| - ContractViolationError and RuntimeError paths are explicitly asserted. | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and its size
wc -l tests/integration/test_startup_integration.py 2>/dev/null || echo "File check needed"Repository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 127
🏁 Script executed:
# Read the full test file to examine the test structure
cat tests/integration/test_startup_integration.pyRepository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 15461
🏁 Script executed:
# Check for Neo4j driver mocking patterns and imports
rg -n "mock|patch|Mock" tests/integration/test_startup_integration.pyRepository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 2164
Use testcontainers-neo4j instead of mocked driver calls in integration tests.
Integration tests in tests/integration/ must exercise the actual Neo4j database contract through real driver interactions. Mocking at the driver boundary prevents detection of regressions in query shape, database selection, and hook behavior. Move mocked-driver cases to unit tests and run this startup/community-export path against a real Neo4j instance via testcontainers-neo4j.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_startup_integration.py` around lines 23 - 29, The
integration tests in tests/integration/test_startup_integration.py currently
mock Neo4j driver calls; change them to use testcontainers-neo4j by starting a
Neo4jContainer in the test fixture (replace the driver-bound mock), point the
code under test (the startup/community-export path invoked by the test) to the
container's bolt URL and credentials, and keep only external IO mocks like
asyncpg.create_pool; move any driver-mocking cases into unit tests. Ensure the
test fixture performs any required DB schema/data setup against the real Neo4j
instance before invoking the code paths that call the real driver so the queries
executed by functions under test succeed.
Comment on lines
+47
to
+65
| @pytest.fixture(autouse=True) | ||
| def _reset_return_channel() -> None: | ||
| """Ensure singleton is fresh for each test.""" | ||
| from engine.graph_return_channel import GraphToEnrichReturnChannel | ||
|
|
||
| GraphToEnrichReturnChannel.reset_instance() | ||
| yield | ||
| GraphToEnrichReturnChannel.reset_instance() | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def mock_neo4j_driver() -> MagicMock: | ||
| driver = MagicMock() | ||
| driver.execute_query = AsyncMock(return_value=[]) | ||
| return driver | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def mock_domain_loader() -> MagicMock: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file to see the actual code
cat -n tests/integration/test_startup_integration.py | sed -n '40,75p'Repository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 1306
🏁 Script executed:
# Check if Iterator is imported in the file
grep -n "from collections.abc import\|from typing import" tests/integration/test_startup_integration.py | head -20Repository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 103
🏁 Script executed:
# Check pytest version and any linting configuration
if [ -f "pyproject.toml" ]; then
grep -A 10 "\[tool.ruff\]\|\[tool.pytest\]\|pytest\|ruff" pyproject.toml | head -30
fiRepository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 871
🏁 Script executed:
# Check if there are other yield fixtures in the codebase to see the pattern
rg -n "def .*\) -> " tests/ --type py -B 1 | grep -A 1 "yield" | head -30Repository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 60
Fix fixture type annotations and decorator syntax.
The _reset_return_channel fixture uses yield but is annotated as returning None, which violates type correctness for generator functions. Yield fixtures must return Iterator[None] (or Generator with appropriate type parameters). Additionally, @pytest.fixture() with empty parentheses should be @pytest.fixture when no arguments are provided.
Required changes
+from collections.abc import Iterator
from typing import Any
@@
`@pytest.fixture`(autouse=True)
-def _reset_return_channel() -> None:
+def _reset_return_channel() -> Iterator[None]:
@@
-@pytest.fixture()
+@pytest.fixture
def mock_neo4j_driver() -> MagicMock:
@@
-@pytest.fixture()
+@pytest.fixture
def mock_domain_loader() -> MagicMock:🧰 Tools
🪛 GitHub Actions: Contract Enforcement
[error] 57-65: ruff PT001: Use @pytest.fixture instead of @pytest.fixture().
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 53-53: Remove this yield statement or annotate function "_reset_return_channel" with "typing.Generator" or one of its supertypes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_startup_integration.py` around lines 47 - 65, The
_reset_return_channel fixture is a generator (uses yield) but is annotated ->
None and its decorator uses empty parentheses; update its signature to return
typing.Iterator[None] (or typing.Generator[None, None, None]) and change the
decorator from `@pytest.fixture`() to `@pytest.fixture`; apply the same decorator
change for other fixtures like mock_neo4j_driver and mock_domain_loader (remove
empty parens) and, if any other fixture uses yield, ensure its return type is
the appropriate Iterator/Generator and import typing.Iterator (or Generator) at
top as needed; keep the calls to GraphToEnrichReturnChannel.reset_instance() and
AsyncMock usage in mock_neo4j_driver unchanged.
Resolve remaining terminology-guard offenders, eliminate contract-scanner false positives in sanitized causal Cypher builders, and bring ruff back to green with bounded mechanical edits. GMP: Phase 0-6 complete; Scope: shared-blockers remediation on fix/shared-blockers-all-prs; Blast radius: engine/tests/tools mechanical lint+contract fixes; Validation: python3 tools/contract_scanner.py=PASS, ruff check .=PASS, terminology grep=PASS; Note: .pre-commit-config.yaml present but pre-commit executable/module unavailable in this environment.
- add l9_postgres_dsn + tenant_allowlist in settings with postgres_dsn alias - switch boot/runtime wiring to L9_POSTGRES_DSN - clean engine/gds/community_export imports - scope community export query by tenant_id while preserving domain DB routing - remove stale noqa on startup() - update startup integration tests for new env var naming - carry forward shared-blocker cleanup onto PR #92 branch Verification: - python3 tools/contract_scanner.py ✅ - ruff check . ✅ - pytest tests/integration/test_startup_integration.py -q ❌ missing local dependency: neo4j GMP: Phase 0-4 complete; scoped PR92 remediation only; local static verification green; dynamic test blocked by missing neo4j package in this environment.
|
❌ Too Many Files Changed ❌ PR Too Large 📋 Best Practices for Large Changes
🚫 This PR is blocked from merging until size limits are met. |
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/boot.py (1)
1-326:⚠️ Potential issue | 🟡 MinorPipeline failure:
ruff format --checkreports this file needs reformatting.The functional changes are sound, but CI is failing because the file needs to be reformatted with
ruff format.Run the following to fix:
ruff format engine/boot.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/boot.py` around lines 1 - 326, CI failure due to formatting: run ruff format on the file to apply code-style fixes; specifically reformat engine/boot.py (which contains GraphLifecycle, _assert_default_weight_sum, _apply_gap_fixes, and _assert_convergence_patch_importable) by executing the suggested command (ruff format engine/boot.py) so ruff's formatting rules are applied and the pipeline passes.
🧹 Nitpick comments (3)
engine/graph/graph_sync_client_fix.py (1)
99-108: Missing type hint ontxparameter.The
txparameter lacks a type annotation. Per coding guidelines, all function parameters require type hints.🔧 Proposed fix
-async def _write_batch_tx(tx, *, entity_type: str, batch: list, tenant_id: str, packet_id: str): +async def _write_batch_tx(tx: Any, *, entity_type: str, batch: list, tenant_id: str, packet_id: str) -> None:Alternatively, if neo4j types are available:
tx: neo4j.AsyncManagedTransaction.As per coding guidelines: "Type hints on every function signature".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/graph/graph_sync_client_fix.py` around lines 99 - 108, The function _write_batch_tx is missing a type annotation for the tx parameter; add an appropriate type hint (e.g., tx: neo4j.AsyncManagedTransaction if neo4j is available, or a generic typing.Protocol/Any such as tx: Any imported from typing) to the signature and update imports accordingly so the signature reads something like async def _write_batch_tx(tx: <chosen-type>, *, entity_type: str, batch: list, tenant_id: str, packet_id: str): ensuring the project linters/type-checkers accept the chosen type.tests/test_algorithmic_upgrades.py (2)
139-141: Consider more specific type hints for_make_fpparameters.The
score_distanddim_domparameters use baredictwhich loses type information. Usingdict[str, float]would improve clarity and enable better static analysis.def _make_fp( - self, persona_id: str, window_id: str, score_dist: dict, dim_dom: dict, entropy: float, concentration: float - ): + self, persona_id: str, window_id: str, score_dist: dict[str, float], dim_dom: dict[str, float], entropy: float, concentration: float + ) -> AlgorithmicFingerprint:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_algorithmic_upgrades.py` around lines 139 - 141, Update the _make_fp function signature to use specific mapping types instead of bare dicts: change the score_dist and dim_dom parameter annotations from dict to dict[str, float] (or typing.Mapping[str, float] if immutability is desired) so static analyzers see the key/value types; update any imports to include typing types if needed and ensure any callers still match the new annotations (or update their annotations) while leaving persona_id, window_id, entropy, and concentration types unchanged.
200-200:asyncio.get_event_loop().run_until_complete()is deprecated.In Python 3.10+, prefer
asyncio.run()for running async code from synchronous contexts. The deprecated pattern still works but may emit deprecation warnings.- asyncio.get_event_loop().run_until_complete(store.persist(MagicMock(), MagicMock())) + asyncio.run(store.persist(MagicMock(), MagicMock()))This applies to other similar usages at lines 278, 283, 302, 309, and 321.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_algorithmic_upgrades.py` at line 200, The test uses the deprecated asyncio.get_event_loop().run_until_complete(...) to run async calls (e.g., asyncio.get_event_loop().run_until_complete(store.persist(MagicMock(), MagicMock()))); replace these calls with asyncio.run(...) to avoid deprecation warnings—call asyncio.run(store.persist(...)) for the shown occurrence and update the other similar usages mentioned (the calls around the other line locations) to use asyncio.run as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.template:
- Around line 8-13: Rename all Neo4j environment variable names to include the
required L9_ prefix in the .env.template (e.g., change NEO4J_URI ->
L9_NEO4J_URI, NEO4J_USERNAME -> L9_NEO4J_USERNAME, NEO4J_PASSWORD ->
L9_NEO4J_PASSWORD, NEO4J_DATABASE -> L9_NEO4J_DATABASE, NEO4J_POOL_SIZE ->
L9_NEO4J_POOL_SIZE) and then update engine/config/settings.py to read those new
names (update the env lookups that populate neo4j_uri, neo4j_username,
neo4j_password, neo4j_database, neo4j_pool_size to use os.getenv("L9_NEO4J_*")
or equivalent). Ensure any references elsewhere (config keys or tests) that
expect the old names are updated to the new L9_ variables to keep runtime
behavior unchanged.
In `@engine/gds/community_export.py`:
- Around line 32-44: Replace use of the standard logging module with structlog:
remove the import of logging, add import structlog, and change the logger
instantiation from logging.getLogger(__name__) to structlog.get_logger(__name__)
in community_export.py (update the symbol logger and keep all existing logging
calls intact); do not add any structlog configuration in this file.
In `@tests/integration/test_startup_integration.py`:
- Around line 329-334: The test currently asserts exact float equality on values
returned by extract_per_field_confidence (result["mfi"] and
result["hdpe_grade"]); replace those == comparisons with pytest.approx checks
(e.g. assert result["mfi"] == pytest.approx(0.6) and assert result["hdpe_grade"]
== pytest.approx(0.6)) to avoid brittle float equality while leaving the absence
checks ("pass_number", "confidence") unchanged.
- Line 1: The file test_startup_integration.py is not formatted to ruff rules;
run ruff format on the test_startup_integration.py module (or invoke your
project's formatter/pre-commit that runs ruff) to apply the formatting changes
and re-run the tests/CI so the pipeline stops flagging the file.
---
Outside diff comments:
In `@engine/boot.py`:
- Around line 1-326: CI failure due to formatting: run ruff format on the file
to apply code-style fixes; specifically reformat engine/boot.py (which contains
GraphLifecycle, _assert_default_weight_sum, _apply_gap_fixes, and
_assert_convergence_patch_importable) by executing the suggested command (ruff
format engine/boot.py) so ruff's formatting rules are applied and the pipeline
passes.
---
Nitpick comments:
In `@engine/graph/graph_sync_client_fix.py`:
- Around line 99-108: The function _write_batch_tx is missing a type annotation
for the tx parameter; add an appropriate type hint (e.g., tx:
neo4j.AsyncManagedTransaction if neo4j is available, or a generic
typing.Protocol/Any such as tx: Any imported from typing) to the signature and
update imports accordingly so the signature reads something like async def
_write_batch_tx(tx: <chosen-type>, *, entity_type: str, batch: list, tenant_id:
str, packet_id: str): ensuring the project linters/type-checkers accept the
chosen type.
In `@tests/test_algorithmic_upgrades.py`:
- Around line 139-141: Update the _make_fp function signature to use specific
mapping types instead of bare dicts: change the score_dist and dim_dom parameter
annotations from dict to dict[str, float] (or typing.Mapping[str, float] if
immutability is desired) so static analyzers see the key/value types; update any
imports to include typing types if needed and ensure any callers still match the
new annotations (or update their annotations) while leaving persona_id,
window_id, entropy, and concentration types unchanged.
- Line 200: The test uses the deprecated
asyncio.get_event_loop().run_until_complete(...) to run async calls (e.g.,
asyncio.get_event_loop().run_until_complete(store.persist(MagicMock(),
MagicMock()))); replace these calls with asyncio.run(...) to avoid deprecation
warnings—call asyncio.run(store.persist(...)) for the shown occurrence and
update the other similar usages mentioned (the calls around the other line
locations) to use asyncio.run as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 996b992f-3f15-409c-b602-19fd6c71fefd
📒 Files selected for processing (102)
.env.templatechassis/errors.pyengine/arbitration/engine.pyengine/arbitration/schema.pyengine/auth/capabilities.pyengine/boot.pyengine/causal/attribution.pyengine/causal/causal_compiler.pyengine/compliance/audit_persistence.pyengine/config/settings.pyengine/contract_enforcement.pyengine/convergence_controller_patch.pyengine/diagnostics/__init__.pyengine/diagnostics/dissimilarity.pyengine/diagnostics/fingerprint.pyengine/gds/community_export.pyengine/gds/scheduler.pyengine/graph/circuit_breaker.pyengine/graph/community_export.pyengine/graph/driver.pyengine/graph/graph_sync_client_fix.pyengine/graph_return_channel.pyengine/handlers.pyengine/hoprag/config.pyengine/hoprag/indexer.pyengine/inference_bridge.pyengine/inference_rule_registry.pyengine/kge/cross_dimensional_ensemble.pyengine/models/outcomes.pyengine/outcomes/schema.pyengine/packet/bridge.pyengine/packet/packet_store.pyengine/personas/composer.pyengine/personas/suppression.pyengine/personas/synthesis.pyengine/scoring/assembler.pyengine/scoring/confidence.pyengine/scoring/helpfulness.pyengine/scoring/importance.pyengine/scoring/pareto_integrator.pyengine/scoring/weight_discovery.pyengine/security/P2_9_llm_schemas.pyengine/startup_wiring.pyengine/state.pyengine/traversal/edge_merger.pyengine/traversal/multihop.pyengine/traversal/pseudo_query.pyl9_core/models.pytests/conftest.pytests/contracts/test_contracts.pytests/gap_fixes/test_gap1_contract.pytests/gap_fixes/test_gap2_return_channel.pytests/gap_fixes/test_gap3_inference_registry.pytests/gap_fixes/test_gap5_audit.pytests/integration/test_admin_handler.pytests/integration/test_graph_driver.pytests/integration/test_handlers.pytests/integration/test_hoprag_pipeline.pytests/integration/test_match_handler.pytests/integration/test_outcomes_handler.pytests/integration/test_startup_integration.pytests/integration/test_sync_handler.pytests/invariants/test_compliance.pytests/invariants/test_configuration.pytests/invariants/test_trust_boundary.pytests/performance/test_query_latency.pytests/performance/test_sync_throughput.pytests/property/test_gates_property.pytests/property/test_scoring_property.pytests/scoring/test_benchmark.pytests/security/test_compliance_security.pytests/security/test_injection.pytests/test_algorithmic_upgrades.pytests/test_pareto_wiring.pytests/unit/test_arbitration.pytests/unit/test_compliance_checker.pytests/unit/test_cross_dimensional_ensemble.pytests/unit/test_cypher_utils.pytests/unit/test_domain_loader.pytests/unit/test_edge_merger.pytests/unit/test_gate_compiler.pytests/unit/test_handlers_enrich_health.pytests/unit/test_handlers_extended.pytests/unit/test_hgkr_gds_dag.pytests/unit/test_loader.pytests/unit/test_multihop_traversal.pytests/unit/test_outcomes.pytests/unit/test_packet_bridge.pytests/unit/test_parameter_resolver.pytests/unit/test_persona_composer.pytests/unit/test_persona_selector.pytests/unit/test_persona_synthesis.pytests/unit/test_sanitize.pytests/unit/test_scoring_assembler.pytests/unit/test_sync_and_traversal.pytests/unit/test_sync_generator.pytests/unit/test_traversal_assembler.pytests/unit/test_wave4_state_resilience.pytests/unit/test_wave6_dormant_features.pytools/contract_report.pytools/hoprag_benchmark.pytools/validate_domain.py
💤 Files with no reviewable changes (8)
- tests/unit/test_arbitration.py
- tests/unit/test_loader.py
- tests/unit/test_sync_and_traversal.py
- tests/unit/test_multihop_traversal.py
- tests/unit/test_wave6_dormant_features.py
- tests/scoring/test_benchmark.py
- tests/unit/test_cross_dimensional_ensemble.py
- tests/unit/test_hgkr_gds_dag.py
✅ Files skipped from review due to trivial changes (79)
- tests/unit/test_domain_loader.py
- tests/integration/test_sync_handler.py
- engine/arbitration/engine.py
- tests/gap_fixes/test_gap3_inference_registry.py
- engine/hoprag/config.py
- engine/scoring/confidence.py
- tests/gap_fixes/test_gap1_contract.py
- tests/integration/test_handlers.py
- tests/unit/test_cypher_utils.py
- engine/scoring/helpfulness.py
- tests/unit/test_parameter_resolver.py
- chassis/errors.py
- tests/unit/test_sanitize.py
- tests/unit/test_handlers_enrich_health.py
- tests/unit/test_outcomes.py
- tests/unit/test_persona_synthesis.py
- engine/personas/synthesis.py
- tests/gap_fixes/test_gap2_return_channel.py
- engine/hoprag/indexer.py
- engine/causal/attribution.py
- engine/causal/causal_compiler.py
- engine/personas/composer.py
- engine/inference_bridge.py
- tools/hoprag_benchmark.py
- tests/invariants/test_configuration.py
- tests/unit/test_wave4_state_resilience.py
- engine/personas/suppression.py
- engine/scoring/weight_discovery.py
- engine/packet/packet_store.py
- engine/state.py
- engine/kge/cross_dimensional_ensemble.py
- tests/invariants/test_compliance.py
- tests/integration/test_outcomes_handler.py
- tests/gap_fixes/test_gap5_audit.py
- tests/unit/test_packet_bridge.py
- tests/integration/test_admin_handler.py
- tests/unit/test_persona_selector.py
- engine/graph/driver.py
- engine/traversal/pseudo_query.py
- engine/graph/circuit_breaker.py
- tests/performance/test_query_latency.py
- engine/compliance/audit_persistence.py
- tests/unit/test_edge_merger.py
- tests/unit/test_traversal_assembler.py
- tests/unit/test_gate_compiler.py
- engine/security/P2_9_llm_schemas.py
- engine/arbitration/schema.py
- tools/contract_report.py
- tests/unit/test_persona_composer.py
- engine/graph/community_export.py
- engine/scoring/pareto_integrator.py
- engine/outcomes/schema.py
- tests/unit/test_handlers_extended.py
- engine/packet/bridge.py
- tests/unit/test_compliance_checker.py
- tests/property/test_scoring_property.py
- engine/traversal/edge_merger.py
- engine/traversal/multihop.py
- l9_core/models.py
- tests/integration/test_match_handler.py
- tools/validate_domain.py
- engine/scoring/assembler.py
- tests/invariants/test_trust_boundary.py
- tests/unit/test_sync_generator.py
- tests/security/test_compliance_security.py
- tests/unit/test_scoring_assembler.py
- engine/gds/scheduler.py
- engine/diagnostics/fingerprint.py
- tests/security/test_injection.py
- engine/handlers.py
- tests/conftest.py
- tests/property/test_gates_property.py
- engine/auth/capabilities.py
- tests/contracts/test_contracts.py
- tests/integration/test_graph_driver.py
- engine/scoring/importance.py
- engine/contract_enforcement.py
- engine/inference_rule_registry.py
- engine/diagnostics/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/config/settings.py
- engine/startup_wiring.py
Comment on lines
+8
to
+13
| # --- Neo4j --- | ||
| NEO4J_URI=bolt://localhost:7687 | ||
| NEO4J_USERNAME=neo4j | ||
| NEO4J_PASSWORD=l9-dev-password | ||
| NEO4J_POOL_SIZE=20 | ||
| NEO4J_PASSWORD=change-me-in-production | ||
| NEO4J_DATABASE=neo4j | ||
| NEO4J_POOL_SIZE=50 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if settings.py expects L9_ prefixed Neo4j vars or unprefixed ones
rg -n "neo4j_uri|neo4j_password|NEO4J_" engine/config/settings.pyRepository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 311
🏁 Script executed:
#!/bin/bash
# Get full context of settings.py, focusing on how env vars are loaded
head -60 engine/config/settings.py | cat -nRepository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 2221
🏁 Script executed:
#!/bin/bash
# Check if there's any pydantic configuration or field aliases for env var mapping
rg -A 5 "class.*Settings|Config|env_prefix|alias" engine/config/settings.py | head -40Repository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 671
🏁 Script executed:
#!/bin/bash
# Search for any L9_ prefixed variables in the entire config directory
rg "L9_" engine/config/Repository: cryptoxdog/Cognitive.Engine.Graphs
Length of output: 995
Fix ENV-001 convention violation: add L9_ prefix to Neo4j environment variables.
The coding guideline ENV-001 requires all infrastructure environment variables to be prefixed with L9_, with merge blocked until compliance. The current .env.template and settings.py define Neo4j variables without this prefix (NEO4J_URI, NEO4J_PASSWORD, NEO4J_DATABASE, NEO4J_POOL_SIZE, etc. in the template; neo4j_uri, neo4j_password, neo4j_database, neo4j_pool_size in settings.py).
Rename these variables to use the L9_ prefix (e.g., L9_NEO4J_URI, L9_NEO4J_PASSWORD) in both .env.template and engine/config/settings.py to comply with the convention.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 11-11: [UnorderedKey] The NEO4J_PASSWORD key should go before the NEO4J_URI key
(UnorderedKey)
[warning] 12-12: [UnorderedKey] The NEO4J_DATABASE key should go before the NEO4J_PASSWORD key
(UnorderedKey)
[warning] 13-13: [UnorderedKey] The NEO4J_POOL_SIZE key should go before the NEO4J_URI key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.template around lines 8 - 13, Rename all Neo4j environment variable
names to include the required L9_ prefix in the .env.template (e.g., change
NEO4J_URI -> L9_NEO4J_URI, NEO4J_USERNAME -> L9_NEO4J_USERNAME, NEO4J_PASSWORD
-> L9_NEO4J_PASSWORD, NEO4J_DATABASE -> L9_NEO4J_DATABASE, NEO4J_POOL_SIZE ->
L9_NEO4J_POOL_SIZE) and then update engine/config/settings.py to read those new
names (update the env lookups that populate neo4j_uri, neo4j_username,
neo4j_password, neo4j_database, neo4j_pool_size to use os.getenv("L9_NEO4J_*")
or equivalent). Ensure any references elsewhere (config keys or tests) that
expect the old names are updated to the new L9_ variables to keep runtime
behavior unchanged.
Comment on lines
+32
to
+44
| import logging | ||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| if TYPE_CHECKING: | ||
| from engine.graph.driver import GraphDriver | ||
|
|
||
| from engine.graph_return_channel import ( | ||
| GraphToEnrichReturnChannel, | ||
| build_graph_inference_result_envelope, | ||
| ) | ||
| from engine.utils.security import sanitize_label | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Use structlog.get_logger(__name__) instead of logging.getLogger().
Engine code must use structlog for logging per coding guidelines. The standard library logging module is not permitted in engine/ code.
-import logging
+import structlog
from typing import TYPE_CHECKING, Any
@@
-logger = logging.getLogger(__name__)
+logger = structlog.get_logger(__name__)As per coding guidelines: "structlog.get_logger(name) for logging — never configure structlog in engine".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/gds/community_export.py` around lines 32 - 44, Replace use of the
standard logging module with structlog: remove the import of logging, add import
structlog, and change the logger instantiation from logging.getLogger(__name__)
to structlog.get_logger(__name__) in community_export.py (update the symbol
logger and keep all existing logging calls intact); do not add any structlog
configuration in this file.
| @@ -0,0 +1,437 @@ | |||
| """ | |||
There was a problem hiding this comment.
Pipeline failure: File needs ruff format.
Both CI pipelines report this file would be reformatted by ruff. Run ruff format tests/integration/test_startup_integration.py before merge.
🧰 Tools
🪛 GitHub Actions: Contract Enforcement
[error] 1-1: Ruff format --check: file would be reformatted.
🪛 GitHub Actions: Refactoring Safety Gate
[error] 1-1: ruff format --check failed. Ruff would reformat this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_startup_integration.py` at line 1, The file
test_startup_integration.py is not formatted to ruff rules; run ruff format on
the test_startup_integration.py module (or invoke your project's
formatter/pre-commit that runs ruff) to apply the formatting changes and re-run
the tests/CI so the pipeline stops flagging the file.
Comment on lines
+329
to
+334
| result = extract_per_field_confidence(fv) | ||
| assert result["mfi"] == 0.6 | ||
| assert result["hdpe_grade"] == 0.6 | ||
| assert "pass_number" not in result | ||
| assert "confidence" not in result | ||
|
|
There was a problem hiding this comment.
Avoid direct float equality comparisons.
Lines 330-331 use == for float comparison which is fragile due to floating-point precision. Use pytest.approx() instead.
- assert result["mfi"] == 0.6
- assert result["hdpe_grade"] == 0.6
+ assert result["mfi"] == pytest.approx(0.6)
+ assert result["hdpe_grade"] == pytest.approx(0.6)🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 330-330: Do not perform equality checks with floating point values.
[warning] 331-331: Do not perform equality checks with floating point values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_startup_integration.py` around lines 329 - 334, The
test currently asserts exact float equality on values returned by
extract_per_field_confidence (result["mfi"] and result["hdpe_grade"]); replace
those == comparisons with pytest.approx checks (e.g. assert result["mfi"] ==
pytest.approx(0.6) and assert result["hdpe_grade"] == pytest.approx(0.6)) to
avoid brittle float equality while leaving the absence checks ("pass_number",
"confidence") unchanged.
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
Integration pass across the CEG engine layer — boot sequence, settings, startup wiring, GDS community export, and startup integration tests.
Changed Files
engine/boot.pyengine/config/settings.pyengine/startup_wiring.py.env.templateNew Files
engine/gds/community_export.pytests/integration/test_startup_integration.pyStats
6 files — 887 insertions, 117 deletions
Summary by CodeRabbit
New Features
Tests
Chores