fixes #73
fixes #73
Conversation
📝 WalkthroughWalkthroughInject Settings via DI across app and services; centralize test_settings in tests; introduce SecurityService CSRF methods and CSRFMiddleware; propagate Settings into metrics, tracing, Kafka/SSE, providers, pod-monitor; convert tests to fixture-driven authenticated AsyncClient and tighten typing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TR as Test Runner
participant TS as test_settings (Settings)
participant App as FastAPI App
participant DI as DI Container / Providers
participant Sec as SecurityService
participant Client as Test AsyncClient
participant Prod as UnifiedProducer
participant Kafka as Kafka Cluster
TR->>TS: instantiate session-scoped Settings (.env.test)
TR->>App: start app with test_settings=TS
App->>DI: build container/providers using TS
DI->>Sec: construct SecurityService(settings=TS)
App->>App: register CSRFMiddleware (uses SecurityService)
Client->>App: POST /api/v1/auth/register (username,password)
App->>Sec: hash password / create user credentials
App->>Client: return set-cookie + CSRF token
Client->>App: POST protected endpoint with X-CSRF-Token header
App->>Sec: validate CSRF via validate_csrf_from_request
App->>Prod: produce event (topic prefix from TS)
Prod->>Kafka: send message
Kafka-->>Prod: ack
App->>Client: respond to API request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/tests/conftest.py:
- Around line 73-78: The app fixture currently instantiates TestSettings()
directly instead of using the test_settings fixture that generates the unique
Kafka topic prefix; update the app fixture to accept the test_settings fixture
(i.e., add test_settings as a parameter to the app fixture) and use that
TestSettings instance when constructing the FastAPI application so the app (and
derived fixtures like client, scope, db) inherit the unique KAFKA_TOPIC_PREFIX
rather than a fresh TestSettings().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/conftest.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/services/sse/kafka_redis_bridge.py">
<violation number="1" location="backend/app/services/sse/kafka_redis_bridge.py:65">
P1: Missing separator between base group_id and suffix. The original code used `.` separator for group_id and `-` for client_id. This is inconsistent with all other usages in the codebase (e.g., `coordinator.py`, `saga_orchestrator.py`, `notification_service.py`) which all use `f"{base}.{suffix}"` pattern.</violation>
</file>
<file name="backend/tests/conftest.py">
<violation number="1" location="backend/tests/conftest.py:25">
P2: Session ID doesn't check `PYTEST_SESSION_ID` environment variable. Consider using `os.environ.get("PYTEST_SESSION_ID") or uuid.uuid4().hex[:8]` to match the PR description and preserve the ability for CI to inject a consistent session ID.</violation>
</file>
<file name="backend/app/core/middlewares/metrics.py">
<violation number="1" location="backend/app/core/middlewares/metrics.py:125">
P2: Removing `OTEL_SDK_DISABLED` check breaks compatibility with the standard OpenTelemetry configuration. This environment variable is an official OTel SDK option to disable all signals. Users who have set `OTEL_SDK_DISABLED=true` (common in local dev or specific deployments) will unexpectedly have metrics enabled after this change, potentially causing connection timeouts or overhead. Consider keeping the standard env var check alongside `settings.TESTING`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/core/middlewares/metrics.py (1)
123-136: Fix misleading comment + unreachableservice.environment="test"branchBecause you
returnonsettings.TESTING(Line 125-127),"service.environment": "test" if settings.TESTING else "production"(Line 134) will always resolve to"production". Also, the comment “or when explicitly disabled” is no longer true.Proposed fix
def setup_metrics(app: FastAPI, logger: logging.Logger) -> None: """Set up OpenTelemetry metrics with OTLP exporter.""" settings = get_settings() - # Fast opt-out for tests or when explicitly disabled + # Fast opt-out for tests if settings.TESTING: logger.info("OpenTelemetry metrics disabled (TESTING)") return # Configure OpenTelemetry resource + # NOTE: If you want "test" here, you can't return early above. resource = Resource.create( { SERVICE_NAME: settings.PROJECT_NAME, SERVICE_VERSION: "1.0.0", - "service.environment": "test" if settings.TESTING else "production", + "service.environment": "production", } )
🤖 Fix all issues with AI agents
In @backend/app/core/middlewares/metrics.py:
- Around line 138-142: The fallback endpoint for OTLP metrics uses a bare
"localhost:4317"; change the default to include the HTTP scheme by setting
endpoint = settings.OTEL_EXPORTER_OTLP_ENDPOINT or "http://localhost:4317" so
OTLPMetricExporter(endpoint=endpoint, insecure=True) uses the explicit
"http://..." format (refer to the endpoint variable and OTLPMetricExporter in
metrics.py and settings.OTEL_EXPORTER_OTLP_ENDPOINT).
In @backend/app/services/sse/kafka_redis_bridge.py:
- Around line 64-66: Update the group naming in kafka_redis_bridge.py to include
the dot separator before the KAFKA_GROUP_SUFFIX: change how group_id and
client_id are constructed so they append a "." before
self.settings.KAFKA_GROUP_SUFFIX (i.e., adjust the expressions that set group_id
and client_id to use a dot separator), ensuring they match the convention used
elsewhere (see group_id and client_id variables in this file and the
KAFKA_GROUP_SUFFIX setting).
In @backend/scripts/seed_users.py:
- Around line 28-35: The file docstring's Environment Variables section is
missing DATABASE_NAME; update the top-level docstring to document DATABASE_NAME
(matching SeedSettings.database_name) as an available env var, include a short
description like "database name for the application" and its default value
("integr8scode_db"), and ensure the name matches the code (DATABASE_NAME) so
environment users can find and override SeedSettings.database_name.
🧹 Nitpick comments (2)
backend/app/core/dishka_lifespan.py (1)
54-54: Minor:enable_console_exporterparameter is always False in this context.Since Line 48 already ensures
not settings.TESTING, theenable_console_exporter=settings.TESTINGparameter on Line 54 will always evaluate toFalse. This parameter could be simplified toenable_console_exporter=Falsefor clarity.♻️ Simplify the parameter
- enable_console_exporter=settings.TESTING, + enable_console_exporter=False,backend/tests/conftest.py (1)
22-37: Strong test isolation implementation.The test_settings fixture correctly implements per-session and per-worker isolation by:
- Loading base configuration from .env.test
- Generating unique identifiers (session_id, worker-specific values)
- Creating isolated resources (DATABASE_NAME, REDIS_DB, Kafka topics/groups)
- Using Pydantic's model_copy for clean Settings customization
The worker_num calculation at line 27 (
sum(_WORKER_ID.encode()) % 16) is a simple deterministic hash to distribute workers across Redis databases 0-15. While unconventional, it's functional and consistent.Optional: Add clarifying comment for worker_num calculation
+ # Deterministic mapping of worker ID to Redis DB (0-15) worker_num = sum(_WORKER_ID.encode()) % 16
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/.env.testbackend/app/core/dishka_lifespan.pybackend/app/core/middlewares/metrics.pybackend/app/events/schema/schema_registry.pybackend/app/services/sse/kafka_redis_bridge.pybackend/app/settings.pybackend/pyproject.tomlbackend/scripts/seed_users.pybackend/tests/conftest.pybackend/tests/integration/events/test_admin_utils.pybackend/tests/load/config.py
💤 Files with no reviewable changes (1)
- backend/.env.test
🧰 Additional context used
🧬 Code graph analysis (4)
backend/tests/integration/events/test_admin_utils.py (2)
backend/tests/conftest.py (2)
app(42-53)test_settings(22-37)backend/app/settings.py (1)
Settings(11-161)
backend/app/core/dishka_lifespan.py (2)
backend/app/core/tracing/config.py (1)
init_tracing(177-197)backend/app/core/tracing/models.py (2)
has_failures(61-63)get_summary(57-59)
backend/tests/load/config.py (2)
frontend/src/lib/api/core/params.gen.ts (1)
Field(5-36)backend/app/schemas_pydantic/replay.py (1)
duration_seconds(57-60)
backend/tests/conftest.py (2)
backend/app/settings.py (1)
Settings(11-161)backend/app/main.py (1)
create_app(44-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Backend
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (12)
backend/pyproject.toml (1)
213-213: Change looks good—OpenTelemetry is properly disabled during tests.The
TESTINGflag is correctly read from the environment variable set in pytest configuration and explicitly disables OpenTelemetry metrics inbackend/app/core/middlewares/metrics.py(line 125) and prevents tracing initialization inbackend/app/core/dishka_lifespan.py(line 48). This centralized approach is cleaner than the previousOTEL_SDK_DISABLEDflag and properly prevents telemetry overhead during test runs.backend/scripts/seed_users.py (1)
81-83: LGTM! Clean migration to Settings pattern.The refactor from
os.getenv()to PydanticBaseSettingsis well-executed. The settings instance is created once per invocation, and all configuration values (URLs, database name, passwords) are now sourced consistently from theSeedSettingsobject.Also applies to: 95-95, 105-105
backend/app/core/dishka_lifespan.py (1)
47-72: LGTM! Tracing isolation for tests looks correct.The conditional initialization of tracing based on
ENABLE_TRACINGandnot TESTINGappropriately prevents exporter retries during tests, which aligns with the PR's goal of fixing flaky tests. The failure checking and separate logging paths are well-structured.backend/tests/load/config.py (2)
12-42: LGTM! Clean migration to Pydantic BaseSettings.The conversion from dataclass to Pydantic
BaseSettingsis well-executed:
- Environment variable loading with
LOAD_prefix is correctly configuredvalidation_aliasis appropriately used for backward compatibility with existing env var names- All fields have explicit types and sensible defaults
- The
api()method logic remains unchangedThis refactoring improves maintainability and aligns with the broader PR pattern of centralizing configuration through Settings objects.
5-6: pydantic-settings is already in project dependencies.
pydantic-settings==2.5.2is already declared inbackend/pyproject.toml, so no further action is needed.Likely an incorrect or invalid review comment.
backend/app/events/schema/schema_registry.py (1)
62-62: LGTM! Settings-based configuration for schema subject prefix.The change from reading
SCHEMA_SUBJECT_PREFIXvia environment variable to usingsettings.SCHEMA_SUBJECT_PREFIXcorrectly centralizes configuration and enables the per-session/worker test isolation described in the PR objectives.backend/app/settings.py (1)
76-76: LGTM! New setting for schema subject prefix isolation.The addition of
SCHEMA_SUBJECT_PREFIXwith an empty string default is appropriate. This field enables per-session/worker isolation for Kafka schema subjects during tests while keeping production behavior unchanged (no prefix).backend/tests/integration/events/test_admin_utils.py (2)
4-4: LGTM: Import addition supports test isolation.The Settings import is required for the updated test signature and enables proper type hinting for the test_settings fixture.
11-12: Excellent: Settings-based test isolation implemented.The switch from environment variables to the test_settings fixture enables proper per-session and per-worker topic isolation, directly addressing the flaky test issues mentioned in the PR objectives. The topic construction correctly combines the unique PREFIX and GROUP_SUFFIX to ensure test runs don't interfere with each other.
backend/tests/conftest.py (3)
17-17: LGTM: Standard pytest-xdist worker identification.The worker ID extraction with a sensible default ("gw0") ensures the fixture works correctly both in parallel (pytest-xdist) and single-worker test runs.
42-48: Clean integration with test_settings fixture.The app fixture now correctly receives and uses the test_settings instance, ensuring the FastAPI application is configured with the same isolated settings used throughout the test session. This maintains consistency across all test fixtures and components.
66-71: LGTM: Improved readability.The multi-line formatting of the AsyncClient initialization enhances readability without any functional changes.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/core/metrics/base.py">
<violation number="1" location="backend/app/core/metrics/base.py:54">
P2: Removing the `config.otlp_endpoint` check means metrics export will be attempted even when no endpoint is configured. This could cause connection errors to the default `localhost:4317` endpoint and spawn unnecessary background threads. Consider preserving the endpoint check:
```python
if settings.TESTING or not settings.ENABLE_TRACING or not config.otlp_endpoint:
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/core/metrics/base.py (2)
42-79: Resource leak: MeterProvider and background threads are not cleaned up.The
_create_metermethod creates aPeriodicExportingMetricReader(line 61-64) that starts a background thread for periodic metric export, and aSdkMeterProvider(line 67) that manages these resources. However:
- The
MeterProvideris not stored as an instance variable- The
close()method (lines 76-79) does nothing to shut down the provider or stop background threadsThis causes resource leaks when
BaseMetricsinstances are destroyed, as the export threads continue running.♻️ Proposed fix to store and clean up MeterProvider
def _create_meter(self, config: MetricsConfig, meter_name: str) -> Meter: """Create a new meter instance for this collector. Args: config: Metrics configuration meter_name: Name for this meter Returns: A new meter instance """ # If tracing/metrics disabled or no OTLP endpoint configured, use NoOp meter to avoid threads/network settings = get_settings() if settings.TESTING or not settings.ENABLE_TRACING: + self._meter_provider = None return NoOpMeterProvider().get_meter(meter_name) resource = Resource.create( {"service.name": config.service_name, "service.version": config.service_version, "meter.name": meter_name} ) reader = PeriodicExportingMetricReader( exporter=OTLPMetricExporter(endpoint=config.otlp_endpoint), export_interval_millis=config.export_interval_millis, ) # Each collector gets its own MeterProvider meter_provider = SdkMeterProvider(resource=resource, metric_readers=[reader]) + self._meter_provider = meter_provider # Return a meter from this provider return meter_provider.get_meter(meter_name) def _create_instruments(self) -> None: """Create metric instruments. Override in subclasses.""" pass def close(self) -> None: """Close the metrics collector and clean up resources.""" - # Subclasses can override if they need cleanup - pass + if self._meter_provider is not None: + self._meter_provider.shutdown()
54-70: Add validation for missing OTLP endpoint before creating exporter.The condition at line 54 returns a no-op meter when testing or tracing is disabled, but it does not validate that
config.otlp_endpointis configured. According to the documented behavior (metrics-reference.md), a no-op meter should be used when "no OTLP endpoint is configured," but the code only checks theENABLE_TRACINGflag. IfENABLE_TRACING=true,TESTING=false, andOTEL_EXPORTER_OTLP_ENDPOINTis not set (defaults toNone), line 62 will instantiateOTLPMetricExporter(endpoint=None), causing a runtime error.Add an endpoint check to the conditional:
-if settings.TESTING or not settings.ENABLE_TRACING: +if settings.TESTING or not settings.ENABLE_TRACING or not config.otlp_endpoint: return NoOpMeterProvider().get_meter(meter_name)This aligns the implementation with the documented behavior and matches the pattern used elsewhere in the codebase (e.g., middlewares/metrics.py line 140).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/.env.testbackend/app/core/metrics/base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/.env.test
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/services/sse/kafka_redis_bridge.py">
<violation number="1" location="backend/app/services/sse/kafka_redis_bridge.py:65">
P2: Inconsistent separator used before `KAFKA_GROUP_SUFFIX`. The rest of the codebase uses `.` (dot) as the separator, but this change uses `-` (hyphen). Consider using `.{suffix}` for consistency with other Kafka consumer group naming patterns.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/app/core/middlewares/metrics.py:
- Line 140: The code calls OTLPMetricExporter with
settings.OTEL_EXPORTER_OTLP_ENDPOINT but Settings defaults that value to None;
add fallback logic so OTLPMetricExporter receives a concrete endpoint (e.g., use
settings.OTEL_EXPORTER_OTLP_ENDPOINT or "localhost:4317") or, if you prefer not
to add a fallback, update the comment above line 140 to state that the endpoint
must be explicitly configured and no default is applied; reference the
OTLPMetricExporter instantiation and settings.OTEL_EXPORTER_OTLP_ENDPOINT when
making the change.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/core/middlewares/metrics.pybackend/app/core/providers.pybackend/app/events/core/producer.pybackend/app/services/sse/kafka_redis_bridge.pybackend/scripts/seed_users.pybackend/tests/integration/events/test_producer_roundtrip.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/scripts/seed_users.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/tests/integration/events/test_producer_roundtrip.py (3)
backend/tests/conftest.py (2)
scope(82-84)test_settings(22-37)backend/app/events/core/producer.py (1)
UnifiedProducer(30-290)backend/app/events/core/types.py (1)
ProducerConfig(29-60)
backend/app/core/providers.py (1)
backend/app/events/core/producer.py (2)
UnifiedProducer(30-290)producer(65-66)
backend/app/events/core/producer.py (1)
backend/app/settings.py (2)
Settings(11-161)get_settings(165-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (4)
backend/app/services/sse/kafka_redis_bridge.py (1)
64-66: No changes needed. The KAFKA_GROUP_SUFFIX is properly defined with defaults.The settings-based approach correctly supports test isolation.
KAFKA_GROUP_SUFFIXis defined inbackend/app/settings.pyas a requiredstrfield with default value"suff", and tests override it with a session-and-worker-based suffix (f"{session_id}.{_WORKER_ID}"). The field is type-hinted asstr(notstr | None), so it cannot beNoneor empty. The code at lines 64–66 is correct and will never produce malformed Kafka consumer identifiers.backend/app/core/providers.py (1)
158-158: LGTM! Settings injection enables test isolation.The addition of
settings=settingsto theUnifiedProducerinitialization correctly propagates the injected Settings instance, enabling per-session/per-worker topic prefix isolation as described in the PR objectives.backend/tests/integration/events/test_producer_roundtrip.py (1)
17-26: LGTM! Test isolation properly implemented.The test signature and producer initialization correctly use
test_settingsto derive dynamic bootstrap servers and topic prefixes, enabling the per-session/per-worker isolation described in the PR objectives.backend/app/events/core/producer.py (1)
37-50: LGTM! Clean settings injection with backward compatibility.The optional
settingsparameter with fallback toget_settings()maintains backward compatibility while enabling test-specific configuration injection. The topic prefix resolution correctly uses the provided settings when available.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/events/core/producer.py">
<violation number="1" location="backend/app/events/core/producer.py:37">
P1: Using `get_settings()` as a default argument evaluates it at module import time, not at call time. This means all `UnifiedProducer` instances will share the same Settings captured when the module was loaded, defeating the test isolation goals of this PR. Use `None` as default and call `get_settings()` at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/app/dlq/manager.py (2)
264-267: Retry policy lookup: normalization + default fallback looks correct; consider a tiny cleanupBehavior-wise this is solid (lookup by normalized topic, then fallback). If you want to keep it compact/readable:
Optional simplification
- retry_policy = self._retry_policies.get(self._normalize_topic(message.original_topic)) - retry_policy = retry_policy or self.default_retry_policy + retry_policy = ( + self._retry_policies.get(self._normalize_topic(message.original_topic)) + or self.default_retry_policy + )
438-440: Store key normalization is good, but keepRetryPolicy.topicconsistent with the key (debuggability)Since you’re normalizing the dict key, consider also aligning the
RetryPolicy.topicvalue with that normalized key (or at least asserting they match). Otherwise, inspecting/logging policies can be confusing (policy says"test123.orders"but it’s stored under"orders", or vice versa).One way to enforce consistency (shape depends on RetryPolicy mutability)
def set_retry_policy(self, topic: str, policy: RetryPolicy) -> None: - self._retry_policies[self._normalize_topic(topic)] = policy + key = self._normalize_topic(topic) + # Option A: assert to keep invariant strict + # assert policy.topic == key, f"RetryPolicy.topic={policy.topic} must match normalized key={key}" + # + # Option B: normalize the policy's topic too (if mutable / supported by your model) + # policy.topic = key + self._retry_policies[key] = policy
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/core/container.pybackend/app/core/providers.pybackend/app/dlq/manager.pybackend/app/events/core/producer.pybackend/app/services/sse/kafka_redis_bridge.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/services/sse/kafka_redis_bridge.py
- backend/app/events/core/producer.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/core/container.py (1)
backend/app/core/providers.py (1)
SagaOrchestratorProvider(690-695)
backend/app/core/providers.py (1)
backend/app/events/core/producer.py (2)
UnifiedProducer(30-289)producer(64-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (2)
backend/app/core/container.py (1)
38-57: VerifySagaOrchestratorProvider()scope/side-effects in the main app container (shared singleton across requests).
AddingSagaOrchestratorProvider()here means the web app container can now resolve (and potentially initialize) the saga orchestrator atScope.APP, which may be shared across concurrent requests. Please confirm that’s intended (thread-safety / no request-specific mutable state) and that initializing it at web-app startup won’t introduce unwanted startup cost or background tasks.backend/app/core/providers.py (1)
153-160: Good: passsettingsintoUnifiedProducerfor deterministic topic-prefixing; verify constructor/API compatibility.
This makes the producer’s topic prefix come from injectedSettings(vs ambient/global settings), which should help test isolation. Please double-checkUnifiedProducer’s constructor signature and that overriding settings here can’t accidentally diverge from other producer configuration (e.g., any settings-derived behavior beyondProducerConfig).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/integration/test_dlq_routes.py (1)
131-143: Use test isolation prefix for topic filtering test.The test uses hardcoded topic
"execution-events"without the test isolation prefix. Since other DLQ tests (test_dlq_retry_immediate, test_dlq_discard_policy, test_dlq_manager) all build topic names usingtest_settings.KAFKA_TOPIC_PREFIX, messages in the DLQ database are stored with prefixed topic names. The filtering does exact matching onoriginal_topic, so filtering by the unprefixed name returns no results. The assertion on line 143 never executes.Add
test_settingsparameter and usef"{test_settings.KAFKA_TOPIC_PREFIX}execution-events"instead of the hardcoded topic name.
🤖 Fix all issues with AI agents
In @backend/app/events/core/producer.py:
- Around line 9-19: The imports in producer.py are unsorted and include an
unused get_settings import causing ruff errors I001 and F401; reorder imports
into stdlib, third‑party, first‑party, then local groups (e.g., keep
confluent_kafka imports together with Producer and KafkaError in the third‑party
group), remove the unused get_settings symbol, and ensure first‑party imports
like app.core.lifecycle.LifecycleEnabled,
app.core.metrics.context.get_event_metrics,
app.dlq.models.DLQMessage/DLQMessageStatus, app.domain.enums.kafka.KafkaTopic,
app.events.schema.schema_registry.SchemaRegistryManager,
app.infrastructure.kafka.events.BaseEvent, app.settings.Settings, and local
.types imports (ProducerConfig, ProducerMetrics, ProducerState) are in the
correct group and sorted to satisfy ruff.
🧹 Nitpick comments (3)
backend/app/events/core/producer.py (2)
29-49: Settings injection looks right; consider letting the formatter handle indentation.
Requiringsettings: Settingsand deriving_topic_prefixfrom it aligns with the test-isolation goal. The parameter indentation in__init__looks off relative to typical black/ruff-format output—worth running the formatter to avoid churn.
174-176: Signature indentation drift inproduce/send_to_dlq—prefer black-style wrapping for stability.
This is cosmetic, but matching formatter output reduces future diffs.Example formatting
- async def produce( - self, event_to_produce: BaseEvent, key: str | None = None, headers: dict[str, str] | None = None - ) -> None: + async def produce( + self, + event_to_produce: BaseEvent, + key: str | None = None, + headers: dict[str, str] | None = None, + ) -> None: @@ - async def send_to_dlq( - self, original_event: BaseEvent, original_topic: str, error: Exception, retry_count: int = 0 - ) -> None: + async def send_to_dlq( + self, + original_event: BaseEvent, + original_topic: str, + error: Exception, + retry_count: int = 0, + ) -> None:Also applies to: 207-209
backend/tests/integration/test_dlq_routes.py (1)
35-35: Consider clarifying the purpose of thetest_userparameter.The
test_userparameter appears in all test method signatures but is never explicitly used in the test bodies. If this fixture provides authentication via side effects (e.g., setting up auth tokens in the client), consider adding a brief comment to clarify this pattern. If it's genuinely unused, it could be removed.Also applies to: 80-80, 116-116, 131-131, 146-146, 186-186, 198-198, 222-222, 259-259, 290-290, 328-328, 358-358
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/events/core/producer.pybackend/tests/integration/test_dlq_routes.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/integration/test_dlq_routes.py (1)
backend/tests/conftest.py (1)
test_settings(22-37)
backend/app/events/core/producer.py (4)
backend/app/settings.py (2)
Settings(11-161)get_settings(165-166)backend/app/events/core/types.py (1)
ProducerConfig(29-60)backend/app/events/schema/schema_registry.py (1)
SchemaRegistryManager(53-229)backend/app/infrastructure/kafka/events/base.py (1)
BaseEvent(13-37)
🪛 GitHub Actions: Ruff Linting
backend/app/events/core/producer.py
[error] 1-19: I001 Import block is un-sorted or un-formatted. Organize imports (ruff) for the step: uv run ruff check . --config pyproject.toml
[error] 15-15: F401 'app.settings.get_settings' imported but unused. Remove unused import.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Backend
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (2)
backend/tests/integration/test_dlq_routes.py (2)
35-35: LGTM: Type annotations modernized to Python 3.9+ style.The update from
Dict[str, str]todict[str, str]uses built-in generic types, which is the idiomatic approach for Python 3.9+.Also applies to: 80-80, 116-116, 131-131, 146-146, 186-186, 198-198, 222-222, 259-259, 290-290, 328-328, 358-358
198-210: LGTM: Test isolation implemented correctly.The addition of
test_settingsparameter and the prefixed topic construction at line 204 properly implements per-session and per-worker test isolation, which aligns with the PR's goal to fix flaky tests.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/dlq/manager.py">
<violation number="1">
P2: Removing topic normalization may cause retry policy lookup mismatches when `original_topic` in messages contains the Kafka prefix but policies are registered without it (or vice versa). The previous `_normalize_topic` method ensured both sides used unprefixed topic names for consistent matching. Consider ensuring that either: (1) all `original_topic` values are stored without prefix, and policies are set without prefix, or (2) restore normalization to handle inconsistencies gracefully.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/tests/integration/test_events_routes.py (1)
310-325: Fix CSRF header in fixture-auth tests: usetest_user["headers"]on mutating requests.
The test callsclient.post("/api/v1/events/publish", json=publish_request)without passingtest_user["headers"]. Since the backend enforces CSRF on POST/PUT/DELETE for authenticated sessions, this request fails CSRF validation before reaching the authorization check. Addheaders=test_user["headers"]to all mutating requests in fixture-auth tests.backend/tests/integration/test_user_settings_routes.py (1)
343-382: Assert login/logout success and replace fragile default-based comparison with user_id check.The test relies on fixture execution order without verifying login success, and the final assertion uses OR logic (
!= theme OR != timezone) which only needs one condition to pass—this can flake if defaults coincidentally match your chosen values. Additionally, the comment acknowledges the fixture order dependency but doesn't eliminate it robustly.Asserting login/logout status codes and comparing
user_idfields makes the test deterministic and independent of default settings.Proposed patch
async def test_settings_isolation_between_users(self, client: AsyncClient, test_user: Dict[str, str], another_user: Dict[str, str]) -> None: """Test that settings are isolated between users.""" - # Login as first user (fixture logs in another_user last, so re-login as test_user) + # Login as first user (do not rely on fixture execution order) login_data = { "username": test_user["username"], "password": test_user["password"] } - await client.post("/api/v1/auth/login", data=login_data) + r_login_1 = await client.post("/api/v1/auth/login", data=login_data) + assert r_login_1.status_code == 200 # Update first user's settings user1_update = { "theme": "dark", "timezone": "America/New_York" } response = await client.put("/api/v1/user/settings/", json=user1_update) assert response.status_code == 200 + user1_settings = UserSettings(**response.json()) # Log out - await client.post("/api/v1/auth/logout") + r_logout = await client.post("/api/v1/auth/logout") + assert r_logout.status_code in (200, 204) # Login as second user login_data = { "username": another_user["username"], "password": another_user["password"] } - await client.post("/api/v1/auth/login", data=login_data) + r_login_2 = await client.post("/api/v1/auth/login", data=login_data) + assert r_login_2.status_code == 200 # Get second user's settings response = await client.get("/api/v1/user/settings/") assert response.status_code == 200 - user2_settings = response.json() + user2_settings = UserSettings(**response.json()) - # Verify second user's settings are not affected by first user's changes - # Second user should have default settings, not the first user's custom settings - assert user2_settings["theme"] != user1_update["theme"] or user2_settings["timezone"] != user1_update[ - "timezone"] + # Verify isolation via identity (robust across varying defaults) + assert user2_settings.user_id != user1_settings.user_idbackend/tests/integration/test_admin_routes.py (1)
30-60: Avoid asserting hard-coded default values; prefer validating type, constraints, and valid ranges instead.The test asserts exact values for monitoring settings (e.g.,
enable_tracing is True,sampling_rate == 0.1), but these are hard-coded Pydantic field defaults that may change or be configurable. While.env.testconfiguration doesn't affect these specific admin settings defaults, the test is still brittle by coupling to exact values rather than validating that returned values conform to their schema constraints.Example (less brittle) assertions
- assert settings.monitoring_settings.enable_tracing is True - assert settings.monitoring_settings.sampling_rate == 0.1 + assert isinstance(settings.monitoring_settings.enable_tracing, bool) + assert isinstance(settings.monitoring_settings.sampling_rate, (int, float)) + assert 0.0 <= settings.monitoring_settings.sampling_rate <= 1.0backend/tests/conftest.py (1)
99-139: Auth fixtures: don't silently accept 400 on register + set CSRF header on authenticated client.Accepting
400on register can hide real regressions (e.g., if validation tightens unexpectedly). More critically, the CSRF token returned from login must be set onclient.headers["X-CSRF-Token"]because the API enforces CSRF validation on POST/PUT/PATCH/DELETE requests, and the current fixtures leave it unset—this will cause authenticated requests to fail CSRF checks.Proposed patch
async def _http_login(client: httpx.AsyncClient, username: str, password: str) -> str: data = {"username": username, "password": password} resp = await client.post("/api/v1/auth/login", data=data) resp.raise_for_status() - return resp.json().get("csrf_token", "") + token = resp.json().get("csrf_token", "") + if not token: + raise RuntimeError("Login succeeded but no csrf_token returned") + return token @pytest_asyncio.fixture async def test_user(client: httpx.AsyncClient): """Function-scoped authenticated user.""" uid = uuid.uuid4().hex[:8] creds = { "username": f"test_user_{uid}", "email": f"test_user_{uid}@example.com", "password": "TestPass123!", "role": "user", } r = await client.post("/api/v1/auth/register", json=creds) - if r.status_code not in (200, 201, 400): + if r.status_code not in (200, 201): pytest.fail(f"Cannot create test user (status {r.status_code}): {r.text}") csrf = await _http_login(client, creds["username"], creds["password"]) + client.headers.update({"X-CSRF-Token": csrf}) return {**creds, "csrf_token": csrf, "headers": {"X-CSRF-Token": csrf}} @pytest_asyncio.fixture async def test_admin(client: httpx.AsyncClient): """Function-scoped authenticated admin.""" uid = uuid.uuid4().hex[:8] creds = { "username": f"admin_user_{uid}", "email": f"admin_user_{uid}@example.com", "password": "AdminPass123!", "role": "admin", } r = await client.post("/api/v1/auth/register", json=creds) - if r.status_code not in (200, 201, 400): + if r.status_code not in (200, 201): pytest.fail(f"Cannot create test admin (status {r.status_code}): {r.text}") csrf = await _http_login(client, creds["username"], creds["password"]) + client.headers.update({"X-CSRF-Token": csrf}) return {**creds, "csrf_token": csrf, "headers": {"X-CSRF-Token": csrf}}
🧹 Nitpick comments (1)
backend/tests/conftest.py (1)
63-73: HTTP client: consider whetherfollow_redirects=Trueis desirable for integration assertions.
If any endpoint accidentally redirects (302/307), this setting can mask issues by auto-following. If you want strictness, disable it; otherwise OK.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/app/events/core/producer.pybackend/tests/conftest.pybackend/tests/e2e/test_execution_routes.pybackend/tests/integration/test_admin_routes.pybackend/tests/integration/test_events_routes.pybackend/tests/integration/test_health_routes.pybackend/tests/integration/test_notifications_routes.pybackend/tests/integration/test_replay_routes.pybackend/tests/integration/test_saga_routes.pybackend/tests/integration/test_saved_scripts_routes.pybackend/tests/integration/test_user_settings_routes.py
💤 Files with no reviewable changes (6)
- backend/tests/integration/test_saved_scripts_routes.py
- backend/tests/e2e/test_execution_routes.py
- backend/tests/integration/test_replay_routes.py
- backend/tests/integration/test_health_routes.py
- backend/tests/integration/test_saga_routes.py
- backend/tests/integration/test_notifications_routes.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/events/core/producer.py (1)
backend/app/events/core/types.py (1)
ProducerConfig(29-60)
backend/tests/integration/test_user_settings_routes.py (1)
backend/tests/conftest.py (1)
another_user(142-162)
backend/tests/conftest.py (2)
backend/app/settings.py (1)
Settings(11-161)backend/app/main.py (1)
create_app(44-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (6)
backend/app/events/core/producer.py (2)
49-49: Good architectural improvement for test isolation.Using the injected settings instance for topic prefix configuration supports per-session/worker isolation and improves testability by eliminating runtime dependencies on global settings.
32-37: All instantiation sites correctly pass the settings parameter. The breaking change has been properly implemented across the codebase.backend/tests/conftest.py (2)
41-54: App lifecycle: good move passingtest_settingsintocreate_app, and closing Dishka container.
This should reduce cross-worker config bleed and resource leakage.
21-37: Remove redundant% 16modulo onREDIS_DBassignment.The code applies modulo 16 twice: once in
worker_num = sum(_WORKER_ID.encode()) % 16and again inREDIS_DB: worker_num % 16. The second modulo is redundant and should be removed. Additionally, note that with pytest-xdist runs exceeding 16 workers, Redis database collisions are unavoidable given Redis's default 16-database limit (0–15).Suggested fix
@pytest.fixture(scope="session") def test_settings() -> Settings: """Provide test settings with a unique Kafka topic prefix for isolation.""" base = Settings(_env_file=".env.test", _env_file_encoding="utf-8") session_id = uuid.uuid4().hex[:8] base_prefix = f"{base.KAFKA_TOPIC_PREFIX.rstrip('.')}." worker_num = sum(_WORKER_ID.encode()) % 16 unique_prefix = f"{base_prefix}{session_id}.{_WORKER_ID}." return base.model_copy( update={ "DATABASE_NAME": f"integr8scode_test_{session_id}_{_WORKER_ID}", - "REDIS_DB": worker_num % 16, + "REDIS_DB": worker_num, "KAFKA_GROUP_SUFFIX": f"{session_id}.{_WORKER_ID}", "SCHEMA_SUBJECT_PREFIX": f"test.{session_id}.{_WORKER_ID}.", "KAFKA_TOPIC_PREFIX": unique_prefix, } )backend/tests/integration/test_admin_routes.py (1)
111-121: No issues found; test is correct.The
test_userfixture properly establishes both authentication and CSRF state: it logs in via_http_login()(which sets session cookies and retrieves the CSRF token), then returns the CSRF token in the dict. However, CSRF validation is skipped for GET requests per the security implementation (seevalidate_csrf_token()inbackend/app/core/security.py, which exempts GET/HEAD/OPTIONS methods). The expected 403 status is correct and comes from theadmin_userdependency enforcing the admin role requirement, not from missing CSRF setup.Likely an incorrect or invalid review comment.
backend/tests/integration/test_events_routes.py (1)
383-399: CSRF validation is not currently enforced on these endpoints.The DELETE and POST routes (
delete_eventat line 277 andreplay_aggregate_eventsat line 308 ofbackend/app/api/routes/events.py) only depend onadmin_userfor authorization. Thevalidate_csrf_tokendependency defined inbackend/app/core/security.pyis never applied to these endpoints, so CSRF token validation does not occur. The tests correctly return 403 for non-admin users due to role-based access control, not CSRF failures. The fixture infrastructure is sound (httpx.AsyncClient persists auth cookies automatically), but CSRF enforcement is not active on these routes.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/integration/idempotency/test_consumer_idempotent.py (1)
62-63: Consider polling for consumer readiness instead of a fixed sleep.While the 2-second sleep addresses the flaky test, a fixed delay may still fail on slower CI systems or waste time on faster ones. Consider polling for partition assignments instead:
♻️ Suggested more robust approach
await wrapper.start([KafkaTopic.EXECUTION_EVENTS]) -# Allow time for consumer to join group and get partition assignments -await asyncio.sleep(2) + +# Wait for consumer to join group and get partition assignments +async def _consumer_ready(): + assignments = wrapper.consumer.assignment() + assert assignments, "Consumer should have partition assignments" + +await eventually(_consumer_ready, timeout=5.0, interval=0.1)This approach verifies the consumer is actually ready rather than assuming a fixed duration is sufficient.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/integration/idempotency/test_consumer_idempotent.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/integration/idempotency/test_consumer_idempotent.py (1)
backend/tests/helpers/events.py (1)
make_execution_requested_event(8-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (1)
backend/tests/integration/idempotency/test_consumer_idempotent.py (1)
14-15: LGTM!The import reorganization is clean and maintains the same functionality.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @backend/tests/integration/events/test_consume_roundtrip.py:
- Around line 16-18: The xdist_group("kafka_consumers") (and similar
kafka_consumers, dlq, user_settings) markers are ineffective because
pyproject.toml uses addopts = "-n 4 --dist loadfile"; change the pytest config
to use --dist=loadgroup (update pyproject.toml addopts to "-n 4 --dist
loadgroup") so the xdist_group markers are honored, or alternatively remove the
xdist_group(...) markers from the affected test files if you prefer to keep
--dist=loadfile; update whichever of these two places (the xdist_group markers
in the test files or the addopts line in pyproject.toml) to make the behavior
consistent.
In @backend/tests/integration/events/test_consumer_lifecycle.py:
- Around line 10-13: The xdist_group marker in the test prevents librdkafka
races only when pytest runs in parallel; update the GitHub Actions job that runs
the integration tests (the workflow step invoking "pytest tests/integration") to
add parallelization grouping by appending "-n auto --dist=loadgroup" to the
pytest command so the xdist_group("kafka_consumers") marker takes effect under
CI when tests run with xdist.
In @backend/tests/integration/events/test_event_store_consumer.py:
- Around line 16-23: Add the pytest marker registration for the xdist_group
marker in the pytest config so pytest no longer warns; specifically, update the
[tool.pytest.ini_options] markers list to include the entry "xdist_group: marks
tests for pytest-xdist grouping" so the marker used by pytestmark (xdist_group)
in test_event_store_consumer.py is registered.
In @backend/tests/integration/idempotency/test_consumer_idempotent.py:
- Around line 69-70: Replace the fixed await asyncio.sleep(2) with a readiness
wait that polls the consumer assignment; call the wrapper.assignment() via the
UnifiedConsumer wrapper inside eventually() (e.g. eventually(lambda:
bool(wrapper.assignment()), timeout=10.0, interval=0.1)) so the test waits for
actual partition assignments and avoids flakiness or extra delay; alternatively
you may remove the sleep entirely if you rely on the subsequent eventually(_one,
...) call to synchronize readiness.
🧹 Nitpick comments (1)
backend/tests/integration/idempotency/test_consumer_idempotent.py (1)
14-16: Avoid relying ontests.helperspackage side effects for a single factory import.If this import was moved to dodge circular-import/ordering issues, consider importing the concrete module instead (less likely to reintroduce cycles via
tests/helpers/__init__.py), e.g.from tests.helpers.events import make_execution_requested_event.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/tests/integration/events/test_consume_roundtrip.pybackend/tests/integration/events/test_consumer_lifecycle.pybackend/tests/integration/events/test_event_dispatcher.pybackend/tests/integration/events/test_event_store_consumer.pybackend/tests/integration/idempotency/test_consumer_idempotent.pybackend/tests/integration/result_processor/test_result_processor.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/tests/integration/events/test_consumer_lifecycle.py (1)
backend/app/events/core/types.py (1)
ConsumerConfig(64-102)
backend/tests/integration/result_processor/test_result_processor.py (6)
backend/tests/conftest.py (2)
app(42-53)db(88-90)backend/tests/unit/conftest.py (2)
app(25-26)db(10-11)backend/app/db/repositories/execution_repository.py (1)
ExecutionRepository(17-91)backend/app/domain/enums/kafka.py (1)
KafkaTopic(7-53)backend/app/domain/execution/models.py (1)
DomainExecutionCreate(73-80)backend/tests/helpers/eventually.py (1)
eventually(7-32)
backend/tests/integration/events/test_event_store_consumer.py (1)
backend/app/domain/enums/auth.py (1)
LoginMethod(4-10)
backend/tests/integration/idempotency/test_consumer_idempotent.py (2)
backend/tests/helpers/events.py (1)
make_execution_requested_event(8-50)backend/tests/helpers/eventually.py (1)
eventually(7-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Backend
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (6)
backend/tests/integration/events/test_event_dispatcher.py (1)
16-18: Ensurexdist_groupis actually enforced (CI flags + marker registration).
pytest.mark.xdist_group("kafka_consumers")typically only changes scheduling if xdist is run with the grouping dist mode (e.g.--dist=loadgroup). Also, if your config enables strict markers, you’ll need to registerxdist_groupinpytest.ini/pyproject.tomlto avoid “unknown marker” failures.backend/tests/integration/events/test_consumer_lifecycle.py (1)
21-24: ConsumerConfig reformat is fine. No behavioral changes.backend/tests/integration/events/test_event_store_consumer.py (1)
6-7:LoginMethodimport looks correct and is used.backend/tests/integration/result_processor/test_result_processor.py (2)
25-32:xdist_group("kafka_consumers")addition is consistent with the rest of the suite.
As above: make sure loadgroup is enabled; otherwise this won’t serialize anything.
5-5: No action required—DomainExecutionCreateis properly re-exported fromapp.domain.execution.The
__init__.pyfile inapp/domain/executioncorrectly imports and exportsDomainExecutionCreate, so the import path used here and elsewhere is valid.backend/tests/integration/idempotency/test_consumer_idempotent.py (1)
18-25: Thexdist_groupcontainment strategy is properly supported and will work as intended.pytest-xdist 3.6.1 is already in the dev dependencies (backend/pyproject.toml), and the
xdist_groupmarker is used consistently across multiple Kafka-related test files. The strategy correctly addresses the documented librdkafka race condition in multi-worker environments.Minor improvement: register
xdist_groupin the[tool.pytest.ini_options]markers list for consistency:markers = [ ... "xdist_group: marks tests to run serially in pytest-xdist", ]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/backend-ci.yml (1)
50-50: Consider making test duration reporting configurable.The
--durations=0flag reports execution time for all tests, which significantly increases CI log verbosity. While useful for profiling slow tests, this overhead may not be necessary for every CI run.Consider making this configurable:
- Use a workflow input or environment variable to enable/disable duration reporting
- Document the rationale if this is intended to be permanent
- Alternatively, use
--durations=10to report only the slowest 10 tests♻️ Example: Make duration reporting configurable
Add a workflow input at the top of the file:
on: workflow_dispatch: inputs: durations: description: 'Number of slowest tests to report (0 for all, empty to disable)' required: false default: ''Then use it conditionally in test commands:
run: | cd backend uv run pytest tests/unit -v -rs \ - --durations=0 \ + ${{ github.event.inputs.durations && format('--durations={0}', github.event.inputs.durations) || '' }} \ --cov=app \ --cov-report=xml --cov-report=termThis allows enabling duration reporting on-demand via workflow_dispatch while keeping regular CI runs clean.
Also applies to: 120-120, 215-215
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/backend-ci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/workers/run_pod_monitor.py (1)
67-74: LGTM! Settings injection aligns with the PR's DI refactor.The addition of the
settingsparameter toinit_tracingis consistent with the broader refactor to inject Settings via dependency injection across the stack. This enables tracing initialization to access configuration like theTESTINGflag to conditionally disable tracing during tests.Minor observation: There's some redundancy in passing both the
settingsobject and individual extracted values (service_version=settings.TRACING_SERVICE_VERSION,sampling_rate=settings.TRACING_SAMPLING_RATE). This is likely intentional for backward compatibility during the transition, but could be simplified in a future cleanup ifinit_tracingcan extract these directly from the settings parameter.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/workers/run_coordinator.pybackend/workers/run_event_replay.pybackend/workers/run_k8s_worker.pybackend/workers/run_pod_monitor.pybackend/workers/run_result_processor.pybackend/workers/run_saga_orchestrator.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (5)
backend/workers/run_k8s_worker.py (1)
67-67: LGTM! Settings properly passed to tracing initialization.The addition of the
settingsparameter enables the tracing initialization to access the full Settings configuration, aligning with the broader Settings-based dependency injection refactor across the codebase.backend/workers/run_saga_orchestrator.py (1)
67-67: LGTM! Settings properly passed to tracing initialization.Consistent with the Settings-based DI pattern applied across all worker entry points.
backend/workers/run_coordinator.py (1)
67-67: LGTM! Settings properly passed to tracing initialization.Consistent with the Settings-based DI pattern applied across all worker entry points.
backend/workers/run_event_replay.py (1)
74-74: LGTM! Settings properly passed to tracing initialization.Consistent with the Settings-based DI pattern applied across all worker entry points.
backend/workers/run_result_processor.py (1)
83-83: LGTM! Settings properly passed to tracing initialization.Consistent with the Settings-based DI pattern applied across all worker entry points.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/pyproject.toml:
- Line 211: Remove the duplicate quiet flag from the addopts setting by editing
the addopts value (currently "-n 4 --dist loadgroup --tb=short -q --no-header
-q") to contain only a single "-q"; keep the rest of the options unchanged so
the test runner still uses 4 workers, loadgroup distribution, short traceback
formatting, and no header.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
backend/pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Backend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
- GitHub Check: Unit Tests
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/core/middlewares/metrics.py">
<violation number="1" location="backend/app/core/middlewares/metrics.py:132">
P2: Hardcoding `service.environment` to "production" removes the ability to distinguish test metrics from production metrics. If `OTEL_EXPORTER_OTLP_ENDPOINT` is configured in a test environment, metrics will be incorrectly tagged as production. Consider preserving the conditional logic based on `settings.TESTING`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
3 issues found across 18 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/tests/unit/conftest.py">
<violation number="1" location="backend/tests/unit/conftest.py:29">
P3: Generator fixture has incorrect return type annotation. Since this fixture uses `yield`, it should either omit the return type or use `Generator[None, None, None]`/`Iterator[None]`.</violation>
</file>
<file name="backend/app/core/metrics/base.py">
<violation number="1" location="backend/app/core/metrics/base.py:51">
P1: The check for `settings.TESTING` and `settings.ENABLE_TRACING` was removed, but the PR description says "metrics NoOp in TESTING". This will cause metrics to be sent during tests (if OTLP endpoint is set), and when tracing is explicitly disabled. The `settings` parameter is passed but unused in the condition.</violation>
</file>
<file name="backend/app/main.py">
<violation number="1" location="backend/app/main.py:130">
P0: Removing module-level `app = create_app()` breaks production deployment. The Dockerfile uses `gunicorn app.main:app` which requires `app` to be available at module level. Either restore `app = create_app()` or update the Dockerfile to use factory mode (e.g., `gunicorn 'app.main:create_app()' --factory` with gunicorn 20.1+).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @backend/app/core/middlewares/metrics.py:
- Around line 128-134: The Resource.create call hard-codes "service.environment"
to "production"; change it to read the environment from your configuration
(e.g., use settings.ENVIRONMENT or settings.ENV with a sensible default) so
metrics are tagged by actual environment; update the Resource.create invocation
that sets SERVICE_NAME, SERVICE_VERSION and "service.environment" to use
settings.ENVIRONMENT (fallback to "production" if missing) instead of the
literal "production".
- Around line 121-125: The setup_metrics function should explicitly skip
initializing metrics when running tests; update setup_metrics to check
settings.TESTING (like the tracing check in dishka_lifespan.py:48) and return
early (with a log message) if TESTING is true, before attempting to read
OTEL_EXPORTER_OTLP_ENDPOINT or set up exporters so metrics cannot be initialized
in test mode even if environment variables are present.
In @backend/app/core/providers.py:
- Around line 630-632: The line constructing event_replay_service with
EventReplayService is >120 chars; break the constructor call across multiple
lines for readability: put EventReplayService(...) on its own line and place
each keyword argument (repository=replay_repository, producer=kafka_producer,
event_store=event_store, settings=settings, logger=logger) on separate indented
lines (or grouped to fit <120 chars) so the resulting lines all stay within the
120-character limit while preserving the same argument names and order.
🧹 Nitpick comments (1)
backend/tests/unit/core/metrics/test_replay_and_security_metrics.py (1)
37-58: LGTM: Test correctly updated for Settings-based DI.The test properly uses the
test_settingsfixture and instantiatesSecurityMetricswith it.💅 Optional: Consider consistent formatting
Line 43 uses semicolons to chain multiple calls, which is inconsistent with the formatting in the rest of the file where calls are on separate lines. For consistency, consider:
- m.update_active_sessions(2); m.increment_active_sessions(); m.decrement_active_sessions() + m.update_active_sessions(2) + m.increment_active_sessions() + m.decrement_active_sessions()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/app/core/metrics/base.pybackend/app/core/metrics/context.pybackend/app/core/middlewares/metrics.pybackend/app/core/providers.pybackend/app/main.pybackend/app/services/event_replay/replay_service.pybackend/app/services/k8s_worker/worker.pybackend/pyproject.tomlbackend/tests/unit/conftest.pybackend/tests/unit/core/metrics/test_base_metrics.pybackend/tests/unit/core/metrics/test_connections_and_coordinator_metrics.pybackend/tests/unit/core/metrics/test_database_and_dlq_metrics.pybackend/tests/unit/core/metrics/test_execution_and_events_metrics.pybackend/tests/unit/core/metrics/test_health_and_rate_limit_metrics.pybackend/tests/unit/core/metrics/test_kubernetes_and_notifications_metrics.pybackend/tests/unit/core/metrics/test_metrics_classes.pybackend/tests/unit/core/metrics/test_metrics_context.pybackend/tests/unit/core/metrics/test_replay_and_security_metrics.pybackend/tests/unit/services/coordinator/test_queue_manager.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/services/k8s_worker/worker.py
- backend/pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (9)
backend/tests/unit/core/metrics/test_execution_and_events_metrics.py (3)
backend/tests/unit/conftest.py (1)
app(66-67)backend/app/core/metrics/execution.py (16)
ExecutionMetrics(5-108)record_script_execution(65-66)record_execution_duration(68-69)increment_active_executions(71-72)decrement_active_executions(74-75)record_memory_usage(77-78)record_error(80-81)update_queue_depth(83-84)record_queue_wait_time(86-87)record_execution_assigned(89-90)record_execution_queued(92-93)record_execution_scheduled(95-96)update_cpu_available(98-99)update_memory_available(101-102)update_gpu_available(104-105)update_allocations_active(107-108)backend/app/core/metrics/events.py (21)
EventMetrics(4-209)record_event_published(98-110)record_event_processing_duration(112-113)record_pod_event_published(115-116)record_event_replay_operation(118-119)update_event_buffer_size(121-122)record_event_buffer_dropped(124-125)record_event_buffer_processed(127-128)record_event_buffer_latency(130-131)set_event_buffer_backpressure(133-135)record_event_buffer_memory_usage(137-138)record_event_stored(140-141)record_events_processing_failed(143-154)record_processing_duration(172-177)record_kafka_message_produced(179-182)record_kafka_message_consumed(184-185)record_kafka_consumer_lag(187-190)record_kafka_production_error(192-193)record_kafka_consumption_error(195-198)update_event_bus_queue_size(200-201)set_event_bus_queue_size(203-209)
backend/app/core/metrics/base.py (1)
backend/app/settings.py (1)
Settings(11-164)
backend/app/core/providers.py (7)
backend/app/core/security.py (1)
SecurityService(23-107)backend/app/settings.py (1)
Settings(11-164)backend/app/core/metrics/context.py (2)
get_event_metrics(204-205)get_event_metrics(256-257)backend/app/core/metrics/events.py (1)
EventMetrics(4-209)backend/app/db/repositories/admin/admin_user_repository.py (1)
AdminUserRepository(22-115)backend/app/services/kafka_event_service.py (1)
KafkaEventService(25-295)backend/app/services/event_replay/replay_service.py (1)
EventReplayService(23-368)
backend/tests/unit/conftest.py (1)
backend/app/core/metrics/context.py (3)
MetricsContext(113-233)initialize_all(123-163)reset_all(166-185)
backend/tests/unit/core/metrics/test_health_and_rate_limit_metrics.py (3)
backend/tests/unit/conftest.py (1)
app(66-67)backend/app/settings.py (1)
Settings(11-164)backend/app/core/metrics/health.py (7)
HealthMetrics(4-109)record_health_check_duration(62-68)record_health_check_failure(70-73)update_health_check_status(75-77)record_health_status(79-83)record_service_health_score(85-86)update_liveness_status(88-90)
backend/tests/unit/core/metrics/test_replay_and_security_metrics.py (4)
backend/tests/unit/conftest.py (1)
app(66-67)backend/app/settings.py (1)
Settings(11-164)backend/app/core/metrics/replay.py (20)
ReplayMetrics(4-180)record_session_created(93-94)update_active_replays(96-103)increment_active_replays(105-106)decrement_active_replays(108-109)record_events_replayed(111-117)record_event_replayed(119-120)record_replay_duration(122-128)record_event_processing_time(130-131)record_replay_error(133-134)record_status_change(136-141)update_sessions_by_status(143-145)record_replay_by_target(147-151)record_speed_multiplier(153-154)record_delay_applied(156-157)record_batch_size(159-160)record_events_filtered(162-163)record_filter_effectiveness(165-168)record_replay_memory_usage(170-171)update_replay_queue_size(173-180)backend/app/core/metrics/security.py (1)
SecurityMetrics(4-322)
backend/tests/unit/core/metrics/test_connections_and_coordinator_metrics.py (2)
backend/tests/unit/conftest.py (1)
app(66-67)backend/app/settings.py (1)
Settings(11-164)
backend/tests/unit/core/metrics/test_database_and_dlq_metrics.py (3)
backend/tests/unit/conftest.py (1)
app(66-67)backend/app/settings.py (1)
Settings(11-164)backend/app/core/metrics/dlq.py (1)
DLQMetrics(4-94)
backend/app/main.py (1)
backend/app/core/middlewares/metrics.py (1)
setup_metrics(121-161)
🪛 GitHub Actions: Ruff Linting
backend/app/core/providers.py
[error] 631-631: E501: Line too long (124 > 120). The line 'repository=replay_repository, producer=kafka_producer, event_store=event_store, settings=settings, logger=logger' exceeds the maximum line length. Consider breaking into multiple lines. Command: uv run ruff check . --config pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Scan Backend
- GitHub Check: Scan Frontend
- GitHub Check: cubic · AI code reviewer
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (32)
backend/tests/unit/services/coordinator/test_queue_manager.py (1)
10-11: LGTM! Good addition for test organization.The module-level
pytestmarkcorrectly applies the unit test marker to all tests in this module, which aligns with the PR's goal of improving test organization and categorization.backend/tests/unit/core/metrics/test_connections_and_coordinator_metrics.py (2)
5-5: LGTM!The import is correctly added to support Settings dependency injection in the test functions.
10-12: LGTM!Both test functions correctly implement the Settings dependency injection pattern. The
test_settingsfixture is injected and passed to the metric constructors, aligning with the PR's goal of centralizing configuration via Settings. Since metrics become no-op under test mode, these smoke tests appropriately verify that metric methods can be called without errors.Also applies to: 25-27
backend/app/core/metrics/context.py (1)
48-61: LGTM! Explicit initialization improves predictability.The removal of lazy initialization in favor of explicit DI-based initialization is a solid architectural improvement. The RuntimeError provides a clear, actionable message that guides developers to call
MetricsContext.initialize_all()during app startup.backend/tests/unit/core/metrics/test_metrics_context.py (1)
15-24: LGTM! Test correctly validates session-scoped initialization.The test properly verifies that metrics initialized via the session fixture maintain singleton behavior within the context, aligning with the new explicit initialization pattern.
backend/tests/unit/core/metrics/test_kubernetes_and_notifications_metrics.py (2)
11-37: LGTM! Proper Settings injection for DI pattern.The test correctly injects the
test_settingsfixture intoKubernetesMetrics, aligning with the new dependency injection approach for metrics initialization.
40-59: LGTM! Consistent Settings injection.The test properly passes
test_settingstoNotificationMetrics, maintaining consistency with the DI-based initialization pattern.backend/app/services/event_replay/replay_service.py (1)
24-41: LGTM! Settings properly propagated to metrics.The service correctly accepts
Settingsvia constructor injection and passes it toReplayMetrics, maintaining the DI pattern across the service hierarchy.backend/app/core/metrics/base.py (2)
22-36: LGTM! Clean dependency injection implementation.The refactor from implicit
get_settings()calls to explicitSettingsinjection improves testability and makes dependencies explicit. The configuration object construction is clear and straightforward.
39-67: ENABLE_TRACING check removed from metrics initialization, but intended behavior is preserved through environment configuration.The concern about removing ENABLE_TRACING is valid: line 51 now only checks
config.otlp_endpointrather than also checking ENABLE_TRACING. However, the architecture ensures this works correctly:
- ENABLE_TRACING is checked at the application initialization level (dishka_lifespan.py line 48) to gate tracing setup entirely
- For TESTING mode, .env.test explicitly sets
OTEL_EXPORTER_OTLP_ENDPOINT=(empty), ensuring the check at line 51 returns NoOpMeterProvider as intended- Metrics correctly achieve NoOp behavior in tests through the empty endpoint configuration
Potential edge case: If ENABLE_TRACING=False but OTEL_EXPORTER_OTLP_ENDPOINT is configured, the meter would be created as a real meter instead of NoOp. Consider adding explicit ENABLE_TRACING check in _create_meter() if this scenario needs to be explicitly handled.
backend/app/core/providers.py (9)
146-148: LGTM: Clean SecurityService provisioning.The SecurityService provider correctly instantiates the service with Settings, enabling centralized security configuration. The APP scope ensures a singleton instance.
247-293: LGTM: Consistent Settings injection across all metrics.All metrics providers now consistently receive and pass Settings to their respective metric classes. This excellent refactoring enables:
- Configuration-driven metrics initialization
- Test isolation with unique settings per test session
- Removal of global state dependencies
342-343: LGTM: Proper SecurityService injection into repository.AdminUserRepository now receives SecurityService via dependency injection, eliminating internal instantiation and improving testability.
415-418: LGTM: AuthService properly wired with SecurityService.The AuthService now receives SecurityService as a constructor parameter, maintaining the DI pattern consistently across authentication components.
431-443: LGTM: KafkaEventService updated with Settings parameter.The KafkaEventService provider now correctly passes Settings to the service, enabling configuration-driven Kafka operations and metrics initialization.
636-652: LGTM: AdminUserService properly wired with SecurityService.AdminUserService now receives SecurityService via dependency injection, completing the security service propagation to admin-related services.
721-735: LGTM: EventReplayProvider consistent with Settings pattern.EventReplayService in the EventReplayProvider follows the same Settings injection pattern as other services, maintaining consistency across the codebase.
159-164: UnifiedProducer correctly accepts the settings parameter.The
settings=settingskeyword argument passed to UnifiedProducer is properly supported by its constructor, which includessettings: Settingsin its signature. The settings object is used within the class (e.g.,self._topic_prefix = settings.KAFKA_TOPIC_PREFIX), confirming the integration is correct.
222-223: EventBusManager signature confirmed. The constructor atbackend/app/services/event_bus.pyacceptssettings: Settingsandlogger: logging.Loggeras parameters, matching the instantiation pattern at lines 222-223.backend/app/main.py (2)
66-66: LGTM: setup_metrics correctly receives Settings.The setup_metrics call now properly passes the Settings instance, enabling configuration-driven OpenTelemetry metrics setup.
144-146: LGTM: Factory pattern enables flexible app initialization.The switch to
factory=Truewith the"app.main:create_app"string is correct. The create_app function properly handles Settings loading by callingget_settings()when settings is None, ensuring configuration is loaded on each factory invocation.backend/tests/unit/core/metrics/test_database_and_dlq_metrics.py (1)
1-45: LGTM: Tests properly updated with Settings fixtures.Both test functions correctly accept and use the test_settings fixture to construct DatabaseMetrics and DLQMetrics, ensuring test isolation and consistent configuration.
backend/tests/unit/conftest.py (1)
28-47: LGTM: Well-designed metrics initialization fixture.The session-scoped autouse fixture properly initializes all metrics with test_settings and ensures cleanup via reset_all. This provides:
- Consistent metrics availability across unit tests
- Proper test isolation with unique settings
- Efficient resource usage via session scope
- Clean state between test runs
backend/tests/unit/core/metrics/test_health_and_rate_limit_metrics.py (1)
9-21: LGTM: Test properly updated with Settings fixture.The test function correctly accepts and uses test_settings to construct HealthMetrics, maintaining consistency with the broader refactoring. The semicolon cleanup on Line 17 is also appreciated.
backend/tests/unit/core/metrics/test_base_metrics.py (2)
10-14: LGTM: DummyMetrics correctly updated for Settings-based DI.The constructor now properly accepts and forwards the Settings instance to BaseMetrics, aligning with the broader DI configuration pattern.
19-23: LGTM: Test properly uses Settings fixture.The test correctly receives the
test_settingsfixture and instantiatesDummyMetricswith it, ensuring proper dependency injection throughout the test lifecycle.backend/tests/unit/core/metrics/test_execution_and_events_metrics.py (2)
12-30: LGTM: Test correctly updated for Settings-based DI.The test properly receives the
test_settingsfixture and instantiatesExecutionMetricswith it. The separation of chained calls into individual lines also improves readability.
33-61: LGTM: Test correctly updated for Settings-based DI.The test properly receives the
test_settingsfixture and instantiatesEventMetricswith it, consistent with the DI pattern applied throughout the codebase.backend/tests/unit/core/metrics/test_replay_and_security_metrics.py (1)
11-35: LGTM: Test correctly updated for Settings-based DI.The test properly receives the
test_settingsfixture and instantiatesReplayMetricswith it, aligning with the broader DI configuration pattern.backend/tests/unit/core/metrics/test_metrics_classes.py (3)
20-30: LGTM: Test correctly updated for Settings-based DI.The test properly receives the
test_settingsfixture and instantiatesConnectionMetricswith it, consistent with the DI pattern.
32-58: LGTM: Test correctly updated for Settings-based DI.The test properly instantiates
EventMetricswithtest_settingsand exercises a comprehensive set of metric methods.
60-71: LGTM: All metric classes correctly instantiated with Settings.All metric classes are properly instantiated with
test_settings, ensuring consistent DI configuration across the entire metrics suite.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/app/settings.py (1)
133-133: Consider using an Enum for stronger type safety.The "production" default is appropriately fail-safe. However, since the comment lists specific expected values (production, staging, development), consider using a
Literaltype or an Enum to enforce valid values at the type level.♻️ Optional: Use Literal for type safety
+from typing import Literal + class Settings(BaseSettings): # ... other fields ... - ENVIRONMENT: str = "production" # deployment environment (production, staging, development) + ENVIRONMENT: Literal["production", "staging", "development"] = "production"backend/app/core/middlewares/metrics.py (1)
136-139: Verifyinsecure=Trueis appropriate for all environments.The OTLP exporter is configured with
insecure=True, which is suitable for local development but may not be appropriate for production. Ensure that production environments use secure connections (TLS) to the OTLP endpoint.Consider making the
insecureparameter configurable via Settings:♻️ Suggested improvement
In
backend/app/settings.py, add:OTEL_EXPORTER_INSECURE: bool = True # Set to False in productionThen update the exporter initialization:
- otlp_exporter = OTLPMetricExporter(endpoint=settings.OTEL_EXPORTER_OTLP_ENDPOINT, insecure=True) + otlp_exporter = OTLPMetricExporter( + endpoint=settings.OTEL_EXPORTER_OTLP_ENDPOINT, + insecure=settings.OTEL_EXPORTER_INSECURE + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/.env.testbackend/Dockerfilebackend/app/core/middlewares/metrics.pybackend/app/core/providers.pybackend/app/settings.pybackend/tests/unit/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/unit/conftest.py (1)
backend/app/core/metrics/context.py (2)
initialize_all(123-163)reset_all(166-185)
🪛 dotenv-linter (4.0.0)
backend/.env.test
[warning] 29-29: [UnorderedKey] The BCRYPT_ROUNDS key should go before the SECURE_COOKIES key
(UnorderedKey)
[warning] 33-33: [UnorderedKey] The ENABLE_TRACING key should go before the RATE_LIMIT_ENABLED key
(UnorderedKey)
[warning] 41-41: [UnorderedKey] The ENVIRONMENT key should go before the LOG_LEVEL key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (18)
backend/app/settings.py (2)
47-48: LGTM!The configurable bcrypt rounds with a secure default of 12 is appropriate. The comment clearly explains the tradeoff, and the test environment override to 4 (shown in
.env.test) provides the intended speed improvement.
79-79: LGTM!Empty string default for schema subject prefix is sensible, allowing flexible per-environment configuration for schema isolation.
backend/Dockerfile (1)
30-31: LGTM!The factory pattern with
--factoryflag correctly aligns with the Settings-based DI refactor. This allowscreate_app()to initialize the application with proper dependency injection.backend/.env.test (3)
29-29: LGTM!Setting
BCRYPT_ROUNDS=4for tests provides the intended speed improvement while maintaining sufficient security for the test environment.
35-36: LGTM!The simplified comment is clearer. The empty
OTEL_EXPORTER_OTLP_ENDPOINTeffectively disables metrics as intended for tests.
41-41: LGTM!Setting
ENVIRONMENT=testappropriately identifies the test environment, aligning with the new Settings field.backend/app/core/middlewares/metrics.py (2)
121-125: LGTM!The Settings injection pattern is correct, and the conditional skip when
OTEL_EXPORTER_OTLP_ENDPOINTis not configured prevents unnecessary initialization errors in test/development environments.
128-134: LGTM!Resource attributes now correctly use Settings fields for service name, version, and environment instead of hardcoded values.
backend/tests/unit/conftest.py (1)
28-47: Dependency ontest_settingsat session scope is properly satisfied.The
test_settingsfixture is correctly defined at session scope in the parentbackend/tests/conftest.py, making it available to this fixture in the unit test conftest. The metrics initialization correctly depends on this session-scoped fixture, and cleanup is properly handled withMetricsContext.reset_all().backend/app/core/providers.py (9)
640-656: Verify AdminUserService constructor accepts security_service parameter.AdminUserService is now injected with SecurityService via dependency injection. Ensure the service constructor has been updated to accept the security_service parameter along with all other dependencies shown in the get_admin_user_service method.
342-343: AdminUserRepository correctly accepts SecurityService via dependency injection.The constructor signature in
backend/app/db/repositories/admin/admin_user_repository.pyhas been properly updated to acceptsecurity_service: SecurityService, and the provider correctly passes it. This removes tight coupling and follows good DI practices.
163-163: No action needed. The UnifiedProducer constructor correctly accepts thesettingsparameter and the initialization at line 163 is valid.
415-418: AuthService constructor properly accepts SecurityService.The AuthService constructor signature correctly matches the dependency injection call, with SecurityService properly injected and utilized within the service for authentication operations.
431-443: KafkaEventService constructor signature verified.The KafkaEventService
__init__method correctly accepts all four parameters (event_repository, kafka_producer, settings, and logger) with the proper type annotations. All parameters are stored as instance variables, and settings is actively used within the service to access configuration values like SERVICE_NAME and SERVICE_VERSION.
222-223: No action needed. Verification shows no other EventBusManager instantiation sites exist in the codebase outside of the DI container. The signature change is properly contained, and the code at lines 222-223 correctly uses the new(settings, logger)signature.Likely an incorrect or invalid review comment.
146-148: SecurityService is stateless and thread-safe for APP scope.Verification confirms SecurityService stores only immutable configuration (Settings reference) and a thread-safe CryptContext instance from passlib. All methods (verify_password, get_password_hash, create_access_token, get_current_user) are pure operations without side effects. This makes the service safe for APP-scoped (singleton) usage with concurrent requests.
248-293: Consistent Settings propagation across all metrics.All metric classes inherit from BaseMetrics, which properly accepts the Settings parameter in its constructor. The provider pattern correctly instantiates each metric with the settings, centralizing configuration across the metrics layer.
627-636: EventReplayService construction is consistent across both providers.EventReplayService is correctly and uniformly constructed with all required parameters, including settings, in both
get_replay_service(lines 627-636) andget_event_replay_service(lines 730-738). The constructor signature matches the parameters being passed in both locations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/app/main.py:
- Around line 69-85: Wrap the MetricsContext.initialize_all(...) call in a
try/except block to catch any exceptions thrown by the metric constructors
(e.g., ConnectionMetrics, CoordinatorMetrics, DatabaseMetrics, etc.), remove any
redundant OTLP endpoint conditional checks here (since base metric constructors
and setup_metrics already handle otlp_endpoint/NoOpMeterProvider), and in the
except handler log the full exception with logger.exception("Failed to
initialize metrics") and then either re-raise to fail fast or log and continue
with a graceful fallback (e.g., proceed without metrics or re-initialize a NoOp
metrics context) depending on your startup policy.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/main.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/main.py (1)
backend/app/core/metrics/context.py (1)
initialize_all(123-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (2)
backend/app/main.py (2)
99-99: LGTM: Settings-driven metrics setup.The expanded signature
setup_metrics(app, settings, logger)aligns with the PR's goal of centralizing Settings-driven dependency injection. This propagates the settings instance through to metrics configuration.
178-179: LGTM: Factory pattern enables proper DI.The factory pattern (
"app.main:create_app"withfactory=True) is the correct approach for dependency injection frameworks. Benefits include:
- Settings loaded per-worker instead of at module import time
- Better testability with injectable settings
- Proper lifecycle management for DI containers
This aligns with the PR's broader Settings-driven DI goals.
There was a problem hiding this comment.
1 issue found across 23 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/services/pod_monitor/monitor.py">
<violation number="1" location="backend/app/services/pod_monitor/monitor.py:149">
P1: Blocking synchronous call in async method will block the event loop. The `get_api_resources()` call makes a network request to the Kubernetes API and should be wrapped in `asyncio.to_thread()` to avoid blocking, consistent with how `list_namespaced_pod` is called elsewhere in this file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
backend/tests/integration/test_replay_routes.py (3)
307-317: Inconsistent test payload structure.This test uses a raw dictionary with
"filters"(plural) and fields like"name","description", and"target_topic"that don't appear in theReplayRequestschema used by other tests. This inconsistency bypasses Pydantic validation and may not align with the actual API schema.♻️ Refactor to use ReplayRequest and ReplayFilter models
- replay_request = { - "name": f"State Test Session {uuid4().hex[:8]}", - "description": "Testing state transitions", - "filters": { - "event_types": ["state.test.event"], - "start_time": (datetime.now(timezone.utc) - timedelta(hours=1)).isoformat(), - "end_time": datetime.now(timezone.utc).isoformat() - }, - "target_topic": "state-test-topic", - "speed_multiplier": 1.0 - } + replay_request = ReplayRequest( + replay_type=ReplayType.QUERY, + target=ReplayTarget.KAFKA, + filter=ReplayFilter( + event_types=[EventType.SYSTEM_ERROR], # or appropriate event type + start_time=datetime.now(timezone.utc) - timedelta(hours=1), + end_time=datetime.now(timezone.utc), + ), + speed_multiplier=1.0, + ).model_dump(mode="json")
382-392: Inconsistent test payload structure.This test also uses a raw dictionary with
"filters"(plural) and extra fields not present in theReplayRequestschema used elsewhere in the file.♻️ Refactor to use ReplayRequest and ReplayFilter models
- replay_request = { - "name": f"Progress Test Session {uuid4().hex[:8]}", - "description": "Testing progress tracking", - "filters": { - "event_types": ["progress.test.event"], - "start_time": (datetime.now(timezone.utc) - timedelta(minutes=30)).isoformat(), - "end_time": datetime.now(timezone.utc).isoformat() - }, - "target_topic": "progress-test-topic", - "speed_multiplier": 10.0 # Fast replay - } + replay_request = ReplayRequest( + replay_type=ReplayType.QUERY, + target=ReplayTarget.KAFKA, + filter=ReplayFilter( + event_types=[EventType.SYSTEM_ERROR], # or appropriate event type + start_time=datetime.now(timezone.utc) - timedelta(minutes=30), + end_time=datetime.now(timezone.utc), + ), + speed_multiplier=10.0, + ).model_dump(mode="json")
343-365: Fix test payload to use ReplayRequest and ReplayFilter models.The test uses a raw dictionary that doesn't match the
ReplayRequestschema:
- Uses
"filters"(plural) instead of"filter"(singular)- Uses
"preserve_timing"instead of"preserve_timestamps"- Uses
"target_topic"(string) instead of"target_topics"(Dict[EventType, str])- Includes unsupported fields
"name"and"description"- Missing required fields
replay_typeandtargetThe filter fields (
aggregate_id,correlation_id,user_id,service_name) andbatch_sizeare all valid and supported.♻️ Refactor to use ReplayRequest and ReplayFilter models
- replay_request = { - "name": f"Complex Filter Session {uuid4().hex[:8]}", - "description": "Testing complex event filters", - "filters": { - "event_types": [ - "execution.requested", - "execution.started", - "execution.completed", - "execution.failed" - ], - "start_time": (datetime.now(timezone.utc) - timedelta(days=30)).isoformat(), - "end_time": datetime.now(timezone.utc).isoformat(), - "aggregate_id": str(uuid4()), - "correlation_id": str(uuid4()), - "user_id": test_admin.get("user_id"), - "service_name": "execution-service" - }, - "target_topic": "complex-filter-topic", - "speed_multiplier": 0.1, # Slow replay - "preserve_timing": False, - "batch_size": 100 - } + replay_request = ReplayRequest( + replay_type=ReplayType.QUERY, + target=ReplayTarget.KAFKA, + filter=ReplayFilter( + event_types=[ + EventType.EXECUTION_REQUESTED, + EventType.EXECUTION_STARTED, + EventType.EXECUTION_COMPLETED, + EventType.EXECUTION_FAILED, + ], + start_time=datetime.now(timezone.utc) - timedelta(days=30), + end_time=datetime.now(timezone.utc), + aggregate_id=str(uuid4()), + correlation_id=str(uuid4()), + user_id=test_admin.get("user_id"), + service_name="execution-service", + ), + speed_multiplier=0.1, + preserve_timestamps=False, + batch_size=100, + target_topics={EventType.EXECUTION_COMPLETED: "complex-filter-topic"}, + ).model_dump(mode="json")backend/tests/integration/services/sse/test_partitioned_event_router.py (1)
58-65: Avoid mutating session-scopedtest_settings(leaks across tests). Line 59 setstest_settings.SSE_CONSUMER_POOL_SIZE = 1, buttest_settingsisscope="session", so this can make unrelated tests order-dependent/flaky.Proposed fix (copy settings for this test)
async def test_router_start_and_stop(redis_client, test_settings: Settings) -> None: - test_settings.SSE_CONSUMER_POOL_SIZE = 1 + settings = test_settings.model_copy(update={"SSE_CONSUMER_POOL_SIZE": 1}) suffix = uuid4().hex[:6] router = SSEKafkaRedisBridge( - schema_registry=SchemaRegistryManager(settings=test_settings, logger=_test_logger), - settings=test_settings, - event_metrics=EventMetrics(test_settings), + schema_registry=SchemaRegistryManager(settings=settings, logger=_test_logger), + settings=settings, + event_metrics=EventMetrics(settings), sse_bus=SSERedisBus( redis_client, exec_prefix=f"sse:exec:{suffix}:", notif_prefix=f"sse:notif:{suffix}:", logger=_test_logger, ), logger=_test_logger, )backend/tests/conftest.py (1)
114-120: Don’t silently accept missing CSRF token. Line 119 returning""will turn failures into confusing 403/CSRF errors later.Proposed fix
async def _http_login(client: httpx.AsyncClient, username: str, password: str) -> str: @@ resp.raise_for_status() json_data: dict[str, str] = resp.json() - return json_data.get("csrf_token", "") + csrf = json_data.get("csrf_token") + assert csrf, "Login response missing csrf_token" + return csrfbackend/tests/integration/test_events_routes.py (1)
35-67: Fix type annotation:test_usershould be typed asUserCredentials, notDict[str, str].The fixture returns a
UserCredentialsTypedDict with fields{username, email, password, role, csrf_token, headers}. Using the exact type eliminates type checking noise and improves IDE support. Note: CSRF validation is not currently wired in the application (thevalidate_csrf_from_requestmethod exists but is never called in route handlers or middleware), so the missing headers on unsafe requests do not cause test failures today. However, the type annotation should still be corrected for clarity.
🤖 Fix all issues with AI agents
In @backend/tests/conftest.py:
- Around line 34-51: The session-scoped fixture test_settings is mutable and can
leak changes between tests; add a new function-scoped fixture (e.g., settings)
that takes test_settings and returns test_settings.model_copy() so each test
gets an independent Settings instance for per-test overrides, leaving
test_settings unchanged and session-scoped; update tests to use settings where
tests modify attributes.
- Around line 107-111: The redis_client fixture yields an async redis client but
lacks teardown, causing connection leaks; update the redis_client
pytest_asyncio.fixture to explicitly close the client after yield by awaiting
client.aclose() in a finally/teardown section so the AsyncGenerator returns the
client and then performs await client.aclose() to clean up the redis.asyncio
connection.
In @backend/tests/integration/test_dlq_routes.py:
- Around line 194-196: The test_set_retry_policy function is missing a type for
the test_settings parameter; import the Settings class (e.g., from app.settings
import Settings) at the top of the test file and update the
test_set_retry_policy signature to annotate test_settings: Settings so the
fixture type matches the conftest.py definition and type checkers recognize it.
In @backend/tests/unit/core/test_security.py:
- Around line 225-230: The test's claim dict is typed too narrowly as data:
dict[str, str | datetime], causing mypy friction when passing to jwt.encode;
change the annotation for data (and to_encode if annotated) to dict[str, Any]
(or use typing.Any) so the claims dict accepts mixed types expected by
jwt.encode and removes the strict-mypy complaint while keeping the same runtime
behavior.
🧹 Nitpick comments (6)
backend/tests/unit/core/test_adaptive_sampling.py (2)
70-71: Move imports to the top of the file.Imports placed inside functions reduce readability and are non-standard. Move these imports to the module level at the top of the file.
♻️ Proposed fix
Move the imports to the top of the file with the other imports:
import time from unittest.mock import patch +from unittest.mock import MagicMock import pytest from app.core.adaptive_sampling import AdaptiveSampler, create_adaptive_sampler +from app.core.config import SettingsAnd remove them from inside the function:
s._running = False - from unittest.mock import MagicMock - from app.core.config import Settings - mock_settings = MagicMock(spec=Settings) mock_settings.TRACING_SAMPLING_RATE = 0.2
73-79: Remove dead code and update outdated comment.The environment variable setup at line 76 appears to be unused since
create_adaptive_samplernow receives settings directly via the parameter. The comment at line 77 is also outdated—it states the function "pulls settings via get_settings," but the implementation now requires an explicit Settings parameter.♻️ Proposed fix
mock_settings = MagicMock(spec=Settings) mock_settings.TRACING_SAMPLING_RATE = 0.2 - monkeypatch.setenv("TRACING_SAMPLING_RATE", "0.2") - # create_adaptive_sampler pulls settings via get_settings; just ensure it constructs + # Verify create_adaptive_sampler constructs correctly with settings sampler = create_adaptive_sampler(mock_settings) sampler._running = Falsebackend/tests/helpers/k8s_fakes.py (1)
202-209: Signature inconsistency with real API and other test fakes.
list_namespaced_poduses positional parameters(namespace, label_selector), but the realCoreV1Api.list_namespaced_podacceptsnamespacepositionally with other parameters as keyword arguments. This differs fromTrackingV1.list_namespaced_podintest_monitor.py(line 673) which uses**kwargs.Consider aligning the signature for consistency:
Suggested alignment
- def list_namespaced_pod(self, namespace: str, label_selector: str) -> Any: # noqa: ARG002 + def list_namespaced_pod(self, namespace: str, **kwargs: Any) -> Any: # noqa: ARG002 """Return configured pods for reconciliation tests.""" class PodList: def __init__(self, items: list[Pod]) -> None: self.items = items return PodList(list(self._pods))backend/tests/unit/services/pod_monitor/test_monitor.py (1)
677-680: Return type inconsistency inTrackingWatch.stream.
TrackingWatch.streamreturns an emptylistwhileFakeWatch.streamreturnsFakeWatchStream. This works here because the test only checkswatch_kwargs, but it breaks the interface contract and could cause confusion in future test modifications.Consider returning
FakeWatchStream([])for consistency:Suggested fix
class TrackingWatch(FakeWatch): - def stream(self, func: Any, **kwargs: Any) -> list[dict[str, Any]]: + def stream(self, func: Any, **kwargs: Any) -> FakeWatchStream: watch_kwargs.append(kwargs) - return [] + return FakeWatchStream([], "rv1")Note: You'll need to import
FakeWatchStreamfromtests.helpers.k8s_fakes.backend/app/services/pod_monitor/monitor.py (1)
148-150: Blocking Kubernetes API call in async method.
self._v1.get_api_resources()is a synchronous network call that could block the event loop during startup. Other Kubernetes API calls in this file (e.g.,list_namespaced_podat line 387-389) correctly useasyncio.to_thread.Consider wrapping for consistency:
Suggested fix
# Verify K8s connectivity (all clients already injected via __init__) - self._v1.get_api_resources() + await asyncio.to_thread(self._v1.get_api_resources) self.logger.info("Successfully connected to Kubernetes API")backend/tests/unit/core/test_logging_and_correlation.py (1)
41-42: Consider direct return if MyPy allows.The intermediate typed variables (
resultandfallback_result) add verbosity. If--strictMyPy permits, you could return directly:return json.loads(output)and
return json.loads(s)However, if this pattern is needed to satisfy type inference, the current approach is acceptable.
Also applies to: 49-50
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/app/core/k8s_clients.pybackend/app/services/pod_monitor/monitor.pybackend/pyproject.tomlbackend/tests/conftest.pybackend/tests/e2e/test_resource_cleaner_orphan.pybackend/tests/helpers/eventually.pybackend/tests/helpers/k8s_fakes.pybackend/tests/helpers/kafka.pybackend/tests/integration/services/sse/test_partitioned_event_router.pybackend/tests/integration/test_alertmanager.pybackend/tests/integration/test_dlq_routes.pybackend/tests/integration/test_events_routes.pybackend/tests/integration/test_replay_routes.pybackend/tests/unit/conftest.pybackend/tests/unit/core/metrics/test_metrics_classes.pybackend/tests/unit/core/test_adaptive_sampling.pybackend/tests/unit/core/test_logging_and_correlation.pybackend/tests/unit/core/test_security.pybackend/tests/unit/schemas_pydantic/test_events_schemas.pybackend/tests/unit/schemas_pydantic/test_execution_schemas.pybackend/tests/unit/schemas_pydantic/test_notification_schemas.pybackend/tests/unit/services/pod_monitor/test_event_mapper.pybackend/tests/unit/services/pod_monitor/test_monitor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/unit/conftest.py
🧰 Additional context used
🧬 Code graph analysis (12)
backend/tests/unit/core/test_logging_and_correlation.py (1)
backend/app/core/logging.py (1)
format(61-98)
backend/tests/unit/core/test_security.py (1)
backend/app/core/security.py (1)
SecurityService(23-107)
backend/tests/conftest.py (2)
backend/app/main.py (1)
create_app(59-160)backend/app/settings.py (1)
Settings(11-165)
backend/tests/unit/core/test_adaptive_sampling.py (2)
backend/app/settings.py (1)
Settings(11-165)backend/app/core/adaptive_sampling.py (1)
create_adaptive_sampler(242-251)
backend/tests/unit/core/metrics/test_metrics_classes.py (2)
backend/tests/unit/conftest.py (1)
app(68-69)backend/app/settings.py (1)
Settings(11-165)
backend/tests/unit/services/pod_monitor/test_monitor.py (3)
backend/app/core/k8s_clients.py (1)
K8sClients(10-17)backend/app/services/pod_monitor/event_mapper.py (1)
PodEventMapper(57-519)backend/app/services/pod_monitor/monitor.py (6)
MonitorState(42-48)PodEvent(72-77)PodMonitor(91-457)WatchEventType(34-39)create_pod_monitor(461-501)state(140-142)
backend/tests/integration/test_alertmanager.py (2)
backend/tests/conftest.py (1)
client(78-86)backend/tests/unit/conftest.py (1)
client(63-64)
backend/tests/unit/services/pod_monitor/test_event_mapper.py (3)
backend/app/infrastructure/kafka/events/execution.py (3)
ExecutionCompletedEvent(86-93)ExecutionFailedEvent(96-105)ExecutionTimeoutEvent(108-115)backend/app/services/pod_monitor/event_mapper.py (3)
PodContext(31-38)PodEventMapper(57-519)_extract_logs(446-474)backend/tests/helpers/k8s_fakes.py (10)
ContainerStatus(54-59)FakeApi(130-135)Meta(12-25)Pod(78-94)Spec(28-31)State(47-51)Status(62-75)Terminated(34-38)read_namespaced_pod_log(134-135)read_namespaced_pod_log(195-196)
backend/tests/integration/services/sse/test_partitioned_event_router.py (2)
backend/app/core/metrics/events.py (1)
EventMetrics(4-209)backend/tests/conftest.py (1)
test_settings(35-51)
backend/tests/integration/test_replay_routes.py (2)
backend/tests/conftest.py (1)
app(56-67)backend/tests/unit/conftest.py (1)
app(68-69)
backend/tests/integration/test_dlq_routes.py (1)
backend/tests/conftest.py (3)
client(78-86)test_user(123-146)test_settings(35-51)
backend/tests/helpers/k8s_fakes.py (3)
backend/tests/unit/services/pod_monitor/test_event_mapper.py (3)
read_namespaced_pod_log(169-170)read_namespaced_pod_log(173-174)read_namespaced_pod_log(177-178)backend/tests/unit/services/idempotency/test_middleware.py (1)
event(35-39)backend/tests/unit/services/pod_monitor/test_monitor.py (4)
stream(678-680)Pod(391-393)list_namespaced_pod(244-245)list_namespaced_pod(673-675)
🪛 GitHub Actions: MyPy Type Checking
backend/pyproject.toml
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/core/test_logging_and_correlation.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/schemas_pydantic/test_notification_schemas.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/helpers/kafka.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/helpers/eventually.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/schemas_pydantic/test_execution_schemas.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/core/test_security.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/e2e/test_resource_cleaner_orphan.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/conftest.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/schemas_pydantic/test_events_schemas.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/core/test_adaptive_sampling.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/integration/test_events_routes.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/app/core/k8s_clients.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/core/metrics/test_metrics_classes.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/services/pod_monitor/test_monitor.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/integration/test_alertmanager.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/unit/services/pod_monitor/test_event_mapper.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/integration/services/sse/test_partitioned_event_router.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/app/services/pod_monitor/monitor.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/integration/test_replay_routes.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/integration/test_dlq_routes.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
backend/tests/helpers/k8s_fakes.py
[error] 1-1: Mypy type checking failed during CI: 438 errors across 68 files. Command: 'uv run mypy --config-file pyproject.toml --strict .'. Process exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Backend
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (44)
backend/tests/helpers/eventually.py (2)
2-3: LGTM: Modern import style.Using
collections.abcforAwaitableandCallablealigns with Python 3.9+ best practices and PEP 585.
9-9: No issues found. All existing calls toeventually()pass async functions, confirming the signature change is not a breaking change in practice. The implementation is internally consistent with the async-only signature.Regarding MyPy errors: These cannot be investigated from the codebase alone and require access to CI logs. The workflow runs with
--strictmode which is very strict, so pre-existing errors across the codebase are possible but unverifiable without seeing the actual report.backend/tests/integration/test_replay_routes.py (3)
11-11: LGTM!The import of
ReplayFilteris correctly added to support the new filter-based replay request structure.
46-56: LGTM!The refactoring to use
ReplayFiltercorrectly wraps event filtering criteria (event_types, start_time, end_time) in a nested structure. The serialization viamodel_dump(mode="json")ensures proper JSON conversion.
410-414: LGTM!The field name changes from
events_replayed/events_totaltoreplayed_events/total_eventsare correctly applied, and the progress calculation logic remains sound.backend/tests/unit/services/pod_monitor/test_event_mapper.py (7)
7-13: LGTM - Explicit event type imports strengthen test assertions.The addition of explicit event type imports (ExecutionCompletedEvent, ExecutionFailedEvent, ExecutionTimeoutEvent, PodRunningEvent) enables stronger type checking via isinstance assertions rather than relying on duck typing with string comparisons alone.
44-47: LGTM - Type hints improve test helper clarity.Adding type hints to the local
Condtest helper class improves readability and type safety.
64-64: LGTM - isinstance checks strengthen test assertions.The added isinstance checks (lines 64, 74, 86, 95) verify that the mapper returns the correct event class types, not just events with matching event_type string values. This catches type-related bugs more effectively than duck typing alone.
Also applies to: 74-74, 86-86, 95-95
136-139: LGTM - Consistent type hints on test helper.Type hints are consistent with the first
Condclass definition (lines 44-47).
154-154: LGTM - Proper type hints on test fixtures and mock overrides.The type hints are appropriate:
- Line 154: Standard pytest.LogCaptureFixture annotation for the caplog fixture
- Lines 169-178: Mock method signatures match the FakeApi base class, with appropriate
# noqa: ARG002suppressions for intentionally unused parameters in test doublesAlso applies to: 169-178
160-160: LGTM - Explicit non-None assertion improves test clarity.The assertion explicitly verifies that log extraction succeeded before checking log field values, making test expectations clearer.
1-223: No action needed. The MyPy workflow references a non-existentpyproject.tomlfile, and there is no evidence in the repository of "438 errors across 68 files." The test file itself already contains proper type hints on all functions and local classes. If MyPy failures exist in the broader codebase, they should be documented separately with specific evidence rather than referenced without context.Likely an incorrect or invalid review comment.
backend/app/core/k8s_clients.py (1)
6-17: LGTM! Clean DI extension for Kubernetes watch client.Adding
watchto theK8sClientsbundle aligns well with the DI pattern. TheWatchobject is stateless and reusable, making it appropriate to include in the frozen dataclass.One minor note:
close_k8s_clientsonly closesapi_client, which appears correct sinceWatch.stop()is called separately inPodMonitor._on_stop(). This separation of concerns is appropriate.Also applies to: 40-40
backend/tests/helpers/k8s_fakes.py (2)
138-165: LGTM! Well-structured fake watch implementation.The
FakeWatchStreamcorrectly mimics the real Kubernetes watch stream's_stop_eventattribute thatPodMonitor._update_resource_versionaccesses. The iterator pattern is properly implemented.
212-224: LGTM!Clean factory helper that provides DI-friendly fake clients for
PodMonitortests.backend/tests/unit/services/pod_monitor/test_monitor.py (6)
33-46: LGTM! Clean test double for KafkaEventService.Inheriting from
KafkaEventServicewhile bypassing__init__is a practical approach for type safety in tests. Thepublished_eventstracking list enables verification without real Kafka.
52-78: LGTM! Well-designed test utilities.
SpyMapperandmake_k8s_clients_diprovide clean, minimal test doubles. The# type: ignore[arg-type]comments are appropriate given the fake types don't match the real Kubernetes client types.
81-97: LGTM! Excellent test factory pattern.
make_pod_monitorcentralizes testPodMonitorcreation with sensible defaults while allowing dependency overrides. This reduces boilerplate and improves test maintainability.
103-122: LGTM!The lifecycle test properly verifies state transitions and cleanup behavior (cache clearing) using the
SpyMapper.
240-261: LGTM!Clean approach to testing error handling by extending
FakeV1Apiand overridinglist_namespaced_podto raise. The assertions correctly verify the failure result.
485-519: LGTM! Thorough context manager test with proper mocking.The dual
monkeypatch.setattrcalls (lines 507-508) correctly handle both the original module and the monitor's local import. This ensures the mock is used regardless of import timing.backend/app/services/pod_monitor/monitor.py (3)
100-123: LGTM! Clean DI refactor with required dependencies.The constructor now enforces dependency injection by requiring
k8s_clientsandevent_mapper. This eliminates nullability checks throughout the class and makes dependencies explicit. The docstring appropriately directs users tocreate_pod_monitor()for automatic dependency creation.
460-501: LGTM! Well-designed factory with proper resource management.The
create_pod_monitorfactory correctly:
- Tracks ownership of created resources via
owns_clients- Creates dependencies only when not provided (enabling both production and test paths)
- Ensures cleanup in
finallyblock only for factory-created clientsThis is a clean implementation of the production dependency factory pattern.
252-258: Defensive handling of internal stream attribute.The try/except for
AttributeErrorcorrectly handles the case where_stop_eventmay not be present. This is appropriate given_stop_eventis an internal implementation detail of the kubernetes-client library.backend/pyproject.toml (2)
215-215: No changes needed — pytest configuration is properly distributed.The
addopts = "--tb=short"is appropriate as a default traceback format. All required test configuration (coverage reporting, verbosity, test durations, and report style) is explicitly specified in the CI workflows for each test type (unit, integration, e2e), which is a reasonable and maintainable design pattern. No functionality has been lost.
184-189: Pydantic.mypy plugin integration: Monitor type-checking progress.The pydantic.mypy plugin with strict settings (
init_forbid_extra,init_typed,warn_required_dynamic_aliases) has been integrated into the type-checking pipeline. The codebase currently uses 184# type: ignoredirectives to manage type-checking issues while incremental fixes are applied (noted as "part 1").To monitor progress on eliminating these suppressions, consider:
- Running
cd backend && uv run mypy --config-file pyproject.toml .locally to identify remaining issues- Incrementally removing
# type: ignorecomments as the codebase improves- Tracking the number of suppressions across releases to measure type-safety improvements
backend/tests/unit/schemas_pydantic/test_notification_schemas.py (1)
9-9: LGTM! Return type annotations improve type safety.The explicit
-> Noneannotations correctly indicate that these test functions don't return values, enhancing type safety and aligning with the broader PR's typing improvements.Also applies to: 31-31
backend/tests/unit/core/test_logging_and_correlation.py (1)
88-88: LGTM! Return type annotation improves type safety.The explicit
-> Noneannotation correctly indicates this test function doesn't return a value.backend/tests/unit/schemas_pydantic/test_events_schemas.py (1)
7-7: LGTM! Return type annotations improve type safety.The explicit
-> Noneannotations correctly indicate that these test functions don't return values, consistent with the PR's typing improvements.Also applies to: 16-16
backend/tests/unit/schemas_pydantic/test_execution_schemas.py (1)
8-8: LGTM! Return type annotations improve type safety.The explicit
-> Noneannotations correctly indicate that these test functions don't return values, aligning with the broader typing improvements in this PR.Also applies to: 13-13, 19-19
backend/tests/unit/core/metrics/test_metrics_classes.py (5)
1-18: LGTM! Imports and test marker properly configured.The addition of
ExecutionStatusandSettingsimports, along with thepytestmark = pytest.mark.unitmarker, correctly supports the Settings-based DI pattern and test categorization.
21-31: LGTM! Settings DI pattern correctly implemented.The test now accepts
test_settings: Settingsand passes it toConnectionMetrics, aligning with the broader PR's dependency injection migration. The test coverage remains comprehensive.
33-59: LGTM! Settings DI pattern correctly implemented.The test now accepts
test_settings: Settingsand passes it toEventMetrics, maintaining comprehensive coverage of the EventMetrics interface while following the new DI pattern.
61-72: LGTM! Settings DI pattern correctly applied across all metrics classes.All metric class instantiations now receive
test_settings, following the Settings-based DI pattern. The use ofExecutionStatus.QUEUEDforExecutionMetricsis appropriate and demonstrates proper enum usage.
1-72: Type annotations in this test file are properly configured.All test functions have proper type hints (
test_settings: Settingsparameter,-> Nonereturn type), all imports resolve correctly, and the Settings class and metrics base class properly accept the Settings type. This file should not contribute to MyPy errors.backend/tests/integration/services/sse/test_partitioned_event_router.py (1)
30-36: Good alignment:EventMetrics(test_settings)keeps metrics construction consistent with Settings-driven DI.backend/tests/e2e/test_resource_cleaner_orphan.py (2)
16-21: Type-only improvement looks good._ensure_kubeconfig() -> Noneand the explicit-> Noneon the async test are consistent with current behavior.Also applies to: 23-25
44-48:_has_cm() -> Noneis appropriate for an “eventually(assert…)” poller.backend/tests/integration/test_alertmanager.py (1)
1-14: LGTM: type annotation matches theclientfixture (httpx.AsyncClient).backend/tests/integration/test_events_routes.py (1)
309-398: Comment-only tweak (no behavioral change).backend/tests/unit/core/test_security.py (1)
11-21: Good: tests now constructSecurityService(test_settings)consistently.Also applies to: 75-78, 286-291
backend/tests/helpers/kafka.py (1)
1-21: LGTM: correct typing for AsyncContainer scope and proper asyncscope.get()API usage.The fixture correctly uses
AsyncContainer(dishka 1.6.0) andawait scope.get(UnifiedProducer)pattern, which matches established patterns throughout the codebase (conftest.py, integration tests). Imports fromcollections.abcand the fixture structure provide proper typing for both the producer and the async callable.backend/tests/integration/test_dlq_routes.py (2)
35-35: LGTM! Modern type annotations.The update from
Dict[str, str]todict[str, str]correctly uses Python 3.9+ built-in generic types, improving readability and reducing reliance on thetypingmodule.Also applies to: 80-80, 112-112, 127-127, 142-142, 182-182, 218-218, 255-255, 286-286, 324-324, 354-354
200-200: Correct use of topic prefix for test isolation.Prepending
test_settings.KAFKA_TOPIC_PREFIXensures proper test isolation and aligns with the PR's Settings-driven approach. The DLQ system normalizes topic names to match retry policies correctly.
There was a problem hiding this comment.
1 issue found across 63 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/tests/unit/services/sse/test_sse_service.py">
<violation number="1" location="backend/tests/unit/services/sse/test_sse_service.py:17">
P0: Incorrect import path - module `app.services.sse.shutdown_manager` does not exist. Should be `app.services.sse.sse_shutdown_manager`. This will cause an `ImportError` at test runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
backend/tests/load/user_runner.py (2)
64-64: Fix import order:timemodule used before import.Line 64 references
time.time()but thetimemodule is imported later on line 85. This will cause aNameErrorat runtime when_flow_saved_scriptsis executed.🐛 Proposed fix
Move the
timeimport to the top of the file with other imports:from __future__ import annotations import asyncio import random +import time from collections.abc import Awaitable from dataclasses import dataclass from typing import CallableAnd remove the duplicate import from line 85:
await c.mark_all_read() -import time - - async def run_user_swarm(cfg: LoadConfig, stats: StatsCollector, clients: int) -> None:Also applies to: 85-85
89-89: Add type parameter to fix MyPy error.The
asyncio.Taskgeneric type requires a type parameter. Since the spawned tasks returnNone, specifyasyncio.Task[None].🔧 Proposed fix
- tasks: list[asyncio.Task] = [] + tasks: list[asyncio.Task[None]] = []backend/tests/load/http_client.py (1)
70-70: Remove the unused type: ignore comment.The mypy type checker indicates this
type: ignoredirective is no longer needed and should be removed to fix the pipeline failure.🔧 Proposed fix
- for cookie in self.client.cookies.jar: # type: ignore[attr-defined] + for cookie in self.client.cookies.jar:backend/tests/unit/events/test_event_dispatcher.py (1)
54-54: Remove the unusedtype: ignorecomment.MyPy indicates this
type: ignorecomment is no longer needed, suggesting the type system now correctly recognizesevent_typeas assignable.🧹 Proposed fix
- e.event_type = EventType.EXECUTION_FAILED # type: ignore[attr-defined] + e.event_type = EventType.EXECUTION_FAILEDbackend/tests/integration/app/test_main_app.py (1)
27-35: Fix MyPy type errors in middleware assertions.MyPy reports non-overlapping container checks on lines 30-35. The type of elements in
middleware_classes(_MiddlewareFactory[P]) doesn't match the middleware class types being checked (e.g.,CORSMiddleware). This causes the pipeline to fail and suggests the assertions might not work as intended.The issue stems from line 27 where
m.clsextracts the class from middleware factories, but the type system doesn't recognize the relationship.🔧 Investigate the correct approach
Consider one of these approaches:
Option 1: Extract classes correctly and ensure type compatibility:
# If m.cls returns the actual class, you may need to cast or adjust the type middleware_classes = {type(m.cls) for m in app.user_middleware}Option 2: Use a different attribute or method to get middleware classes:
# Check FastAPI/Starlette documentation for the correct way to extract middleware classes middleware_classes = {m.__class__ for m in app.user_middleware}Option 3: Inspect the actual middleware stack structure:
Run this script to understand the middleware structure:
#!/bin/bash # Description: Search for middleware-related code and FastAPI app configuration # Find how middlewares are added to the app rg -n 'add_middleware|middleware' backend/app/main.py -A 3 -B 3 # Look for similar test patterns rg -n 'user_middleware' --type=py -C 3Verify the correct approach based on the FastAPI/Starlette version in use.
backend/tests/integration/dlq/test_dlq_discard_policy.py (1)
48-48: Usetest_settings.KAFKA_BOOTSTRAP_SERVERSinstead of hardcoded address.The Producer is created with hardcoded
"localhost:9092"while the rest of the test usestest_settingsfor configuration. This inconsistency could cause test failures if Kafka runs on a different address.Suggested fix
- producer = Producer({"bootstrap.servers": "localhost:9092"}) + producer = Producer({"bootstrap.servers": test_settings.KAFKA_BOOTSTRAP_SERVERS})backend/tests/integration/dlq/test_dlq_retry_immediate.py (1)
51-51: Inconsistent with Settings-driven configuration.The Producer is hardcoded to use
"localhost:9092"instead oftest_settings.KAFKA_BOOTSTRAP_SERVERS, which contradicts the PR objective of centralizing Settings-driven dependency injection. Line 30 correctly usestest_settingsfor the DLQ manager.🔧 Proposed fix
- prod = Producer({"bootstrap.servers": "localhost:9092"}) + prod = Producer({"bootstrap.servers": test_settings.KAFKA_BOOTSTRAP_SERVERS})backend/tests/integration/dlq/test_dlq_manager.py (1)
46-46: Inconsistent with Settings-driven configuration.The Producer is hardcoded to use
"localhost:9092"instead oftest_settings.KAFKA_BOOTSTRAP_SERVERS, which contradicts the PR objective of centralizing Settings-driven dependency injection. Line 29 correctly usestest_settingsfor the DLQ manager.🔧 Proposed fix
- producer = Producer({"bootstrap.servers": "localhost:9092"}) + producer = Producer({"bootstrap.servers": test_settings.KAFKA_BOOTSTRAP_SERVERS})backend/tests/integration/test_events_routes.py (2)
39-43: Be consistent about allowing 404 vs requiring 200 for/api/v1/events/userto avoid re-flaking tests.
You already allow404in some tests for portability, but others hard-assert200on the same endpoint family. Consider a single helper/guard: if404,pytest.skip(...).Also applies to: 95-97, 188-190, 400-402, 433-435
459-502:test_events_isolation_between_users: theuser_idassertions are ineffective.The
test_userandtest_adminfixtures return dictionaries withusername,password,role,csrf_token, andheaders—but nouser_id. Consequently,test_user.get("user_id", meta["user_id"])defaults tometa["user_id"], making assertions likeassert meta["user_id"] == test_user.get("user_id", meta["user_id"])equivalent toassert meta["user_id"] == meta["user_id"]. This tautology fails to validate isolation. Extract and assert the actual user IDs from the login or registration response instead.backend/tests/integration/services/idempotency/test_redis_repository.py (1)
56-69: Increase TTL in tests to reduce CI flakiness (5s is tight).
A slow runner can legitimately expire the key before update/ttl checks.Proposed adjustment
def sample_record() -> IdempotencyRecord: return IdempotencyRecord( key="test-key", status=IdempotencyStatus.PROCESSING, event_type="test.event", event_id="event-123", created_at=datetime(2025, 1, 15, 10, 30, 45, tzinfo=timezone.utc), - ttl_seconds=5, + ttl_seconds=60, completed_at=None, processing_duration_ms=None, error=None, result_json=None, )Also applies to: 95-124
backend/tests/integration/test_dlq_routes.py (2)
133-146: Update topic filter to use correct name with test environment prefix.This test filters DLQ messages by the hardcoded topic
"execution-events"(line 136), but:
- The enum value is
KafkaTopic.EXECUTION_EVENTS = "execution_events"(underscore, not hyphen)- Test environment topics are prefixed with
KAFKA_TOPIC_PREFIX(e.g.,"prefix.execution_events"), as shown in other DLQ tests (test_dlq_manager.py, test_dlq_retry_immediate.py)- Repository filtering uses exact equality, so the unprefixed topic name will never match actual DLQ messages
The test will silently pass with an empty message list since the for loop (line 144) doesn't execute. Replace the hardcoded topic with the prefixed enum value:
Suggested fix
test_topic = f"{test_settings.KAFKA_TOPIC_PREFIX}{KafkaTopic.EXECUTION_EVENTS}"Also import
KafkaTopicat the top of the test file.
1-22: Fix the MyPy type export error inapp/schemas_pydantic/dlq.pyand correct thetest_userparameter type annotation.The MyPy failure is due to
DLQMessageStatusbeing imported but not explicitly exported. Add an__all__list toapp/schemas_pydantic/dlq.pythat includes all exported schema classes. Additionally, thetest_userparameter is annotated asdict[str, str]throughout the test file, but the fixture returnsUserCredentials(a TypedDict withusername: str,email: str,password: str,role: str,csrf_token: str,headers: dict[str, str]). Update all method signatures to usetest_user: UserCredentialsand importUserCredentialsfromconftest.
🤖 Fix all issues with AI agents
In @backend/tests/e2e/conftest.py:
- Line 12: The MyPy error is caused by using the generic redis.Redis without
type arguments in the _cleanup fixture signature; keep the explicit return type
AsyncGenerator[None, None] but suppress the missing type-arg complaint by adding
a per-annotation ignore to the redis.Redis parameter (i.e., change the parameter
annotation for redis_client: redis.Redis to include a "# type: ignore[type-arg]"
comment), leaving function name _cleanup and other logic unchanged.
In @backend/tests/integration/dlq/test_dlq_discard_policy.py:
- Line 16: The tests.helpers package is missing an explicit export list, causing
MyPy to complain about make_execution_requested_event; update
backend/tests/helpers/__init__.py to define an __all__ that includes
"make_execution_requested_event" so the symbol is explicitly exported from the
package and recognized by the type checker.
In @backend/tests/integration/events/test_consume_roundtrip.py:
- Line 16: The import error comes from tests.helpers not exporting
make_execution_requested_event; update tests/helpers/__init__.py to re-export
that symbol (e.g., add "from .some_module import make_execution_requested_event"
and include it in __all__ or otherwise expose it) so that "from tests.helpers
import make_execution_requested_event" in test_consume_roundtrip.py resolves
correctly.
In @backend/tests/integration/events/test_schema_registry_roundtrip.py:
- Line 8: The test imports make_execution_requested_event from tests.helpers but
that symbol is defined in tests.helpers.events; update the import to "from
tests.helpers.events import make_execution_requested_event" so the name resolves
for mypy, or alternatively export it from tests.helpers by importing
make_execution_requested_event into tests/helpers/__init__.py and adding it to
__all__ so tests.helpers exposes the symbol.
In @backend/tests/integration/test_sse_routes.py:
- Around line 46-88: The second test uses a fixed user_id which risks cross-test
interference and also relies on asyncio.sleep(0.1) which is brittle; change the
constant user_id to a randomized one (like how test_notification_stream_service
creates user_id) and remove the sleep by waiting for an explicit
readiness/confirmation from the SSE stream or Redis bus before publishing.
Specifically, update the test that currently sets user_id = "test-user-id" to
generate a unique id (e.g., via uuid4) and replace the await asyncio.sleep(0.1)
used before bus.publish_notification with a readiness check (e.g., wait for the
"connected" event from create_notification_stream or use a subscription
confirmation method on SSERedisBus) so publishing only happens after the
subscription is established.
In @backend/tests/integration/test_user_settings_routes.py:
- Line 2: Import the actual fixture TypedDict (UserCredentials) used in
conftest.py and change all test parameter annotations from dict[str, str] to
UserCredentials: add an import for UserCredentials at the top of the file, then
update every test function signature that accepts test_user or another_user
(e.g., functions referencing parameters at the listed usages) to use the
UserCredentials type instead of dict[str, str].
In @backend/tests/load/plot_report.py:
- Around line 46-47: The mypy error comes from annotating tl as TimelineData
while using report.get("timeline", {}) whose default {} isn’t provably
TimelineData; in function plot_timeline replace the annotated pattern by either
casting the result (import cast from typing and set tl = cast(TimelineData,
report.get("timeline", {}))) or remove the explicit annotation and let mypy
infer tl from report: ReportDict (i.e., tl = report.get("timeline", {})); update
the single usage of tl accordingly.
- Around line 71-72: The mypy error comes from using report.get(...) with a
TypedDict and the explicit eps: Dict[str, EndpointData] annotation; update the
assignment in _top_endpoints so it matches the fix used at line 47 — remove the
strict annotation or add a type ignore on the .get() call for consistency (e.g.,
assign eps = report.get("endpoints", {}) # type: ignore) so mypy stops
complaining while preserving the intended default empty dict behavior for eps.
In @backend/tests/unit/services/sse/test_kafka_redis_bridge.py:
- Around line 49-56: The model_dump return annotation on _DummyEvent is too
narrow; change its return type from dict[str, str | None] to dict[str, Any] to
match BaseEvent.model_dump, and add the necessary Any import (from typing import
Any) at the top of the test file; keep the method body unchanged.
- Around line 35-43: The _StubDispatcher currently stores a single handler per
EventType causing overwrites; change its handlers type to dict[EventType,
list[Callable[[BaseEvent], Awaitable[None]]]] and implement register_handler on
_StubDispatcher to append the fn to the list (creating the list if missing) to
match EventDispatcher's contract, and update tests that access a handler (e.g.,
retrieving h from disp.handlers[EventType.EXECUTION_STARTED]) to use the first
element of the list (index 0).
In @backend/tests/unit/services/sse/test_shutdown_manager.py:
- Line 49: The test uses cast(SSEKafkaRedisBridge, router) to bypass type checks
even though DummyRouter doesn't implement that interface; update the test to
preserve type safety by either (a) introducing a small Protocol with the
required async aclose method (e.g., RouterProtocol) and make DummyRouter
implement it and change SSEShutdownManager.set_router to accept RouterProtocol,
or (b) replace DummyRouter with a proper AsyncMock that defines aclose and pass
that to SSEShutdownManager.set_router (no cast). Ensure references to
set_router, SSEShutdownManager, DummyRouter, and SSEKafkaRedisBridge are updated
accordingly.
In @backend/tests/unit/services/sse/test_sse_service.py:
- Around line 122-126: The function _make_fake_settings currently returns a
MagicMock typed as Settings which can trigger MyPy "incompatible return value";
fix it by importing and using typing.cast to cast the MagicMock to Settings
(e.g., return cast(Settings, mock)) so the return type matches the annotation,
ensuring you reference the MagicMock created in _make_fake_settings and the
Settings type.
🧹 Nitpick comments (21)
backend/tests/load/http_client.py (1)
108-110: Reconsider the cookie merge strategy.The change from assignment to
.update()seems unnecessary here. Sincesis a freshAsyncClientinstance created on line 108, its cookie jar starts empty. Using.update()suggests preserving pre-existing cookies, but there are none in this context. The previous assignment pattern (s.cookies = self.client.cookies.copy()) more clearly expressed the intent to copy all authentication cookies to the streaming client.♻️ Proposed revert to assignment pattern
- s.cookies.update(self.client.cookies) + s.cookies = self.client.cookies.copy()backend/tests/integration/events/test_schema_registry_roundtrip.py (1)
23-23: Consider using direct attribute access or explicit attribute check.The defensive
getattr(back, "execution_id", None)obscures the test's intent. In a roundtrip serialization test,execution_idshould always be present after deserialization. If it's missing, that's a bug that should raise anAttributeErrorwith a clear message rather than a generic assertion failure.💡 Clearer alternatives
Option 1: Direct attribute access (recommended)
- assert back.event_id == ev.event_id and getattr(back, "execution_id", None) == ev.execution_id + assert back.event_id == ev.event_id and back.execution_id == ev.execution_idOption 2: Explicit attribute check with better error message
- assert back.event_id == ev.event_id and getattr(back, "execution_id", None) == ev.execution_id + assert back.event_id == ev.event_id + assert hasattr(back, "execution_id"), "Deserialized event missing execution_id attribute" + assert back.execution_id == ev.execution_idbackend/tests/load/strategies.py (1)
53-54: Good refactor for clarity.Renaming to
label_dictandannotation_dictimproves readability by making it clear these are dictionary strategy builders, distinguishing them from the string keys used in the data structures.backend/tests/unit/services/sse/test_sse_shutdown_manager.py (2)
3-3: Avoidcast()here; use a properly-typed fake router to keep type-safety (and reduce runtime surprise).
mgr.set_router(cast(SSEKafkaRedisBridge, _FakeRouter()))bypasses mypy instead of satisfying it. Since this PR is about DI + typing stability, it’s better if the fake actually conforms (similar tobackend/tests/unit/services/sse/test_sse_service.py’s_FakeRouter(SSEKafkaRedisBridge)pattern).Proposed change
-from typing import cast +from typing import Any from app.services.sse.kafka_redis_bridge import SSEKafkaRedisBridge from app.services.sse.sse_shutdown_manager import SSEShutdownManager @@ -class _FakeRouter: +class _FakeRouter(SSEKafkaRedisBridge): def __init__(self) -> None: - self.stopped = False + # Skip parent __init__ + self.stopped = False async def stop(self) -> None: self.stopped = True + + def get_stats(self) -> dict[str, int | bool]: # type: ignore[override] + # Only if SSEShutdownManager reads stats; harmless otherwise. + return {"num_consumers": 0, "active_executions": 0, "is_running": False, "total_buffers": 0} @@ - mgr.set_router(cast(SSEKafkaRedisBridge, _FakeRouter())) + mgr.set_router(_FakeRouter())Also applies to: 9-9, 26-26
65-68: The current code is type-safe; prefer explicitreturn boolfor clarity.The
_initiated()function with-> Noneand an assertion is technically valid foreventually(), which acceptsCallable[[], Awaitable[T]]and returnsT(None in this case). However, asserting inside the function is less idiomatic than explicitly returning a boolean predicate. The proposed change improves code clarity without fixing a type error.Suggested change
- async def _initiated() -> None: - assert mgr.is_shutting_down() is True + async def _initiated() -> bool: + return mgr.is_shutting_down() is Truebackend/tests/load/plot_report.py (1)
34-37: Consider adding runtime validation for the loaded JSON.The function assumes
json.load()returns data matching theReportDictstructure. While the TypedDict withtotal=Falsehandles missing fields gracefully, malformed JSON could cause runtime errors downstream.Optional: Add basic validation
You could add a simple validation check:
def _load_report(path: str | Path) -> ReportDict: with open(path, "r", encoding="utf-8") as f: result: ReportDict = json.load(f) # Basic validation if not isinstance(result, dict): raise ValueError(f"Invalid report format in {path}") return resultHowever, for a load testing utility script, the current approach is pragmatic and acceptable.
backend/tests/integration/services/notifications/test_notification_service.py (1)
18-21: Consider simplifying the redundant get operation.Lines 20-21 fetch the notification immediately after creation just to assert it's not None. Since you already have the
createdobject, you could:
- Assert directly on
created(e.g.,assert created.notification_id is not None)- Or if verification is needed, assert that
gotmatches thecreatednotification's propertiesThe current pattern adds an unnecessary database round-trip without validating the data integrity.
♻️ Proposed simplification
n = DomainNotificationCreate(user_id="u1", severity=NotificationSeverity.MEDIUM, tags=["x"], channel=NotificationChannel.IN_APP, subject="s", body="b") created = await repo.create_notification(n) -got = await repo.get_notification(created.notification_id, "u1") -assert got is not None +assert created.notification_id is not NoneOr, if you want to verify the round-trip:
n = DomainNotificationCreate(user_id="u1", severity=NotificationSeverity.MEDIUM, tags=["x"], channel=NotificationChannel.IN_APP, subject="s", body="b") created = await repo.create_notification(n) got = await repo.get_notification(created.notification_id, "u1") -assert got is not None +assert got is not None +assert got.notification_id == created.notification_id +assert got.subject == "s"backend/tests/unit/services/sse/test_sse_service.py (1)
140-155: Prevent test hangs: wrap allagen.__anext__()calls with timeouts (you only do it once today).Several
await agen.__anext__()calls have no timeout; if the generator stalls, the suite can hang indefinitely (especially with “return None on any exception” in the fake subscription).One way to do it (minimal local helper)
@@ _test_logger = logging.getLogger("test.services.sse.sse_service") + +async def _anext(agen: Any, timeout: float = 1.0) -> Any: + return await asyncio.wait_for(agen.__anext__(), timeout=timeout) @@ - first = await agen.__anext__() + first = await _anext(agen) @@ - stat = await agen.__anext__() + stat = await _anext(agen) @@ - failed = await agen.__anext__() + failed = await _anext(agen) @@ - await agen.__anext__() # connected - await agen.__anext__() # status + await _anext(agen) # connected + await _anext(agen) # status @@ - evt = await agen.__anext__() + evt = await _anext(agen) @@ - connected = await agen.__anext__() + connected = await _anext(agen) @@ - hb = await agen.__anext__() + hb = await _anext(agen) @@ - notif = await agen.__anext__() + notif = await _anext(agen)Also applies to: 177-189, 200-229
backend/tests/unit/services/pod_monitor/test_monitor.py (4)
485-509: Prefer module-object monkeypatch over string targets (often nicer with typing + refactors).Using
"pkg.mod.attr"string targets works, but tends to be less type-checker-friendly and more brittle during refactors.Proposed refactor
async def test_create_pod_monitor_context_manager(monkeypatch: pytest.MonkeyPatch) -> None: @@ - monkeypatch.setattr(k8s_clients_module, "create_k8s_clients", mock_create_clients) - monkeypatch.setattr("app.services.pod_monitor.monitor.create_k8s_clients", mock_create_clients) + import app.services.pod_monitor.monitor as pod_monitor_module + + monkeypatch.setattr(k8s_clients_module, "create_k8s_clients", mock_create_clients) + monkeypatch.setattr(pod_monitor_module, "create_k8s_clients", mock_create_clients)
176-206: Addasyncio.wait_foraround awaits of looping coroutines to prevent suite hangs.A regression in the loop exit conditions could hang CI indefinitely (especially
_watch_pods()/_reconciliation_loop()).Example pattern to apply in these tests
- await pm._watch_pods() + await asyncio.wait_for(pm._watch_pods(), timeout=1.0)- task = asyncio.create_task(pm._reconciliation_loop()) + task = asyncio.create_task(asyncio.wait_for(pm._reconciliation_loop(), timeout=2.0))Also applies to: 413-434, 436-482, 640-662, 698-721
576-594: Assert spawned tasks are cancelled/done afteraclose()(leak prevention).
test_stop_with_tasksrelies onaclose()to stop two tasks that otherwise wait forever; asserting task completion helps catch leaks (and avoids “Task was destroyed but it is pending” warnings if behavior changes).Proposed assertion add
await pm.aclose() assert pm._state == MonitorState.STOPPED assert len(pm._tracked_pods) == 0 + assert pm._watch_task is None or pm._watch_task.done() + assert pm._reconcile_task is None or pm._reconcile_task.done()
52-63: Consider a small Protocol for the mapper used bymake_pod_monitorto avoidtype: ignore[arg-type].Right now the tests pass non-
PodEventMapperinstances (e.g.,SpyMapper) via# type: ignore[arg-type]. A localProtocolfor the minimal surface (clear_cache,map_pod_event) would keep the flexibility without suppressions.Also applies to: 81-97, 103-123
backend/tests/integration/services/rate_limit/test_rate_limit_service.py (1)
275-276: Minor: String forward reference unnecessary.Since
Awaitableis already imported at line 3, you can useAwaitable[int]directly instead of the string form"Awaitable[int]".Suggested simplification
- awaitable_result = cast("Awaitable[int]", svc.redis.sadd(index_key, sw_key)) + awaitable_result = cast(Awaitable[int], svc.redis.sadd(index_key, sw_key))backend/tests/integration/idempotency/test_consumer_idempotent.py (1)
71-72: Replace fixed sleep with polling for consumer readiness.The 2-second sleep is a timing-based workaround that could introduce test flakiness. If the consumer joins faster than 2 seconds, the test wastes time; if it takes longer, the test could fail intermittently.
♻️ Suggested improvement using eventually helper
The codebase already has an
eventuallyhelper (imported on line 18) that could replace the sleep:await wrapper.start([KafkaTopic.EXECUTION_EVENTS]) - # Allow time for consumer to join group and get partition assignments - await asyncio.sleep(2) + + # Wait for consumer to join group and get partition assignments + async def _consumer_ready() -> None: + # Consumer is ready when it has partition assignments + # This could check wrapper.consumer._consumer.assignment() or similar + assert True # Replace with actual readiness check + + await eventually(_consumer_ready, timeout=5.0, interval=0.1)Note: You'll need to determine the appropriate readiness check (e.g., checking partition assignments).
backend/tests/integration/idempotency/test_idempotency.py (2)
406-418: Use a stable hash for custom idempotency keys (avoidhash()randomness).
hash(script)changes between processes; swapping tosha256makes the test reflect real-world idempotency key expectations.Proposed refactor
+import hashlib ... def extract_script_key(event: BaseEvent) -> str: # Custom key based on script content only script: str = getattr(event, "script", "") - return f"script:{hash(script)}" + digest = hashlib.sha256(script.encode("utf-8")).hexdigest() + return f"script:{digest}"
61-72: Direct_repoaccess in tests is fine, but it’s tight coupling.
If there’s a public test hook / repository accessor, prefer it to reduce churn when internals change.Also applies to: 121-125, 169-171, 246-247, 338-342
backend/tests/integration/test_sse_routes.py (1)
149-171: Concurrent stream test: consider bounding the per-stream wait.
Ifcreate_notification_stream()ever yields non-dataevents first, this can returnFalsetoo early. A small bounded loop/time limit per stream would make this more robust.backend/tests/unit/services/saga/test_saga_orchestrator_unit.py (2)
58-90: Fakes inheriting from concrete classes while skipping__init__is brittle.
Prefer small fakes that implement the needed interface (orMagicMock(spec=...)) rather than subclassingUnifiedProducer/IdempotencyManager/EventStorewith uninitialized base state.Also applies to: 116-127
134-143: Unit-test-only lifecycle toggling is OK, but consider a helper to reduce private coupling.
E.g., a local_start_orch(orch)helper to centralize the private flag if it changes again.backend/app/core/providers.py (1)
99-120: Redis client creation: consider moving timeouts to Settings (and confirm redis-py async API).
Hardcoding connect/read timeouts is fine, but making them configurable tends to help in CI/prod variance; also confirmaclose()availability for yourredisversion.backend/tests/integration/test_dlq_routes.py (1)
20-21: Consider applying the_RetryRequestTypedDict consistently.The
_RetryRequestTypedDict is defined but only used once (line 377). The same payload structure is created at lines 237-239 without the type annotation. For consistency and better type safety, consider annotating that usage as well.♻️ Suggested fix for consistent typing
if messages_data["total"] > 0 and messages_data["messages"]: # Collect event IDs to retry event_ids = [msg["event_id"] for msg in messages_data["messages"][:2]] # Retry the messages - retry_request = { + retry_request: _RetryRequest = { "event_ids": event_ids }
| from app.infrastructure.kafka.events.base import BaseEvent | ||
| from app.settings import Settings | ||
|
|
||
| from tests.helpers import make_execution_requested_event |
There was a problem hiding this comment.
Same tests.helpers export issue.
This file has the same MyPy error as test_dlq_discard_policy.py. Once tests/helpers/__init__.py is fixed to export make_execution_requested_event, this will be resolved.
🧰 Tools
🪛 GitHub Actions: MyPy Type Checking
[error] 16-16: Module "tests.helpers" does not explicitly export attribute "make_execution_requested_event" [attr-defined]
🤖 Prompt for AI Agents
In @backend/tests/integration/events/test_consume_roundtrip.py at line 16, The
import error comes from tests.helpers not exporting
make_execution_requested_event; update tests/helpers/__init__.py to re-export
that symbol (e.g., add "from .some_module import make_execution_requested_event"
and include it in __all__ or otherwise expose it) so that "from tests.helpers
import make_execution_requested_event" in test_consume_roundtrip.py resolves
correctly.
| from app.events.schema.schema_registry import MAGIC_BYTE, SchemaRegistryManager | ||
| from app.settings import Settings | ||
|
|
||
| from tests.helpers import make_execution_requested_event |
There was a problem hiding this comment.
Fix the import to resolve mypy pipeline failure.
The import fails type checking because make_execution_requested_event is not exported from tests.helpers. Based on the relevant code snippets, the function is defined in tests.helpers.events.
🔧 Proposed fix
-from tests.helpers import make_execution_requested_event
+from tests.helpers.events import make_execution_requested_eventAlternatively, if you prefer to keep the current import path, add the function to tests/helpers/__init__.py:
from tests.helpers.events import make_execution_requested_event
__all__ = ["make_execution_requested_event"]📝 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.
| from tests.helpers import make_execution_requested_event | |
| from tests.helpers.events import make_execution_requested_event |
🧰 Tools
🪛 GitHub Actions: MyPy Type Checking
[error] 8-8: Module "tests.helpers" does not explicitly export attribute "make_execution_requested_event" [attr-defined]
🤖 Prompt for AI Agents
In @backend/tests/integration/events/test_schema_registry_roundtrip.py at line
8, The test imports make_execution_requested_event from tests.helpers but that
symbol is defined in tests.helpers.events; update the import to "from
tests.helpers.events import make_execution_requested_event" so the name resolves
for mypy, or alternatively export it from tests.helpers by importing
make_execution_requested_event into tests/helpers/__init__.py and adding it to
__all__ so tests.helpers exposes the symbol.
| async def test_notification_stream_service(self, scope: AsyncContainer, test_user: dict[str, str]) -> None: | ||
| """Test SSE notification stream directly through service (httpx doesn't support SSE streaming).""" | ||
| sse_service: SSEService = await scope.get(SSEService) | ||
| bus: SSERedisBus = await scope.get(SSERedisBus) | ||
| user_id = f"user-{uuid4().hex[:8]}" | ||
|
|
||
| # Create notification stream generator | ||
| stream_gen = sse_service.create_notification_stream(user_id) | ||
|
|
||
| # Collect events with timeout | ||
| events = [] | ||
| async def collect_events(): | ||
| async for event in stream_gen: | ||
| if "data" in event: | ||
| data = json.loads(event["data"]) | ||
| events.append(data) | ||
| if data.get("event_type") == "notification" and data.get("subject") == "Hello": | ||
| break | ||
|
|
||
| # Start collecting events | ||
| collect_task = asyncio.create_task(collect_events()) | ||
|
|
||
| # Wait until the initial 'connected' event is received | ||
| async def _connected() -> None: | ||
| assert len(events) > 0 and events[0].get("event_type") == "connected" | ||
| await eventually(_connected, timeout=2.0, interval=0.05) | ||
|
|
||
| # Publish a notification | ||
| notification = RedisNotificationMessage( | ||
| notification_id=f"notif-{uuid4().hex[:8]}", | ||
| severity=NotificationSeverity.MEDIUM, | ||
| status=NotificationStatus.PENDING, | ||
| tags=[], | ||
| subject="Hello", | ||
| body="World", | ||
| action_url="", | ||
| created_at="2024-01-01T00:00:00Z", | ||
| ) | ||
| await bus.publish_notification(user_id, notification) | ||
|
|
||
| # Wait for collection to complete | ||
| try: | ||
| await asyncio.wait_for(collect_task, timeout=2.0) | ||
| except asyncio.TimeoutError: | ||
| collect_task.cancel() | ||
|
|
||
| # Verify we got notification | ||
| notif_events = [e for e in events if e.get("event_type") == "notification" and e.get("subject") == "Hello"] | ||
| assert len(notif_events) > 0 | ||
|
|
||
| events: list[dict[str, Any]] = [] | ||
| notification_received = False | ||
|
|
||
| async with aclosing(sse_service.create_notification_stream(user_id)) as stream: | ||
| try: | ||
| async with asyncio.timeout(3.0): | ||
| async for event in stream: | ||
| if "data" not in event: | ||
| continue | ||
| data = json.loads(event["data"]) | ||
| events.append(data) | ||
|
|
||
| # After connected, publish the notification | ||
| if data.get("event_type") == "connected": | ||
| # Small delay to let Redis pubsub subscription fully establish | ||
| await asyncio.sleep(0.1) | ||
| notification = RedisNotificationMessage( | ||
| notification_id=f"notif-{uuid4().hex[:8]}", | ||
| severity=NotificationSeverity.MEDIUM, | ||
| status=NotificationStatus.PENDING, | ||
| tags=[], | ||
| subject="Hello", | ||
| body="World", | ||
| action_url="", | ||
| created_at="2024-01-01T00:00:00Z", | ||
| ) | ||
| await bus.publish_notification(user_id, notification) | ||
|
|
||
| # Stop when we receive the notification | ||
| if data.get("event_type") == "notification" and data.get("subject") == "Hello": | ||
| notification_received = True | ||
| break | ||
| except TimeoutError: | ||
| pass | ||
|
|
||
| assert notification_received, f"Expected notification, got events: {events}" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the CI configuration files
find . -type f -name "*.yml" -o -name "*.yaml" | head -20Repository: HardMax71/Integr8sCode
Length of output: 820
🏁 Script executed:
# Also check for pyproject.toml or setup.py to see Python version requirements
fd -e toml -e cfg -e ini | grep -E "(pyproject|setup|tox|poetry)"Repository: HardMax71/Integr8sCode
Length of output: 88
🏁 Script executed:
# Read the test file to see lines 90-130
wc -l backend/tests/integration/test_sse_routes.pyRepository: HardMax71/Integr8sCode
Length of output: 114
🏁 Script executed:
# Read the full test file to understand context
cat -n backend/tests/integration/test_sse_routes.py | head -150Repository: HardMax71/Integr8sCode
Length of output: 8065
🏁 Script executed:
cat backend/pyproject.toml | head -50Repository: HardMax71/Integr8sCode
Length of output: 1347
🏁 Script executed:
cat .github/workflows/backend-ci.yml | grep -A 20 "python-version"Repository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
# Search for any version guards or fallbacks for asyncio.timeout
rg "asyncio.timeout|version_info|sys.version" backend/tests/integration/test_sse_routes.pyRepository: HardMax71/Integr8sCode
Length of output: 163
🏁 Script executed:
# Check if there's a conftest or common imports that might provide fallback
fd conftest.py | xargs cat -n 2>/dev/null | head -100Repository: HardMax71/Integr8sCode
Length of output: 3913
🏁 Script executed:
# Get more info from backend-ci.yml
cat .github/workflows/backend-ci.yml | head -100Repository: HardMax71/Integr8sCode
Length of output: 2620
🏁 Script executed:
# Double-check the specific lines mentioned for user_id usage
sed -n '46,130p' backend/tests/integration/test_sse_routes.py | grep -n "user_id"Repository: HardMax71/Integr8sCode
Length of output: 420
Constant user_id in test_execution_event_stream_service (line 93) creates test isolation risk; replace fixed sleep with readiness signal.
The first test (lines 46-88) correctly randomizes user_id, but the second test (lines 90-130) uses a constant user_id = "test-user-id" which can cause interference across parallel test runs and violate test isolation. Additionally, both tests rely on asyncio.sleep(0.1) to allow Redis pubsub subscription to establish—this is timing-sensitive and fragile. Replace the sleep with an explicit readiness signal (e.g., a subscription confirmation event or status flag) if available in the Redis bus implementation.
asyncio.timeout (Python 3.11+) is fully supported by the project's Python 3.12+ requirement.
🤖 Prompt for AI Agents
In @backend/tests/integration/test_sse_routes.py around lines 46 - 88, The
second test uses a fixed user_id which risks cross-test interference and also
relies on asyncio.sleep(0.1) which is brittle; change the constant user_id to a
randomized one (like how test_notification_stream_service creates user_id) and
remove the sleep by waiting for an explicit readiness/confirmation from the SSE
stream or Redis bus before publishing. Specifically, update the test that
currently sets user_id = "test-user-id" to generate a unique id (e.g., via
uuid4) and replace the await asyncio.sleep(0.1) used before
bus.publish_notification with a readiness check (e.g., wait for the "connected"
event from create_notification_stream or use a subscription confirmation method
on SSERedisBus) so publishing only happens after the subscription is
established.
| class _StubDispatcher(EventDispatcher): | ||
| """Stub EventDispatcher for testing.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| self.handlers: dict[EventType, object] = {} | ||
| self.handlers: dict[EventType, Callable[[BaseEvent], Awaitable[None]]] = {} | ||
|
|
||
| def register_handler(self, et: EventType, fn: object) -> None: | ||
| def register_handler(self, et: EventType, fn: Callable[[BaseEvent], Awaitable[None]]) -> None: | ||
| self.handlers[et] = fn | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the EventDispatcher class
fd -t f -name "*.py" | xargs rg "class EventDispatcher" -B 2 -A 10Repository: HardMax71/Integr8sCode
Length of output: 1173
🏁 Script executed:
# Also check imports in the test file to understand where EventDispatcher comes from
head -30 backend/tests/unit/services/sse/test_kafka_redis_bridge.pyRepository: HardMax71/Integr8sCode
Length of output: 1021
🏁 Script executed:
# Read the EventDispatcher class to find register_handler method
cat backend/app/events/core/dispatcher.pyRepository: HardMax71/Integr8sCode
Length of output: 7723
🏁 Script executed:
# Read the test file to see _StubDispatcher usage
cat backend/tests/unit/services/sse/test_kafka_redis_bridge.py | head -100Repository: HardMax71/Integr8sCode
Length of output: 3161
🏁 Script executed:
# Search for uses of _StubDispatcher in the test file
rg "_StubDispatcher" backend/tests/unit/services/sse/test_kafka_redis_bridge.py -A 5Repository: HardMax71/Integr8sCode
Length of output: 495
🏁 Script executed:
# Find and read _register_routing_handlers to see if it registers multiple handlers
rg "_register_routing_handlers" --type py -A 30Repository: HardMax71/Integr8sCode
Length of output: 9579
Fix _StubDispatcher to properly inherit from EventDispatcher.
The stub's handler storage is incompatible with the parent class. EventDispatcher stores handlers as a list per event type and appends during registration, but _StubDispatcher stores a single handler and replaces it. This violates the Liskov Substitution Principle—if multiple handlers are registered for the same event type, the second call will overwrite the first instead of appending.
Update the stub to match the parent's contract:
class _StubDispatcher(EventDispatcher):
"""Stub EventDispatcher for testing."""
def __init__(self) -> None:
self.handlers: dict[EventType, list[Callable[[BaseEvent], Awaitable[None]]]] = {}
def register_handler(self, et: EventType, fn: Callable[[BaseEvent], Awaitable[None]]) -> None:
if et not in self.handlers:
self.handlers[et] = []
self.handlers[et].append(fn)Then update the test to retrieve the handler from the list: h = disp.handlers[EventType.EXECUTION_STARTED][0]
🤖 Prompt for AI Agents
In @backend/tests/unit/services/sse/test_kafka_redis_bridge.py around lines 35 -
43, The _StubDispatcher currently stores a single handler per EventType causing
overwrites; change its handlers type to dict[EventType,
list[Callable[[BaseEvent], Awaitable[None]]]] and implement register_handler on
_StubDispatcher to append the fn to the list (creating the list if missing) to
match EventDispatcher's contract, and update tests that access a handler (e.g.,
retrieving h from disp.handlers[EventType.EXECUTION_STARTED]) to use the first
element of the list (index 0).
| class _DummyEvent(BaseEvent): | ||
| """Dummy event for testing.""" | ||
| execution_id: str | None = None | ||
| topic: ClassVar[KafkaTopic] = KafkaTopic.EXECUTION_EVENTS | ||
|
|
||
| def model_dump(self, **kwargs: object) -> dict[str, str | None]: | ||
| return {"execution_id": self.execution_id} | ||
|
|
There was a problem hiding this comment.
Fix the return type annotation to match BaseEvent.
The model_dump return type dict[str, str | None] is too narrow and likely causes the MyPy failure. The parent class BaseEvent.model_dump typically returns dict[str, Any].
🔧 Proposed fix
- def model_dump(self, **kwargs: object) -> dict[str, str | None]:
+ def model_dump(self, **kwargs: object) -> dict[str, Any]:
return {"execution_id": self.execution_id}Don't forget to add the import at the top:
from collections.abc import Callable
-from typing import Awaitable, ClassVar
+from typing import Any, Awaitable, ClassVar
from unittest.mock import MagicMockCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @backend/tests/unit/services/sse/test_kafka_redis_bridge.py around lines 49 -
56, The model_dump return annotation on _DummyEvent is too narrow; change its
return type from dict[str, str | None] to dict[str, Any] to match
BaseEvent.model_dump, and add the necessary Any import (from typing import Any)
at the top of the test file; keep the method body unchanged.
| mgr = SSEShutdownManager(drain_timeout=0.01, notification_timeout=0.01, force_close_timeout=0.01, logger=_test_logger) | ||
| router = DummyRouter() | ||
| mgr.set_router(router) | ||
| mgr.set_router(cast(SSEKafkaRedisBridge, router)) |
There was a problem hiding this comment.
Avoid using cast() to bypass type safety in tests.
The cast(SSEKafkaRedisBridge, router) suppresses type checking but DummyRouter doesn't actually implement the SSEKafkaRedisBridge interface. This undermines the type safety improvements introduced elsewhere in this PR.
♻️ Better approaches to maintain type safety
Option 1: Define a Protocol (recommended)
from typing import Protocol
class RouterProtocol(Protocol):
async def aclose(self) -> None: ...
class DummyRouter:
def __init__(self) -> None:
self.stopped = False
async def aclose(self) -> None:
self.stopped = TrueThen update SSEShutdownManager.set_router to accept RouterProtocol instead of the concrete class:
# In backend/app/services/sse/sse_shutdown_manager.py
def set_router(self, router: RouterProtocol) -> None:
self._router = routerOption 2: Use pytest-mock or unittest.mock
from unittest.mock import AsyncMock
async def test_shutdown_force_close_calls_router_stop_and_rejects_new() -> None:
mgr = SSEShutdownManager(...)
router = AsyncMock()
router.aclose = AsyncMock()
mgr.set_router(router)
# ... rest of test
router.aclose.assert_called_once()🤖 Prompt for AI Agents
In @backend/tests/unit/services/sse/test_shutdown_manager.py at line 49, The
test uses cast(SSEKafkaRedisBridge, router) to bypass type checks even though
DummyRouter doesn't implement that interface; update the test to preserve type
safety by either (a) introducing a small Protocol with the required async aclose
method (e.g., RouterProtocol) and make DummyRouter implement it and change
SSEShutdownManager.set_router to accept RouterProtocol, or (b) replace
DummyRouter with a proper AsyncMock that defines aclose and pass that to
SSEShutdownManager.set_router (no cast). Ensure references to set_router,
SSEShutdownManager, DummyRouter, and SSEKafkaRedisBridge are updated
accordingly.
| def _make_fake_settings() -> Settings: | ||
| mock = MagicMock(spec=Settings) | ||
| mock.SSE_HEARTBEAT_INTERVAL = 0 | ||
| return mock | ||
|
|
There was a problem hiding this comment.
Make _make_fake_settings() type-check cleanly (avoid MyPy “incompatible return value”).
Returning a MagicMock from a function annotated -> Settings can trip MyPy depending on settings. Casting keeps the intent explicit.
Proposed fix
-from unittest.mock import MagicMock
+from typing import cast
+from unittest.mock import MagicMock
@@
def _make_fake_settings() -> Settings:
mock = MagicMock(spec=Settings)
mock.SSE_HEARTBEAT_INTERVAL = 0
- return mock
+ return cast(Settings, mock)🤖 Prompt for AI Agents
In @backend/tests/unit/services/sse/test_sse_service.py around lines 122 - 126,
The function _make_fake_settings currently returns a MagicMock typed as Settings
which can trigger MyPy "incompatible return value"; fix it by importing and
using typing.cast to cast the MagicMock to Settings (e.g., return cast(Settings,
mock)) so the return type matches the annotation, ensuring you reference the
MagicMock created in _make_fake_settings and the Settings type.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/tests/unit/services/pod_monitor/test_monitor.py (1)
176-208: Potential flake:reconcile_interval_seconds = 1withwait_for(..., timeout=1.0)is too tight.
Depending on how_reconciliation_loop()schedules its first iteration (and CI load), this can intermittently time out.Proposed fix (reduce interval + increase timeout a bit)
@@ - cfg.reconcile_interval_seconds = 1 # Use 1 second for testing + cfg.reconcile_interval_seconds = 0.01 # Keep loop responsive in CI @@ - await asyncio.wait_for(evt.wait(), timeout=1.0) + await asyncio.wait_for(evt.wait(), timeout=2.0) @@ - cfg.reconcile_interval_seconds = 1 # Use 1 second for testing + cfg.reconcile_interval_seconds = 0.01 # Keep loop responsive in CI @@ - await asyncio.wait_for(hit.wait(), timeout=1.0) + await asyncio.wait_for(hit.wait(), timeout=2.0)Also applies to: 698-722
backend/tests/integration/test_notifications_routes.py (1)
33-33: Fix type annotation:test_userandtest_adminshould beUserCredentials, notDict[str, str].The fixture definitions in
backend/tests/conftest.pyreturnUserCredentials(a TypedDict with fields: username, email, password, role, csrf_token, headers), but the test method signatures declaretest_user: Dict[str, str]andtest_admin: Dict[str, str]. This creates a type inconsistency that misleads readers and type checkers.📝 Proposed fix: Update type annotations
Update the imports at the top of the file:
+from tests.conftest import UserCredentialsThen update all affected test signatures, for example:
-async def test_list_user_notifications(self, client: AsyncClient, test_user: Dict[str, str]) -> None: +async def test_list_user_notifications(self, client: AsyncClient, test_user: UserCredentials) -> None:Apply similar changes to all other test methods that accept
test_userortest_adminparameters.Also applies to: 60-60, 75-75, 91-91, 118-119, 134-134, 152-152, 185-185, 222-222, 246-246, 273-273, 302-302, 314-314, 342-344, 381-381
🤖 Fix all issues with AI agents
In @backend/tests/integration/events/test_producer_roundtrip.py:
- Around line 41-53: The test creates a UnifiedProducer instance bypassing
__init__ (UP via object.__new__) but doesn't set p.logger, so UP._handle_stats
may raise AttributeError when the exception handler runs; set p.logger =
logging.getLogger(__name__) in the test setup before calling UP._handle_stats to
ensure the logger exists (also import logging at top of test if missing).
In @backend/tests/unit/services/pod_monitor/test_monitor.py:
- Around line 322-344: Remove the now-unnecessary "# type: ignore[arg-type]"
from the test invocation(s) so mypy won't fail; specifically, delete the `#
type: ignore[arg-type]` after the make_pod(...) call (and also remove the one
after make_pod_monitor(...) if it exists/is unused) in
test_process_pod_event_exception_handling so the test no longer contains an
unused ignore.
In @backend/tests/unit/services/sse/test_sse_service.py:
- Around line 118-119: Remove the unnecessary MyPy suppression on the test
helper: in the get_stats method definition inside test_sse_service.py (the def
get_stats(self) -> dict[str, int | bool] declaration) delete the trailing " #
type: ignore[override]" comment since the signature is compatible with the
parent and the ignore is not needed; update only that line to remove the comment
and keep the return dict unchanged.
🧹 Nitpick comments (5)
backend/tests/helpers/sse.py (1)
8-8: Type annotations improved!The updated type hints make the return types more specific and improve type safety. The change from generic
dicttodict[str, object]is correct for SSE event data.💡 Optional: Consider using
Anyinstead ofobjectfor JSON dataWhile
dict[str, object]is correct,dict[str, Any]is more idiomatic in Python typing for JSON data structures. This would require importingAnyfromtyping.-from typing import AsyncIterator, Iterable +from typing import Any, AsyncIterator, IterableThen update the return types:
-async def stream_sse(client: AsyncClient, url: str, timeout: float = 20.0) -> AsyncIterator[dict[str, object]]: +async def stream_sse(client: AsyncClient, url: str, timeout: float = 20.0) -> AsyncIterator[dict[str, Any]]:-) -> dict[str, object]: +) -> dict[str, Any]:Apply similar changes to lines 48 and 58.
Also applies to: 34-34, 48-48, 58-58
backend/tests/unit/services/pod_monitor/test_monitor.py (2)
30-98: Nice consolidation of test doubles/factories (FakeKafkaEventService,make_*helpers).
This should reduce mock sprawl and make future DI refactors cheaper.Optional hardening
- Consider storing
published_eventsaslist[tuple[Any, str | None]](or a small Protocol) to avoid inheriting fromKafkaEventServicewhile skippingsuper().__init__()—less coupling to internal fields.
550-594: Idempotency + task cleanup tests are useful guardrails.
Optional: consider asserting the tasks are actually cancelled (ifaclose()guarantees it) to prevent “pending task” leaks from being silently ignored.backend/tests/conftest.py (1)
114-119: Consider failing fast if CSRF token is missing.The function returns an empty string when
csrf_tokenis not present in the response (line 119). If CSRF tokens are required for authenticated requests, returning an empty string could lead to confusing test failures downstream rather than immediately flagging the authentication issue.💡 Proposed alternative: Raise error if CSRF token missing
async def _http_login(client: httpx.AsyncClient, username: str, password: str) -> str: data = {"username": username, "password": password} resp = await client.post("/api/v1/auth/login", data=data) resp.raise_for_status() json_data: dict[str, str] = resp.json() - return json_data.get("csrf_token", "") + csrf_token = json_data.get("csrf_token") + if not csrf_token: + raise ValueError("Login response missing csrf_token") + return csrf_tokenbackend/tests/unit/services/sse/test_sse_service.py (1)
32-39: Consider extracting the timeout constant.The hardcoded 0.5-second timeout in
asyncio.wait_foris a magic number that appears in test logic. Extract it to a module-level constant for easier maintenance and clearer intent.♻️ Proposed refactor
+_FAKE_SUBSCRIPTION_TIMEOUT = 0.5 + class _FakeSubscription(SSERedisSubscription): def __init__(self) -> None: # Skip parent __init__ - no real Redis pubsub self._q: asyncio.Queue[dict[str, Any] | None] = asyncio.Queue() self.closed = False async def get[T: BaseModel](self, model: type[T]) -> T | None: try: - raw = await asyncio.wait_for(self._q.get(), timeout=0.5) + raw = await asyncio.wait_for(self._q.get(), timeout=_FAKE_SUBSCRIPTION_TIMEOUT) if raw is None: return None return model.model_validate(raw)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/app/settings.pybackend/tests/conftest.pybackend/tests/helpers/__init__.pybackend/tests/helpers/sse.pybackend/tests/integration/app/test_main_app.pybackend/tests/integration/events/test_producer_roundtrip.pybackend/tests/integration/test_dlq_routes.pybackend/tests/integration/test_notifications_routes.pybackend/tests/load/cli.pybackend/tests/load/http_client.pybackend/tests/load/user_runner.pybackend/tests/unit/events/test_event_dispatcher.pybackend/tests/unit/services/pod_monitor/test_monitor.pybackend/tests/unit/services/sse/test_sse_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/settings.py
- backend/tests/load/http_client.py
- backend/tests/unit/events/test_event_dispatcher.py
🧰 Additional context used
🧬 Code graph analysis (5)
backend/tests/integration/test_notifications_routes.py (1)
backend/tests/conftest.py (1)
app(56-67)
backend/tests/integration/app/test_main_app.py (1)
backend/tests/conftest.py (1)
app(56-67)
backend/tests/integration/test_dlq_routes.py (5)
backend/tests/conftest.py (3)
app(56-67)client(78-86)test_settings(35-51)backend/app/dlq/models.py (1)
DLQMessageStatus(10-16)backend/app/schemas_pydantic/dlq.py (5)
DLQBatchRetryResponse(70-78)DLQMessageDetail(95-117)DLQMessageResponse(22-39)DLQMessagesResponse(59-67)DLQTopicSummaryResponse(81-92)backend/app/settings.py (1)
Settings(11-165)backend/tests/load/http_client.py (1)
result(98-99)
backend/tests/helpers/sse.py (1)
backend/tests/conftest.py (1)
client(78-86)
backend/tests/load/user_runner.py (1)
backend/tests/load/http_client.py (1)
APIClient(29-180)
🪛 GitHub Actions: MyPy Type Checking
backend/tests/unit/services/pod_monitor/test_monitor.py
[error] 337-337: Unused "type: ignore" comment [unused-ignore] while running 'uv run mypy --config-file pyproject.toml --strict .'.
backend/tests/unit/services/sse/test_sse_service.py
[error] 118-118: Unused "type: ignore" comment [unused-ignore] while running 'uv run mypy --config-file pyproject.toml --strict .'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Backend
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (35)
backend/tests/helpers/__init__.py (1)
3-5: Good: explicit re-export via__all__keeps the test-helper public API stable.No issues with the change. Only thing to watch long-term: importing from
.eventsat package import time can introduce side effects if that module starts doing heavier setup—keep.eventsimport-light.backend/tests/load/cli.py (1)
78-78: LGTM! Type suppression removal improves type safety.The removal of the inline type suppression is appropriate now that
LoadConfiguses Pydantic BaseSettings with proper field typing.backend/tests/integration/test_dlq_routes.py (4)
1-22: LGTM! Good type safety improvements.The addition of
TypedDictfor_RetryRequestand expanded DLQ schema imports improve type safety and align with the PR's Settings-driven configuration goals. All imports are properly utilized throughout the tests.
41-41: LGTM! Consistent type annotations for authentication fixture.The
test_user: dict[str, str]type annotation is consistently applied across all test methods, improving type safety and aligning with the fixture-based authentication approach described in the PR objectives.Also applies to: 86-86, 118-118, 133-133, 148-148, 188-188, 225-225, 262-262, 293-293, 331-331, 361-361
377-379: LGTM! Proper use of TypedDict for request payload.The
_RetryRequestTypedDict annotation provides clear documentation of the expected payload structure and enables type checking for the retry request.
200-223: LGTM! Settings-driven topic prefix correctly tests isolation.The integration of
test_settings.KAFKA_TOPIC_PREFIXfor topic construction properly tests the DLQ retry policy with isolated topic prefixes. The endpoint/api/v1/dlq/retry-policyreturns aMessageResponsewith the message format"Retry policy set for topic {topic}", making the test assertions on message content and included topic name valid.backend/tests/unit/services/pod_monitor/test_monitor.py (18)
4-22: Imports/DI test wiring looks coherent and keeps the tests type-oriented.
The new imports and explicit dependency types (K8sClients,KafkaEventService,PodMonitor, etc.) make the DI-centric tests easier to read/maintain.
103-123: Lifecycle test is deterministic by bypassing the real watch loop.
TheSpyMapper.clear_cache()assertion is a good regression check for cleanup on stop.
125-138: Watch flow test validates RV tracking without a real stream.
Settingpm._state = RUNNINGexplicitly makes the intent clear.
140-152: Good “invalid raw event + reconnect attempts” coverage.
Settingwatch_reconnect_delay = 0keeps this test fast/stable.
161-174: Status shape assertions cover the config echo + counters.
This is a solid contract-style test forget_status().
210-237: Reconciliation success case asserts missing/extra pod handling and pruning.
The assertions onprocessedand_tracked_podsare the right behavioral checks.
239-262: Exception path test is clear and checks error propagation intoReconciliationResult.
264-320: Full_process_pod_eventcoverage is strong (add/delete/ignored phase).
This hits tracking + RV updates + publish gating in one place without being too brittle.
345-365: Publish happy-path verifies partition key selection (execution-id).
Assertingpublished_events[0][1] == "exec1"is the key behavior here—nice.
367-398: Publish exception handling test is good “should not raise” coverage.
399-411: Max reconnect attempts transitions to STOPPING as expected.
413-434: Main loop test is deterministic and avoids real sleeps.
436-460: APIException vs generic exception branches are covered cleanly.
Resetting_last_resource_versionon 410 is a good behavioral assertion.Also applies to: 640-662, 462-482
484-520:create_pod_monitorcontext-manager test correctly monkeypatches K8s client construction.
Patch coverage here is valuable given the factory/DI shift.
522-548: Injectedk8s_clientsDI path is validated (_clients,_v1identity checks).
612-638: Raw event metadata parsing test is a good contract check for RV extraction.
664-696: Field selector propagation is covered (checks kwargs passed into the watch layer).
724-743: Start-with-reconciliation wires both tasks and asserts they’re created.backend/tests/load/user_runner.py (1)
5-5: LGTM! Type annotations improved for clarity.The updated type annotations correctly reflect that the task functions are async callables returning
Awaitable[None]rather thanasyncio.Future, and the task list is properly typed aslist[asyncio.Task[None]]. These changes improve type safety and align with the actual function signatures.Also applies to: 18-18, 89-89
backend/tests/integration/events/test_producer_roundtrip.py (1)
19-28: LGTM! Settings-based DI integration is correct.The test now properly uses dependency-injected
SettingsandAsyncContainer, with bootstrap servers sourced fromtest_settings.KAFKA_BOOTSTRAP_SERVERS. This aligns with the PR's goal of centralizing Settings-driven configuration.backend/tests/integration/app/test_main_app.py (1)
19-19: LGTM! Type hints and middleware introspection improved.The added type annotations and the shift to name-based middleware comparison improve robustness. Filtering routes to
Routeinstances avoids potential issues with other routing objects.Also applies to: 23-23, 27-35, 38-38
backend/tests/integration/test_notifications_routes.py (1)
342-378: LGTM! Manual login in isolation test is appropriate.The
test_notifications_isolation_between_userstest correctly performs manual login for both users to verify notification isolation. This is the right approach since the test needs to actively switch between authenticated sessions.backend/tests/conftest.py (3)
19-28: LGTM! Well-structured UserCredentials type.The
UserCredentialsTypedDict clearly defines the structure of test user credentials with all necessary fields (username, email, password, role, csrf_token, headers). This improves type safety across test fixtures.
34-51: LGTM! Session-scoped settings with proper test isolation.The session-scoped
test_settingsfixture provides good test isolation through unique prefixes for Kafka topics, consumer groups, schema subjects, and per-worker database/Redis separation. Loading from.env.testestablishes a consistent baseline configuration.
123-146: Don't silently accept status 400 during registration in fixtures.The registration endpoint returns 400 only for duplicate usernames (not validation failures, which return 422). Accepting 400 without verifying the user was created will cause the fixture to fail later at login with an unclear error. Either ensure unique credentials to prevent 400 entirely, or explicitly handle the duplicate case rather than continuing to
_http_login().The same applies to the
test_adminandanother_userfixtures.Likely an incorrect or invalid review comment.
backend/tests/unit/services/sse/test_sse_service.py (4)
128-131: LGTM!The
_decodehelper cleanly extracts and parses JSON payloads from SSE events, simplifying test assertions.
134-189: LGTM!Test updates properly integrate the new Settings parameter and use the helper functions effectively while maintaining test clarity.
191-229: LGTM!The test correctly validates the notification stream lifecycle, including heartbeat emission, message delivery, and graceful shutdown. The use of
Noneto signal stream termination aligns with the updatedpushsignature.
231-236: LGTM!The health status test is correctly updated to use the new Settings-based initialization.
| def test_producer_handle_stats_path() -> None: | ||
| # Directly run stats parsing to cover branch logic; avoid relying on timing | ||
| from app.events.core.producer import ProducerMetrics | ||
| from app.events.core.producer import UnifiedProducer as UP | ||
| from app.events.core import ProducerMetrics | ||
| from app.events.core import UnifiedProducer as UP | ||
|
|
||
| m = ProducerMetrics() | ||
| p = object.__new__(UP) # bypass __init__ safely for method call | ||
| # Inject required attributes | ||
| p._metrics = m # type: ignore[attr-defined] | ||
| p._stats_callback = None # type: ignore[attr-defined] | ||
| p._metrics = m | ||
| p._stats_callback = None | ||
| payload = json.dumps({"msg_cnt": 1, "topics": {"t": {"partitions": {"0": {"msgq_cnt": 2, "rtt": {"avg": 5}}}}}}) | ||
| UP._handle_stats(p, payload) # type: ignore[misc] | ||
| UP._handle_stats(p, payload) | ||
| assert m.queue_size == 1 and m.avg_latency_ms > 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the UnifiedProducer class and its _handle_stats method
find . -type f -name "*.py" | xargs grep -l "class UnifiedProducer"Repository: HardMax71/Integr8sCode
Length of output: 103
🏁 Script executed:
# Read the producer.py file to find _handle_stats method and examine its implementation
cat -n ./backend/app/events/core/producer.py | head -200Repository: HardMax71/Integr8sCode
Length of output: 9140
Initialize self.logger in test setup.
The _handle_stats method accesses self.logger at line 109 in the exception handler. If any exception occurs during stats parsing, the test will fail with AttributeError. While the current test payload parses successfully and avoids this path, the incomplete initialization leaves the test fragile to edge cases or invalid input.
Add p.logger = logging.getLogger(__name__) to the test setup.
🤖 Prompt for AI Agents
In @backend/tests/integration/events/test_producer_roundtrip.py around lines 41
- 53, The test creates a UnifiedProducer instance bypassing __init__ (UP via
object.__new__) but doesn't set p.logger, so UP._handle_stats may raise
AttributeError when the exception handler runs; set p.logger =
logging.getLogger(__name__) in the test setup before calling UP._handle_stats to
ensure the logger exists (also import logging at top of test if missing).
There was a problem hiding this comment.
3 issues found across 18 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/tests/conftest.py">
<violation number="1" location="backend/tests/conftest.py:113">
P1: This Redis client is managed by Dishka's DI container, which already closes it when the scope exits (see `RedisProvider.get_redis_client` in providers.py). Calling `aclose()` here causes a double-close issue. The `db` fixture follows the correct pattern by not closing the container-managed dependency.</violation>
</file>
<file name="backend/app/core/middlewares/csrf.py">
<violation number="1" location="backend/app/core/middlewares/csrf.py:37">
P1: Security middleware should fail-closed, not fail-open. If the SecurityService cannot be obtained, the request is allowed through without CSRF validation, leaving authenticated users vulnerable to CSRF attacks. Consider returning a 500 error instead of allowing the request to proceed.</violation>
<violation number="2" location="backend/app/core/middlewares/csrf.py:40">
P2: Accessing `asgi_app.state.dishka_container` may raise an AttributeError if the container was not initialized. Use `getattr` with a default value for safer access.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| try: | ||
| yield client | ||
| finally: | ||
| await client.aclose() |
There was a problem hiding this comment.
P1: This Redis client is managed by Dishka's DI container, which already closes it when the scope exits (see RedisProvider.get_redis_client in providers.py). Calling aclose() here causes a double-close issue. The db fixture follows the correct pattern by not closing the container-managed dependency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/conftest.py, line 113:
<comment>This Redis client is managed by Dishka's DI container, which already closes it when the scope exits (see `RedisProvider.get_redis_client` in providers.py). Calling `aclose()` here causes a double-close issue. The `db` fixture follows the correct pattern by not closing the container-managed dependency.</comment>
<file context>
@@ -107,7 +107,10 @@ async def db(scope: AsyncContainer) -> AsyncGenerator[Database, None]:
+ try:
+ yield client
+ finally:
+ await client.aclose()
</file context>
| await client.aclose() | |
| pass # Redis client lifecycle managed by DI container |
| if self.security_service is None: | ||
| asgi_app = scope.get("app") | ||
| if asgi_app: | ||
| container = asgi_app.state.dishka_container | ||
| async with container() as container_scope: |
There was a problem hiding this comment.
P1: Security middleware should fail-closed, not fail-open. If the SecurityService cannot be obtained, the request is allowed through without CSRF validation, leaving authenticated users vulnerable to CSRF attacks. Consider returning a 500 error instead of allowing the request to proceed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/app/core/middlewares/csrf.py, line 37:
<comment>Security middleware should fail-closed, not fail-open. If the SecurityService cannot be obtained, the request is allowed through without CSRF validation, leaving authenticated users vulnerable to CSRF attacks. Consider returning a 500 error instead of allowing the request to proceed.</comment>
<file context>
@@ -0,0 +1,64 @@
+ return
+
+ # Try to get service from Dishka container if not initialized
+ if self.security_service is None:
+ asgi_app = scope.get("app")
+ if asgi_app:
</file context>
| if self.security_service is None: | |
| asgi_app = scope.get("app") | |
| if asgi_app: | |
| container = asgi_app.state.dishka_container | |
| async with container() as container_scope: | |
| if self.security_service is None: | |
| # Fail closed: if we can't get the security service, reject the request | |
| response = JSONResponse( | |
| status_code=500, | |
| content={"detail": "Security service unavailable"}, | |
| ) | |
| await response(scope, receive, send) | |
| return |
| if self.security_service is None: | ||
| asgi_app = scope.get("app") | ||
| if asgi_app: | ||
| container = asgi_app.state.dishka_container |
There was a problem hiding this comment.
P2: Accessing asgi_app.state.dishka_container may raise an AttributeError if the container was not initialized. Use getattr with a default value for safer access.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/app/core/middlewares/csrf.py, line 40:
<comment>Accessing `asgi_app.state.dishka_container` may raise an AttributeError if the container was not initialized. Use `getattr` with a default value for safer access.</comment>
<file context>
@@ -0,0 +1,64 @@
+ if self.security_service is None:
+ asgi_app = scope.get("app")
+ if asgi_app:
+ container = asgi_app.state.dishka_container
+ async with container() as container_scope:
+ self.security_service = await container_scope.get(SecurityService)
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @backend/app/core/middlewares/csrf.py:
- Around line 36-48: The middleware must not lazily mutate self.security_service
during requests; change the constructor of the CSRF middleware to require a
ready SecurityService instance (or a synchronous provider) and store it in
self.security_service at initialization time, remove the async container lookup
block (the async with container() and get(SecurityService) code) from the
__call__/dispatch path, and instead raise an explicit error (e.g., RuntimeError)
if __init__ is given no SecurityService; update the app factory to resolve and
pass the SecurityService into the middleware at startup so no async
initialization or silent fallback occurs.
In @backend/tests/unit/services/pod_monitor/test_monitor.py:
- Around line 36-41: FakeKafkaEventService.__init__ currently sets underscored
attributes (_event_repository, _kafka_producer, _settings, _logger) and omits
self.metrics, breaking LSP/type compatibility and risking AttributeError; update
the constructor to initialize the public names used by KafkaEventService
(event_repository, kafka_producer, settings, logger) and also set self.metrics
(e.g., to a MagicMock) and self.published_events so the fake matches the real
service, or alternatively invoke KafkaEventService.__init__ with mock arguments
to ensure all expected attributes are initialized.
🧹 Nitpick comments (15)
backend/tests/unit/services/sse/test_sse_service.py (3)
32-39: Consider making the timeout configurable to prevent CI flakiness.The hardcoded 0.5-second timeout could cause intermittent test failures in slower CI environments or under heavy load. Consider making this configurable via a class parameter or constant, or at minimum document why 0.5s was chosen.
♻️ Proposed refactor to make timeout configurable
class _FakeSubscription(SSERedisSubscription): - def __init__(self) -> None: + def __init__(self, timeout: float = 0.5) -> None: # Skip parent __init__ - no real Redis pubsub self._q: asyncio.Queue[dict[str, Any] | None] = asyncio.Queue() self.closed = False + self._timeout = timeout async def get[T: BaseModel](self, model: type[T]) -> T | None: try: - raw = await asyncio.wait_for(self._q.get(), timeout=0.5) + raw = await asyncio.wait_for(self._q.get(), timeout=self._timeout) if raw is None: return None return model.model_validate(raw)
63-79: Consider aligning the return type with the parent interface.The
type: ignore[override]on line 74 indicates that_FakeRepo._Statusdoesn't match the expected return type fromSSERepository.get_execution_status(). While acceptable for testing, this weakens type safety. Consider either:
- Returning the actual expected type from the parent interface
- Using a Protocol if the test only needs specific attributes
196-198: Remove redundant assignment (nitpick).
SSE_HEARTBEAT_INTERVALis already set to0in_make_fake_settings()(line 124), so the reassignment on line 197 is redundant.♻️ Proposed cleanup
- settings = _make_fake_settings() - settings.SSE_HEARTBEAT_INTERVAL = 0 # emit immediately - svc = SSEService(repository=repo, router=_FakeRouter(), sse_bus=bus, shutdown_manager=sm, settings=settings, logger=_test_logger) + svc = SSEService(repository=repo, router=_FakeRouter(), sse_bus=bus, shutdown_manager=sm, settings=_make_fake_settings(), logger=_test_logger)Or, if you want to emphasize the zero interval for readability, add a comment to
_make_fake_settings()instead.backend/tests/unit/core/test_adaptive_sampling.py (1)
71-76: Remove redundant environment variable setup.Line 74 sets the
TRACING_SAMPLING_RATEenvironment variable, but this is no longer needed since the test now passes a mock Settings object directly tocreate_adaptive_sampler. The environment variable was only necessary when settings were retrieved globally.♻️ Proposed simplification
mock_settings = MagicMock(spec=Settings) mock_settings.TRACING_SAMPLING_RATE = 0.2 - monkeypatch.setenv("TRACING_SAMPLING_RATE", "0.2") # create_adaptive_sampler pulls settings via get_settings; just ensure it constructs sampler = create_adaptive_sampler(mock_settings)backend/tests/helpers/auth.py (2)
36-36: Type annotation is overly restrictive.The type
dict[str, str]assumes all values in the JSON response are strings. In practice,response.json()returnsAnyand could contain nested objects, arrays, or other types. Use a more flexible type annotation.♻️ Proposed fix
- json_data: dict[str, str] = response.json() + json_data: dict[str, Any] = response.json()Don't forget to import
Anyfromtyping:-from typing import TypedDict +from typing import Any, TypedDict
37-42: Validate that CSRF token is present and non-empty.The function uses
.get("csrf_token", "")which silently returns an empty string if the token is missing from the response. Tests consuming this helper would then proceed with an empty CSRF token, potentially masking authentication issues.♻️ Proposed fix to fail fast when token is missing
json_data: dict[str, str] = response.json() - csrf_token = json_data.get("csrf_token", "") + csrf_token = json_data.get("csrf_token", "") + assert csrf_token, "CSRF token missing from login response" return AuthResult( csrf_token=csrf_token, headers={"X-CSRF-Token": csrf_token}, )backend/app/core/security.py (1)
84-121: Consider using a more explicit return type for skip scenarios.The method returns
"skip"as a magic string for cases where validation is skipped. While functional, this could be clearer with anOptional[str]return type (returningNonefor skip) or a dedicated result type.Current consumers must check
if result == "skip"which is error-prone.♻️ Optional: Use Optional[str] for clearer semantics
- def validate_csrf_from_request(self, request: Request) -> str: + def validate_csrf_from_request(self, request: Request) -> str | None: """Validate CSRF token from HTTP request using double-submit cookie pattern. Returns: - "skip" if validation was skipped (safe method, exempt path, or unauthenticated) + None if validation was skipped (safe method, exempt path, or unauthenticated) The validated token string if validation passed Raises: CSRFValidationError: If token is missing or invalid """ # Skip CSRF validation for safe methods if request.method in ("GET", "HEAD", "OPTIONS"): - return "skip" + return None # Skip CSRF validation for auth endpoints if request.url.path in self.CSRF_EXEMPT_PATHS: - return "skip" + return None # Skip CSRF validation for non-API endpoints (health, metrics, etc.) if not request.url.path.startswith("/api/"): - return "skip" + return None # Check if user is authenticated first (has access_token cookie) access_token = request.cookies.get("access_token") if not access_token: # If not authenticated, skip CSRF validation (auth will be handled by other dependencies) - return "skip" + return Nonebackend/tests/integration/test_dlq_routes.py (1)
42-42: Inconsistent type annotation - should useUserCredentialsfor consistency.This test uses
dict[str, str]while other tests in this file (lines 201, 226, 263, 363) useUserCredentials. For consistency and type safety across the test suite, all tests should useUserCredentials.♻️ Use UserCredentials type consistently
@pytest.mark.asyncio - async def test_get_dlq_statistics(self, client: AsyncClient, test_user: dict[str, str]) -> None: + async def test_get_dlq_statistics(self, client: AsyncClient, test_user: UserCredentials) -> None:Apply similar changes to:
- Line 87:
test_list_dlq_messages- Line 119:
test_filter_dlq_messages_by_status- Line 134:
test_filter_dlq_messages_by_topic- Line 149:
test_get_single_dlq_message_detail- Line 189:
test_get_nonexistent_dlq_message- Line 295:
test_get_dlq_topics_summary- Line 333:
test_dlq_message_paginationbackend/tests/integration/test_user_settings_routes.py (1)
60-60: Inconsistent type annotation - should useUserCredentials.This test uses
dict[str, str]while other tests in this class useUserCredentials. For consistency, this should also useUserCredentials.♻️ Use UserCredentials type
@pytest.mark.asyncio - async def test_get_user_settings(self, client: AsyncClient, test_user: dict[str, str]) -> None: + async def test_get_user_settings(self, client: AsyncClient, test_user: UserCredentials) -> None:backend/tests/unit/services/saga/test_saga_orchestrator_unit.py (4)
31-44: Consider the implications of the metadata default factory.The
default_factoryformetadatacreates a newAvroEventMetadatainstance for each_FakeEvent, which is correct for avoiding shared mutable state. However, all instances will have identical values (service_name="test", etc.). If tests need to verify metadata propagation or uniqueness, consider whether the helper function_make_event()should allow customizing metadata.
82-93: Minimal test doubles may cause fragile tests.Both
_FakeStoreand_FakeAllocimplement no methods beyond__init__. This means:
- If
SagaOrchestratorcalls any methods on these dependencies, tests will raiseAttributeError.- This is acceptable if these dependencies truly aren't used during these specific unit tests.
- However, it makes tests fragile: if the orchestrator implementation changes to use these dependencies, tests will break in non-obvious ways.
Consider whether these fakes should implement stub methods (returning
Noneor empty collections) to be more resilient, or useMagicMocklike the other dependencies (SchemaRegistryManager,Settings).♻️ Alternative: use MagicMock for unused dependencies
def _orch() -> SagaOrchestrator: return SagaOrchestrator( config=SagaConfig(name="t", enable_compensation=True, store_events=True, publish_commands=False), saga_repository=_FakeRepo(), producer=_FakeProd(), schema_registry_manager=MagicMock(spec=SchemaRegistryManager), settings=MagicMock(spec=Settings), - event_store=_FakeStore(), + event_store=MagicMock(spec=EventStore), - idempotency_manager=_FakeIdem(), + idempotency_manager=MagicMock(spec=IdempotencyManager), - resource_allocation_repository=_FakeAlloc(), + resource_allocation_repository=MagicMock(spec=ResourceAllocationRepository), logger=_test_logger, )
138-146: Consider testing through public API instead of internal state.This test directly manipulates
_lifecycle_started(a private property) and calls_handle_event()(a private method). While unit tests sometimes need to test internal logic, this approach makes the test fragile to refactoring:
- If the lifecycle management changes (e.g., renamed property, different state model), the test breaks even if public behavior is correct.
- The test is coupled to implementation details rather than the orchestrator's contract.
Since the comment notes "deep behavior covered by integration", consider whether this unit test should also operate through the public API (e.g., using
start()andstop()methods if available), or if testing these internals is essential for isolating complex logic.
149-169: Internal method testing has more justification here, but still consider public API.This test verifies the short-circuit behavior when a saga already exists, which is valuable edge-case testing. However, it still accesses internal methods (
_should_trigger_saga,_start_saga), making it fragile to refactoring.The test logic is sound: it verifies that when a saga with the same
execution_idandsaga_namealready exists,_start_sagareturns the existing ID rather than creating a duplicate. If this behavior is critical and hard to trigger through the public API, testing it directly is reasonable. Otherwise, consider whether integration tests can cover this scenario through the public interface.backend/tests/unit/services/pod_monitor/test_monitor.py (1)
373-381: Consider extracting inline test doubles to module level.
FailingKafkaEventService(lines 373-381), similar inline doubles likeFailMapper(lines 326-332), andMockMapper(lines 269-280) are defined inside test functions. While this works, extracting commonly useful test doubles to module level (nearFakeKafkaEventServiceandSpyMapper) would improve reusability.backend/app/services/pod_monitor/monitor.py (1)
213-213: Clarify initialization guarantee comment.The comment states "guaranteed initialized by start()" but the clients are actually guaranteed by
__init__(as correctly noted in lines 238 and 386). Consider updating for consistency.📝 Suggested clarification
- # self._v1 and self._watch are guaranteed initialized by start() + # self._v1 and self._watch are guaranteed initialized by __init__
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
backend/.envbackend/app/core/middlewares/__init__.pybackend/app/core/middlewares/csrf.pybackend/app/core/security.pybackend/app/main.pybackend/app/services/pod_monitor/monitor.pybackend/tests/conftest.pybackend/tests/helpers/__init__.pybackend/tests/helpers/auth.pybackend/tests/integration/test_dlq_routes.pybackend/tests/integration/test_notifications_routes.pybackend/tests/integration/test_saved_scripts_routes.pybackend/tests/integration/test_user_settings_routes.pybackend/tests/unit/core/test_adaptive_sampling.pybackend/tests/unit/services/pod_monitor/test_monitor.pybackend/tests/unit/services/saga/test_saga_orchestrator_unit.pybackend/tests/unit/services/sse/test_kafka_redis_bridge.pybackend/tests/unit/services/sse/test_sse_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/.env
- backend/tests/unit/services/sse/test_kafka_redis_bridge.py
🧰 Additional context used
🧬 Code graph analysis (9)
backend/tests/integration/test_saved_scripts_routes.py (2)
backend/tests/conftest.py (5)
app(56-67)UserCredentials(19-27)client(78-86)test_user(126-149)test_admin(153-176)backend/tests/helpers/auth.py (1)
login_user(13-42)
backend/tests/helpers/auth.py (1)
backend/tests/conftest.py (1)
client(78-86)
backend/app/main.py (2)
backend/app/core/metrics/context.py (1)
initialize_all(123-163)backend/app/core/middlewares/metrics.py (2)
setup_metrics(121-161)MetricsMiddleware(19-118)
backend/tests/integration/test_dlq_routes.py (2)
backend/tests/conftest.py (3)
app(56-67)UserCredentials(19-27)client(78-86)backend/app/dlq/models.py (1)
DLQMessageStatus(10-16)
backend/tests/conftest.py (2)
backend/app/main.py (1)
create_app(60-162)backend/app/settings.py (1)
Settings(11-165)
backend/tests/unit/core/test_adaptive_sampling.py (2)
backend/app/core/adaptive_sampling.py (2)
AdaptiveSampler(15-239)create_adaptive_sampler(242-251)backend/app/settings.py (1)
Settings(11-165)
backend/tests/integration/test_user_settings_routes.py (2)
backend/tests/conftest.py (5)
app(56-67)UserCredentials(19-27)client(78-86)test_user(126-149)another_user(180-203)backend/tests/helpers/auth.py (1)
login_user(13-42)
backend/tests/helpers/__init__.py (2)
backend/tests/helpers/auth.py (2)
AuthResult(6-10)login_user(13-42)backend/tests/helpers/events.py (1)
make_execution_requested_event(8-50)
backend/tests/integration/test_notifications_routes.py (2)
backend/tests/conftest.py (5)
app(56-67)UserCredentials(19-27)client(78-86)test_user(126-149)test_admin(153-176)backend/tests/helpers/auth.py (1)
login_user(13-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Frontend
- GitHub Check: cubic · AI code reviewer
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (48)
backend/tests/unit/services/sse/test_sse_service.py (5)
1-24: LGTM!The imports and test setup are well-structured. Adding the
pytestmark = pytest.mark.unitmarker is a good practice for organizing test categories.
50-61: LGTM!The fake bus implementation properly inherits from the interface and returns correctly typed subscriptions.
81-120: LGTM!Both fake classes properly implement their interfaces with appropriate test instrumentation. The shutdown manager's tracking of registered/unregistered connections is useful for test verification.
122-131: LGTM!Using
MagicMock(spec=Settings)is good practice as it provides attribute-level type checking. The_decodehelper is clean and properly typed.
134-236: Test logic is sound and comprehensive.The tests effectively verify:
- Stream termination on failure events
- Result payload inclusion
- Notification delivery and heartbeats
- Health status structure
The use of DI-friendly fakes and explicit typing improves test maintainability.
backend/app/core/middlewares/__init__.py (1)
2-2: LGTM!The CSRFMiddleware export follows the existing pattern for other middleware in this module.
Also applies to: 9-9
backend/tests/helpers/__init__.py (1)
3-3: LGTM!The auth helper exports are properly added to the public API.
Also applies to: 6-6
backend/app/core/middlewares/csrf.py (1)
56-56: Remove misleading comment or clarify that return value is intentionally unused.The method does return a meaningful value (
"skip"or the validated token string), but the return value is discarded in the middleware. Either remove the comment about the return value, or update it to clarify that the return value is not used because the middleware only cares about the validation side effects (raisingCSRFValidationErroron failure or allowing the request to proceed on success).backend/tests/integration/test_saved_scripts_routes.py (4)
7-9: LGTM!Import updates are correctly structured -
SavedScriptResponsefor response validation,UserCredentialsTypedDict for type safety, andlogin_userhelper for re-authentication flows.
35-50: LGTM!The test correctly uses
UserCredentialstype annotation and passes CSRF headers for the POST request. The comment on line 49 clearly documents the header requirement.
236-257: LGTM!The re-authentication flow is correctly implemented:
- Re-login as regular user to get fresh CSRF token
- Create script with user's headers
- Re-login as admin before access checks
This properly handles the shared client scenario where fixtures may have logged in different users.
392-416: LGTM!Session persistence test correctly demonstrates the re-authentication flow:
- First session: login, create script with fresh headers
- Logout
- Second session: login again, verify script persists
The explicit login steps ensure CSRF tokens are properly refreshed across session boundaries.
backend/app/main.py (4)
34-48: LGTM!The metrics imports are well-organized and comprehensive, covering all service domains (Connection, Coordinator, Database, DLQ, Event, Execution, Health, Kubernetes, Notification, RateLimit, Replay, Security).
70-86: LGTM!Eager initialization of all metrics contexts at app startup ensures metrics are available before any requests are processed. Each metrics instance receives the settings for configuration consistency.
100-108: Middleware ordering appears correct for CSRF handling.The middleware stack order (bottom-to-top execution for requests):
- CORSMiddleware (handles preflight OPTIONS)
- CacheControlMiddleware
- RequestSizeLimitMiddleware
- CorrelationMiddleware
- CSRFMiddleware (skips OPTIONS via
validate_csrf_from_request)- RateLimitMiddleware
- MetricsMiddleware
Since
CSRFMiddlewareskips validation for OPTIONS requests (as seen inSecurityService.validate_csrf_from_request), CORS preflight requests will pass through correctly.
179-181: LGTM!Using
factory=Truewith the import string pattern is the recommended approach for multi-worker uvicorn deployments. This ensures each worker process creates its own app instance, avoiding shared state issues.backend/app/core/security.py (2)
24-30: LGTM!Good DI pattern -
SecurityServicenow acceptsSettingsand configures bcrypt rounds fromself.settings.BCRYPT_ROUNDS. This makes the service testable and configurable.
77-82: LGTM!Using
frozensetforCSRF_EXEMPT_PATHSis appropriate - it's immutable and provides O(1) lookup. The exempt paths (login, register, logout) correctly cover the authentication endpoints that can't have CSRF tokens yet.backend/tests/integration/test_dlq_routes.py (3)
21-22: LGTM!The
_RetryRequestTypedDict provides clear typing for the retry payload structure.
201-223: LGTM!Good use of
test_settings.KAFKA_TOPIC_PREFIXto derive topic names for retry policy tests. This ensures test isolation when running with pytest-xdist.
379-382: LGTM!Good use of the
_RetryRequestTypedDict to type the empty retry request payload.backend/tests/conftest.py (5)
19-27: LGTM!The
UserCredentialsTypedDict provides a clear contract for test fixtures, including all necessary fields for authentication:username,password,role,csrf_token, andheaders.
34-51: LGTM!Excellent isolation strategy for pytest-xdist:
- Unique session ID per test session
- Worker-specific prefixes for Kafka topics, consumer groups, and schema subjects
- Isolated database names per worker
- Redis DB selection based on worker number
This prevents resource conflicts when running tests in parallel.
107-113: LGTM!The
redis_clientfixture properly closes the connection in thefinallyblock, ensuring cleanup even if tests fail.
117-122: LGTM!The
_http_loginhelper correctly extracts thecsrf_tokenfrom the JSON response, enabling tests to obtain fresh CSRF tokens for authenticated requests.
125-149: LGTM!The
test_userfixture follows a clean pattern:
- Generate unique credentials
- Register via API
- Login to obtain CSRF token
- Return complete
UserCredentialswith headersThe status code check for 400 handles the case where the user might already exist (idempotent registration).
backend/tests/integration/test_user_settings_routes.py (4)
13-37: LGTM!Well-structured TypedDicts for test payloads:
_NotificationSettingsand_EditorSettingsfor nested settings_UpdateSettingsDatawithtotal=Falsefor partial updatesThis improves test maintainability and catches type errors early.
39-40: LGTM!Using
xdist_group(name="user_settings")ensures these tests run sequentially on a single worker, avoiding state conflicts from concurrent settings modifications.
369-399: LGTM!The isolation test correctly:
- Re-authenticates as first user for fresh CSRF token
- Updates settings with proper headers
- Logs out explicitly
- Re-authenticates as second user
- Verifies settings are isolated
The re-authentication pattern properly handles the shared client scenario.
402-442: LGTM!The persistence test uses the typed
_EditorSettingsand_UpdateSettingsDatato construct update payloads, then verifies settings persist across logout/login cycles. Good demonstration of the TypedDict usage.backend/tests/integration/test_notifications_routes.py (4)
4-13: LGTM!Imports are correctly updated to include:
NotificationChannelandNotificationStatusenums for validationDeleteNotificationResponsefor delete operation assertionsUserCredentialsandlogin_userfor auth handling
33-57: LGTM!The test correctly uses
UserCredentialstype and validates notification structure against the domain enums (NotificationChannel,NotificationStatus).
273-299: LGTM!The delete notification test:
- Uses
headers=test_user["headers"]for the DELETE request- Validates response against
DeleteNotificationResponseschema- Verifies the notification is actually removed from the list
341-368: LGTM!The isolation test correctly uses
login_userhelper to re-authenticate between users, ensuring fresh CSRF tokens are obtained before making requests. This properly tests that notifications are scoped per-user.backend/tests/unit/services/saga/test_saga_orchestrator_unit.py (8)
1-24: LGTM!The imports are well-organized and all necessary for the comprehensive test doubles layer introduced in this file.
47-59: LGTM!Clean test double with both
savedandexistingstorage, allowing tests to verify persistence operations and pre-populate state.
72-79: Same concern: verify parent__init__can be safely skipped.Like
_FakeProd, this test double skipsIdempotencyManager.__init__(). Ensure the parent class doesn't initialize required state that would causeAttributeErrorduring test execution.
96-104: LGTM!Clean test double for a saga step that always succeeds. The type parameter
_FakeEventis correctly applied.
107-117: LGTM!Proper test saga implementation with correct type annotations and integration with the test step.
120-131: LGTM!The factory function creates a properly configured orchestrator with a sensible mix of test doubles and mocks.
134-135: LGTM!Helpful utility function that improves test readability and reduces boilerplate.
62-69: The skipped parent__init__is safe in this context.The
_FakeProd.produce()method completely overrides the parent implementation, so the uninitialized attributes (_config,_schema_registry,_producer,_topic_prefix, etc.) are never accessed. The realproduce()method relies on these attributes, but the fake bypasses it entirely by returningNone. Since tests only callproduce()and don't invoke lifecycle methods or access producer state, this test double pattern is appropriate.backend/tests/unit/services/pod_monitor/test_monitor.py (2)
65-78: LGTM: Clean DI-friendly test helper.This factory properly assembles
K8sClientswith test doubles, enabling DI-based construction in tests.
90-90: No issues found.FakeApi("{}")is the expected default for the test factory function. The empty JSON object string appropriately serves as a minimal placeholder when specific pod logs are not relevant to individual tests.backend/app/services/pod_monitor/monitor.py (4)
100-124: LGTM: Clean dependency injection pattern.The constructor now requires all dependencies explicitly, eliminating lazy initialization and improving type safety. The docstring appropriately directs users to the
create_pod_monitor()factory for automatic dependency creation.
148-150: Verify error handling strategy for connectivity check.The Kubernetes connectivity verification (line 149) has no error handling, so failures will propagate to the caller. Confirm this fail-fast behavior is intentional. If K8s is temporarily unavailable, should the monitor implement retry logic, or is it expected that the caller/orchestrator will handle restarts?
496-501: LGTM: Proper resource ownership and cleanup.The factory correctly tracks whether it created the
k8s_clientsand ensures cleanup only for owned resources. Thefinallyblock guarantees cleanup even if monitor initialization or startup fails, which is the correct behavior.
485-486: No issues found.PodEventMapperdoes not require cleanup—it holds no unmanaged resources (the logger, API client, and event cache are all safely managed as references or in-memory data structures). The factory pattern correctly tracks cleanup only forK8sClients, which owns actual resources.
| # Try to get service from Dishka container if not initialized | ||
| if self.security_service is None: | ||
| asgi_app = scope.get("app") | ||
| if asgi_app: | ||
| container = asgi_app.state.dishka_container | ||
| async with container() as container_scope: | ||
| self.security_service = await container_scope.get(SecurityService) | ||
|
|
||
| if self.security_service is None: | ||
| # If we can't get the service, allow the request through | ||
| # (other auth mechanisms will handle protection) | ||
| await self.app(scope, receive, send) | ||
| return |
There was a problem hiding this comment.
Lazy initialization pattern has multiple issues.
This lazy initialization approach has several concerns:
-
Mutating instance state in async context (line 42): Setting
self.security_servicein an ASGI middleware is problematic because middleware instances are shared across all requests. If two requests arrive concurrently before initialization, both might try to initialize simultaneously, potentially causing race conditions. -
Potential resource leak (lines 41-42): The async context manager
async with container()might not properly clean up ifawait container_scope.get(SecurityService)raises an exception. -
Silent fallback is a security risk (lines 44-48): If the SecurityService cannot be retrieved, the middleware allows all requests through without CSRF protection. This could create a security vulnerability if the service fails to initialize for any reason.
🔒 Recommended approach: require SecurityService at initialization
Instead of lazy initialization, require the SecurityService to be provided at middleware construction time (via DI/app factory):
def __init__(
self,
app: ASGIApp,
- security_service: SecurityService | None = None,
+ security_service: SecurityService,
) -> None:
self.app = app
self.security_service = security_service
async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
if scope["type"] != "http":
await self.app(scope, receive, send)
return
- # Try to get service from Dishka container if not initialized
- if self.security_service is None:
- asgi_app = scope.get("app")
- if asgi_app:
- container = asgi_app.state.dishka_container
- async with container() as container_scope:
- self.security_service = await container_scope.get(SecurityService)
-
- if self.security_service is None:
- # If we can't get the service, allow the request through
- # (other auth mechanisms will handle protection)
- await self.app(scope, receive, send)
- return
-
# Build request object for validation
request = Request(scope, receive=receive)Then ensure the middleware is constructed with the SecurityService in your app factory/main.py.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @backend/app/core/middlewares/csrf.py around lines 36 - 48, The middleware
must not lazily mutate self.security_service during requests; change the
constructor of the CSRF middleware to require a ready SecurityService instance
(or a synchronous provider) and store it in self.security_service at
initialization time, remove the async container lookup block (the async with
container() and get(SecurityService) code) from the __call__/dispatch path, and
instead raise an explicit error (e.g., RuntimeError) if __init__ is given no
SecurityService; update the app factory to resolve and pass the SecurityService
into the middleware at startup so no async initialization or silent fallback
occurs.
| def __init__(self) -> None: | ||
| # Skip parent __init__ - use MagicMocks for required attributes | ||
| self._event_repository = MagicMock() | ||
| self._kafka_producer = MagicMock() | ||
| self._settings = MagicMock() | ||
| self._logger = MagicMock() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the KafkaEventService class
find . -type f -name "*.py" | xargs rg "class KafkaEventService" -A 1Repository: HardMax71/Integr8sCode
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Look for files in the services directory related to kafka/events
fd -e py "kafka|event" --search-path backend/services 2>/dev/null | head -20Repository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Read the KafkaEventService class and __init__ method
cat ./backend/app/services/kafka_event_service.pyRepository: HardMax71/Integr8sCode
Length of output: 10714
🏁 Script executed:
#!/bin/bash
# Read the test file to see the actual FakeKafkaEventService implementation
cat -n ./backend/tests/unit/services/pod_monitor/test_monitor.py | sed -n '30,60p'Repository: HardMax71/Integr8sCode
Length of output: 1241
🏁 Script executed:
#!/bin/bash
# Check how FakeKafkaEventService is used in the test file
rg "FakeKafkaEventService\|publish_base_event\|publish_event\|publish_execution" ./backend/tests/unit/services/pod_monitor/test_monitor.py -A 2 -B 2Repository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Read more of the test file to see how FakeKafkaEventService is used
cat -n ./backend/tests/unit/services/pod_monitor/test_monitor.py | head -100Repository: HardMax71/Integr8sCode
Length of output: 3808
🏁 Script executed:
#!/bin/bash
# Check if metrics is accessed in KafkaEventService methods
rg "self\.metrics" ./backend/app/services/kafka_event_service.pyRepository: HardMax71/Integr8sCode
Length of output: 400
🏁 Script executed:
#!/bin/bash
# Check if PodMonitor accesses any KafkaEventService attributes or methods beyond publish_base_event
rg "kafka_event_service\." ./backend/services/pod_monitor/monitor.py -B 2 -A 2Repository: HardMax71/Integr8sCode
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Find the PodMonitor class file
find . -name "monitor.py" -type f | grep -i podRepository: HardMax71/Integr8sCode
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Check how PodMonitor uses kafka_event_service
rg "kafka_event_service\." ./backend/app/services/pod_monitor/monitor.py -B 2 -A 2Repository: HardMax71/Integr8sCode
Length of output: 308
Fix attribute naming and add missing metrics initialization to FakeKafkaEventService.
Skipping KafkaEventService.__init__ has two issues:
-
Attribute naming mismatch: The fake sets
_event_repository,_kafka_producer,_settings,_logger(with underscores), but the parent setsevent_repository,kafka_producer,settings,logger(without underscores). This breaks type compatibility and LSP. -
Missing
self.metricsinitialization: The parent callsget_event_metrics()and assigns toself.metrics. While the overriddenpublish_base_eventin the fake doesn't use it, this creates a fragile test double. If PodMonitor or another component calls other KafkaEventService methods (likepublish_eventorpublish_execution_event), it will fail with an AttributeError.
Either call the parent __init__ with mock arguments or explicitly initialize all attributes with correct names:
def __init__(self) -> None:
self.event_repository = MagicMock()
self.kafka_producer = MagicMock()
self.settings = MagicMock()
self.logger = MagicMock()
self.metrics = MagicMock() # Mock metrics to prevent AttributeError
self.published_events: list[tuple[BaseEvent, str | None]] = []🤖 Prompt for AI Agents
In @backend/tests/unit/services/pod_monitor/test_monitor.py around lines 36 -
41, FakeKafkaEventService.__init__ currently sets underscored attributes
(_event_repository, _kafka_producer, _settings, _logger) and omits self.metrics,
breaking LSP/type compatibility and risking AttributeError; update the
constructor to initialize the public names used by KafkaEventService
(event_repository, kafka_producer, settings, logger) and also set self.metrics
(e.g., to a MagicMock) and self.published_events so the fake matches the real
service, or alternatively invoke KafkaEventService.__init__ with mock arguments
to ensure all expected attributes are initialized.
There was a problem hiding this comment.
4 issues found across 10 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/tests/integration/test_saved_scripts_routes.py">
<violation number="1" location="backend/tests/integration/test_saved_scripts_routes.py:226">
P1: Test logic is broken: after logout, the test attempts to retrieve the script without re-authenticating. The `test_user` fixture does not automatically re-login after logout - it only sets up authentication once. This test either fails with 401 or doesn't test cross-session persistence correctly. You need to re-login after logout, or use a different approach to test persistence.</violation>
</file>
<file name="backend/tests/integration/test_user_settings_routes.py">
<violation number="1" location="backend/tests/integration/test_user_settings_routes.py:418">
P1: The `test_user` fixture returns a standard `httpx.AsyncClient` which does not have an `auth_data` attribute. This will cause an `AttributeError` at runtime. Either the fixture needs to store auth credentials on the client, or hardcode the test credentials here.</violation>
</file>
<file name="backend/tests/conftest.py">
<violation number="1" location="backend/tests/conftest.py:134">
P2: Resource leak: if `raise_for_status()` throws, the `httpx.AsyncClient` is never closed. Wrap the post-creation logic in try/except to ensure cleanup on login failure.</violation>
</file>
<file name="backend/tests/integration/test_events_routes.py">
<violation number="1" location="backend/tests/integration/test_events_routes.py:486">
P2: Test assertion weakened: `assert meta["user_id"] is not None` does not verify user isolation. Consider storing user_id during fixture creation (e.g., from the login response) and comparing it here, or at minimum verify that `user_event_ids` and `admin_event_ids` don't overlap.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| # Verify it's deleted by trying to get it | ||
| get_response = await client.get(f"/api/v1/scripts/{script_id}") | ||
| get_response = await test_user.get(f"/api/v1/scripts/{script_id}") |
There was a problem hiding this comment.
P1: Test logic is broken: after logout, the test attempts to retrieve the script without re-authenticating. The test_user fixture does not automatically re-login after logout - it only sets up authentication once. This test either fails with 401 or doesn't test cross-session persistence correctly. You need to re-login after logout, or use a different approach to test persistence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/integration/test_saved_scripts_routes.py, line 226:
<comment>Test logic is broken: after logout, the test attempts to retrieve the script without re-authenticating. The `test_user` fixture does not automatically re-login after logout - it only sets up authentication once. This test either fails with 401 or doesn't test cross-session persistence correctly. You need to re-login after logout, or use a different approach to test persistence.</comment>
<file context>
@@ -215,30 +213,27 @@ async def test_delete_saved_script(self, client: AsyncClient, test_user: UserCre
# Verify it's deleted by trying to get it
- get_response = await client.get(f"/api/v1/scripts/{script_id}")
+ get_response = await test_user.get(f"/api/v1/scripts/{script_id}")
assert get_response.status_code in [404, 403]
</file context>
| "username": username, | ||
| "password": password, | ||
| }) | ||
| login_resp.raise_for_status() |
There was a problem hiding this comment.
P2: Resource leak: if raise_for_status() throws, the httpx.AsyncClient is never closed. Wrap the post-creation logic in try/except to ensure cleanup on login failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/conftest.py, line 134:
<comment>Resource leak: if `raise_for_status()` throws, the `httpx.AsyncClient` is never closed. Wrap the post-creation logic in try/except to ensure cleanup on login failure.</comment>
<file context>
@@ -113,91 +103,80 @@ async def redis_client(scope: AsyncContainer) -> AsyncGenerator[redis.Redis, Non
- "headers": {"X-CSRF-Token": csrf},
- }
+ })
+ login_resp.raise_for_status()
+ csrf = login_resp.json().get("csrf_token", "")
+ c.headers["X-CSRF-Token"] = csrf
</file context>
| login_resp.raise_for_status() | |
| try: | |
| login_resp.raise_for_status() | |
| except httpx.HTTPStatusError: | |
| await c.aclose() | |
| raise |
| # User events should belong to the user | ||
| # We can't access the user_id directly from the fixture anymore | ||
| # but we can verify events are returned (isolation is enforced server-side) | ||
| assert meta["user_id"] is not None |
There was a problem hiding this comment.
P2: Test assertion weakened: assert meta["user_id"] is not None does not verify user isolation. Consider storing user_id during fixture creation (e.g., from the login response) and comparing it here, or at minimum verify that user_event_ids and admin_event_ids don't overlap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/integration/test_events_routes.py, line 486:
<comment>Test assertion weakened: `assert meta["user_id"] is not None` does not verify user isolation. Consider storing user_id during fixture creation (e.g., from the login response) and comparing it here, or at minimum verify that `user_event_ids` and `admin_event_ids` don't overlap.</comment>
<file context>
@@ -457,46 +457,37 @@ async def test_event_pagination(self, client: AsyncClient, test_user: dict[str,
+ # User events should belong to the user
+ # We can't access the user_id directly from the fixture anymore
+ # but we can verify events are returned (isolation is enforced server-side)
+ assert meta["user_id"] is not None
for event in admin_events["events"]:
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/tests/integration/test_saved_scripts_routes.py (1)
383-410: Test logic is flawed after logout.The comment on line 404 claims "test_user fixture handles re-authentication," but fixtures don't re-authenticate after explicit logout. After
test_user.post("/api/v1/auth/logout"), the session cookie is invalidated, and subsequent requests with the same client will fail authentication since fixtures are created once per test invocation.This test will likely return 401 on line 405-406, not 200.
🐛 Proposed fix: Remove logout/re-login expectation or use fresh client
Either remove the logout test (since cross-session persistence is better tested with a new client instance) or acknowledge the test now validates that scripts exist after logout but require re-authentication:
# Logout logout_response = await test_user.post("/api/v1/auth/logout") assert logout_response.status_code == 200 - # Script should still exist after logout/login (test_user fixture handles re-authentication) + # After logout, the script should still exist in the database + # but accessing it requires re-authentication (expect 401) get_response = await test_user.get(f"/api/v1/scripts/{script_id}") - assert get_response.status_code == 200 + assert get_response.status_code == 401 # Session invalidated after logoutAlternatively, if the intent is to test true persistence, this test should be removed or restructured to use a fresh client with a new login.
backend/tests/integration/test_user_settings_routes.py (1)
391-432: Critical:AsyncClienthas noauth_dataattribute — test will fail.The pipeline failure confirms this: line 418 references
test_user.auth_data["username"]andtest_user.auth_data["password"], buthttpx.AsyncClienthas noauth_dataattribute. The_create_authenticated_clienthelper in conftest.py does not attach credentials to the returned client.🐛 Proposed fix: Store credentials or restructure the test
Option 1: Extend the helper to attach auth_data (preferred)
In
backend/tests/conftest.py, modify_create_authenticated_client:async def _create_authenticated_client( app: FastAPI, username: str, email: str, password: str, role: str ) -> httpx.AsyncClient: """Create and return an authenticated client with CSRF header set.""" c = httpx.AsyncClient( transport=ASGITransport(app=app), base_url="https://test", timeout=30.0, follow_redirects=True, ) + # Store credentials for tests that need to re-login + c.auth_data = {"username": username, "password": password} # type: ignore[attr-defined] r = await c.post("/api/v1/auth/register", json={Option 2: Restructure the test to avoid re-login
@pytest.mark.asyncio - async def test_settings_persistence(self, client: AsyncClient, test_user: AsyncClient) -> None: + async def test_settings_persistence(self, test_user: AsyncClient) -> None: """Test that settings persist across login sessions.""" - # Already authenticated via test_user fixture - # Update settings editor_settings: _EditorSettings = { ... } ... response = await test_user.put("/api/v1/user/settings/", json=update_data) assert response.status_code == 200 - # Log out - await client.post("/api/v1/auth/logout") - - # Log back in as same user - await login_user(client, test_user.auth_data["username"], test_user.auth_data["password"]) - - # Get settings again - response = await client.get("/api/v1/user/settings/") + # Verify settings are returned correctly (persistence is inherent to DB storage) + response = await test_user.get("/api/v1/user/settings/") assert response.status_code == 200 persisted_settings = UserSettings(**response.json()) ...backend/tests/integration/test_notifications_routes.py (1)
89-114: Fix false-positive in “mark as read” verification (no assertion the notification was found).
If the updated notification isn’t in the returned page, the test currently passes anyway.Proposed fix
- for notif in updated_data["notifications"]: + found = False + for notif in updated_data["notifications"]: if notif["notification_id"] == notification_id: assert notif["status"] == "read" + found = True break + assert found, "Updated notification not present in list response; pagination may have hidden it"backend/tests/integration/test_events_routes.py (2)
32-53: Be consistent about “this endpoint may be 404” handling for/api/v1/events/user.
You allow 404 intest_get_user_events, but several later tests hard-require 200 from the same resource; that inconsistency will break in the exact deployments you mention.Example fix pattern (skip when endpoint missing)
- user_events_response = await test_user.get("/api/v1/events/user?limit=10") - assert user_events_response.status_code == 200 + user_events_response = await test_user.get("/api/v1/events/user?limit=10") + if user_events_response.status_code == 404: + pytest.skip("Deployment does not expose /api/v1/events/user") + assert user_events_response.status_code == 200Also applies to: 175-205, 247-260, 395-409, 430-457, 460-493
282-303:common_typesis never asserted (test currently doesn’t test what it claims).Proposed fix
- # At least some common types should be present - for event_type in event_types: + for event_type in event_types: assert isinstance(event_type, str) assert len(event_type) > 0 + + assert any(t in event_types for t in common_types), "Expected at least one common event type"
🤖 Fix all issues with AI agents
In @backend/tests/integration/test_notifications_routes.py:
- Around line 131-147: In test_mark_all_notifications_as_read, avoid asserting
unread_count == 0 which is flaky; instead call GET
/api/v1/notifications/unread-count once before calling POST
/api/v1/notifications/mark-all-read to capture initial_count, then POST to mark
all read, then GET unread-count again and assert new_count <= initial_count (and
optionally assert new_count == 0 only in controlled test environments); also
remove the duplicated unread-count call so the test only queries the count twice
(once before, once after) and uses monotonic non-increasing assertion to prevent
races from background notification creation.
🧹 Nitpick comments (2)
backend/tests/conftest.py (1)
23-41: Minor redundancy in REDIS_DB calculation.On line 30,
worker_num = sum(_WORKER_ID.encode()) % 16already produces a value in range [0, 15]. Then line 35 appliesworker_num % 16again, which is redundant.♻️ Simplify REDIS_DB assignment
worker_num = sum(_WORKER_ID.encode()) % 16 unique_prefix = f"{base_prefix}{session_id}.{_WORKER_ID}." return base.model_copy( update={ "DATABASE_NAME": f"integr8scode_test_{session_id}_{_WORKER_ID}", - "REDIS_DB": worker_num % 16, + "REDIS_DB": worker_num, "KAFKA_GROUP_SUFFIX": f"{session_id}.{_WORKER_ID}",backend/tests/integration/test_events_routes.py (1)
73-105: Filtering assertion is overly permissive and a bit brittle.
Substring matching can pass unexpectedly (and theor len(...)==0is redundant inside the loop).Suggested tightening
- for event in events_response.events: - if event.event_type: # Some events might have been created - assert any(event_type in event.event_type for event_type in event_types) or len( - events_response.events) == 0 + allowed = set(event_types) + for event in events_response.events: + if event.event_type: + assert event.event_type in allowed
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/tests/conftest.pybackend/tests/e2e/test_execution_routes.pybackend/tests/integration/test_admin_routes.pybackend/tests/integration/test_dlq_routes.pybackend/tests/integration/test_events_routes.pybackend/tests/integration/test_notifications_routes.pybackend/tests/integration/test_replay_routes.pybackend/tests/integration/test_saga_routes.pybackend/tests/integration/test_saved_scripts_routes.pybackend/tests/integration/test_user_settings_routes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/e2e/test_execution_routes.py
🧰 Additional context used
🧬 Code graph analysis (9)
backend/tests/integration/test_dlq_routes.py (4)
backend/tests/conftest.py (3)
app(46-57)test_user(141-152)test_settings(24-41)backend/tests/unit/conftest.py (1)
app(68-69)backend/app/dlq/models.py (1)
DLQMessageStatus(10-16)backend/app/schemas_pydantic/dlq.py (5)
DLQBatchRetryResponse(70-78)DLQMessageDetail(95-117)DLQMessageResponse(22-39)DLQMessagesResponse(59-67)DLQTopicSummaryResponse(81-92)
backend/tests/integration/test_saga_routes.py (1)
backend/tests/conftest.py (2)
test_user(141-152)another_user(171-182)
backend/tests/integration/test_notifications_routes.py (1)
backend/tests/conftest.py (3)
app(46-57)test_user(141-152)test_admin(156-167)
backend/tests/integration/test_replay_routes.py (1)
backend/tests/conftest.py (3)
app(46-57)test_user(141-152)test_admin(156-167)
backend/tests/conftest.py (2)
backend/app/main.py (1)
create_app(60-162)backend/app/settings.py (1)
Settings(11-165)
backend/tests/integration/test_saved_scripts_routes.py (1)
backend/tests/conftest.py (3)
app(46-57)test_user(141-152)test_admin(156-167)
backend/tests/integration/test_events_routes.py (3)
backend/tests/conftest.py (3)
app(46-57)test_user(141-152)test_admin(156-167)backend/app/services/coordinator/queue_manager.py (1)
user_id(34-35)backend/app/domain/events/event_models.py (1)
correlation_id(60-61)
backend/tests/integration/test_user_settings_routes.py (3)
backend/tests/conftest.py (4)
app(46-57)test_user(141-152)client(68-76)another_user(171-182)backend/tests/helpers/auth.py (1)
login_user(13-42)backend/tests/helpers/eventually.py (1)
eventually(8-36)
backend/tests/integration/test_admin_routes.py (1)
backend/tests/conftest.py (2)
test_admin(156-167)test_user(141-152)
🪛 GitHub Actions: MyPy Type Checking
backend/tests/integration/test_user_settings_routes.py
[error] 418-418: mypy --config-file pyproject.toml --strict .: 'AsyncClient' has no attribute 'auth_data' (tests/integration/test_user_settings_routes.py:418).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Backend
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (20)
backend/tests/integration/test_saga_routes.py (3)
24-31: LGTM!The migration from manual login to the
test_userfixture is correctly applied. The test properly validates 404 behavior for non-existent sagas using the authenticated client.
143-164: LGTM!Multi-user access control test correctly uses both
test_userandanother_userfixtures. This properly validates that saga isolation works between different authenticated users.
274-291: LGTM!Concurrent saga access test is well-structured. The test properly validates that multiple concurrent requests succeed with the authenticated fixture.
backend/tests/integration/test_saved_scripts_routes.py (2)
32-87: LGTM!The create and retrieve test is well-structured. It properly validates UUID format, response structure, and data integrity using the authenticated fixture.
233-264: LGTM!Cross-user access control test correctly uses
test_userandtest_adminfixtures to validate that admin cannot access scripts created by regular users. This is a proper isolation test.backend/tests/integration/test_dlq_routes.py (3)
20-21: LGTM!The
_RetryRequestTypedDict provides proper type safety for the retry request payload structure.
199-222: LGTM!The retry policy test correctly uses
test_settings.KAFKA_TOPIC_PREFIXto construct an isolated topic name. This aligns with the broader Settings-driven isolation approach in the PR.
376-382: LGTM!Good use of the
_RetryRequestTypedDict for explicit typing of the empty retry request, improving code clarity and type safety.backend/tests/integration/test_replay_routes.py (3)
19-31: LGTM!The test correctly validates that non-admin users receive 403 Forbidden when accessing replay endpoints. Using
test_userfixture for this negative test case is appropriate.
37-62: LGTM!The migration to
ReplayFilternested structure for filter parameters is clean and aligns with the domain model changes. The test properly validates session creation with the admin fixture.
393-402: LGTM!Progress tracking test correctly uses the
eventuallyhelper to poll for session progress without fixed sleeps. The field name changes toreplayed_eventsandtotal_eventsappear consistent with the ReplaySession model updates.backend/tests/integration/test_admin_routes.py (3)
28-59: LGTM!Admin settings retrieval test is well-structured. The test validates the complete SystemSettings structure including execution limits, security settings, and monitoring settings with appropriate default value assertions.
159-215: LGTM!Complete user CRUD lifecycle test is comprehensive. It properly validates create, get, get-overview, update, delete operations and verifies deletion by confirming 404 on subsequent get.
357-364: Potential CSRF header staleness after logout.After logout on line 358, the
test_adminclient still has the original CSRF token in headers. The subsequent login on lines 360-363 uses form data (data=), which typically bypasses CSRF for login endpoints. However, if the new session returns a new CSRF token, subsequent POST requests in this test would use the stale token.Since this is the last operation in the test, it doesn't cause issues here, but this pattern could be problematic if extended.
Consider whether the login endpoint is CSRF-exempt. If not, the stale CSRF token could cause failures.
backend/tests/conftest.py (2)
110-137: LGTM!The
_create_authenticated_clienthelper is well-designed:
- Handles both fresh registration and existing user (400) scenarios
- Extracts CSRF token from login response and sets it in client headers
- Properly cleans up client on failure
This centralizes authenticated client creation and improves test consistency.
140-152: LGTM!The
test_userfixture correctly uses the helper to create an authenticated user client with CSRF headers pre-configured. The unique UID prevents collisions in parallel test execution.backend/tests/integration/test_user_settings_routes.py (1)
12-36: LGTM!The TypedDict definitions provide excellent type safety for settings payloads. The
total=Falseon_UpdateSettingsDatacorrectly allows partial updates.backend/tests/integration/test_notifications_routes.py (2)
31-35: Fixture-based authenticated clients are a solid simplification.
Usingtest_user/test_admindirectly makes these tests shorter and avoids repeated login flows.Also applies to: 58-63, 73-77, 116-124, 149-154, 182-193, 219-232, 243-260, 270-309, 311-337, 339-359, 361-370
4-11: No changes needed—assertions work correctly with StringEnum.The test assertions comparing Enum objects to string values are safe.
StringEnumextends Python'sStrEnum, which mixes instrand natively supports equality with string values (e.g.,NotificationChannel.IN_APP == "in_app"isTrue). Pydantic v2 returnsStringEnumobjects for enum-typed fields, and these objects behave transparently as strings in comparisons and membership checks. The assertions at lines 4–11, 47–55, 162–166, 199–204, 237–241, and 264–268 will function correctly without the proposed_val()helper.backend/tests/integration/test_events_routes.py (1)
1-13: Schema-aligned assertions look good (metadata + event_version + hourly stats typing).Also applies to: 33-65, 107-173, 207-245, 221-245, 305-349, 351-377, 379-393, 395-424
| async def test_events_isolation_between_users(self, test_user: AsyncClient, | ||
| test_admin: AsyncClient) -> None: | ||
| """Test that events are properly isolated between users.""" | ||
| # Get events as regular user | ||
| user_login_data = { | ||
| "username": test_user["username"], | ||
| "password": test_user["password"] | ||
| } | ||
| user_login_response = await client.post("/api/v1/auth/login", data=user_login_data) | ||
| assert user_login_response.status_code == 200 | ||
|
|
||
| user_events_response = await client.get("/api/v1/events/user?limit=10") | ||
| user_events_response = await test_user.get("/api/v1/events/user?limit=10") | ||
| assert user_events_response.status_code == 200 | ||
|
|
||
| user_events = user_events_response.json() | ||
| user_event_ids = [e["event_id"] for e in user_events["events"]] | ||
|
|
||
| # Get events as admin (without include_all_users flag) | ||
| admin_login_data = { | ||
| "username": test_admin["username"], | ||
| "password": test_admin["password"] | ||
| } | ||
| admin_login_response = await client.post("/api/v1/auth/login", data=admin_login_data) | ||
| assert admin_login_response.status_code == 200 | ||
|
|
||
| admin_events_response = await client.get("/api/v1/events/user?limit=10") | ||
| admin_events_response = await test_admin.get("/api/v1/events/user?limit=10") | ||
| assert admin_events_response.status_code == 200 | ||
|
|
||
| admin_events = admin_events_response.json() | ||
| admin_event_ids = [e["event_id"] for e in admin_events["events"]] | ||
|
|
||
| # Events should be different (unless users share some events) | ||
| # But user IDs in events should be different | ||
| # Each user should only see their own events | ||
| # We verify this by checking that if events exist, they have user_id in metadata | ||
| for event in user_events["events"]: | ||
| meta = event.get("metadata") or {} | ||
| if meta.get("user_id"): | ||
| assert meta["user_id"] == test_user.get("user_id", meta["user_id"]) | ||
| # User events should belong to the user | ||
| # We can't access the user_id directly from the fixture anymore | ||
| # but we can verify events are returned (isolation is enforced server-side) | ||
| assert meta["user_id"] is not None | ||
|
|
||
| for event in admin_events["events"]: | ||
| meta = event.get("metadata") or {} | ||
| if meta.get("user_id"): | ||
| assert meta["user_id"] == test_admin.get("user_id", meta["user_id"]) | ||
| # Admin events should belong to the admin | ||
| # We verify events are returned (isolation is enforced server-side) | ||
| assert meta["user_id"] is not None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find any "me", "whoami", or "current" endpoints in the API
rg -nP '("/api/v1/[^"]*(?:me|whoami|current)[^"]*")' backend/tests backend/app -SRepository: HardMax71/Integr8sCode
Length of output: 3750
🏁 Script executed:
# Also search more broadly for endpoint patterns that might return user info
rg -n 'def.*(?:me|current|whoami)' backend/app --type py -iRepository: HardMax71/Integr8sCode
Length of output: 45377
🏁 Script executed:
# Examine conftest.py to see how test clients are created
fd conftest.py backend/testsRepository: HardMax71/Integr8sCode
Length of output: 190
🏁 Script executed:
# Check test_auth_routes.py to see what /api/v1/auth/me returns
grep -A 10 'me_response = await client.get("/api/v1/auth/me")' backend/tests/integration/test_auth_routes.py | head -30Repository: HardMax71/Integr8sCode
Length of output: 1153
🏁 Script executed:
# Look at the auth endpoint handler
grep -A 20 'async def get_current_user_profile' backend/app/api/routes/auth.pyRepository: HardMax71/Integr8sCode
Length of output: 652
🏁 Script executed:
# Examine conftest.py for the integration tests
cat backend/tests/integration/conftest.pyRepository: HardMax71/Integr8sCode
Length of output: 790
🏁 Script executed:
# Search for test_user and test_admin fixtures
rg -n "def test_user|def test_admin" backend/testsRepository: HardMax71/Integr8sCode
Length of output: 1133
🏁 Script executed:
# Look at backend/tests/conftest.py (parent conftest)
cat backend/tests/conftest.pyRepository: HardMax71/Integr8sCode
Length of output: 5832
🏁 Script executed:
# Search for _create_authenticated_client
rg -n "_create_authenticated_client|create_authenticated_client" backend/testsRepository: HardMax71/Integr8sCode
Length of output: 357
🏁 Script executed:
# Check UserResponse schema to see what fields it has
rg -n "class UserResponse" backend/app -A 15Repository: HardMax71/Integr8sCode
Length of output: 1170
🏁 Script executed:
# Look at the actual test in test_events_routes.py around lines 460-493
sed -n '450,500p' backend/tests/integration/test_events_routes.pyRepository: HardMax71/Integr8sCode
Length of output: 2262
🏁 Script executed:
# Check if there are other tests that verify user isolation more effectively
rg -n "user_id\|metadata.user_id" backend/tests/integration/test_events_routes.py -B 2 -A 2Repository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
# Search for any tests that do verify user_id equality in events
rg -n "assert.*user_id\|assertEqual.*user_id" backend/tests/integrationRepository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
# Check the event API response structure
rg -n "class.*EventResponse\|class EventDetail" backend/app/schemas_pydantic -A 10Repository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
# Check if there are patterns in other tests that fetch current user and verify isolation
rg -n '"/api/v1/auth/me"' backend/tests/integration -B 3 -A 5Repository: HardMax71/Integr8sCode
Length of output: 4061
🏁 Script executed:
# Check if there are any other multi-user isolation tests in the repo
rg -n "test.*isolation\|another_user\|second.*user" backend/tests/integration/test_events_routes.pyRepository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
# Look at the event response schema to understand metadata structure
rg -n "class.*Event.*Response\|metadata.*:" backend/app/schemas_pydantic/events.py -A 5 | head -50Repository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
# Check if the /api/v1/auth/me endpoint is already used in test_events_routes.py
rg -n "/auth/me" backend/tests/integration/test_events_routes.pyRepository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
# Look at the full test to see if events are created before checking isolation
sed -n '400,500p' backend/tests/integration/test_events_routes.pyRepository: HardMax71/Integr8sCode
Length of output: 4357
🏁 Script executed:
# Check the events API route handler to see how filtering works
rg -n "async def get_user_events\|/events/user" backend/app/api/routes/events.py -B 5 -A 20Repository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
# Check if there are fixture setups that create events in test_events_routes.py
rg -n "def.*setup\|@pytest.fixture" backend/tests/integration/test_events_routes.py | head -20Repository: HardMax71/Integr8sCode
Length of output: 48
Isolation test too weak to catch regressions.
The test only asserts metadata.user_id is not None without verifying that events actually belong to the authenticated user. This cannot detect if one user accidentally gains access to another user's events.
Suggested fix: Use the existing /api/v1/auth/me endpoint to fetch the authenticated user's ID, then assert that all returned events have matching event.metadata.user_id:
me_response = await test_user.get("/api/v1/auth/me")
assert me_response.status_code == 200
user_id = me_response.json()["user_id"]
user_events_response = await test_user.get("/api/v1/events/user?limit=10")
user_events = user_events_response.json()
for event in user_events["events"]:
assert event["metadata"]["user_id"] == user_idRepeat similarly for test_admin. This ensures the test catches any regression in user isolation enforcement.
| async def test_mark_all_notifications_as_read(self, test_user: AsyncClient) -> None: | ||
| """Test marking all notifications as read.""" | ||
| # Login first | ||
| login_data = { | ||
| "username": test_user["username"], | ||
| "password": test_user["password"] | ||
| } | ||
| login_response = await client.post("/api/v1/auth/login", data=login_data) | ||
| assert login_response.status_code == 200 | ||
|
|
||
| # Mark all as read | ||
| mark_all_response = await client.post("/api/v1/notifications/mark-all-read") | ||
| mark_all_response = await test_user.post("/api/v1/notifications/mark-all-read") | ||
| assert mark_all_response.status_code == 204 | ||
|
|
||
| # Verify all are now read | ||
| # Verify via unread-count only (list endpoint pagination can hide remaining) | ||
| unread_response = await client.get("/api/v1/notifications/unread-count") | ||
| unread_response = await test_user.get("/api/v1/notifications/unread-count") | ||
| assert unread_response.status_code == 200 | ||
|
|
||
| # Also verify unread count is 0 | ||
| count_response = await client.get("/api/v1/notifications/unread-count") | ||
| count_response = await test_user.get("/api/v1/notifications/unread-count") | ||
| assert count_response.status_code == 200 | ||
| count_data = count_response.json() | ||
| assert count_data["unread_count"] == 0 | ||
|
|
There was a problem hiding this comment.
Avoid flakiness: unread_count == 0 can be unstable in “real backend” integration runs.
If background activity creates notifications between calls, this will intermittently fail. Also, the unread-count endpoint is called twice with no extra assertion value.
Less flaky assertion (monotonic non-increasing)
- unread_response = await test_user.get("/api/v1/notifications/unread-count")
- assert unread_response.status_code == 200
-
- # Also verify unread count is 0
- count_response = await test_user.get("/api/v1/notifications/unread-count")
- assert count_response.status_code == 200
- count_data = count_response.json()
- assert count_data["unread_count"] == 0
+ before = (await test_user.get("/api/v1/notifications/unread-count")).json()["unread_count"]
+ after_resp = await test_user.get("/api/v1/notifications/unread-count")
+ assert after_resp.status_code == 200
+ after = after_resp.json()["unread_count"]
+ assert isinstance(after, int) and after >= 0
+ assert after <= before📝 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.
| async def test_mark_all_notifications_as_read(self, test_user: AsyncClient) -> None: | |
| """Test marking all notifications as read.""" | |
| # Login first | |
| login_data = { | |
| "username": test_user["username"], | |
| "password": test_user["password"] | |
| } | |
| login_response = await client.post("/api/v1/auth/login", data=login_data) | |
| assert login_response.status_code == 200 | |
| # Mark all as read | |
| mark_all_response = await client.post("/api/v1/notifications/mark-all-read") | |
| mark_all_response = await test_user.post("/api/v1/notifications/mark-all-read") | |
| assert mark_all_response.status_code == 204 | |
| # Verify all are now read | |
| # Verify via unread-count only (list endpoint pagination can hide remaining) | |
| unread_response = await client.get("/api/v1/notifications/unread-count") | |
| unread_response = await test_user.get("/api/v1/notifications/unread-count") | |
| assert unread_response.status_code == 200 | |
| # Also verify unread count is 0 | |
| count_response = await client.get("/api/v1/notifications/unread-count") | |
| count_response = await test_user.get("/api/v1/notifications/unread-count") | |
| assert count_response.status_code == 200 | |
| count_data = count_response.json() | |
| assert count_data["unread_count"] == 0 | |
| async def test_mark_all_notifications_as_read(self, test_user: AsyncClient) -> None: | |
| """Test marking all notifications as read.""" | |
| # Mark all as read | |
| mark_all_response = await test_user.post("/api/v1/notifications/mark-all-read") | |
| assert mark_all_response.status_code == 204 | |
| # Verify all are now read | |
| # Verify via unread-count only (list endpoint pagination can hide remaining) | |
| before = (await test_user.get("/api/v1/notifications/unread-count")).json()["unread_count"] | |
| after_resp = await test_user.get("/api/v1/notifications/unread-count") | |
| assert after_resp.status_code == 200 | |
| after = after_resp.json()["unread_count"] | |
| assert isinstance(after, int) and after >= 0 | |
| assert after <= before |
🤖 Prompt for AI Agents
In @backend/tests/integration/test_notifications_routes.py around lines 131 -
147, In test_mark_all_notifications_as_read, avoid asserting unread_count == 0
which is flaky; instead call GET /api/v1/notifications/unread-count once before
calling POST /api/v1/notifications/mark-all-read to capture initial_count, then
POST to mark all read, then GET unread-count again and assert new_count <=
initial_count (and optionally assert new_count == 0 only in controlled test
environments); also remove the duplicated unread-count call so the test only
queries the count twice (once before, once after) and uses monotonic
non-increasing assertion to prevent races from background notification creation.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/tests/conftest.py:
- Around line 111-138: The _create_authenticated_client function can leak the
httpx.AsyncClient 'c' if login_resp.raise_for_status() raises; ensure the client
is closed on login failures by catching the exception from the login request (or
checking login_resp.status_code similar to registration), calling await
c.aclose() before re-raising or failing the test, and only returning 'c' after a
successful login and CSRF extraction; target symbols:
_create_authenticated_client, c, login_resp, login_resp.raise_for_status(), and
c.aclose().
🧹 Nitpick comments (3)
backend/app/core/dishka_lifespan.py (1)
46-72: Consider simplifying the redundantenable_console_exporterparameter.On line 54,
enable_console_exporter=settings.TESTINGis alwaysFalsebecause the enclosing condition on line 47 ensuresnot settings.TESTING. Consider simplifying this toenable_console_exporter=Falsefor clarity, or verify whether console exporter should be enabled in some non-test scenarios (e.g., development).♻️ Proposed simplification
if settings.ENABLE_TRACING and not settings.TESTING: instrumentation_report = init_tracing( service_name=settings.TRACING_SERVICE_NAME, settings=settings, logger=logger, service_version=settings.TRACING_SERVICE_VERSION, sampling_rate=settings.TRACING_SAMPLING_RATE, - enable_console_exporter=settings.TESTING, + enable_console_exporter=False, adaptive_sampling=settings.TRACING_ADAPTIVE_SAMPLING, )backend/tests/integration/test_user_settings_routes.py (2)
305-309: The_tickassertion is always true and provides no meaningful validation.The inner async function
_tickonly asserts that time is monotonically non-decreasing, which is virtually always true. Theeventuallycall with a 0.5s timeout is essentially just a 50ms sleep (first iteration succeeds immediately). If the intent is to ensure a time gap for restore point distinction, a simpleawait asyncio.sleep(0.05)would be clearer.♻️ Suggested simplification
- async def _tick() -> None: - now = datetime.now(timezone.utc) - assert (now - prev).total_seconds() >= 0 - - await eventually(_tick, timeout=0.5, interval=0.05) + import asyncio + await asyncio.sleep(0.05) # Ensure restore point timestamp is distinct
367-388: Test isolation assertion may produce false negatives.The assertion at lines 387-388 uses
orlogic, meaning the test passes if either theme or timezone differs. A newly created user might coincidentally have matching defaults for one of these values. Consider asserting that the second user's settings were not modified by checking against their original values before user1's update.♻️ Proposed fix for more robust isolation verification
+ # Get second user's original settings before any changes + response = await another_user.get("/api/v1/user/settings/") + assert response.status_code == 200 + user2_original = response.json() + # Update first user's settings user1_update = { "theme": "dark", "timezone": "America/New_York" } response = await test_user.put("/api/v1/user/settings/", json=user1_update) assert response.status_code == 200 # Get second user's settings response = await another_user.get("/api/v1/user/settings/") assert response.status_code == 200 user2_settings = response.json() - # Verify second user's settings are not affected by first user's changes - # Second user should have default settings, not the first user's custom settings - assert user2_settings["theme"] != user1_update["theme"] or user2_settings["timezone"] != user1_update[ - "timezone"] + # Verify second user's settings remain unchanged + assert user2_settings["theme"] == user2_original["theme"] + assert user2_settings["timezone"] == user2_original["timezone"]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/core/dishka_lifespan.pybackend/tests/conftest.pybackend/tests/integration/test_user_settings_routes.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/integration/test_user_settings_routes.py (2)
backend/tests/conftest.py (4)
app(47-58)test_user(142-153)client(69-77)another_user(172-183)backend/tests/helpers/eventually.py (1)
eventually(8-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Integration Tests
- GitHub Check: E2E Tests
🔇 Additional comments (11)
backend/app/core/dishka_lifespan.py (2)
17-17: LGTM: Import updated for DI-based Settings.The import change from
get_settingstoSettingscorrectly supports the new dependency injection pattern.
30-33: LGTM: Clean DI-based settings retrieval.Retrieving Settings and logger from the DI container is the correct approach and aligns with the PR's goal of centralizing configuration via dependency injection.
backend/tests/conftest.py (4)
23-42: Well-structured test isolation via Settings fixture.The
test_settingsfixture correctly creates isolated test environments per worker using a combination of session UUID and worker ID. The approach of usingmodel_copy(update=...)to override settings is clean and idiomatic for Pydantic v2.
98-104: Explicit Redis cleanup is good, but consider the fixture scope.The
finallyblock ensures the Redis client is closed properly, which is a solid improvement. However, thescopefixture (which provides the Redis client) is function-scoped whileredis_clientalso appears function-scoped. The explicitaclose()call may be redundant if the container scope already handles cleanup—but it's safer to have explicit cleanup, so this is fine.
141-153: LGTM! Clean authenticated client fixtures.The
test_user,test_admin, andanother_userfixtures follow a consistent pattern: generate unique credentials, create authenticated clients, and properly close them in the teardown phase. The use ofuuid.uuid4().hex[:8]for unique IDs is appropriate for test isolation.Also applies to: 156-168, 171-183
46-58: Session-scoped app fixture with proper cleanup.The docstring explains the rationale for session scope (avoiding Pydantic schema validator memory issues), and the cleanup correctly checks for the container before closing. This is well-designed.
backend/tests/integration/test_user_settings_routes.py (5)
11-35: Good use of TypedDict for payload typing.The TypedDicts (
_NotificationSettings,_EditorSettings,_UpdateSettingsData) provide clear type documentation for test payloads. Using underscore prefix indicates internal/private usage, which is appropriate.Note:
_UpdateSettingsDatausestotal=Falsemaking all fields optional, which aligns with partial update semantics.
37-38: Appropriate xdist grouping for stateful tests.Using
xdist_group(name="user_settings")ensures these tests run sequentially on a single worker, which is necessary given that some tests modify shared user state and verify persistence.
57-101: Comprehensive validation of settings structure.The test thoroughly validates the response structure including nested objects, value ranges, and optional fields. The conditional check for
languagefield (line 74) handles deployment variance gracefully.
103-161: Well-structured settings update test with proper assertions.The test validates both the update response and verifies all nested fields were correctly persisted. Using
_UpdateSettingsDatatype hint improves readability.
390-423: Good use of TypedDict for persistence test.Properly uses
_EditorSettingsand_UpdateSettingsDatato type the payloads, and the assertions correctly verify that settings were persisted across requests.
| async def _create_authenticated_client( | ||
| app: FastAPI, username: str, email: str, password: str, role: str | ||
| ) -> httpx.AsyncClient: | ||
| """Create and return an authenticated client with CSRF header set.""" | ||
| c = httpx.AsyncClient( | ||
| transport=ASGITransport(app=app), | ||
| base_url="https://test", | ||
| timeout=30.0, | ||
| follow_redirects=True, | ||
| ) | ||
| r = await c.post("/api/v1/auth/register", json={ | ||
| "username": username, | ||
| "email": email, | ||
| "password": password, | ||
| "role": role, | ||
| }) | ||
| if r.status_code not in (200, 201, 400): | ||
| await c.aclose() | ||
| pytest.fail(f"Cannot create {role} (status {r.status_code}): {r.text}") | ||
|
|
||
| @pytest.fixture | ||
| def test_admin_credentials(): | ||
| uid = uuid.uuid4().hex[:8] | ||
| return { | ||
| "username": f"admin_user_{uid}", | ||
| "email": f"admin_user_{uid}@example.com", | ||
| "password": "AdminPass123!", | ||
| "role": "admin", | ||
| } | ||
| login_resp = await c.post("/api/v1/auth/login", data={ | ||
| "username": username, | ||
| "password": password, | ||
| }) | ||
| login_resp.raise_for_status() | ||
| csrf = login_resp.json().get("csrf_token", "") | ||
| c.headers["X-CSRF-Token"] = csrf | ||
| return c |
There was a problem hiding this comment.
Missing error handling for login failures in authenticated client creation.
The function calls raise_for_status() on the login response (line 135), but if it fails, the client c is never closed, leading to a resource leak. This differs from the registration error handling at lines 127-129 which properly closes the client.
🐛 Proposed fix to ensure client cleanup on login failure
login_resp = await c.post("/api/v1/auth/login", data={
"username": username,
"password": password,
})
- login_resp.raise_for_status()
+ if login_resp.status_code >= 400:
+ await c.aclose()
+ pytest.fail(f"Login failed for {username} (status {login_resp.status_code}): {login_resp.text}")
csrf = login_resp.json().get("csrf_token", "")
c.headers["X-CSRF-Token"] = csrf
return c📝 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.
| async def _create_authenticated_client( | |
| app: FastAPI, username: str, email: str, password: str, role: str | |
| ) -> httpx.AsyncClient: | |
| """Create and return an authenticated client with CSRF header set.""" | |
| c = httpx.AsyncClient( | |
| transport=ASGITransport(app=app), | |
| base_url="https://test", | |
| timeout=30.0, | |
| follow_redirects=True, | |
| ) | |
| r = await c.post("/api/v1/auth/register", json={ | |
| "username": username, | |
| "email": email, | |
| "password": password, | |
| "role": role, | |
| }) | |
| if r.status_code not in (200, 201, 400): | |
| await c.aclose() | |
| pytest.fail(f"Cannot create {role} (status {r.status_code}): {r.text}") | |
| @pytest.fixture | |
| def test_admin_credentials(): | |
| uid = uuid.uuid4().hex[:8] | |
| return { | |
| "username": f"admin_user_{uid}", | |
| "email": f"admin_user_{uid}@example.com", | |
| "password": "AdminPass123!", | |
| "role": "admin", | |
| } | |
| login_resp = await c.post("/api/v1/auth/login", data={ | |
| "username": username, | |
| "password": password, | |
| }) | |
| login_resp.raise_for_status() | |
| csrf = login_resp.json().get("csrf_token", "") | |
| c.headers["X-CSRF-Token"] = csrf | |
| return c | |
| async def _create_authenticated_client( | |
| app: FastAPI, username: str, email: str, password: str, role: str | |
| ) -> httpx.AsyncClient: | |
| """Create and return an authenticated client with CSRF header set.""" | |
| c = httpx.AsyncClient( | |
| transport=ASGITransport(app=app), | |
| base_url="https://test", | |
| timeout=30.0, | |
| follow_redirects=True, | |
| ) | |
| r = await c.post("/api/v1/auth/register", json={ | |
| "username": username, | |
| "email": email, | |
| "password": password, | |
| "role": role, | |
| }) | |
| if r.status_code not in (200, 201, 400): | |
| await c.aclose() | |
| pytest.fail(f"Cannot create {role} (status {r.status_code}): {r.text}") | |
| login_resp = await c.post("/api/v1/auth/login", data={ | |
| "username": username, | |
| "password": password, | |
| }) | |
| if login_resp.status_code >= 400: | |
| await c.aclose() | |
| pytest.fail(f"Login failed for {username} (status {login_resp.status_code}): {login_resp.text}") | |
| csrf = login_resp.json().get("csrf_token", "") | |
| c.headers["X-CSRF-Token"] = csrf | |
| return c |
🤖 Prompt for AI Agents
In @backend/tests/conftest.py around lines 111 - 138, The
_create_authenticated_client function can leak the httpx.AsyncClient 'c' if
login_resp.raise_for_status() raises; ensure the client is closed on login
failures by catching the exception from the login request (or checking
login_resp.status_code similar to registration), calling await c.aclose() before
re-raising or failing the test, and only returning 'c' after a successful login
and CSRF extraction; target symbols: _create_authenticated_client, c,
login_resp, login_resp.raise_for_status(), and c.aclose().
Summary by cubic
Added CSRF protection middleware and expanded Settings-driven dependency injection across backend, events, and metrics to improve consistency and test stability. Tracing is gated in tests, metrics are skipped without an OTLP endpoint, Kafka/schema prefixes are standardized via Settings, and Kafka consumer tests run serially.
Bug Fixes
Refactors
Written for commit 726e2f9. Summary will update on new commits.
Summary by CodeRabbit
Refactor
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.