Codex/pr6 fix

[pahuldeepp]

Summary by CodeRabbit

• New Features

  • Subscription plan enforcement with quota/feature gates and billing grace handling.
  • Audit logging for device creation; improved Redis/cluster support and cross-pod circuit coordination.

• Infrastructure

  • New CI workflows: Chaos, E2E, Performance Budget, Terraform; DR and prod Terraform environments and remote state backend.
  • Kafka MirrorMaker connector and k6 performance script added.

• Bug Fixes

  • Stricter service responses, DB query timeouts, health check improvements, and safer error handling.

• Tests

  • Expanded Playwright E2E suites, new chaos experiments and orchestration, and CI test/ci targets.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds multiple CI/CD workflows (chaos, e2e, perf, terraform), a chaos test suite and Playwright E2E tests, DR Terraform modules and global DB/Redis modules, Redis cluster/universal-client support, gateway plan-enforcement middleware and audit hooks, cross-pod circuit sync via Redis, type/linting/dependency updates, DB migrations, and assorted app fixes.

Changes

Cohort / File(s)	Summary
GitHub Actions Workflows \.github/workflows/chaos.yml, \.github/workflows/e2e.yml, \.github/workflows/perf.yml, \.github/workflows/terraform.yml, \.github/workflows/cd.yml, \.github/workflows/ci.yml, \.github/workflows/security.yml	Added new chaos/e2e/perf/terraform workflows; updated CI toolchain versions, security scan/reporting steps, schedules, inputs, artifacts, and Slack failure notify.
Makefile Makefile	Introduced COMPOSE var; refactored docker-compose targets; added ci, test targets (test-go, test-gateway, test-dashboard, test-e2e), lint/typecheck/fix targets; expanded health checks.
Chaos test suite tests/chaos/*, tests/chaos/*.yaml, tests/chaos/*.sh	New chaos experiments, orchestration (run-all.sh), docs, scripts for pod-kill, kafka-consumer-pause, projection-lag, redis-outage, network-partition; CI integration and artifact upload.
Playwright E2E tests/e2e/playwright.config.ts, tests/e2e/*.spec.ts, tests/e2e/fixtures/*	Added Playwright config, Chromium/Firefox projects, mock-auth fixture and mocks, and test suites for auth, billing, devices; CI job builds static site and uploads HTML/JUnit artifacts.
Terraform — modules & environments infra/terraform/modules/aurora-global/main.tf, infra/terraform/modules/elasticache-global/main.tf, infra/terraform/environments/prod/*, infra/terraform/environments/dr/*, infra/terraform/backend.tf	New Aurora/ElastiCache global modules; prod and DR environment entrypoints/providers; S3/DynamoDB backend; DR consumes prod outputs.
Kafka DR connector infra/kafka/mirrormaker2-connector.json	Added MirrorMaker2 connector config to replicate selected topics between primary and DR Kafka clusters using SASL_SSL.
BFF — Redis & dev tooling apps/bff/src/datasources/redis.ts, apps/bff/package.json	Support Redis Cluster via REDIS_CLUSTER_NODES; getMany fetches concurrently with Promise.all; added ESLint/typecheck scripts and devDependencies.
BFF — circuit breaker apps/bff/src/lib/circuitBreaker.ts	Added best-effort Redis-backed syncFromRedis/syncToRedis to coordinate circuit state across pods; Redis errors are tolerated.
Gateway — plan enforcement & auditing apps/gateway/src/middleware/planEnforcement.ts, apps/gateway/src/server.ts, apps/gateway/src/services/device.ts, apps/gateway/src/routes/*	New Redis-cached tenant plan/device-count lookups, PLAN_LIMITS, middleware factories (subscription/quota/feature enforcement), invalidatePlanCache, audit logging on device create success/failure, and gRPC response validation; minor route error handling tweaks.
Read-model / saga / jobs-worker apps/read-model-builder/..., apps/saga-orchestrator/..., apps/jobs-worker/src/connection.ts	Broadened Redis client types to redis.UniversalClient; added RabbitMQ heartbeat/close logging; added saga status validator and marshal error handling; added DB query timeouts via withTimeout.
DB migrations & schema apps/telemetry-service/migrations/*	Added tenants.email and tenants.current_period_end; created alert_rules and bulk_import_jobs tables and indexes; updated down migrations to drop added objects safely.
Search indexer & ES writes apps/search-indexer/main.py	Telemetry indexing now upserts device state into DEVICE_INDEX and time-series telemetry into TELEMETRY_INDEX using composite IDs to avoid duplicates.
Load tests & perf scripts/load-tests/performance-budget.js, \.github/workflows/perf.yml	Added k6 performance-budget test script, CI job provisioning Postgres/Redis services, and artifact upload; thresholds for latency/error rates configured.
App typing, lint and misc fixes apps/bff/src/datasources/postgres.ts, apps/bff/src/datasources/elasticsearch.ts, apps/dashboard/*, libs/*, go.mod, .golangci.yml, libs/migrate/migrate.go	Tightened TypeScript/Go types, improved error typing/handling, lint config changes, dependency version updates, minor code cleanups (ingest buffer nolint, migrate DSN change).
Messaging & queue TTL apps/workflow-alerts/src/index.ts	Added x-message-ttl (24h) to ALERT_QUEUE declaration.
E2E dashboard mocks & Vite apps/dashboard/src/e2e/auth0Mock.tsx, apps/dashboard/vite.config.ts, apps/dashboard/index.html	Added Auth0 mock provider for E2E, conditional aliasing via VITE_E2E_MOCK_AUTH, and updated HTML title to "GrainGuard".

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Gateway
    participant Redis as Redis
    participant PgDB as PostgreSQL

    rect rgba(100, 200, 255, 0.5)
    Note over Client,PgDB: Gateway plan-enforcement and device create flow
    Client->>Gateway: POST /devices (tenant header)
    Gateway->>Redis: GET tenant_plan:{tenantId}
    alt cache hit
        Redis-->>Gateway: tenant plan
    else cache miss
        Gateway->>PgDB: SELECT tenants WHERE id=tenantId
        PgDB-->>Gateway: tenant row
        Gateway->>Redis: SET tenant_plan:{tenantId}
    end
    Gateway->>Redis: GET device_count:{tenantId}
    alt under quota
        Gateway->>Gateway: call device service (gRPC)
        Gateway-->>Client: 201 Created
    else quota exceeded
        Gateway-->>Client: 403 Quota exceeded
    end
    end

Loading

sequenceDiagram
    participant ChaosRunner
    participant Kubernetes
    participant Consumer as read-model-builder
    participant Kafka

    rect rgba(255, 150, 100, 0.5)
    Note over ChaosRunner,Kafka: Kafka consumer-pause experiment (chaos)
    ChaosRunner->>Kubernetes: scale deployment read-model-builder to 0
    Consumer--X Kafka: consumption paused
    ChaosRunner->>Kafka: measure consumer lag (via kafka-consumer-groups)
    ChaosRunner->>Kubernetes: scale deployment back to original replicas
    Consumer->>Kafka: resume consumption
    loop poll until deadline
        ChaosRunner->>Kafka: check lag < MAX_LAG
    end
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• feat(audit): add immutable audit logging for device operations #1: Related — adds audit logging infrastructure referenced by new writeAuditLog calls in apps/gateway/src/server.ts.
• feat(bff): add PgBouncer pooling, Redis cache with stampede protectio… #5: Related — modifies BFF datasource/Redis behavior similar to apps/bff/src/datasources/postgres.ts and apps/bff/src/datasources/redis.ts changes in this PR.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/pr6-fix

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[coderabbitai]

Actionable comments posted: 48

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (9)

apps/bff/src/lib/circuitBreaker.ts (1)

69-92: ⚠️ Potential issue | 🟠 Major

HALF_OPEN is still per-pod, not coordinated.

Once the timeout expires, every instance can flip itself from OPEN to HALF_OPEN and run fn() because that transition is never shared or reserved. In a multi-pod rollout this violates the “one request allowed” contract and can send a burst of recovery probes to Postgres. Use a shared lock/lease or publish HALF_OPEN before probing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/bff/src/lib/circuitBreaker.ts` around lines 69 - 92, The current execute
method flips this.state from "OPEN" to "HALF_OPEN" locally (after await
this.syncFromRedis()), allowing every pod to probe concurrently; change the
logic so the transition to HALF_OPEN is coordinated via Redis (e.g., acquire a
short-lived distributed lock/lease or atomically set a shared circuit-state key
with NX+TTL) before running fn(): attempt to atomically mark the circuit as
HALF_OPEN in Redis (or publish the HALF_OPEN state) and only proceed with the
probe if you successfully acquire the lease; if the lease acquisition fails,
keep throwing the OPEN error; ensure you still call this.syncFromRedis() after
the atomic update and release the lease/clear the HALF_OPEN mark on
success/failure so other instances can observe the new state (reference execute,
syncFromRedis, state, name, lastFailureTime, timeout).

apps/gateway/src/lib/audit.ts (1)

26-39: 🧹 Nitpick | 🔵 Trivial

Deduplicate AuditEventType literals to avoid drift.

The newly added device/webhook event literals are already present later in the same union. Keep each event literal in one place to avoid accidental divergence during future edits.

♻️ Proposed cleanup

   | "billing.portal_accessed"
-  | "device.created"
-  | "device.creation_failed"
-  | "device.registered"
-  | "webhook_endpoint.created"
   // API keys
   | "api_key.created"
   | "api_key.revoked"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/lib/audit.ts` around lines 26 - 39, AuditEventType contains
duplicate string literals (e.g., "device.created", "device.creation_failed",
"device.registered", "webhook_endpoint.created", "api_key.created",
"api_key.revoked", "device.deleted") which risks future drift; edit the
AuditEventType union to keep each event literal exactly once (remove the
repeated entries and optionally group related events together for readability),
ensure the final union includes all unique events, and run the TypeScript build
to verify no missing references.

apps/read-model-builder/internal/projection/device_projection.go (1)

91-96: ⚠️ Potential issue | 🟠 Major

Scope idempotency writes by tenant to prevent cross-tenant suppression.

Line 91 writes only event_id into processed_events. If IDs ever collide across tenants, one tenant’s event can suppress another tenant’s processing. Include tenant_id in this write and in the uniqueness key.

As per coding guidelines, "Verify tenant_id in all DB writes."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/read-model-builder/internal/projection/device_projection.go` around
lines 91 - 96, The INSERT into processed_events currently writes only event_id
which can cause cross-tenant suppression; update the SQL in device_projection.go
to insert both event_id and tenant_id (e.g., INSERT INTO
processed_events(event_id, tenant_id) VALUES ($1, $2) ...), pass
envelope.TenantId alongside envelope.EventId to the query, and adjust the Scan
target if you return both columns; also ensure the processed_events uniqueness
constraint/index and any migrations are updated to include tenant_id so
idempotency is scoped per tenant.

apps/read-model-builder/internal/projection/telemetry_projection.go (4)

301-307: ⚠️ Potential issue | 🟠 Major

Check rows.Err() after the first cursor is closed.

If the processed_events cursor stops on a decode/network error, newEventIDs becomes incomplete and the batch continues with a partially filtered event set. The second cursor already handles this correctly; the first one should too.

🛠️ Minimal fix

 		for rows.Next() {
 			var id string
 			if err := rows.Scan(&id); err == nil {
 				newEventIDs[id] = struct{}{}
 			}
 		}
 		rows.Close()
+		if err := rows.Err(); err != nil {
+			observability.EventsRetry.Inc()
+			return err
+		}

As per coding guidelines, "Check pgx rows.Err() after rows.Close()."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/read-model-builder/internal/projection/telemetry_projection.go` around
lines 301 - 307, The first rows iteration that fills newEventIDs (processing the
processed_events cursor) does not check rows.Err() after rows.Close(); update
the block that iterates over rows (the loop that scans into newEventIDs) to call
rows.Close() then check rows.Err() and handle/propagate the error the same way
the second cursor does (e.g., return the error or log and abort the batch) so
that decoding/network errors don't produce a partial newEventIDs set.

---

98-99: ⚠️ Potential issue | 🟠 Major

Keep the single-event writer tenant-scoped.

tenantID is accepted as-is here, and the history insert omits tenant_id entirely while the batch path requires and writes it. That makes the single-event path inconsistent with the rest of the projection logic and risks misattributed history rows.

🛠️ Minimal fix

 		tenantID := event.Data.TenantID
+		if tenantID == "" {
+			observability.EventsRetry.Inc()
+			return errors.New("missing tenantId")
+		}
@@
 		_, err = tx.Exec(
 			ctx,
 			`INSERT INTO device_telemetry_history
-			 (device_id, temperature, humidity, recorded_at)
-			 VALUES ($1, $2, $3, $4)`,
-			deviceID, temperature, humidity, recordedAt,
+			 (device_id, tenant_id, temperature, humidity, recorded_at)
+			 VALUES ($1, $2, $3, $4, $5)`,
+			deviceID, tenantID, temperature, humidity, recordedAt,
 		)

As per coding guidelines, "Verify tenant_id in all DB writes."

Also applies to: 154-160

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/read-model-builder/internal/projection/telemetry_projection.go` around
lines 98 - 99, The single-event write path currently reads tenantID from
event.Data.TenantID but omits tenant_id when inserting history, making it
inconsistent with the batch path; update the single-event insert in the
projection (where tenantID and event.Data.TenantID are referenced) to include
tenant_id in the INSERT payload and ensure the same tenantID value is explicitly
written and validated before DB writes (also apply the same change to the other
single-event block around the later insertion at the second occurrence
referenced in the comment); verify the code paths that build the history record
use the tenantID variable and perform a null/empty check prior to executing the
DB insert to satisfy "Verify tenant_id in all DB writes."

---

131-149: ⚠️ Potential issue | 🔴 Critical

Don't retry stale telemetry forever.

The ON CONFLICT ... WHERE EXCLUDED.recorded_at >= ... RETURNING version path legitimately returns no row for out-of-order samples. Treating that pgx.ErrNoRows as retryable means a stale event can be marked in processed_events and still keep getting requeued as a poison message.

---

409-436: ⚠️ Potential issue | 🟠 Major

Populate Redis from the deduped event set.

versions comes from dedupedEvents, but eventByDevice is built from newEvents. If an older event for the same device appears later in the batch, the cache payload can be older than the version you just wrote.

🛠️ Minimal fix

-		eventByDevice := make(map[uuid.UUID]parsedEvent, len(newEvents))
-		for _, e := range newEvents {
+		eventByDevice := make(map[uuid.UUID]parsedEvent, len(dedupedEvents))
+		for _, e := range dedupedEvents {
 			eventByDevice[e.deviceID] = e
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/read-model-builder/internal/projection/telemetry_projection.go` around
lines 409 - 436, The bug is that eventByDevice is populated from newEvents
causing a possible mismatch with versions (from dedupedEvents); instead, when
iterating versions in the Redis loop, find the matching parsedEvent by both
deviceID and version (e.g., search newEvents for an event with e.deviceID ==
v.deviceID && e.version == v.version) or build a lookup keyed by
deviceID+version before the Redis loop, then use that matched parsedEvent
(temperature, humidity, recordedAt) when marshalling cachePayload and writing
dataKey/versionKey; update references to eventByDevice to use the version-aware
lookup so the cached payload always matches the version written.

apps/read-model-builder/internal/consumer/message_handler.go (2)

30-37: 🧹 Nitpick | 🔵 Trivial

Consider logging unknown event types instead of silently ignoring.

Returning nil for unknown event types silently drops them. This could hide issues when new event types are added but handlers aren't updated. Consider adding observability:

♻️ Suggested improvement

+import "log/slog"
+
 		switch envelope.EventType {
 		case "telemetry.recorded":
 			return telemetryHandler(message)
 		case "device_created_v1":
 			return deviceHandler(message)
 		default:
+			slog.Debug("ignoring unknown event type", "type", envelope.EventType)
 			return nil
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/read-model-builder/internal/consumer/message_handler.go` around lines 30
- 37, The switch on envelope.EventType currently returns nil for unknown events,
silently dropping them; update the default case in message_handler.go to log the
unexpected event before returning by using the project's logger (include
envelope.EventType and any identifier from message/envelope such as an event ID)
so handlers like telemetryHandler and deviceHandler are not silently bypassed;
ensure the log is at an appropriate level (warn/info) and includes enough
context to trace the message.

---

21-21: ⚠️ Potential issue | 🔴 Critical

Pass context to projection handlers for proper timeout/cancellation handling.

The ctx parameter received in the handler function (lines 21, 32, 34) is not passed to telemetryHandler or deviceHandler. Both handlers currently only accept []byte and internally call context.Background(), ignoring caller context. Update handler signatures to accept context.Context so timeouts and cancellations from the consumer are properly respected.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/read-model-builder/internal/consumer/message_handler.go` at line 21, The
handler returned by the consumer currently ignores the incoming ctx when
invoking downstream handlers; change the signatures of telemetryHandler and
deviceHandler to accept a context.Context (e.g., telemetryHandler(ctx
context.Context, payload []byte) error and deviceHandler(ctx context.Context,
payload []byte) error), update their internal implementations to use the passed
ctx instead of context.Background(), and update all call sites in the returned
function (the func(ctx context.Context, message []byte) error) to pass the
incoming ctx into telemetryHandler and deviceHandler so timeouts/cancellations
are respected.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/chaos.yml:
- Around line 18-27: The workflow currently assigns NAMESPACE directly from
github.event.inputs.namespace (env: NAMESPACE: ${{ github.event.inputs.namespace
|| 'grainguard-dev' }}) allowing arbitrary destructive targets; change this to
validate the input against an explicit allowlist (e.g., a hard-coded array of
safe namespaces) before exporting NAMESPACE, and fail the run if the provided
name is not in the allowlist; implement the check in an early step (or use an
expressions-based mapping) that references the input and sets/echoes the
approved NAMESPACE variable only when it matches the allowed set, otherwise exit
with an error to prevent running pod-kill/network-partition against unapproved
namespaces.

In @.github/workflows/e2e.yml:
- Around line 40-44: The YAML linter flags extra spaces after colons in the
environment mapping lines (e.g., VITE_AUTH0_DOMAIN, VITE_AUTH0_CLIENT_ID,
VITE_AUTH0_AUDIENCE, VITE_BFF_URL, VITE_GATEWAY_URL and the lines at 57-59);
normalize spacing by removing extra spaces after each colon so each mapping uses
a single space (KEY: ${{ secrets.NAME }}), ensuring consistent alignment across
those entries to satisfy the linter.
- Around line 69-74: The JUnit artifact upload step ("Upload test results
(JUnit)" using actions/upload-artifact@v4) is missing the same retention and
missing-file handling as the HTML report step; update the step by adding the
with keys retention-days (set to the same number used elsewhere, e.g., 7) and
if-no-files-found (set to "ignore") alongside the existing name and path so the
step won't fail if tests produce no XML and artifacts are retained consistently.
- Around line 28-30: The CI step named "Install E2E deps" currently runs "npm
install --save-dev `@playwright/test` typescript ts-node" which can mutate
package.json; change it to use "npm ci" when a lockfile exists
(tests/e2e/package-lock.json or yarn.lock) for deterministic installs, and fall
back to plain "npm install `@playwright/test` typescript ts-node" (no --save-dev)
if no lockfile is present; keep the same step name and working-directory
("tests/e2e") and ensure the command selection is conditional in the workflow so
CI never modifies package.json.

In @.github/workflows/perf.yml:
- Around line 25-27: Fix the YAML spacing errors in the environment block by
removing extra spaces after the colons so each key: value pair uses a single
space (e.g., POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB) and apply the same
consistent formatting to the other affected env sections (lines around the block
referenced by the reviewer, e.g., the block covering lines 65-77) to satisfy the
linter.
- Around line 61-63: The background gateway and BFF processes started with "npx
ts-node src/server.ts &" (and the analogous "npx ts-node ... bff ... &") lack
cleanup on failure; add a teardown step that runs after artifacts are uploaded
with "if: always()" and invokes process termination commands (e.g., pkill -f
matching "ts-node src/server.ts" and the BFF command) so leftover processes are
killed (use || true to avoid failing the cleanup step); update the workflow to
include this cleanup step after the artifact upload step to ensure background
processes are removed on job failure or completion.

In @.github/workflows/terraform.yml:
- Around line 23-29: The Terraform workflow matrix is missing the "dr"
environment so changes under infra/terraform/environments/dr won't run
init/validate/plan; update the matrix.env array in the strategy section (the
"env: [dev, prod]" entry) to include "dr" so the defaults.run.working-directory
(infra/terraform/environments/${{ matrix.env }}) will execute for the dr
environment as well.

In `@apps/bff/src/lib/circuitBreaker.ts`:
- Around line 43-49: syncFromRedis() currently updates local state only when
shared.failureCount > this.failureCount, which prevents remote CLOSED
(failureCount=0) from overriding a local OPEN; modify the cache payload to
include a monotonic version/timestamp (e.g., add shared.version or reuse
shared.lastFailureTime as lastUpdated) and change syncFromRedis() (and any
publish/update code that writes the cache) to compare that timestamp/version
instead of raw failureCount so the most recent update always wins; update logic
in methods named syncFromRedis, the cache set/publish calls that write
`cb:${this.name}`, and the State/failureCount handling to accept remote state
when shared.version > this.version (or shared.lastUpdated > this.lastUpdated),
then apply shared.state, failureCount, and lastFailureTime accordingly.

In `@apps/gateway/src/middleware/planEnforcement.ts`:
- Around line 185-188: The middleware functions enforceDeviceQuota,
enforceBulkDeviceQuota, enforceAlertRuleQuota, and requireFeature currently use
a non-null assertion on req.user (e.g., req.user!.tenantId) which will throw if
unauthenticated; update each middleware to defensively validate req.user (same
pattern as requireActiveSubscription) before accessing tenantId—if req.user is
missing return a 401/appropriate error or call next with an authentication error
so tenantId access is safe and consistent across these handlers.
- Around line 134-141: The invalidatePlanCache function currently swallows Redis
errors; update its catch to log the error (including tenantId and the error
object) at a warn or debug level instead of ignoring it. Specifically, inside
invalidatePlanCache (which calls redis.del for `tenant_plan:${tenantId}` and
`device_count:${tenantId}`) catch the exception as e and call the project logger
(e.g., logger.warn or processLogger.warn) with a clear message like "Failed to
invalidate plan cache for tenant" plus tenantId and the error to aid debugging.
Ensure the log call is non-throwing and preserves the function's non-critical
behavior.
- Around line 77-81: Wrap the database call using pool.query inside a try-catch
in the planEnforcement middleware (the block using pool.query with tenantId that
selects plan, subscription_status, current_period_end from tenants) so any
connection/timeouts are caught; on catch, log the error with context and return
an appropriate HTTP error response (e.g., res.status(500).json or call
next(err)) so the request doesn't crash and callers get a clear error instead of
an unhandled rejection. Ensure the catch references the same symbols
(pool.query, tenantId) and preserves existing control flow when the query
succeeds.
- Around line 70-75: The try/catch around redis.get needs to also protect
JSON.parse so malformed cached payloads don't crash the request: inside the
existing try block (where you call redis.get(cacheKey) and currently do `if
(cached) return JSON.parse(cached) as TenantPlan`), wrap the JSON.parse step in
its own try/catch (or check with a safe parse helper) so parse errors are
caught, log or delete the invalid cache entry (using cacheKey and redis) and
fall through to load the TenantPlan from the DB; ensure references to redis.get,
cacheKey, JSON.parse and the TenantPlan return remain correct.
- Around line 145-151: The middleware requireActiveSubscription currently calls
next() when req.user?.tenantId is falsy, allowing unauthenticated requests to
silently bypass enforcement; update it to explicitly fail for missing
authentication by checking if (!req.user || !req.user.tenantId) and returning a
401 response (or next with an Unauthorized error) instead of next(), so behavior
matches other middleware that expects req.user!.tenantId and prevents silent
bypass; alternatively, if permissive behavior is intentional, add a clear
comment above requireActiveSubscription documenting that it allows
unauthenticated access.

In `@apps/gateway/src/server.ts`:
- Line 208: The audit meta currently persists String(err) (in the meta object
assignment near the request handling in server.ts), which may leak sensitive
details; implement a sanitizer (e.g., sanitizeAuditError or getAuditErrorMeta)
that accepts the thrown error and returns a safe { code: string, message: string
} pair (use err.code or map known error types to stable codes, derive a short
non-sensitive message, truncate to a safe length, and never include stack or
full error text), then replace meta: { serialNumber: req.body?.serialNumber,
error: String(err) } with meta: { serialNumber: req.body?.serialNumber, error:
sanitizeAuditError(err) } so only the sanitized code/message pair is persisted.
- Around line 191-200: The audit write calls to logAuditEvent inside the POST
/devices handler are not awaited so the response (res.json) can be returned
before audit persistence completes; update the handler to await the async
logAuditEvent calls (or use Promise.all when logging multiple events) before
sending the response, and catch/log any errors from logAuditEvent so failures
don't crash the request flow (refer to the logAuditEvent invocation and the
res.json(result) return in the POST /devices handler to locate the spots to
change).

In `@apps/jobs-worker/src/connection.ts`:
- Around line 22-28: The close handler currently always warns and leaves
connection/channel state set, causing false alarms and stale ch/conn; update the
connection close logic (the conn.on("close") handler) to differentiate planned
shutdowns by using an "isClosing" (or similar) flag set when you intentionally
close the connection, only log a warning if isClosing is false, and always clear
the shared ch and conn references (set ch = null and conn = null) on close so no
stale state remains; also ensure any channel-level close handlers clear channel
state similarly.
- Line 18: The amqplib heartbeat option is being passed as the second
(socket/TLS) argument to amqpConnect and thus ignored; update the call in
connection.ts (the amqpConnect invocation that assigns conn) to provide
heartbeat in the first argument instead — either append ?heartbeat=30 to
RABBITMQ_URL or replace RABBITMQ_URL with a connection object like { protocol,
hostname, port, username, password, heartbeat: 30 } and keep any TLS/socket
options as the second argument so the heartbeat is honored by RabbitMQ.

In `@apps/read-model-builder/cmd/main.go`:
- Around line 87-90: The current loop splitting clusterNodes and appending to
addrs can push empty strings (from trailing commas/spaces) causing invalid Redis
addresses; update the logic that iterates over strings.Split(clusterNodes, ",")
to trim each entry (strings.TrimSpace) and skip any empty results before
appending to addrs (so addrs only contains non-empty addresses) — this touches
the clusterNodes split loop and the addrs slice used later when constructing the
Redis cluster client.

In `@apps/read-model-builder/internal/projection/telemetry_projection.go`:
- Line 42: In HandleTelemetry, fix three issues: (1) In the single-event history
insert (the non-batch path) add tenant_id to the INSERT column list and include
tenant_id in the arg slice to match the batch path; (2) after closing the
processed_events cursor (the rows returned by the query/iterator), immediately
check rows.Err() and if non-nil call observability.EventsRetry.Inc() and return
the error; and (3) remove the duplicate nil-check for redisClient (the repeated
if redisClient == nil) so only the first check remains. Ensure you update the
same function names/variables used in the diff (HandleTelemetry, rows,
redisClient, observability.EventsRetry) so the fixes integrate cleanly.

In `@apps/saga-orchestrator/internal/orchestrator/provision_saga.go`:
- Around line 132-135: The error message for the json.Marshal call that's part
of the quota allocation step is incorrect: when marshalling cmd (cmdBytes,
detachErr := json.Marshal(cmd)) the wrapped error says "marshal
tenant.detach_device" but this branch is handling quota.allocate_device; update
the fmt.Errorf wrap to reference "quota.allocate_device" (e.g., return
fmt.Errorf("marshal quota.allocate_device command: %w", detachErr)) so the error
points to the correct operation (search for cmdBytes, detachErr or the allocate
step in provision_saga.go to locate the change).

In `@apps/saga-orchestrator/internal/repository/postgres_saga_repository.go`:
- Around line 17-23: IsEventProcessed and MarkEventProcessed perform DB
operations without using the withTimeout helper; update both functions to call
withTimeout(ctx) at start, replace uses of the original ctx with the returned
context for all DB calls, and defer the returned cancel function immediately
(e.g., defer cancel()) so queries are bounded by dbQueryTimeout; reference the
withTimeout function and the IsEventProcessed and MarkEventProcessed methods to
locate and apply this change.

In `@apps/search-indexer/main.py`:
- Around line 109-110: The current broad except block around the ES operations
only logs a generic "Telemetry index error" via log.error; split or wrap each
Elasticsearch operation (e.g., the telemetry indexing call and the device
indexing call) into its own try/except so you can log which operation failed,
and on failure call log.error with a clear message that includes the operation
name (e.g., "telemetry index" or "device index") and the exception details;
update the existing except Exception as e usage (and any log.error calls) to
include the operation context and the exception object to make debugging easier.
- Around line 92-108: The composite doc_id creation can yield "device:None" when
payload lacks recorded_at, causing silent overwrites; change the logic around
payload.get("recorded_at", '') to explicitly validate/normalize recorded_at
(e.g., recorded_at = payload.get("recorded_at") or str(uuid.uuid4())) and import
uuid, then build doc_id = f"{device_id}:{recorded_at}" and use that doc_id in
the existing self.es.update call to ensure unique IDs for events without
timestamps.

In `@apps/telemetry-service/migrations/000009_sso_alert_rules_bulk.up.sql`:
- Around line 23-24: Add SQL CHECK constraints to enforce the commented
invariants: constrain operator to only the allowed values (e.g. CHECK(operator
IN ('>','<','>=','<=','=='))), constrain status to the allowed status values
(e.g. CHECK(status IN (...))), and add numeric checks for import counters so
they are non-negative and do not exceed total_rows (e.g. CHECK(imported_rows >=
0 AND imported_rows <= total_rows) and similarly for other counter columns).
Apply these CHECKs to the table definitions that include the operator and status
columns and to the import counter/total_rows columns mentioned on lines 43-46 so
invalid writes from other services are rejected at the DB level.

In `@apps/workflow-alerts/src/index.ts`:
- Around line 40-44: The alert publisher lacks stable idempotency metadata:
update the publishAlert function to set a deterministic messageId (e.g., hash of
tenantId + alertId/timestamp or use alert.uniqueKey) and include tenant-scoped
headers (e.g., tenantId) on the published message so retries/DLQ replays can be
correlated; then update the alert consumer
(apps/jobs-worker/src/handlers/alert.ts) to check msg.properties.messageId (and
tenant header) against a processed-messages store/cache before invoking
processAlert, short-circuiting duplicates and marking messageIds as processed
after successful fan-out.

In `@infra/kafka/mirrormaker2-connector.json`:
- Around line 6-7: MirrorMaker2 is currently configured with
"source.cluster.alias": "primary" which causes mirrored topics to be prefixed
(e.g., primary.device.events) and will break existing consumers; change the
MirrorMaker2 config by setting "replication.policy.class" to the
IdentityReplicationPolicy (or equivalent class name for your MM2 version) so
original topic names are preserved instead of prefixing, or alternatively update
all consumer/producer references (e.g., apps/workflow-alerts/src/index.ts
expecting risk.scores and tools/publish-test-event/main.go or
scripts/test-producer/main.go expecting device.events) to use the prefixed
names; update the mirrormaker config that currently contains
"source.cluster.alias" and "target.cluster.alias" to include the
"replication.policy.class":
"org.apache.kafka.connect.mirror.IdentityReplicationPolicy" (or your platform's
identity policy) and restart the connector.

In `@infra/terraform/environments/prod/main.tf`:
- Line 8: Extract the duplicated VPC CIDR string into a Terraform local and
reference it from each module call: add a locals block defining vpc_cidr =
"10.1.0.0/16" and replace the inline "10.1.0.0/16" values passed to the modules
(the vpc_cidr argument used in the module blocks) with local.vpc_cidr; apply the
same replacement for all occurrences of vpc_cidr in this file to keep the value
consistent and easier to maintain.

In `@infra/terraform/modules/aurora-global/main.tf`:
- Around line 54-63: Add conditional force_destroy to the aws_rds_global_cluster
resource to allow non-prod tear-downs: inside resource "aws_rds_global_cluster"
"this" (which is only created when var.is_secondary is false), set force_destroy
= true when this is a non-production environment (use your existing env
indicator variable or add one) so that global cluster deletion is forced for
non-prod; ensure the condition references the same environment flag you use
elsewhere (e.g., var.environment or similar) and keep force_destroy = false for
prod to avoid accidental data loss.
- Around line 97-99: Update the misleading comment above the
aws_rds_cluster_instance "this" resource: replace the phrase "one writer, one
reader in primary" with a clarification that Aurora instances are not designated
writer/reader at creation time and that count = var.is_secondary ? 1 : 2 results
in two instances in the primary (one will be promoted to writer at runtime and
the other acts as a failover/read replica) and a single read replica in the
secondary; keep reference to the count expression (var.is_secondary) so future
readers understand the topology intent.
- Around line 15-19: The variable blocks in
infra/terraform/modules/aurora-global/main.tf use semicolons instead of the
required HCL `=` assignment operator (e.g., variable "db_password",
"instance_class", "engine_version", "is_secondary", "global_cluster_id"); update
each variable declaration to use `=` for attributes (type = string, sensitive =
true, default = "…", etc.) so Terraform HCL syntax is valid and `terraform
validate`/`init` will succeed.
- Around line 107-108: The RDS enhanced monitoring configuration sets
monitoring_interval > 0 but omits monitoring_role_arn, which is required; create
an IAM role/policy for RDS Enhanced Monitoring, expose a variable (e.g.,
var.monitoring_role_arn) and attach that role ARN to the DB instance resource by
setting monitoring_role_arn = var.monitoring_role_arn alongside
monitoring_interval and performance_insights_enabled, then ensure the module
caller can pass the role ARN (or create the role inside the module and reference
its aws_iam_role.arn).

In `@infra/terraform/modules/elasticache-global/main.tf`:
- Around line 26-31: The ingress rule currently opens port 6379 to var.vpc_cidr
(in the ingress block) which is too broad; change the rule to restrict access to
the Redis/ElastiCache client security groups instead of CIDR by replacing
cidr_blocks with the security group reference(s) (e.g., use security_groups or
security_group_ids pointing to a var.client_security_group_ids or the specific
aws_security_group resource(s)); ensure the module input
(var.client_security_group_ids) is defined as a list(string) and update any
callers to pass the client/service SGs so only those security groups can reach
port 6379 on the cache.

In `@Makefile`:
- Around line 117-118: The Makefile currently swallows any non-zero exit from
the golangci-lint step by using the fallback `|| echo "Install: ..."` after the
`golangci-lint run ./...` command; remove that behavior so real lint failures
fail CI — either delete the `|| echo "Install: ..."` fallback or replace it with
logic that prints the install hint but then exits non-zero (i.e., print the hint
and re-exit with status 1) so `golangci-lint run ./...` failures are propagated;
look for the `golangci-lint run ./...` invocation and its `|| echo "Install:
..."` fallback in the Makefile and change it accordingly.
- Around line 72-73: The test-e2e Makefile target currently runs Playwright from
apps/dashboard (test-e2e) and misses the new repo-level suite; decide whether to
run both suites or deprecate the dashboard suite. If both should run, update the
test-e2e recipe to invoke two runs: one using apps/dashboard's config (to keep
smoke.spec.ts) and one using the repo-level tests/e2e/playwright.config.ts (to
run auth.spec.ts, billing.spec.ts, devices.spec.ts) — reference the test-e2e
target, apps/dashboard playwright.config.ts and tests/e2e/playwright.config.ts;
if the dashboard suite is deprecated, replace the single command with a call to
npx playwright test -c tests/e2e/playwright.config.ts (removing reliance on
smoke.spec.ts).

In `@scripts/load-tests/performance-budget.js`:
- Around line 21-23: The script defines JWT (variable JWT) but continues running
when it's empty, causing the test to only hit /health; add a fail-fast check
near the top (after JWT is set) that validates JWT is non-empty and exits with a
clear error message and non-zero status if missing, so the authenticated gateway
call and BFF GraphQL query (the critical path exercised later) run only when JWT
is provided; if unauthenticated runs are desired, add an explicit mode flag
instead of allowing JWT to be empty.
- Around line 28-38: The health-check failures for the /health endpoint are not
being counted into the budget, so a failing health check doesn't increment
error_rate or total_errors and no threshold watches the check(); modify the
health-check logic (the function or block that performs the /health request and
the call to check()) to explicitly increment the existing total_errors and
error_rate counters when the /health response is non-200 or times out, and add a
dedicated threshold entry in the thresholds object (e.g.,
"health_check_failures": ["count==0"] or a tagged metric like
"error_rate{endpoint:health}": ["rate<0.01"]) so the threshold system will fail
the budget when /health failures occur. Ensure you reference and update the same
metric names used elsewhere (total_errors, error_rate, and thresholds) so the
new increments are counted by the budget.

In `@tests/chaos/kafka-consumer-pause.sh`:
- Around line 73-80: After calling scale_consumer for each entry in CONSUMERS,
do not immediately sleep; instead poll/wait until the consumer pods are actually
gone or not running before starting the PAUSE_SECONDS timer. Concretely: after
the loop that calls scale_consumer and logs scaling, query the cluster for pods
belonging to each consumer (use the same label/identifier that scale_consumer
targets) and wait until there are zero running pods (or they are in
Terminated/Succeeded) for all consumers (with a short timeout/retries), then log
and call sleep "$PAUSE_SECONDS". Keep references: CONSUMERS, scale_consumer,
PAUSE_SECONDS, and the existing log calls.
- Around line 31-35: The awk pipeline in the kafka-consumer-groups.sh invocation
uses $NF (last field) which picks CLIENT-ID/- instead of the LAG column; update
the awk expression in tests/chaos/kafka-consumer-pause.sh (the kubectl ...
kafka-consumer-groups.sh ... | awk ... line) to detect the header "LAG" and use
that column index for summing (i.e., compute the column number where the header
equals "LAG" on NR==1, then on NR>1 add that column if it matches a numeric
pattern, and print sum+0 in the END) instead of using $NF.

In `@tests/chaos/network-partition.yaml`:
- Around line 21-27: The steady-state-hypothesis currently only runs
rollout/status and sleeps and never verifies the claimed buffering/recovery
behavior; update the probes under steady-state-hypothesis (referencing the probe
list and the probe named telemetry-service-healthy) to include explicit checks
for producer buffering and consumer-lag recovery: add a probe that queries the
telemetry producer metric (e.g., producer_buffered_records or an HTTP/Prometheus
endpoint) to assert buffering occurred during the partition, and add a probe
that polls the Kafka consumer lag metric (consumer_lag) until it returns to
baseline within the configured 2-minute window with retries and clear failure
tolerance; ensure these probes replace or complement the sleeps and use existing
probe provider types (HTTP/command/prometheus) so the experiment fails if
buffering didn’t happen or lag doesn’t recover.
- Around line 58-64: Remove the unsupported "--stdin" flag from the kubectl
apply arguments: update the arguments list where "kubectl" is invoked with
["apply","-f","-","--stdin"] (the apply step in the YAML) to use only
["apply","-f","-"] so kubectl reads the manifest from stdin without the
unrecognized flag.

In `@tests/chaos/pod-kill.yaml`:
- Around line 65-79: The current kill actions (e.g., action name
"kill-gateway-pod") use `kubectl delete pod -l ...` which deletes all matching
pods; change them to resolve a single pod name from the label selector and
delete just that pod (for example: use `kubectl get pods -l app=gateway -n
"${namespace}"` to pick one pod and then `kubectl delete pod <pod-name>
--grace-period=0 --force`) so the experiment only kills one instance; apply the
same single-pod selection-and-delete fix to the other two kill actions
referenced in the file.

In `@tests/chaos/projection-lag.sh`:
- Around line 58-91: Before scaling down the read-model-builder, record its
current replica count (e.g., ORIGINAL_REPLICAS=$(kubectl get
deployment/read-model-builder -n "$NAMESPACE" -o jsonpath='{.spec.replicas}'))
and register a cleanup trap (trap restore_read_model_builder EXIT) that always
restores that recorded replica count; replace the hardcoded kubectl scale ...
--replicas=1 and subsequent rollout status with a function
(restore_read_model_builder) that uses kubectl scale
deployment/read-model-builder -n "$NAMESPACE" --replicas="$ORIGINAL_REPLICAS"
and waits with kubectl rollout status (and handles failures/errors), so the
script guarantees rollback to the original state even if the test errors or
exits early.

In `@tests/chaos/redis-outage.sh`:
- Around line 117-123: The warm-up check currently only logs elapsed_ms but does
not enforce the SLA; modify the block around http_check, t_start/t_end and
elapsed_ms to compare elapsed_ms against a configurable threshold (e.g.,
WARMUP_SLA_MS) and fail the script if exceeded: after computing elapsed_ms, call
a failure path (exit non-zero or invoke existing failure handler) when
elapsed_ms > WARMUP_SLA_MS, and include a clear error log via log showing the
measured time and the SLA; keep use of http_check and log but ensure the script
returns non-zero on SLA breach so slow recoveries fail the test.
- Around line 89-99: The panic log scan runs too early and uses a relative
--since window that can miss late failures or include pre-outage noise; move the
entire panic check block (the kubectl logs call that sets panic_count, the
comparison and the log "✓ saga-orchestrator" lines) so it executes after the
outage sleep completes (i.e., after computing and sleeping remaining_sleep), and
ensure remaining_sleep is computed safely (remaining_sleep=$(( OUTAGE_SECONDS -
15 )) and only slept when >0) so the kubectl logs call for panic_count uses
--since="${OUTAGE_SECONDS}s" against the full outage period; update variable
names (panic_count, NAMESPACE, OUTAGE_SECONDS) and the fail invocation
unchanged.

In `@tests/e2e/auth.spec.ts`:
- Around line 14-18: The test "protected route redirects to login" currently
only checks for a login button (loginBtn) after page.goto("/billing"), which
doesn't prove a redirect; either update the assertion to explicitly verify the
navigation (e.g., use page or expect(page) to check the URL contains or equals
the login route after calling page.goto("/billing")) or rename the test to
reflect that it only checks for an inline auth UI (e.g., "protected route shows
login UI"); modify the test body around the page.goto("/billing")/loginBtn and
expect calls accordingly so the behavior and the test name match.

In `@tests/e2e/billing.spec.ts`:
- Around line 29-33: The test named "Upgrade button for Starter exists and is
clickable" only checks visibility/enabled state; add an actual user interaction
by calling await upgradeBtn.click() and then assert the expected result of that
click (for example await expect(page).toHaveURL(/billing/) if it navigates, or
await expect(page.getByRole('dialog', { name: /upgrade/i })).toBeVisible() if it
opens a modal). Update the test around the upgradeBtn variable so the click and
one clear post-click assertion validate the interaction rather than just
presence.

In `@tests/e2e/devices.spec.ts`:
- Around line 75-79: The current wait (refreshRequest =
page.waitForResponse(...)) matches any POST to /graphql and is flaky; change the
predicate to assert the specific GraphQL operation triggered by the Refresh
button (e.g., check request.postData() or response.json() for the operationName
or the query string that corresponds to the refresh operation) so only the
Refresh's network response satisfies the wait before awaiting refreshBtn.click
and refreshRequest; update the predicate inside page.waitForResponse used to
create refreshRequest to look for that specific operation name or unique query
text.

In `@tests/e2e/fixtures/mockAuth.ts`:
- Around line 13-23: The fake JWT payload (PAYLOAD created via b64) hardcodes
the aud claim to "https://api.grainguard.com" causing mismatch with the
configurable audience; change the aud value to read the same configurable
audience used by the cache (VITE_AUTH0_AUDIENCE) so the seeded token and cache
key align. Locate the PAYLOAD constant and replace the hardcoded aud with the
runtime/config value (the same VITE_AUTH0_AUDIENCE used elsewhere), and ensure
any other places in this file that set aud (e.g., the similar entries around
lines ~30–32) are updated to use VITE_AUTH0_AUDIENCE as well. Ensure you import
or reference the existing config/env accessor used by the test setup rather than
hardcoding a string.

---

Outside diff comments:
In `@apps/bff/src/lib/circuitBreaker.ts`:
- Around line 69-92: The current execute method flips this.state from "OPEN" to
"HALF_OPEN" locally (after await this.syncFromRedis()), allowing every pod to
probe concurrently; change the logic so the transition to HALF_OPEN is
coordinated via Redis (e.g., acquire a short-lived distributed lock/lease or
atomically set a shared circuit-state key with NX+TTL) before running fn():
attempt to atomically mark the circuit as HALF_OPEN in Redis (or publish the
HALF_OPEN state) and only proceed with the probe if you successfully acquire the
lease; if the lease acquisition fails, keep throwing the OPEN error; ensure you
still call this.syncFromRedis() after the atomic update and release the
lease/clear the HALF_OPEN mark on success/failure so other instances can observe
the new state (reference execute, syncFromRedis, state, name, lastFailureTime,
timeout).

In `@apps/gateway/src/lib/audit.ts`:
- Around line 26-39: AuditEventType contains duplicate string literals (e.g.,
"device.created", "device.creation_failed", "device.registered",
"webhook_endpoint.created", "api_key.created", "api_key.revoked",
"device.deleted") which risks future drift; edit the AuditEventType union to
keep each event literal exactly once (remove the repeated entries and optionally
group related events together for readability), ensure the final union includes
all unique events, and run the TypeScript build to verify no missing references.

In `@apps/read-model-builder/internal/consumer/message_handler.go`:
- Around line 30-37: The switch on envelope.EventType currently returns nil for
unknown events, silently dropping them; update the default case in
message_handler.go to log the unexpected event before returning by using the
project's logger (include envelope.EventType and any identifier from
message/envelope such as an event ID) so handlers like telemetryHandler and
deviceHandler are not silently bypassed; ensure the log is at an appropriate
level (warn/info) and includes enough context to trace the message.
- Line 21: The handler returned by the consumer currently ignores the incoming
ctx when invoking downstream handlers; change the signatures of telemetryHandler
and deviceHandler to accept a context.Context (e.g., telemetryHandler(ctx
context.Context, payload []byte) error and deviceHandler(ctx context.Context,
payload []byte) error), update their internal implementations to use the passed
ctx instead of context.Background(), and update all call sites in the returned
function (the func(ctx context.Context, message []byte) error) to pass the
incoming ctx into telemetryHandler and deviceHandler so timeouts/cancellations
are respected.

In `@apps/read-model-builder/internal/projection/device_projection.go`:
- Around line 91-96: The INSERT into processed_events currently writes only
event_id which can cause cross-tenant suppression; update the SQL in
device_projection.go to insert both event_id and tenant_id (e.g., INSERT INTO
processed_events(event_id, tenant_id) VALUES ($1, $2) ...), pass
envelope.TenantId alongside envelope.EventId to the query, and adjust the Scan
target if you return both columns; also ensure the processed_events uniqueness
constraint/index and any migrations are updated to include tenant_id so
idempotency is scoped per tenant.

In `@apps/read-model-builder/internal/projection/telemetry_projection.go`:
- Around line 301-307: The first rows iteration that fills newEventIDs
(processing the processed_events cursor) does not check rows.Err() after
rows.Close(); update the block that iterates over rows (the loop that scans into
newEventIDs) to call rows.Close() then check rows.Err() and handle/propagate the
error the same way the second cursor does (e.g., return the error or log and
abort the batch) so that decoding/network errors don't produce a partial
newEventIDs set.
- Around line 98-99: The single-event write path currently reads tenantID from
event.Data.TenantID but omits tenant_id when inserting history, making it
inconsistent with the batch path; update the single-event insert in the
projection (where tenantID and event.Data.TenantID are referenced) to include
tenant_id in the INSERT payload and ensure the same tenantID value is explicitly
written and validated before DB writes (also apply the same change to the other
single-event block around the later insertion at the second occurrence
referenced in the comment); verify the code paths that build the history record
use the tenantID variable and perform a null/empty check prior to executing the
DB insert to satisfy "Verify tenant_id in all DB writes."
- Around line 409-436: The bug is that eventByDevice is populated from newEvents
causing a possible mismatch with versions (from dedupedEvents); instead, when
iterating versions in the Redis loop, find the matching parsedEvent by both
deviceID and version (e.g., search newEvents for an event with e.deviceID ==
v.deviceID && e.version == v.version) or build a lookup keyed by
deviceID+version before the Redis loop, then use that matched parsedEvent
(temperature, humidity, recordedAt) when marshalling cachePayload and writing
dataKey/versionKey; update references to eventByDevice to use the version-aware
lookup so the cached payload always matches the version written.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3c04aabb-58ff-4c3b-9cf3-ada723305c84

📥 Commits

Reviewing files that changed from the base of the PR and between 416c45d and 8a97319.

⛔ Files ignored due to path filters (4)

• apps/bff/package-lock.json is excluded by !**/package-lock.json
• apps/gateway/package-lock.json is excluded by !**/package-lock.json
• apps/jobs-worker/package-lock.json is excluded by !**/package-lock.json
• go.sum is excluded by !**/*.sum

📒 Files selected for processing (53)

• .github/workflows/chaos.yml
• .github/workflows/e2e.yml
• .github/workflows/perf.yml
• .github/workflows/terraform.yml
• Makefile
• apps/bff/package.json
• apps/bff/src/datasources/redis.ts
• apps/bff/src/lib/circuitBreaker.ts
• apps/gateway/package.json
• apps/gateway/src/lib/audit.ts
• apps/gateway/src/middleware/planEnforcement.ts
• apps/gateway/src/routes/teamMembers.ts
• apps/gateway/src/server.ts
• apps/gateway/src/services/device.ts
• apps/jobs-worker/src/connection.ts
• apps/read-model-builder/cmd/main.go
• apps/read-model-builder/internal/consumer/message_handler.go
• apps/read-model-builder/internal/projection/device_projection.go
• apps/read-model-builder/internal/projection/telemetry_projection.go
• apps/saga-orchestrator/internal/domain/saga.go
• apps/saga-orchestrator/internal/health/checker.go
• apps/saga-orchestrator/internal/orchestrator/provision_saga.go
• apps/saga-orchestrator/internal/repository/postgres_saga_repository.go
• apps/search-indexer/main.py
• apps/telemetry-service/migrations/000007_saas_columns.down.sql
• apps/telemetry-service/migrations/000007_saas_columns.up.sql
• apps/telemetry-service/migrations/000009_sso_alert_rules_bulk.down.sql
• apps/telemetry-service/migrations/000009_sso_alert_rules_bulk.up.sql
• apps/workflow-alerts/src/index.ts
• go.mod
• infra/kafka/mirrormaker2-connector.json
• infra/terraform/backend.tf
• infra/terraform/environments/dr/main.tf
• infra/terraform/environments/dr/providers.tf
• infra/terraform/environments/prod/main.tf
• infra/terraform/environments/prod/providers.tf
• infra/terraform/environments/prod/variables.tf
• infra/terraform/modules/aurora-global/main.tf
• infra/terraform/modules/elasticache-global/main.tf
• libs/health/health.go
• scripts/load-tests/performance-budget.js
• tests/chaos/README.md
• tests/chaos/kafka-consumer-pause.sh
• tests/chaos/network-partition.yaml
• tests/chaos/pod-kill.yaml
• tests/chaos/projection-lag.sh
• tests/chaos/redis-outage.sh
• tests/chaos/run-all.sh
• tests/e2e/auth.spec.ts
• tests/e2e/billing.spec.ts
• tests/e2e/devices.spec.ts
• tests/e2e/fixtures/mockAuth.ts
• tests/e2e/playwright.config.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Avoid --save-dev in CI; prefer npm ci if lockfile exists.

Using npm install --save-dev modifies package.json, which is undesirable in CI. If tests/e2e/package-lock.json exists, use npm ci for deterministic installs. Otherwise, drop --save-dev:

♻️ Suggested fix

       - name: Install E2E deps
-        run: npm install --save-dev `@playwright/test` typescript ts-node
+        run: npm ci
         working-directory: tests/e2e

Or if no lockfile exists:

       - name: Install E2E deps
-        run: npm install --save-dev `@playwright/test` typescript ts-node
+        run: npm install `@playwright/test` typescript ts-node
         working-directory: tests/e2e

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- name: Install E2E deps
	run: npm install --save-dev @playwright/test typescript ts-node
	working-directory: tests/e2e
	- name: Install E2E deps
	run: npm install `@playwright/test` typescript ts-node
	working-directory: tests/e2e

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/e2e.yml around lines 28 - 30, The CI step named "Install
E2E deps" currently runs "npm install --save-dev `@playwright/test` typescript
ts-node" which can mutate package.json; change it to use "npm ci" when a
lockfile exists (tests/e2e/package-lock.json or yarn.lock) for deterministic
installs, and fall back to plain "npm install `@playwright/test` typescript
ts-node" (no --save-dev) if no lockfile is present; keep the same step name and
working-directory ("tests/e2e") and ensure the command selection is conditional
in the workflow so CI never modifies package.json.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add retention-days and if-no-files-found for consistency.

The JUnit XML artifact upload is missing settings that the HTML report upload has. This could cause workflow failures if the file is missing:

♻️ Suggested fix

       - name: Upload test results (JUnit)
         uses: actions/upload-artifact@v4
         if: always()
         with:
           name: playwright-results-${{ github.run_number }}
           path: tests/e2e/playwright-results.xml
+          retention-days: 14
+          if-no-files-found: ignore

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- name: Upload test results (JUnit)
	uses: actions/upload-artifact@v4
	if: always()
	with:
	name: playwright-results-${{ github.run_number }}
	path: tests/e2e/playwright-results.xml
	- name: Upload test results (JUnit)
	uses: actions/upload-artifact@v4
	if: always()
	with:
	name: playwright-results-${{ github.run_number }}
	path: tests/e2e/playwright-results.xml
	retention-days: 14
	if-no-files-found: ignore

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/e2e.yml around lines 69 - 74, The JUnit artifact upload
step ("Upload test results (JUnit)" using actions/upload-artifact@v4) is missing
the same retention and missing-file handling as the HTML report step; update the
step by adding the with keys retention-days (set to the same number used
elsewhere, e.g., 7) and if-no-files-found (set to "ignore") alongside the
existing name and path so the step won't fail if tests produce no XML and
artifacts are retained consistently.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Warm-up SLA is not enforced, only observed.

The script claims a bounded warm-up check but currently never fails on slow recovery, so this can produce false passes.

Proposed fix

@@
 OUTAGE_SECONDS="${OUTAGE_SECONDS:-45}"
+MAX_WARMUP_MS="${MAX_WARMUP_MS:-30000}"
@@
 elapsed_ms=$(( (t_end - t_start) / 1000000 ))
 log "  Response time after restore: ${elapsed_ms}ms"
+(( elapsed_ms <= MAX_WARMUP_MS )) \
+  || fail "Cache warm-up exceeded ${MAX_WARMUP_MS}ms (actual: ${elapsed_ms}ms)"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/chaos/redis-outage.sh` around lines 117 - 123, The warm-up check
currently only logs elapsed_ms but does not enforce the SLA; modify the block
around http_check, t_start/t_end and elapsed_ms to compare elapsed_ms against a
configurable threshold (e.g., WARMUP_SLA_MS) and fail the script if exceeded:
after computing elapsed_ms, call a failure path (exit non-zero or invoke
existing failure handler) when elapsed_ms > WARMUP_SLA_MS, and include a clear
error log via log showing the measured time and the SLA; keep use of http_check
and log but ensure the script returns non-zero on SLA breach so slow recoveries
fail the test.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

“Clickable” is not verified in this test.

Line 29 says clickable, but the test only checks visibility/enabled state. Add an actual click (and a resulting assertion) so the test validates interaction, not just presence.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/billing.spec.ts` around lines 29 - 33, The test named "Upgrade
button for Starter exists and is clickable" only checks visibility/enabled
state; add an actual user interaction by calling await upgradeBtn.click() and
then assert the expected result of that click (for example await
expect(page).toHaveURL(/billing/) if it navigates, or await
expect(page.getByRole('dialog', { name: /upgrade/i })).toBeVisible() if it opens
a modal). Update the test around the upgradeBtn variable so the click and one
clear post-click assertion validate the interaction rather than just presence.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Invalid Terraform variable syntax: use = instead of ;.

Terraform HCL uses = for attribute assignment, not ;. These variable declarations will fail during terraform init or terraform validate.

🐛 Proposed fix

-variable "db_password"      { type = string; sensitive = true }
-variable "instance_class"   { type = string; default = "db.r6g.large" }
-variable "engine_version"   { type = string; default = "15.4" }
-variable "is_secondary"     { type = bool; default = false }
-variable "global_cluster_id"{ type = string; default = "" }  # required when is_secondary=true
+variable "db_password" {
+  type      = string
+  sensitive = true
+}
+variable "instance_class" {
+  type    = string
+  default = "db.r6g.large"
+}
+variable "engine_version" {
+  type    = string
+  default = "15.4"
+}
+variable "is_secondary" {
+  type    = bool
+  default = false
+}
+variable "global_cluster_id" {
+  type    = string
+  default = ""  # required when is_secondary=true
+}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	variable "db_password" { type = string; sensitive = true }
	variable "instance_class" { type = string; default = "db.r6g.large" }
	variable "engine_version" { type = string; default = "15.4" }
	variable "is_secondary" { type = bool; default = false }
	variable "global_cluster_id"{ type = string; default = "" } # required when is_secondary=true
	variable "db_password" {
	type = string
	sensitive = true
	}
	variable "instance_class" {
	type = string
	default = "db.r6g.large"
	}
	variable "engine_version" {
	type = string
	default = "15.4"
	}
	variable "is_secondary" {
	type = bool
	default = false
	}
	variable "global_cluster_id" {
	type = string
	default = "" # required when is_secondary=true
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@infra/terraform/modules/aurora-global/main.tf` around lines 15 - 19, The
variable blocks in infra/terraform/modules/aurora-global/main.tf use semicolons
instead of the required HCL `=` assignment operator (e.g., variable
"db_password", "instance_class", "engine_version", "is_secondary",
"global_cluster_id"); update each variable declaration to use `=` for attributes
(type = string, sensitive = true, default = "…", etc.) so Terraform HCL syntax
is valid and `terraform validate`/`init` will succeed.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Comment inaccurately describes instance topology.

The comment states "one writer, one reader in primary" but Aurora doesn't designate instances as writer/reader at creation time. Aurora automatically promotes one instance as writer. With count = 2, you get two instances where one becomes writer and one becomes reader (failover target). With count = 1 in DR, you only get one read replica, which is correct for a secondary region.

Consider clarifying the comment to avoid confusion during maintenance.

📝 Suggested clarification

-# ── DB instances (one writer, one reader in primary; one reader in secondary) ─
+# ── DB instances (primary: 2 instances for HA failover; secondary: 1 read replica) ─

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	# ── DB instances (one writer, one reader in primary; one reader in secondary) ─
	resource "aws_rds_cluster_instance" "this" {
	count = var.is_secondary ? 1 : 2 # primary: 1 writer + 1 reader; DR: 1 reader
	# ── DB instances (primary: 2 instances for HA failover; secondary: 1 read replica) ─
	resource "aws_rds_cluster_instance" "this" {
	count = var.is_secondary ? 1 : 2 # primary: 1 writer + 1 reader; DR: 1 reader

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@infra/terraform/modules/aurora-global/main.tf` around lines 97 - 99, Update
the misleading comment above the aws_rds_cluster_instance "this" resource:
replace the phrase "one writer, one reader in primary" with a clarification that
Aurora instances are not designated writer/reader at creation time and that
count = var.is_secondary ? 1 : 2 results in two instances in the primary (one
will be promoted to writer at runtime and the other acts as a failover/read
replica) and a single read replica in the secondary; keep reference to the count
expression (var.is_secondary) so future readers understand the topology intent.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Missing monitoring_role_arn for Enhanced Monitoring.

When monitoring_interval is set to a value greater than 0, AWS requires a monitoring_role_arn that grants RDS permissions to publish Enhanced Monitoring metrics to CloudWatch Logs. Without it, the cluster instance creation will fail.

🐛 Proposed fix: add IAM role for Enhanced Monitoring

Add a variable and IAM role, then reference it in the instance:

+variable "monitoring_role_arn" {
+  type        = string
+  default     = ""
+  description = "ARN of IAM role for RDS Enhanced Monitoring. If empty, a role will be created."
+}
+
+resource "aws_iam_role" "rds_monitoring" {
+  count = var.monitoring_role_arn == "" ? 1 : 0
+  name  = "${local.name}-rds-monitoring"
+
+  assume_role_policy = jsonencode({
+    Version = "2012-10-17"
+    Statement = [{
+      Action = "sts:AssumeRole"
+      Effect = "Allow"
+      Principal = {
+        Service = "monitoring.rds.amazonaws.com"
+      }
+    }]
+  })
+}
+
+resource "aws_iam_role_policy_attachment" "rds_monitoring" {
+  count      = var.monitoring_role_arn == "" ? 1 : 0
+  role       = aws_iam_role.rds_monitoring[0].name
+  policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonRDSEnhancedMonitoringRole"
+}

 resource "aws_rds_cluster_instance" "this" {
   count = var.is_secondary ? 1 : 2

   identifier         = "${local.name}-aurora-${count.index}"
   cluster_identifier = aws_rds_cluster.this.id
   instance_class     = var.instance_class
   engine             = "aurora-postgresql"
   engine_version     = var.engine_version

   performance_insights_enabled = true
   monitoring_interval          = 60
+  monitoring_role_arn          = var.monitoring_role_arn != "" ? var.monitoring_role_arn : aws_iam_role.rds_monitoring[0].arn

   tags = { Name = "${local.name}-aurora-${count.index}" }
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	performance_insights_enabled = true
	monitoring_interval = 60 # Enhanced Monitoring: 1-min granularity
	variable "monitoring_role_arn" {
	type = string
	default = ""
	description = "ARN of IAM role for RDS Enhanced Monitoring. If empty, a role will be created."
	}
	resource "aws_iam_role" "rds_monitoring" {
	count = var.monitoring_role_arn == "" ? 1 : 0
	name = "${local.name}-rds-monitoring"
	assume_role_policy = jsonencode({
	Version = "2012-10-17"
	Statement = [{
	Action = "sts:AssumeRole"
	Effect = "Allow"
	Principal = {
	Service = "monitoring.rds.amazonaws.com"
	}
	}]
	})
	}
	resource "aws_iam_role_policy_attachment" "rds_monitoring" {
	count = var.monitoring_role_arn == "" ? 1 : 0
	role = aws_iam_role.rds_monitoring[0].name
	policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonRDSEnhancedMonitoringRole"
	}
	resource "aws_rds_cluster_instance" "this" {
	count = var.is_secondary ? 1 : 2
	identifier = "${local.name}-aurora-${count.index}"
	cluster_identifier = aws_rds_cluster.this.id
	instance_class = var.instance_class
	engine = "aurora-postgresql"
	engine_version = var.engine_version
	performance_insights_enabled = true
	monitoring_interval = 60 # Enhanced Monitoring: 1-min granularity
	monitoring_role_arn = var.monitoring_role_arn != "" ? var.monitoring_role_arn : aws_iam_role.rds_monitoring[0].arn
	tags = { Name = "${local.name}-aurora-${count.index}" }
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@infra/terraform/modules/aurora-global/main.tf` around lines 107 - 108, The
RDS enhanced monitoring configuration sets monitoring_interval > 0 but omits
monitoring_role_arn, which is required; create an IAM role/policy for RDS
Enhanced Monitoring, expose a variable (e.g., var.monitoring_role_arn) and
attach that role ARN to the DB instance resource by setting monitoring_role_arn
= var.monitoring_role_arn alongside monitoring_interval and
performance_insights_enabled, then ensure the module caller can pass the role
ARN (or create the role inside the module and reference its aws_iam_role.arn).

[coderabbitai]

Actionable comments posted: 19

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

apps/saga-orchestrator/internal/orchestrator/provision_saga.go (1)

179-179: ⚠️ Potential issue | 🟡 Minor

Marshal error silently discarded in compensation path.

json.Marshal error is ignored here, which is inconsistent with the error handling added elsewhere in this PR. While marshal failures on simple maps are rare, discarding the error could result in publishing empty or malformed command bytes to Kafka.

Proposed fix

-	cmdBytes, _ := json.Marshal(cmd)
+	cmdBytes, cmdErr := json.Marshal(cmd)
+	if cmdErr != nil {
+		return fmt.Errorf("marshal tenant.detach_device command: %w", cmdErr)
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/saga-orchestrator/internal/orchestrator/provision_saga.go` at line 179,
The json.Marshal call for cmd currently ignores its error (cmdBytes, _ :=
json.Marshal(cmd)), which can produce malformed/empty payloads in the
compensation path; change this to capture and handle the error (e.g., err :=
json.Marshal(cmd) -> cmdBytes, err := json.Marshal(cmd)) and then propagate or
return/log a wrapped error consistent with the rest of this PR before attempting
to publish (handle in the same function that constructs/publishes the
compensation message where cmd and cmdBytes are used).

apps/gateway/src/routes/devicesImport.ts (1)

136-142: ⚠️ Potential issue | 🟠 Major

Use a transaction client for the bulk import operation, not pool-level queries.

The job record is inserted with pool.query() at line 136–142, and the status is updated at lines 173–178, but the entire device processing loop (lines 148–170) runs outside any transaction. If the server crashes mid-import, the job remains in "running" status with no recovery mechanism.

Per coding guidelines, all mutating routes must use a real transaction client with explicit BEGIN/COMMIT/ROLLBACK. Other routes in this codebase (e.g., tenants.ts, teamMembers.ts) follow this pattern; devicesImportRouter should too.

Consider wrapping the entire operation in a single transaction, or if that causes locking issues with large imports, add a cleanup job that flags stale "running" entries after a timeout.

Current pattern (problematic)

const jobRow = await pool.query(
  `INSERT INTO bulk_import_jobs (tenant_id, created_by, total_rows, status)
   VALUES ($1, $2, $3, 'running')
   RETURNING id`,
  [tenantId, userId, total]
);
const jobId = jobRow.rows[0].id;
// ... device loop runs here, outside transaction ...
await pool.query(
  `UPDATE bulk_import_jobs
     SET status = $1, completed_at = NOW(), success_rows = $2, failed_rows = $3
   WHERE id = $4`,
  [...]
);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/routes/devicesImport.ts` around lines 136 - 142, The INSERT
using pool.query and the device-processing loop must run inside a real
transaction client instead of pool-level queries: acquire a client via
pool.connect(), call await client.query('BEGIN'), replace the INSERT into
bulk_import_jobs (jobRow/jobId), all device upserts/processing queries in the
loop, and the final UPDATE of bulk_import_jobs to use client.query, then COMMIT
on success or ROLLBACK in the catch and always client.release() in finally;
ensure errors set status appropriately on rollback; if a single transaction is
impractical for very large imports, keep the job insert/update in a transaction
per logical unit and add/implement a cleanup job to mark stale "running"
bulk_import_jobs after a timeout.

♻️ Duplicate comments (3)

apps/saga-orchestrator/internal/orchestrator/provision_saga.go (1)

132-135: ⚠️ Potential issue | 🟡 Minor

Error message references wrong command type.

The variable is named detachErr and the error message says "marshal tenant.detach_device command", but this code is marshaling the quota.allocate_device command. This will mislead debugging efforts.

Proposed fix

-	cmdBytes, detachErr := json.Marshal(cmd)
-	if detachErr != nil {
-		return fmt.Errorf("marshal tenant.detach_device command: %w", detachErr)
+	cmdBytes, cmdErr := json.Marshal(cmd)
+	if cmdErr != nil {
+		return fmt.Errorf("marshal quota.allocate_device command: %w", cmdErr)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/saga-orchestrator/internal/orchestrator/provision_saga.go` around lines
132 - 135, The error message and variable name are misleading: when marshaling
the quota.allocate_device command the code uses detachErr and logs "marshal
tenant.detach_device command"; update the error handling in provision_saga.go to
reflect the actual operation by renaming detachErr to a clearer name (e.g.,
marshalErr or allocErr) and change the fmt.Errorf message to reference "marshal
quota.allocate_device command" (update the error wrap in the same stmt where
cmdBytes, <var> := json.Marshal(cmd) is called, so callers and logs correctly
reflect quota.allocate_device).

apps/saga-orchestrator/internal/repository/postgres_saga_repository.go (1)

102-119: ⚠️ Potential issue | 🟠 Major

Timeout hardening is still incomplete for IsEventProcessed and MarkEventProcessed.

These two methods perform unbounded database calls without using the withTimeout helper, leaving them vulnerable to hanging indefinitely if the database becomes unresponsive.

Proposed fix

 func (r *PostgresSagaRepository) IsEventProcessed(ctx context.Context, eventID string) (bool, error) {
+    ctx, cancel := withTimeout(ctx)
+    defer cancel()
     var exists bool
     err := r.pool.QueryRow(ctx, `
         SELECT EXISTS(SELECT 1 FROM saga_processed_events WHERE event_id = $1)
     `, eventID).Scan(&exists)
     return exists, err
 }

 func (r *PostgresSagaRepository) MarkEventProcessed(ctx context.Context, eventID string, sagaID uuid.UUID) error {
+    ctx, cancel := withTimeout(ctx)
+    defer cancel()
     _, err := r.pool.Exec(ctx, `
         INSERT INTO saga_processed_events (event_id, saga_id)
         VALUES ($1, $2)
         ON CONFLICT (event_id) DO NOTHING
     `, eventID, sagaID)
     return err
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/saga-orchestrator/internal/repository/postgres_saga_repository.go`
around lines 102 - 119, Both IsEventProcessed and MarkEventProcessed perform
unbounded DB calls; wrap their calls with the existing withTimeout helper to
avoid hanging: in IsEventProcessed call withTimeout(ctx) to get ctxTimed, defer
cancel(), then use r.pool.QueryRow(ctxTimed, ...) and Scan; in
MarkEventProcessed call withTimeout(ctx), defer cancel(), then use
r.pool.Exec(ctxTimed, ...) and return the error; ensure you call the returned
cancel function in both methods.

scripts/load-tests/performance-budget.js (1)

74-76: ⚠️ Potential issue | 🟠 Major

Count /health failures in the budget.

This request runs on every iteration, but a 500/timeout here never touches errorRate or totalErrors, so the thresholds can stay green while the gateway health check is red.

🔧 Suggested change

   const healthRes = http.get(`${GATEWAY_URL}/health`, { tags: { endpoint: "gateway" } });
-  check(healthRes, { "gateway /health 200": (r) => r.status === 200 });
+  const healthOk = check(healthRes, { "gateway /health 200": (r) => r.status === 200 });
+  if (!healthOk) {
+    errorRate.add(1);
+    totalErrors.add(1);
+  } else {
+    errorRate.add(0);
+  }
   gatewayP95.add(healthRes.timings.duration);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/load-tests/performance-budget.js` around lines 74 - 76, The /health
call results (healthRes) are not contributing to the error metrics, so update
the health check block to record failures into the existing error tracking
(e.g., errorRate and totalErrors) whenever the check fails or healthRes.status
!== 200 (including timeouts or 5xx). Keep the existing check() and
gatewayP95.add(healthRes.timings.duration) but add logic that evaluates the
check result or status and increments errorRate and totalErrors accordingly so
gateway /health failures are counted in the budget.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 81-87: The ESLint job ("ESLint" step) must fail if the target
package (matrix.app) has no lint script rather than silently skip; update the
step that currently runs a conditional "if npm run | grep -qE ..." so that when
no lint script is found it exits non‑zero (or explicitly checks an allowlist of
exempt apps from the matrix) instead of echoing and succeeding. In practice
modify the "ESLint" run logic to detect absence of the lint script for
matrix.app and call exit 1 (or assert matrix.app is in an explicit exemption
list) so the CI shows a failing job when a lint script is missing or renamed.
- Around line 26-30: The CI uses golangci-lint Action with "version: latest",
which makes linting non-reproducible; change the "version" field in the
golangci-lint step (the golangci/golangci-lint-action@v6 usage) from "latest" to
a specific released linter tag (e.g., "v2.11.4") so the linter binary is pinned,
and configure dependency automation (Dependabot or similar) to update that
pinned version going forward.

In @.github/workflows/perf.yml:
- Around line 121-123: The "Run performance budget" job step uses
continue-on-error: true which masks k6 failures and disables the budget gate;
remove the continue-on-error setting (or set it to false) from the "Run
performance budget" step so a non-zero k6 exit will fail the workflow and block
regressions, leaving the artifact upload step's if: always() intact so artifacts
still upload after a failing k6 run.
- Around line 91-95: The workflow sets POSTGRES_* env vars but the BFF
datasource (uses READ_DATABASE_URL / READ_DB_* in
apps/bff/src/datasources/postgres.ts) reads the read-replica variables, so
Postgres connections fall back to postgres-read/grainguard_read; update the
workflow to export the read-replica environment variables instead of (or in
addition to) POSTGRES_* — specifically set READ_DATABASE_URL or the
READ_DB_HOST, READ_DB_PORT, READ_DB_USER, READ_DB_PASSWORD, and READ_DB_NAME
variables to match the running container so the BFF's datasource (which expects
READ_DB_*) picks up the correct DB config.
- Around line 64-69: The workflow sets DATABASE_URL and REDIS_URL which the
gateway ignores; update the env names to the keys the gateway code expects:
replace DATABASE_URL with READ_DATABASE_URL and also set WRITE_DATABASE_URL (or
the WRITE_DB_* variants used in apps/gateway/src/database/db.ts) pointing to the
local Postgres container, and replace REDIS_URL with REDIS_HOST and REDIS_PORT
(per apps/gateway/src/cache/redis.ts) with the host and port of the Redis
service; ensure the values match the existing container/service names (e.g.,
host "localhost"/service name and ports 5432/6379) so the gateway picks up the
intended local services.

In @.github/workflows/security.yml:
- Around line 42-51: The Trivy step "Run Trivy vulnerability scan" currently
combines continue-on-error: true and exit-code: "0", which disables blocking for
CRITICAL/HIGH findings; update the workflow to enforce failures by either
removing continue-on-error and setting exit-code: "1" (or omitting exit-code so
the action fails normally) for aquasecurity/trivy-action@master, and if needed
add a .trivyignore to accept known risks, or alternatively add a clear
comment/docstring in the job explaining the intentional risk acceptance if you
truly want non-blocking behavior.

In @.github/workflows/terraform.yml:
- Around line 100-108: The "apply" job currently targets only the dev
environment (job name: apply, environment: dev, working-directory:
infra/terraform/environments/dev); either add equivalent GitHub Actions jobs for
prod and dr with appropriate branch/ref conditions and environment protection
rules (e.g., separate apply-prod and apply-dr jobs conditioned on
refs/heads/master or protected-release branches and using environment: prod and
environment: dr) or, if production/dr deploys are intentionally manual, add a
clear comment above the apply job documenting the deployment strategy and why
prod/dr are excluded so future maintainers understand the workflow.
- Around line 38-46: The step "Check required Terraform secrets" currently
interpolates secrets inline in the shell which can break with special
characters; update the step (id: secrets) to export the secrets into environment
variables (e.g., via the step's env: AWS_TF_ROLE_ARN: ${{
secrets.AWS_TF_ROLE_ARN }}, TF_VAR_DB_PASSWORD: ${{ secrets.TF_VAR_DB_PASSWORD
}}) and then perform the existence check using those env vars (if [ -n
"$AWS_TF_ROLE_ARN" ] && [ -n "$TF_VAR_DB_PASSWORD" ]; then ... ) and write the
result to GITHUB_OUTPUT as before; this avoids direct inline interpolation and
handles special characters safely.

In @.golangci.yml:
- Around line 5-12: Re-enable the errcheck linter by removing or undoing its
exclusion in .golangci.yml (add "errcheck" under enable) and then fix the
ignored error returns flagged by errcheck: handle the error returned by defer
writer.Close() in apps/ingest-service/main.go (check and log/return on close
error), capture and check the error from json.Marshal (replace resp, _ :=
json.Marshal(...) with proper err handling), and check the return values from
w.Write(...) calls (handle write errors and propagate or log). Run golangci-lint
to find any additional unchecked errors and either handle them or add narrowly
scoped exceptions with comments.

In `@apps/dashboard/src/features/settings/SettingsPage.tsx`:
- Around line 45-47: The catch block in the loadAll function drops the error
object; modify the catch to accept the error (e.g., catch (error)) and log or
report it before showing the user toast so diagnostics are preserved—use the
existing logger or console.error/error reporting utility and include the error
message/stack, then call toast.error("Failed to load account info") as before;
update the catch in SettingsPage.tsx's loadAll function accordingly.
- Around line 212-236: The toggle buttons in SettingsPage are missing accessible
switch semantics; update the mapped button (inside the array.map that references
notifPrefs, typedKey, savingPrefs and calls handleSavePrefs) to expose
role="switch" and an aria-checked attribute bound to the current enabled
boolean, and provide an accessible label via aria-label or aria-labelledby (use
the same label string used in the <p> for consistency); keep the disabled prop
but also reflect it with aria-disabled when savingPrefs is true so assistive
tech and RTL queries can reliably detect state and label.
- Around line 212-214: The inline array of toggles is using a string assertion
("as TogglePrefKey") which bypasses compile-time checks; instead define the
toggles as a typed constant (e.g., const toggleItems: ToggleItem[] = [ ... ])
where ToggleItem has key: TogglePrefKey, label, desc, then map over toggleItems
and use the typed key directly (remove the `as TogglePrefKey` cast) so
TypeScript will catch any invalid keys when referencing notifPrefs[typedKey] in
the SettingsPage rendering.

In `@apps/dashboard/src/lib/auth0.ts`:
- Around line 33-35: The module currently assigns window.__getToken at top-level
which can throw in SSR/test runtimes; change the DEV-only assignment for
getAccessTokenSilently to first verify a browser global exists (e.g., typeof
window !== 'undefined' or typeof globalThis !== 'undefined' and window is
defined) before writing to window; update the block that sets window.__getToken
to run only when import.meta.env.DEV is true AND the window/globalThis check
passes so getAccessTokenSilently is only attached in real browser environments.
- Around line 1-4: Replace the generic aliases and wrapper types with Auth0's
real types: change TokenOptions -> GetTokenSilentlyOptions and RedirectOptions
-> RedirectLoginOptions<AppState>, and update GetTokenFn to reflect the overload
of getAccessTokenSilently so it can return either Promise<string> or
Promise<TokenEndpointResponse> when called with { detailedResponse: true };
likewise ensure LoginWithRedirectFn uses RedirectLoginOptions<AppState>.
Reference the existing symbols TokenOptions, RedirectOptions, GetTokenFn,
LoginWithRedirectFn and the Auth0 types GetTokenSilentlyOptions,
RedirectLoginOptions<AppState>, and TokenEndpointResponse when making the
replacements.

In `@apps/gateway/src/routes/devicesImport.ts`:
- Around line 187-190: The per-device error emit calls in devicesImport.ts
currently send raw err.message in production; update the per-device error
handling block (the emit(...) calls inside the per-device processing loop around
the existing lines handling individual device failures) to mirror the
parse-error sanitization: send the full err.message only when
process.env.NODE_ENV !== 'production' and otherwise send a generic string (e.g.
"upload_device_failed"), keeping the same event shape (error property and
message property) so the frontend continues to read event.message consistently.

In `@apps/ingest-service/main.go`:
- Around line 285-289: The sync.Pool currently boxes []byte into any causing the
SA6002 allocation; change the pool to store *[]byte (e.g. declare bodyPool as
sync.Pool{New: func() any { b := make([]byte, 0, 32*1024); return &b }}), update
the consumer code to do bufPtr := bodyPool.Get().(*[]byte); buf := *bufPtr (use
buf for reads) and when done reset and put back with *bufPtr = (*bufPtr)[:0];
bodyPool.Put(bufPtr), and remove the //nolint:staticcheck suppression; ensure
all other references to bodyPool.Get()/Put() follow this pointer pattern.

In `@apps/saga-orchestrator/internal/repository/postgres_saga_repository.go`:
- Around line 54-55: The SQL query that selects by correlation_id currently
includes an unnecessary ORDER BY created_at DESC LIMIT 1 despite correlation_id
being UNIQUE in the migrations; remove the trailing "ORDER BY created_at DESC
LIMIT 1" from the query (the string containing 'ORDER BY created_at DESC LIMIT
1' used in the repository method that looks up by correlation_id) so the SELECT
simply filters on correlation_id, or if you want explicit defense-in-depth, add
a short code comment above the query explaining why the clause is intentionally
kept.

In `@go.mod`:
- Around line 21-23: The go.mod has a version mismatch: update the module
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc from v1.41.0 to
v1.42.0 so it matches go.opentelemetry.io/otel and go.opentelemetry.io/otel/sdk;
after changing the version, run go mod tidy to verify and synchronize all
OpenTelemetry modules (ensure any other otel/* modules use the same v1.42.0).

In `@libs/migrate/migrate.go`:
- Around line 22-25: Refactor the dsnWithTable assignment to use a clear if-else
rather than "set-then-override": check strings.Contains(dsn, "?") and if true
set dsnWithTable = dsn + "&x-migrations-table=" + tableParam else set
dsnWithTable = dsn + "?x-migrations-table=" + tableParam; update the block that
currently declares dsnWithTable, dsn and tableParam to follow this if-else
pattern for clarity.

---

Outside diff comments:
In `@apps/gateway/src/routes/devicesImport.ts`:
- Around line 136-142: The INSERT using pool.query and the device-processing
loop must run inside a real transaction client instead of pool-level queries:
acquire a client via pool.connect(), call await client.query('BEGIN'), replace
the INSERT into bulk_import_jobs (jobRow/jobId), all device upserts/processing
queries in the loop, and the final UPDATE of bulk_import_jobs to use
client.query, then COMMIT on success or ROLLBACK in the catch and always
client.release() in finally; ensure errors set status appropriately on rollback;
if a single transaction is impractical for very large imports, keep the job
insert/update in a transaction per logical unit and add/implement a cleanup job
to mark stale "running" bulk_import_jobs after a timeout.

In `@apps/saga-orchestrator/internal/orchestrator/provision_saga.go`:
- Line 179: The json.Marshal call for cmd currently ignores its error (cmdBytes,
_ := json.Marshal(cmd)), which can produce malformed/empty payloads in the
compensation path; change this to capture and handle the error (e.g., err :=
json.Marshal(cmd) -> cmdBytes, err := json.Marshal(cmd)) and then propagate or
return/log a wrapped error consistent with the rest of this PR before attempting
to publish (handle in the same function that constructs/publishes the
compensation message where cmd and cmdBytes are used).

---

Duplicate comments:
In `@apps/saga-orchestrator/internal/orchestrator/provision_saga.go`:
- Around line 132-135: The error message and variable name are misleading: when
marshaling the quota.allocate_device command the code uses detachErr and logs
"marshal tenant.detach_device command"; update the error handling in
provision_saga.go to reflect the actual operation by renaming detachErr to a
clearer name (e.g., marshalErr or allocErr) and change the fmt.Errorf message to
reference "marshal quota.allocate_device command" (update the error wrap in the
same stmt where cmdBytes, <var> := json.Marshal(cmd) is called, so callers and
logs correctly reflect quota.allocate_device).

In `@apps/saga-orchestrator/internal/repository/postgres_saga_repository.go`:
- Around line 102-119: Both IsEventProcessed and MarkEventProcessed perform
unbounded DB calls; wrap their calls with the existing withTimeout helper to
avoid hanging: in IsEventProcessed call withTimeout(ctx) to get ctxTimed, defer
cancel(), then use r.pool.QueryRow(ctxTimed, ...) and Scan; in
MarkEventProcessed call withTimeout(ctx), defer cancel(), then use
r.pool.Exec(ctxTimed, ...) and return the error; ensure you call the returned
cancel function in both methods.

In `@scripts/load-tests/performance-budget.js`:
- Around line 74-76: The /health call results (healthRes) are not contributing
to the error metrics, so update the health check block to record failures into
the existing error tracking (e.g., errorRate and totalErrors) whenever the check
fails or healthRes.status !== 200 (including timeouts or 5xx). Keep the existing
check() and gatewayP95.add(healthRes.timings.duration) but add logic that
evaluates the check result or status and increments errorRate and totalErrors
accordingly so gateway /health failures are counted in the budget.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d0c4bdfc-c414-485c-8aae-993c41b6b1f4

📥 Commits

Reviewing files that changed from the base of the PR and between 8a97319 and d9b8925.

⛔ Files ignored due to path filters (3)

• apps/bff/package-lock.json is excluded by !**/package-lock.json
• apps/gateway/package-lock.json is excluded by !**/package-lock.json
• go.sum is excluded by !**/*.sum

📒 Files selected for processing (21)

• .github/workflows/cd.yml
• .github/workflows/ci.yml
• .github/workflows/perf.yml
• .github/workflows/security.yml
• .github/workflows/terraform.yml
• .golangci.yml
• apps/bff/src/server.ts
• apps/dashboard/eslint.config.js
• apps/dashboard/src/features/settings/SettingsPage.tsx
• apps/dashboard/src/lib/auth0.ts
• apps/gateway/src/routes/billing.ts
• apps/gateway/src/routes/devicesImport.ts
• apps/ingest-service/main.go
• apps/saga-orchestrator/Dockerfile
• apps/saga-orchestrator/internal/consumer/kafka_consumer.go
• apps/saga-orchestrator/internal/orchestrator/provision_saga.go
• apps/saga-orchestrator/internal/repository/postgres_saga_repository.go
• go.mod
• infra/security/trivy.yaml
• libs/migrate/migrate.go
• scripts/load-tests/performance-budget.js

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't let a missing lint script pass this matrix entry.

This changes the job from “lint these apps” to “lint them if a script happens to exist.” A renamed or removed script will now silently green the check. If an app should be exempt, remove it from the matrix or model that exception explicitly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 81 - 87, The ESLint job ("ESLint"
step) must fail if the target package (matrix.app) has no lint script rather
than silently skip; update the step that currently runs a conditional "if npm
run | grep -qE ..." so that when no lint script is found it exits non‑zero (or
explicitly checks an allowlist of exempt apps from the matrix) instead of
echoing and succeeding. In practice modify the "ESLint" run logic to detect
absence of the lint script for matrix.app and call exit 1 (or assert matrix.app
is in an explicit exemption list) so the CI shows a failing job when a lint
script is missing or renamed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

The gateway env block is using names the app ignores.

apps/gateway/src/database/db.ts:1-29 reads READ_DATABASE_URL plus WRITE_DATABASE_URL/WRITE_DB_*, and apps/gateway/src/cache/redis.ts:1-14 reads REDIS_HOST/REDIS_PORT. DATABASE_URL and REDIS_URL here are ignored, so the gateway falls back to its postgres/redis defaults instead of the local service containers.

🛠️ Suggested change

-          DATABASE_URL:     postgres://grainguard:grainguard@localhost:5432/grainguard
-          REDIS_URL:        redis://localhost:6379
+          READ_DATABASE_URL: postgres://grainguard:grainguard@localhost:5432/grainguard
+          WRITE_DATABASE_URL: postgres://grainguard:grainguard@localhost:5432/grainguard?sslmode=disable
+          REDIS_HOST: localhost
+          REDIS_PORT: 6379

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	env:
	PORT: 3000
	NODE_ENV: development
	AUTH_ENABLED: "false"
	DATABASE_URL: postgres://grainguard:grainguard@localhost:5432/grainguard
	REDIS_URL: redis://localhost:6379
	env:
	PORT: 3000
	NODE_ENV: development
	AUTH_ENABLED: "false"
	READ_DATABASE_URL: postgres://grainguard:grainguard@localhost:5432/grainguard
	WRITE_DATABASE_URL: postgres://grainguard:grainguard@localhost:5432/grainguard?sslmode=disable
	REDIS_HOST: localhost
	REDIS_PORT: 6379

🧰 Tools

🪛 Checkov (3.2.510)

[medium] 68-69: Basic Auth Credentials

(CKV_SECRET_4)

🪛 YAMLlint (1.38.0)

[error] 65-65: too many spaces after colon

(colons)

---

[error] 66-66: too many spaces after colon

(colons)

---

[error] 67-67: too many spaces after colon

(colons)

---

[error] 68-68: too many spaces after colon

(colons)

---

[error] 69-69: too many spaces after colon

(colons)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/perf.yml around lines 64 - 69, The workflow sets
DATABASE_URL and REDIS_URL which the gateway ignores; update the env names to
the keys the gateway code expects: replace DATABASE_URL with READ_DATABASE_URL
and also set WRITE_DATABASE_URL (or the WRITE_DB_* variants used in
apps/gateway/src/database/db.ts) pointing to the local Postgres container, and
replace REDIS_URL with REDIS_HOST and REDIS_PORT (per
apps/gateway/src/cache/redis.ts) with the host and port of the Redis service;
ensure the values match the existing container/service names (e.g., host
"localhost"/service name and ports 5432/6379) so the gateway picks up the
intended local services.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

BFF Postgres is wired with the wrong variables.

apps/bff/src/datasources/postgres.ts:12-16 uses READ_DATABASE_URL or READ_DB_*. The POSTGRES_* values here are ignored, so any GraphQL path that touches Postgres falls back to postgres-read/grainguard_read.

🛠️ Suggested change

-          POSTGRES_HOST: localhost
-          POSTGRES_PORT: 5432
-          POSTGRES_USER: grainguard
-          POSTGRES_PASSWORD: grainguard
-          POSTGRES_DB: grainguard
+          READ_DATABASE_URL: postgres://grainguard:grainguard@localhost:5432/grainguard

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	POSTGRES_HOST: localhost
	POSTGRES_PORT: 5432
	POSTGRES_USER: grainguard
	POSTGRES_PASSWORD: grainguard
	POSTGRES_DB: grainguard
	READ_DATABASE_URL: postgres://grainguard:grainguard@localhost:5432/grainguard

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/perf.yml around lines 91 - 95, The workflow sets
POSTGRES_* env vars but the BFF datasource (uses READ_DATABASE_URL / READ_DB_*
in apps/bff/src/datasources/postgres.ts) reads the read-replica variables, so
Postgres connections fall back to postgres-read/grainguard_read; update the
workflow to export the read-replica environment variables instead of (or in
addition to) POSTGRES_* — specifically set READ_DATABASE_URL or the
READ_DB_HOST, READ_DB_PORT, READ_DB_USER, READ_DB_PASSWORD, and READ_DB_NAME
variables to match the running container so the BFF's datasource (which expects
READ_DB_*) picks up the correct DB config.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

continue-on-error disables the budget gate.

k6 can exit non-zero when a threshold is breached, but this step is forced green, so the workflow never blocks a regression. The artifact upload already uses if: always(), so it will still run after a failing k6 step.

🛠️ Suggested change

       - name: Run performance budget
-        continue-on-error: true
         run: |

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- name: Run performance budget
	continue-on-error: true
	run: |
	- name: Run performance budget
	run: |

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/perf.yml around lines 121 - 123, The "Run performance
budget" job step uses continue-on-error: true which masks k6 failures and
disables the budget gate; remove the continue-on-error setting (or set it to
false) from the "Run performance budget" step so a non-zero k6 exit will fail
the workflow and block regressions, leaving the artifact upload step's if:
always() intact so artifacts still upload after a failing k6 run.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Security scans configured to never fail the workflow.

Combining continue-on-error: true with exit-code: "0" means CRITICAL and HIGH vulnerabilities will never block the pipeline. This significantly degrades the security gate—vulnerabilities become purely informational.

If the intent is to prevent blocking on unfixed upstream issues, consider:

• Using exit-code: "1" without continue-on-error and maintaining a .trivyignore file for accepted risks
• Or at minimum, documenting this decision as intentional risk acceptance

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/security.yml around lines 42 - 51, The Trivy step "Run
Trivy vulnerability scan" currently combines continue-on-error: true and
exit-code: "0", which disables blocking for CRITICAL/HIGH findings; update the
workflow to enforce failures by either removing continue-on-error and setting
exit-code: "1" (or omitting exit-code so the action fails normally) for
aquasecurity/trivy-action@master, and if needed add a .trivyignore to accept
known risks, or alternatively add a clear comment/docstring in the job
explaining the intentional risk acceptance if you truly want non-blocking
behavior.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the pool currently stores []byte values, the pattern staticcheck flags as SA6002.
rg -n -C2 'bodyPool\s*=|make\(\[\]byte, 0, 4096\)|bodyPool\.Put\(buf\[:0\]\)' apps/ingest-service/main.go

Repository: pahuldeepp/GrainGuard-

Length of output: 360

---

🏁 Script executed:

# Get the function context around lines 285-289 to see all buf usages
sed -n '270,310p' apps/ingest-service/main.go | cat -n

Repository: pahuldeepp/GrainGuard-

Length of output: 1412

---

🏁 Script executed:

# Search for the function containing this code and show all buf usages
rg -n "buf" apps/ingest-service/main.go -B5 -A15 | grep -A20 "285:"

Repository: pahuldeepp/GrainGuard-

Length of output: 711

---

🏁 Script executed:

# Get a broader context of the function to understand hot path claim and all buf operations
sed -n '250,320p' apps/ingest-service/main.go | cat -n

Repository: pahuldeepp/GrainGuard-

Length of output: 2468

---

🏁 Script executed:

# Find all bodyPool usages in the file
rg -n "bodyPool" apps/ingest-service/main.go

Repository: pahuldeepp/GrainGuard-

Length of output: 207

---

Fix the pool instead of suppressing SA6002.

sync.Pool storing []byte values boxes the slice header into any, which is exactly the allocation staticcheck warns about. On this hot path, suppressing the warning keeps the overhead without fixing it. The pool declaration at line 120 must also change.

♻️ Suggested change

-	bodyPool   = sync.Pool{New: func() any { return make([]byte, 0, 4096) }}
+	bodyPool   = sync.Pool{New: func() any { return new([4096]byte) }}
...
-	buf := bodyPool.Get().([]byte)
+	buf := bodyPool.Get().(*[4096]byte)
 	defer func() {
-		//nolint:staticcheck // sync.Pool stores []byte values for reuse in this hot path.
-		bodyPool.Put(buf[:0])
+		bodyPool.Put(buf)
 	}()
 
 	lr := io.LimitReader(r.Body, 4096)
-	n, err := io.ReadFull(lr, buf[:cap(buf)])
+	n, err := io.ReadFull(lr, buf[:])

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/ingest-service/main.go` around lines 285 - 289, The sync.Pool currently
boxes []byte into any causing the SA6002 allocation; change the pool to store
*[]byte (e.g. declare bodyPool as sync.Pool{New: func() any { b := make([]byte,
0, 32*1024); return &b }}), update the consumer code to do bufPtr :=
bodyPool.Get().(*[]byte); buf := *bufPtr (use buf for reads) and when done reset
and put back with *bufPtr = (*bufPtr)[:0]; bodyPool.Put(bufPtr), and remove the
//nolint:staticcheck suppression; ensure all other references to
bodyPool.Get()/Put() follow this pointer pattern.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

ORDER BY ... LIMIT 1 is unnecessary given the UNIQUE constraint.

Per the schema in migrations/000001_create_sagas.up.sql, correlation_id has a UNIQUE constraint, so there can only ever be one row per correlation ID. The ORDER BY created_at DESC LIMIT 1 adds query overhead without providing any safety benefit.

If the intent is defense-in-depth against future schema changes, consider documenting that explicitly; otherwise, remove the ordering clause.

Suggested simplification

     row := r.pool.QueryRow(ctx, `
         SELECT saga_id, saga_type, correlation_id, status, current_step, payload, COALESCE(last_error,'')
         FROM sagas
         WHERE correlation_id = $1
-        ORDER BY created_at DESC
-        LIMIT 1
     `, correlationID)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	ORDER BY created_at DESC
	LIMIT 1
	row := r.pool.QueryRow(ctx, `
	SELECT saga_id, saga_type, correlation_id, status, current_step, payload, COALESCE(last_error,'')
	FROM sagas
	WHERE correlation_id = $1
	`, correlationID)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/saga-orchestrator/internal/repository/postgres_saga_repository.go`
around lines 54 - 55, The SQL query that selects by correlation_id currently
includes an unnecessary ORDER BY created_at DESC LIMIT 1 despite correlation_id
being UNIQUE in the migrations; remove the trailing "ORDER BY created_at DESC
LIMIT 1" from the query (the string containing 'ORDER BY created_at DESC LIMIT
1' used in the repository method that looks up by correlation_id) so the SELECT
simply filters on correlation_id, or if you want explicit defense-in-depth, add
a short code comment above the query explaining why the clause is intentionally
kept.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

OpenTelemetry version mismatch: otlptracegrpc is at v1.41.0 while other modules are at v1.42.0.

Line 22 shows go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.41.0, but the core OTel modules (otel, otel/sdk) and other exporters (otlptrace, otlptracehttp, metric, trace) are at v1.42.0. OpenTelemetry modules should be kept in sync to avoid compatibility issues.

Proposed fix

 	go.opentelemetry.io/otel v1.42.0
-	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.41.0
+	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.42.0
 	go.opentelemetry.io/otel/sdk v1.42.0

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	go.opentelemetry.io/otel v1.42.0
	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.41.0
	go.opentelemetry.io/otel/sdk v1.41.0
	google.golang.org/grpc v1.79.1
	go.opentelemetry.io/otel/sdk v1.42.0
	go.opentelemetry.io/otel v1.42.0
	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.42.0
	go.opentelemetry.io/otel/sdk v1.42.0

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@go.mod` around lines 21 - 23, The go.mod has a version mismatch: update the
module go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc from
v1.41.0 to v1.42.0 so it matches go.opentelemetry.io/otel and
go.opentelemetry.io/otel/sdk; after changing the version, run go mod tidy to
verify and synchronize all OpenTelemetry modules (ensure any other otel/*
modules use the same v1.42.0).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider a slightly cleaner structure using if-else.

The current "set-then-override" pattern works correctly, but an if-else structure would be marginally clearer by avoiding the redundant initial assignment when the DSN already contains query parameters.

Optional refactor

-    dsnWithTable := dsn + "?x-migrations-table=" + tableParam
-    if strings.Contains(dsn, "?") {
-        dsnWithTable = dsn + "&x-migrations-table=" + tableParam
+    var dsnWithTable string
+    if strings.Contains(dsn, "?") {
+        dsnWithTable = dsn + "&x-migrations-table=" + tableParam
+    } else {
+        dsnWithTable = dsn + "?x-migrations-table=" + tableParam
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	dsnWithTable := dsn + "?x-migrations-table=" + tableParam
	if strings.Contains(dsn, "?") {
	dsnWithTable = dsn + "&x-migrations-table=" + tableParam
	} else {
	dsnWithTable = dsn + "?x-migrations-table=" + tableParam
	}
	var dsnWithTable string
	if strings.Contains(dsn, "?") {
	dsnWithTable = dsn + "&x-migrations-table=" + tableParam
	} else {
	dsnWithTable = dsn + "?x-migrations-table=" + tableParam
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@libs/migrate/migrate.go` around lines 22 - 25, Refactor the dsnWithTable
assignment to use a clear if-else rather than "set-then-override": check
strings.Contains(dsn, "?") and if true set dsnWithTable = dsn +
"&x-migrations-table=" + tableParam else set dsnWithTable = dsn +
"?x-migrations-table=" + tableParam; update the block that currently declares
dsnWithTable, dsn and tableParam to follow this if-else pattern for clarity.

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

apps/saga-orchestrator/internal/consumer/kafka_consumer.go (1)

275-281: 🧹 Nitpick | 🔵 Trivial

Consider context-aware sleep for faster shutdown.

The time.Sleep(time.Second) at line 279 doesn't respect context cancellation. If FetchMessage returns a non-context error during shutdown, this introduces up to 1 second of unnecessary delay.

♻️ Optional: Use select with timer for context-aware sleep

 			observability.KafkaFetchErrors.Inc()

 			log.Printf("FetchMessage error: %v", err)

-			time.Sleep(time.Second)
+			select {
+			case <-time.After(time.Second):
+			case <-ctx.Done():
+				break fetchLoop
+			}

 			continue

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/saga-orchestrator/internal/consumer/kafka_consumer.go` around lines 275
- 281, Replace the blocking time.Sleep(time.Second) in the FetchMessage error
path with a context-aware wait so shutdown isn't delayed: after logging and
incrementing observability.KafkaFetchErrors.Inc(), wait using a select that
races ctx.Done() against a time.After(1 * time.Second) (or a timer) and
returns/continues immediately if ctx is cancelled; update the error-handling
block around the FetchMessage loop to use this select so the consumer respects
context cancellation during the retry delay.

apps/bff/src/datasources/postgres.ts (2)

308-320: ⚠️ Potential issue | 🔴 Critical

Scope these writes to the authenticated tenant instead of trusting caller state.

createDevice accepts a raw tenantId from the caller and updateDevice matches only on device_id. Neither write runs under a tenant-scoped query, so the database is not enforcing tenant isolation for either insert or update. Pass the authenticated tenant separately and execute both paths through tenantQuery or an equivalent tenant-guarded statement.

🔐 Proposed fix

-  async createDevice(input: { serialNumber: string; tenantId: string }) {
-    const result = await cbQuery(
+  async createDevice(tenantId: string, input: { serialNumber: string }) {
+    const result = await tenantQuery(
+      tenantId,
       `INSERT INTO device_projections (device_id, tenant_id, serial_number, created_at)
        VALUES (gen_random_uuid(), $1, $2, NOW())
        RETURNING device_id, tenant_id, serial_number, created_at`,
-      [input.tenantId, input.serialNumber]
+      [tenantId, input.serialNumber]
     );
@@
-  async updateDevice(deviceId: string, input: { serialNumber?: string }) {
-    const result = await cbQuery(
+  async updateDevice(tenantId: string, deviceId: string, input: { serialNumber?: string }) {
+    const result = await tenantQuery(
+      tenantId,
       `UPDATE device_projections
        SET serial_number = COALESCE($1, serial_number)
-       WHERE device_id = $2
+       WHERE device_id = $2 AND tenant_id = $3
        RETURNING device_id, tenant_id, serial_number, created_at`,
-      [input.serialNumber || null, deviceId]
+      [input.serialNumber ?? null, deviceId, tenantId]
     );

As per coding guidelines, "Check all queries use tenantId for RLS."

Also applies to: 323-335

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/bff/src/datasources/postgres.ts` around lines 308 - 320, createDevice
(and the corresponding updateDevice path) currently accepts and uses a
caller-supplied tenantId, allowing writes outside the authenticated tenant's RLS
context; change these methods to take the authenticated tenant separately and
execute the INSERT/UPDATE through the tenant-scoped query helper (e.g.,
tenantQuery) or add tenantId to the WHERE/RETURNING in a tenant-guarded
statement so the DB enforces RLS (reference createDevice and updateDevice
identifiers) — do not trust the incoming tenantId parameter, always use the
authenticated tenant from the request/session and run the SQL via tenantQuery.

---

56-75: ⚠️ Potential issue | 🟠 Major

Do not cache or query getAllDevices without tenant scope.

This helper has no tenantId input, reads all rows from device_projections, and caches the result under devices:all:${limit}. In any tenant-facing path, that turns both the SQL result and the Redis entry into shared cross-tenant state. Thread tenantId into the method and scope the cache key by tenant, or move the global list into an explicit admin-only API.

🔐 Proposed fix

-  async getAllDevices(limit: number = 20) {
-    const cacheKey = `devices:all:${limit}`;
+  async getAllDevices(tenantId: string, limit: number = 20) {
+    const cacheKey = `devices:all:${tenantId}:${limit}`;
@@
-      const result = await cbQuery(
+      const result = await tenantQuery(
+        tenantId,
         `SELECT device_id, tenant_id, serial_number, created_at
          FROM device_projections
          ORDER BY created_at DESC
          LIMIT $1`,
         [limit]
       );

As per coding guidelines, "Check all queries use tenantId for RLS." and "Verify cache keys include tenantId."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/bff/src/datasources/postgres.ts` around lines 56 - 75, The getAllDevices
method currently reads and caches all rows across tenants; change its signature
(getAllDevices) to accept a tenantId parameter, include tenantId in the SQL
query (add "WHERE tenant_id = $N" and pass tenantId as a parameter to cbQuery)
and scope the cache key to the tenant (e.g., `devices:all:${tenantId}:${limit}`)
so cached entries and DB reads are tenant-specific; ensure the
cache.acquireLock/cache.get/cache.set calls use the new tenant-scoped cacheKey
and that the query parameters order matches the prepared statement.

♻️ Duplicate comments (1)

.golangci.yml (1)

6-11: ⚠️ Potential issue | 🟠 Major

Re-enable critical linters, especially gosec for security scanning.

Beyond errcheck (flagged in previous review), the removal of gosec, typecheck, and unused significantly weakens static analysis:

Linter	Purpose
gosec	Detects security vulnerabilities (SQL injection, hardcoded credentials, weak crypto)
typecheck	Catches type errors before build
unused	Identifies dead code

Reducing from ~12 linters to 3 substantially decreases code quality coverage.

, (for errcheck specifically)

Suggested fix to restore essential linters

 linters:
   default: none
   enable:
     - govet
     - staticcheck
+    - errcheck
     - ineffassign
+    - typecheck
+    - unused
+    - gosec
+    - bodyclose

#!/bin/bash
# Check for potential security issues that gosec would catch
rg -n "sql\.Query|Exec\(" --type go -g '!vendor/*' | head -20
# Check for hardcoded credentials patterns
rg -n -i "password\s*=\s*[\"']|secret\s*=\s*[\"']" --type go -g '!vendor/*' | head -10

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.golangci.yml around lines 6 - 11, The current .golangci.yml disables most
linters and only enables govet, staticcheck and ineffassign; re-enable the
critical linters recommended in the review by adding gosec, typecheck and unused
(and errcheck if previously flagged) back into the enable list so static
analysis covers security issues, type problems and dead code; update the
"linters: enable:" section to include - gosec, - typecheck, - unused (and -
errcheck) alongside the existing linters to restore the intended checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/perf.yml:
- Around line 110-116: The workflow uses continue-on-error: true for the "Wait
for gateway" and "Wait for BFF" steps which masks startup failures; remove or
set continue-on-error to false for those steps (the steps with names "Wait for
gateway" and "Wait for BFF" that run npx wait-on tcp:3000 and npx wait-on
tcp:4000) so the job fails if the services don't come up within the timeout,
optionally increase the --timeout value if needed to reduce flaky failures.
- Around line 46-51: The Node setup step currently only uses
apps/gateway/package-lock.json for cache invalidation (cache-dependency-path) so
BFF dependency changes won't bust the Node/npm cache; update the "Set up
Node.js" action configuration to include both lockfiles (gateway and BFF) in
cache-dependency-path so changes to either apps/gateway/package-lock.json or
apps/bff/package-lock.json invalidate the cache and rebuild dependencies. Ensure
the value for cache-dependency-path in the "Set up Node.js" step contains both
paths (newline- or separator-delimited as supported by actions/setup-node@v4).
- Around line 98-100: The workflow exposes ELASTICSEARCH_URL, CASSANDRA_HOST,
and CASSANDRA_PORT but does not start those services; either add Elasticsearch
and Cassandra service containers to the GitHub Actions job (match the
docker-compose.yml service names/ports: elasticsearch -> 9200 and cassandra ->
9042, include simple healthchecks/wait-for logic so the job waits until services
are ready) or remove those environment variables from the k6 job; this will
prevent failures in resolvers like searchDevices and
getTelemetryHistoryFromCassandra that expect those services.

In @.golangci.yml:
- Line 1: The CI is failing due to a mismatch between the linter config and the
action: .golangci.yml uses version: "2" (v2.x format) but the workflow uses
golangci/golangci-lint-action@v6 (v1.x only); either update the workflow action
to golangci/golangci-lint-action@v8 (or later) so it supports v2 configs, or
change the config's version: "2" to version: "1" to remain v1-compatible—modify
the CI workflow reference to the newer action name or edit the .golangci.yml
version field accordingly (search for .golangci.yml and the workflow step
referencing golangci/golangci-lint-action@v6).

In `@apps/bff/src/datasources/elasticsearch.ts`:
- Line 47: Replace the unconditional console.log in the Elasticsearch search
path (the line printing "[ES] searching:" with JSON.stringify(params)) with
conditionalized/debug-level logging: use the project's logger (not console) or
guard the print with an environment check (e.g., only in development), and
redact or omit sensitive fields such as params.tenantId before logging (e.g.,
clone params then delete or mask tenantId) so production logs never contain
tenant identifiers; update the call site that prints params to use the logger
and masked params.
- Line 48: Replace the manual cast on es.search with the Elasticsearch client's
generic type parameter: call es.search<TSource>(params) using the appropriate
source type (e.g., ESHit['_source'] or a dedicated interface) instead of casting
the function signature; then update the subsequent mapping (where you access
hit._source) to account for _source possibly being undefined by using
null-coalescing/default values when reading fields. This change targets uses of
es.search, the params variable, and the mapping over hits to ensure type-safe
_source access without bypassing the client's built-in types.

In `@apps/bff/src/datasources/postgres.ts`:
- Around line 28-38: tenantQuery currently uses SET LOCAL outside an explicit
transaction so the tenant setting is lost; change tenantQuery to open an
explicit transaction on the acquired client (client.query('BEGIN')), set the
tenant with transaction-local set_config("app.current_tenant_id", tenantId,
true) (e.g. await client.query('SELECT set_config($1,$2,true)',
['app.current_tenant_id', tenantId])), run the requested query, then COMMIT and
return the result; on any error perform ROLLBACK before rethrowing and always
release the client in the existing finally block; update references to
postgresCircuitBreaker, pool.connect, and tenantQuery accordingly.

In `@apps/dashboard/src/e2e/auth0Mock.tsx`:
- Around line 87-99: The mock currently uses window.location.assign in
loginWithRedirect and logout which forces full reloads; replace those calls with
SPA navigation by using history.pushState/replaceState (or router navigation if
available) to update the URL and then trigger a PopStateEvent so client routing
runs, and ensure you invoke the provided onRedirectCallback (if present) with
the appState/returnTo so the mock matches Auth0's contract; update the
loginWithRedirect function to push the returnTo/redirect_uri and call
onRedirectCallback({ returnTo }) or similar, and update logout to clear
TOKEN_KEY, setToken(null), update history to returnTo, dispatch popstate, and
call onRedirectCallback when appropriate.

In `@tests/e2e/devices.spec.ts`:
- Around line 30-36: The backdrop-click test "modal closes on backdrop click" is
brittle because it clicks at position {x:5,y:5} on the dialog element (variable
dialog) assuming role="dialog" is on the overlay; instead update the test to
target a more explicit backdrop locator or add a clarifying comment: locate the
backdrop using a stable selector (e.g., a data-testid like
data-testid="modal-backdrop", a class such as ".modal-backdrop", or a specific
role/aria attribute from RegisterDeviceModal.tsx) and perform the click on that
locator, or if you cannot change the markup add a comment next to the
dialog.click explaining why the position is used and the coupling to
RegisterDeviceModal.tsx.

In `@tests/e2e/fixtures/mockAuth.ts`:
- Around line 21-24: The mock auth fixture defines duplicate claim namespaces
for tenant_id and roles (keys "https://grainguard.com/tenant_id",
"https://grainguard/tenant_id", "https://grainguard.com/roles",
"https://grainguard/roles"); decide whether both namespaces are needed — if not,
remove the redundant entries and keep a single canonical key (e.g.,
"https://grainguard.com/..."), or if both are required for backward
compatibility, add a brief inline comment above these keys stating that both
namespaces are intentional and why (so future reviewers understand the
duplication).

---

Outside diff comments:
In `@apps/bff/src/datasources/postgres.ts`:
- Around line 308-320: createDevice (and the corresponding updateDevice path)
currently accepts and uses a caller-supplied tenantId, allowing writes outside
the authenticated tenant's RLS context; change these methods to take the
authenticated tenant separately and execute the INSERT/UPDATE through the
tenant-scoped query helper (e.g., tenantQuery) or add tenantId to the
WHERE/RETURNING in a tenant-guarded statement so the DB enforces RLS (reference
createDevice and updateDevice identifiers) — do not trust the incoming tenantId
parameter, always use the authenticated tenant from the request/session and run
the SQL via tenantQuery.
- Around line 56-75: The getAllDevices method currently reads and caches all
rows across tenants; change its signature (getAllDevices) to accept a tenantId
parameter, include tenantId in the SQL query (add "WHERE tenant_id = $N" and
pass tenantId as a parameter to cbQuery) and scope the cache key to the tenant
(e.g., `devices:all:${tenantId}:${limit}`) so cached entries and DB reads are
tenant-specific; ensure the cache.acquireLock/cache.get/cache.set calls use the
new tenant-scoped cacheKey and that the query parameters order matches the
prepared statement.

In `@apps/saga-orchestrator/internal/consumer/kafka_consumer.go`:
- Around line 275-281: Replace the blocking time.Sleep(time.Second) in the
FetchMessage error path with a context-aware wait so shutdown isn't delayed:
after logging and incrementing observability.KafkaFetchErrors.Inc(), wait using
a select that races ctx.Done() against a time.After(1 * time.Second) (or a
timer) and returns/continues immediately if ctx is cancelled; update the
error-handling block around the FetchMessage loop to use this select so the
consumer respects context cancellation during the retry delay.

---

Duplicate comments:
In @.golangci.yml:
- Around line 6-11: The current .golangci.yml disables most linters and only
enables govet, staticcheck and ineffassign; re-enable the critical linters
recommended in the review by adding gosec, typecheck and unused (and errcheck if
previously flagged) back into the enable list so static analysis covers security
issues, type problems and dead code; update the "linters: enable:" section to
include - gosec, - typecheck, - unused (and - errcheck) alongside the existing
linters to restore the intended checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6814e86e-3651-45b0-b951-d62a0152435a

📥 Commits

Reviewing files that changed from the base of the PR and between d9b8925 and e1fa0f0.

📒 Files selected for processing (14)

• .github/workflows/e2e.yml
• .github/workflows/perf.yml
• .golangci.yml
• apps/bff/src/datasources/elasticsearch.ts
• apps/bff/src/datasources/postgres.ts
• apps/dashboard/index.html
• apps/dashboard/src/e2e/auth0Mock.tsx
• apps/dashboard/src/features/devices/components/RegisterDeviceModal.tsx
• apps/dashboard/vite.config.ts
• apps/saga-orchestrator/internal/consumer/kafka_consumer.go
• tests/e2e/auth.spec.ts
• tests/e2e/billing.spec.ts
• tests/e2e/devices.spec.ts
• tests/e2e/fixtures/mockAuth.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Cache key only includes gateway dependencies.

The cache-dependency-path only references apps/gateway/package-lock.json, so changes to BFF dependencies won't invalidate the cache. This could cause stale dependencies for the BFF app.

♻️ Suggested fix to include both lockfiles

       - name: Set up Node.js
         uses: actions/setup-node@v4
         with:
           node-version: "20"
           cache: npm
-          cache-dependency-path: apps/gateway/package-lock.json
+          cache-dependency-path: |
+            apps/gateway/package-lock.json
+            apps/bff/package-lock.json

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- name: Set up Node.js
	uses: actions/setup-node@v4
	with:
	node-version: "20"
	cache: npm
	cache-dependency-path: apps/gateway/package-lock.json
	- name: Set up Node.js
	uses: actions/setup-node@v4
	with:
	node-version: "20"
	cache: npm
	cache-dependency-path: |
	apps/gateway/package-lock.json
	apps/bff/package-lock.json

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/perf.yml around lines 46 - 51, The Node setup step
currently only uses apps/gateway/package-lock.json for cache invalidation
(cache-dependency-path) so BFF dependency changes won't bust the Node/npm cache;
update the "Set up Node.js" action configuration to include both lockfiles
(gateway and BFF) in cache-dependency-path so changes to either
apps/gateway/package-lock.json or apps/bff/package-lock.json invalidate the
cache and rebuild dependencies. Ensure the value for cache-dependency-path in
the "Set up Node.js" step contains both paths (newline- or separator-delimited
as supported by actions/setup-node@v4).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if the load test script references endpoints that might need Elasticsearch or Cassandra
rg -n -C3 'search|elastic|cassandra' scripts/load-tests/performance-budget.js || echo "No direct references found"

# Check BFF datasources for how these are used
fd -t f -e ts datasources --exec cat {} \; 2>/dev/null | head -100

Repository: pahuldeepp/GrainGuard-

Length of output: 92

---

🏁 Script executed:

# Read the perf.yml workflow to understand what's being tested
cat -n .github/workflows/perf.yml

Repository: pahuldeepp/GrainGuard-

Length of output: 5663

---

🏁 Script executed:

# Check what's in the load test script
cat -n scripts/load-tests/performance-budget.js

Repository: pahuldeepp/GrainGuard-

Length of output: 5897

---

🏁 Script executed:

# Check if there's a docker-compose or service setup in the workflow
fd -t f docker-compose | xargs cat 2>/dev/null

Repository: pahuldeepp/GrainGuard-

Length of output: 50378

---

🏁 Script executed:

# Look for any references to elasticsearch or cassandra in the BFF codebase
rg -l 'elasticsearch|cassandra' --type ts --type js

Repository: pahuldeepp/GrainGuard-

Length of output: 171

---

🏁 Script executed:

# Check BFF datasource initialization
head -50 apps/bff/src/datasources/elasticsearch.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 1352

---

🏁 Script executed:

head -50 apps/bff/src/datasources/cassandra.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 1356

---

🏁 Script executed:

# Check if BFF server initialization requires these datasources
grep -A 20 "datasources" apps/bff/src/server.ts | head -40

Repository: pahuldeepp/GrainGuard-

Length of output: 48

---

🏁 Script executed:

# Look for server.ts to see initialization
cat -n apps/bff/src/server.ts | head -100

Repository: pahuldeepp/GrainGuard-

Length of output: 3961

---

🏁 Script executed:

# Search for where elasticsearch and cassandra are actually used in resolvers
rg -n "search\.|getCassandraClient|searchDevices|getTelemetryHistory" apps/bff/src/resolvers.ts | head -30

Repository: pahuldeepp/GrainGuard-

Length of output: 500

---

Add missing Elasticsearch and Cassandra service containers to the k6 performance test workflow.

The BFF environment references ELASTICSEARCH_URL, CASSANDRA_HOST, and CASSANDRA_PORT, but the workflow defines only postgres and redis services. While the load test currently passes because it runs only an introspection query ({ __typename }), the BFF resolvers do depend on these services: the searchDevices resolver requires Elasticsearch and getTelemetryHistoryFromCassandra requires Cassandra. If the test is expanded to exercise these endpoints, it will fail due to missing infrastructure. Either provision these services in the workflow (as they are defined in docker-compose.yml) or remove the environment variables.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/perf.yml around lines 98 - 100, The workflow exposes
ELASTICSEARCH_URL, CASSANDRA_HOST, and CASSANDRA_PORT but does not start those
services; either add Elasticsearch and Cassandra service containers to the
GitHub Actions job (match the docker-compose.yml service names/ports:
elasticsearch -> 9200 and cassandra -> 9042, include simple
healthchecks/wait-for logic so the job waits until services are ready) or remove
those environment variables from the k6 job; this will prevent failures in
resolvers like searchDevices and getTelemetryHistoryFromCassandra that expect
those services.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

continue-on-error on wait steps masks startup failures.

If the gateway or BFF fail to start within the timeout, the workflow silently continues and k6 runs against non-existent services. This produces confusing failures and wastes CI time.

🛠️ Suggested fix

       - name: Wait for gateway
-        continue-on-error: true
         run: npx wait-on tcp:3000 --timeout 30000

       - name: Wait for BFF
-        continue-on-error: true
         run: npx wait-on tcp:4000 --timeout 30000

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- name: Wait for gateway
	continue-on-error: true
	run: npx wait-on tcp:3000 --timeout 30000
	- name: Wait for BFF
	continue-on-error: true
	run: npx wait-on tcp:4000 --timeout 30000
	- name: Wait for gateway
	run: npx wait-on tcp:3000 --timeout 30000
	- name: Wait for BFF
	run: npx wait-on tcp:4000 --timeout 30000

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/perf.yml around lines 110 - 116, The workflow uses
continue-on-error: true for the "Wait for gateway" and "Wait for BFF" steps
which masks startup failures; remove or set continue-on-error to false for those
steps (the steps with names "Wait for gateway" and "Wait for BFF" that run npx
wait-on tcp:3000 and npx wait-on tcp:4000) so the job fails if the services
don't come up within the timeout, optionally increase the --timeout value if
needed to reduce flaky failures.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check CI workflow for golangci-lint version configuration
rg -n "golangci-lint" .github/workflows/ -A5 -B2

Repository: pahuldeepp/GrainGuard-

Length of output: 496

---

🌐 Web query:

golangci-lint-action v6 version latest compatibility

💡 Result:

The latest version of golangci-lint-action in the v6 major version is v6.5.2, released around March 2025. It is compatible with golangci-lint v1.x versions (e.g., v1.64 as shown in examples), allowing specification via the 'version' input as 'v1.xx', 'latest', or specific patches. In v6.5.1, a restriction was added to enforce golangci-lint v1 (not v2). It requires actions/setup-go before use and has changes like removed 'annotations' option since v6.0.0. golangci-lint latest is v2.11.2 (March 2026), but v6 action is for v1.x. For v2 support, use v8+ of the action.

Citations:

• 1: https://github.com/golangci/golangci-lint-action/tree/v6.5.2
• 2: https://github.com/golangci/golangci-lint-action/releases
• 3: https://github.com/golangci/golangci-lint-action
• 4: build(deps): bump golangci/golangci-lint-action from 6.5.0 to 6.5.1 in the github-actions group golangci/golangci-lint#5574
• 5: https://github.com/marketplace/actions/golangci-lint
• 6: https://newreleases.io/project/github/golangci/golangci-lint-action/release/v6.5.2

---

Update CI action to v8+ or downgrade config to v1 format.

The .golangci.yml config uses version: "2" (golangci-lint v2.x format), but the CI workflow uses golangci/golangci-lint-action@v6, which is restricted to golangci-lint v1.x only. This mismatch will cause CI failures. Either:

1. Upgrade the CI action to @v8 or later (supports v2.x), or
2. Downgrade the config to version: "1" format (v1.x compatible)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.golangci.yml at line 1, The CI is failing due to a mismatch between the
linter config and the action: .golangci.yml uses version: "2" (v2.x format) but
the workflow uses golangci/golangci-lint-action@v6 (v1.x only); either update
the workflow action to golangci/golangci-lint-action@v8 (or later) so it
supports v2 configs, or change the config's version: "2" to version: "1" to
remain v1-compatible—modify the CI workflow reference to the newer action name
or edit the .golangci.yml version field accordingly (search for .golangci.yml
and the workflow step referencing golangci/golangci-lint-action@v6).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🌐 Web query:

@elastic/elasticsearch TypeScript generic types for search method

💡 Result:

In the @elastic/elasticsearch Node.js client, the search method supports TypeScript generics to type the document hits in the response. Define an interface for your document type and pass it as a generic parameter to client.search. Example from official documentation: import { Client } from '@elastic/elasticsearch' const client = new Client({ /* config */ }) interface Document { character: string quote: string } const result = await client.search<Document>({ index: 'game-of-thrones', query: { match: { quote: 'winter' } } }) console.log(result.hits.hits) // hits[0]._source typed as Document The generic T types the _source field in SearchHit<T> within the SearchResponse. Types are generated from the elasticsearch-specification repository, providing complete API definitions including snake_case parameters. For full request/response typing, import { estypes } from '@elastic/elasticsearch' and use types like estypes.SearchRequest, though the primary usage is the document generic as shown. Legacy versions used more complex generics like client.search<SearchResponse<Source>, SearchBody>, but current versions simplify to the document type.

Citations:

• 1: https://www.elastic.co/docs/reference/elasticsearch/clients/javascript/typescript
• 2: https://github.com/elastic/elasticsearch-specification
• 3: https://github.com/elastic/elasticsearch-specification/blob/main/specification/indices/create/IndicesCreateRequest.ts
• 4: Refactored type definitions elastic/elasticsearch-js#1119
• 5: https://github.com/elastic/elasticsearch-specification/blob/main/specification/_types/mapping/TypeMapping.ts

---

🏁 Script executed:

# First, let's check the file size and view the relevant section
wc -l apps/bff/src/datasources/elasticsearch.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 110

---

🏁 Script executed:

# View the file with context around line 48
head -60 apps/bff/src/datasources/elasticsearch.ts | tail -30

Repository: pahuldeepp/GrainGuard-

Length of output: 1123

---

🏁 Script executed:

# Also check the imports and ESHit definition
head -50 apps/bff/src/datasources/elasticsearch.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 1352

---

🏁 Script executed:

# Check the full method to understand the complete context
cat -n apps/bff/src/datasources/elasticsearch.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 2493

---

🏁 Script executed:

# Check if there are any other uses of es.search in the codebase
rg "es\.search" apps/bff/src/

Repository: pahuldeepp/GrainGuard-

Length of output: 233

---

🏁 Script executed:

# Check what version of `@elastic/elasticsearch` is being used
cat apps/bff/package.json | grep -A 5 -B 5 "elastic"

Repository: pahuldeepp/GrainGuard-

Length of output: 394

---

🏁 Script executed:

# Check if there are resolvers in this path that need auth verification per guidelines
fd -t f "*.ts" apps/bff/src/ | xargs grep -l "searchDevices\|elasticsearch" | head -5

Repository: pahuldeepp/GrainGuard-

Length of output: 349

---

🏁 Script executed:

# Verify if the search function is used in resolvers and if auth is handled
rg "searchDevices" apps/bff/src/ -A 3 -B 3

Repository: pahuldeepp/GrainGuard-

Length of output: 1748

---

🏁 Script executed:

# Check the resolver file for authentication context
head -100 apps/bff/src/resolvers.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 3749

---

🏁 Script executed:

# Check tenantFilter function to ensure auth is enforced
rg "tenantFilter" apps/bff/src/ -B 5 -A 5

Repository: pahuldeepp/GrainGuard-

Length of output: 3750

---

Replace the function type cast with Elasticsearch client's generic type parameter.

The current approach casts es.search to a custom function signature, which bypasses the library's built-in type safety. The @elastic/elasticsearch client (v8.x) supports generic type parameters for typing the response's _source field.

♻️ Proposed refactor using ES client generics

-      const result = await (es.search as (p: typeof params) => Promise<{ hits: { total: unknown; hits: ESHit[] } }>)(params);
+      const result = await es.search<ESHit['_source']>(params);

Then update the mapping to handle the optional _source:

-      return result.hits.hits.map((hit: ESHit) => ({
+      return result.hits.hits.map((hit) => ({
-        deviceId:     hit._source.device_id,
+        deviceId:     hit._source?.device_id,

Note: The ES client types _source as potentially undefined, so null-coalescing operators are appropriate for defensive access.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/bff/src/datasources/elasticsearch.ts` at line 48, Replace the manual
cast on es.search with the Elasticsearch client's generic type parameter: call
es.search<TSource>(params) using the appropriate source type (e.g.,
ESHit['_source'] or a dedicated interface) instead of casting the function
signature; then update the subsequent mapping (where you access hit._source) to
account for _source possibly being undefined by using null-coalescing/default
values when reading fields. This change targets uses of es.search, the params
variable, and the mapping over hits to ensure type-safe _source access without
bypassing the client's built-in types.

[coderabbitai]

Actionable comments posted: 8

♻️ Duplicate comments (5)

tests/e2e/devices.spec.ts (2)

78-82: ⚠️ Potential issue | 🟡 Minor

Match the refresh request, not any devices query.

The predicate still accepts any POST /graphql whose payload mentions devices, so a background devices request can satisfy refreshRequest before the click-triggered refetch arrives. Match the specific operation name or unique query text fired by Refresh.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/devices.spec.ts` around lines 78 - 82, The current refreshRequest
predicate (created via page.waitForResponse) is too broad and matches any POST
/graphql mentioning "devices"; update the predicate in refreshRequest to parse
response.request().postData() as JSON and match the specific GraphQL operation
name or unique query string used by the refresh action (e.g. check
parsed.operationName === "<RefreshOperationName>" or that parsed.query contains
a unique fragment of the refresh query). This ensures refreshRequest only
resolves for the click-triggered RefreshDevices request rather than other
background devices queries.

---

30-35: 🧹 Nitpick | 🔵 Trivial

Use a stable backdrop locator here.

This click only works while the backdrop itself owns role="dialog". If that role moves to the modal panel, the test will start clicking content instead of the backdrop. Prefer a dedicated backdrop selector or document the coupling inline.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/devices.spec.ts` around lines 30 - 35, The test currently clicks
the dialog locator (dialog) which is unstable if the role="dialog" shifts to the
panel; update the test "modal closes on backdrop click" to target a dedicated
backdrop locator instead of dialog (for example a data-test/data-testid
attribute like data-testid="backdrop" or a semantic role such as
role="presentation") and replace the dialog.click call with a click on that
backdrop locator so the test always clicks the backdrop regardless of where
role="dialog" lives.

.github/workflows/e2e.yml (1)

28-34: ⚠️ Potential issue | 🟠 Major

Pin the transient Node tooling used by this job.

This workflow still resolves E2E tooling at runtime: npm install --no-save @playwright/test typescript ts-node pulls whatever is current, and npx serve / npx wait-on will do the same if they are not declared locally. That makes CI nondeterministic and prone to unrelated breakage from upstream releases. Prefer a checked-in manifest + lockfile for tests/e2e and npm ci, or at least pin exact versions for all transient packages.

If tests/e2e has no lockfile and these tools are not declared in-repo, this job is floating them on every run.

#!/bin/bash
set -euo pipefail

echo "Lockfiles under tests/e2e:"
fd -HI '^(package-lock\.json|npm-shrinkwrap\.json|yarn\.lock|pnpm-lock\.yaml)$' tests/e2e 2>/dev/null || true

echo
echo "Declared versions for E2E tooling:"
python - <<'PY'
import json
from pathlib import Path

targets = {"@playwright/test", "typescript", "ts-node", "serve", "wait-on"}

for path in Path(".").rglob("package.json"):
    if any(part in {"node_modules", "dist", "build"} for part in path.parts):
        continue
    try:
        data = json.loads(path.read_text())
    except Exception:
        continue

    hits = []
    for section in ("dependencies", "devDependencies"):
        for name, version in (data.get(section) or {}).items():
            if name in targets:
                hits.append((section, name, version))

    if hits:
        print(f"== {path} ==")
        for section, name, version in hits:
            print(f"{section}: {name} -> {version}")
PY

Also applies to: 47-52

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/e2e.yml around lines 28 - 34, The workflow currently
installs transient Node tooling at runtime in the "Install E2E deps" and
"Install Playwright browsers" steps, making CI nondeterministic; fix by pinning
versions or using a lockfile: ensure tests/e2e includes a package.json and
lockfile (package-lock.json / pnpm-lock.yaml / yarn.lock) and replace the npm
install step with npm ci in the "Install E2E deps" job (or explicitly list exact
versions for `@playwright/test`, typescript, ts-node, serve, wait-on in that step)
so the job uses the checked-in manifest/lockfile or fixed versions instead of
floating installs.

tests/e2e/billing.spec.ts (1)

29-32: ⚠️ Potential issue | 🟡 Minor

Verify the Starter upgrade action, not just its presence.

This still doesn't prove the Starter CTA works: .first() only assumes the first Upgrade button belongs to Starter, and the test never clicks it. Scope the locator to the Starter card and assert the post-click effect.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/billing.spec.ts` around lines 29 - 32, The test currently only
checks presence of an Upgrade button using upgradeBtn = page.getByRole("button",
{ name: "Upgrade" }).first(); — scope the locator to the Starter card (e.g.,
locate the Starter card/container via its heading or role and then find the
Upgrade button inside it instead of .first()), then perform the click on that
scoped upgradeBtn and assert a concrete post-click effect (for example, a
billing modal/dialog appears, the page navigates to the upgrade flow, or a
confirmation element/text for the Starter upgrade is shown). Update the test
"Starter upgrade button is visible and enabled" to replace the .first() locator
with the scoped locator and add await upgradeBtn.click() followed by an
assertion on the expected dialog/URL/element to prove the CTA works.

apps/gateway/src/middleware/planEnforcement.ts (1)

72-77: ⚠️ Potential issue | 🟡 Minor

Evict malformed tenant-plan cache entries after the parse failure.

Right now the bad value stays in Redis, so every request will re-emit the warning and fall back to Postgres until the TTL expires. Delete cacheKey here before continuing.

🛠️ Minimal fix

       try {
         return JSON.parse(cached) as TenantPlan;
       } catch (error) {
         console.warn("Invalid tenant plan cache entry", { tenantId, error });
+        await redis.del(cacheKey).catch(() => {});
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/middleware/planEnforcement.ts` around lines 72 - 77, When
JSON.parse(cached) fails in the catch block of the tenant-plan cache handling,
evict the malformed Redis entry before continuing: in the catch for parsing
cached (the value cast to TenantPlan), call the Redis client to delete cacheKey
(e.g. await redisClient.del(cacheKey) or await redis.del(cacheKey) depending on
your client var), optionally log the eviction, then continue falling back to
Postgres; update the catch block where cached, cacheKey and TenantPlan are
referenced to perform this deletion.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/chaos.yml:
- Around line 71-79: The "Run — all experiments" step runs
tests/chaos/run-all.sh which invokes tests/chaos/projection-lag.sh that defaults
STRICT_ALERT_CHECK=0 when the env var is absent; to make the
full-suite/scheduled path use the same strict assertions as the dedicated
projection-lag step, add STRICT_ALERT_CHECK: "1" to the env block of the "Run —
all experiments" step (the step that runs run-all.sh) so projection-lag.sh
receives the same setting.
- Around line 29-33: The chaos job is currently unguarded and can run
overlapping experiments; add a GitHub Actions concurrency block to the "chaos"
job to serialize runs per namespace by grouping on the namespace input (falling
back to the default allowlist namespace) so only one run per namespace executes
at a time; add a concurrency key under the chaos job that uses a group like
"chaos-${{ github.event.inputs.namespace || 'grainguard-dev' }}" and set
cancel-in-progress as appropriate (usually false to queue rather than cancel) so
the serialization is namespace-scoped.

In @.github/workflows/ci.yml:
- Around line 26-30: Update the golangci-lint GitHub Action invocation to use a
patch-pinned version instead of the minor tag: in the workflow step named
"golangci-lint" change the uses/with version value from "v2.11" to the
patch-level release (e.g., "v2.11.4") so the golangci/golangci-lint-action@...
run and the with.version setting are fixed to an exact patch, ensuring
reproducible lint results.

In `@apps/gateway/src/middleware/planEnforcement.ts`:
- Around line 264-275: The quota check in the plan enforcement uses only
currentCount from getDeviceCount(tenantId) and ignores the incoming batch size,
so change the check in the block around limits.devices to compute incomingCount
(the number of devices being created/imported from the request payload) and
compare currentCount + incomingCount against limits.devices (e.g., use req.body
length or the parsed import rows as incomingCount); ideally perform this check
and the insertion inside the same transaction/atomic operation to avoid race
conditions — update the logic that references limits.devices, currentCount and
getDeviceCount(tenantId) to include incomingCount and return the same 403
payload when currentCount + incomingCount > limits.devices.
- Around line 118-140: getDeviceCount currently reads from the read-only pool
and a short-lived Redis cache which can undercount concurrent writes; change the
quota checks (enforceDeviceQuota, enforceBulkDeviceQuota, enforceAlertRuleQuota)
to read the authoritative count using the same primary transaction client used
for the mutation instead of pool.query (i.e., accept/propagate a transaction
client/PoolClient into getDeviceCount and call client.query for the COUNT), and
avoid relying on the Redis cache inside the transaction path (either bypass the
cache for the transactional read or only update the cache after committing);
ensure call sites pass the active transaction client to getDeviceCount so the
mutation and quota read are on the same primary transaction client.
- Around line 196-209: When tenant.subscriptionStatus === "past_due" in the
planEnforcement middleware, treat a missing or invalid tenant.currentPeriodEnd
as a failure (closed) instead of falling through: if currentPeriodEnd is
null/unparsable, immediately return the 403 subscription_past_due response (same
shape as the existing response); otherwise compute periodEnd and gracePeriodEnd
as currently done and only allow through if now <= gracePeriodEnd, otherwise
return the 403. Update the logic around periodEnd/gracePeriodEnd (variables
periodEnd, gracePeriodEnd) so the absence or invalid date does not call next()
but returns the error response.
- Around line 312-322: This exported requireFeature middleware is unused; either
remove it or narrow its parameter to only boolean-valued plan keys to avoid
numeric keys slipping through. If removing, delete the requireFeature export and
any tests; if keeping, change the signature on requireFeature to accept only
PlanLimits keys whose value type is boolean (use a conditional/mapped type to
extract boolean-valued keys), keep the runtime check (value === false) and
retain references to getTenantPlan, PLAN_LIMITS and PlanLimits as they are used
to evaluate the feature flag.

In `@tests/e2e/fixtures/mockAuth.ts`:
- Around line 121-151: The current GraphQL mock in mockAuth.ts indiscriminately
uses query.includes(...) which causes false positives (e.g., "me" matches
"name") and masks unhandled queries; update the logic in the route handler that
reads route.request().postDataJSON() to parse the GraphQL operation name (e.g.,
via regex on the "query" string or by parsing the operation block) and match
explicit operation names (e.g., the operations that should return MOCK_DEVICES
or the "Me" operation) instead of substring checks; for any unknown operation
return a non-200 failure (or throw) so tests fail fast and new GraphQL traffic
isn't silently ignored.

---

Duplicate comments:
In @.github/workflows/e2e.yml:
- Around line 28-34: The workflow currently installs transient Node tooling at
runtime in the "Install E2E deps" and "Install Playwright browsers" steps,
making CI nondeterministic; fix by pinning versions or using a lockfile: ensure
tests/e2e includes a package.json and lockfile (package-lock.json /
pnpm-lock.yaml / yarn.lock) and replace the npm install step with npm ci in the
"Install E2E deps" job (or explicitly list exact versions for `@playwright/test`,
typescript, ts-node, serve, wait-on in that step) so the job uses the checked-in
manifest/lockfile or fixed versions instead of floating installs.

In `@apps/gateway/src/middleware/planEnforcement.ts`:
- Around line 72-77: When JSON.parse(cached) fails in the catch block of the
tenant-plan cache handling, evict the malformed Redis entry before continuing:
in the catch for parsing cached (the value cast to TenantPlan), call the Redis
client to delete cacheKey (e.g. await redisClient.del(cacheKey) or await
redis.del(cacheKey) depending on your client var), optionally log the eviction,
then continue falling back to Postgres; update the catch block where cached,
cacheKey and TenantPlan are referenced to perform this deletion.

In `@tests/e2e/billing.spec.ts`:
- Around line 29-32: The test currently only checks presence of an Upgrade
button using upgradeBtn = page.getByRole("button", { name: "Upgrade" }).first();
— scope the locator to the Starter card (e.g., locate the Starter card/container
via its heading or role and then find the Upgrade button inside it instead of
.first()), then perform the click on that scoped upgradeBtn and assert a
concrete post-click effect (for example, a billing modal/dialog appears, the
page navigates to the upgrade flow, or a confirmation element/text for the
Starter upgrade is shown). Update the test "Starter upgrade button is visible
and enabled" to replace the .first() locator with the scoped locator and add
await upgradeBtn.click() followed by an assertion on the expected
dialog/URL/element to prove the CTA works.

In `@tests/e2e/devices.spec.ts`:
- Around line 78-82: The current refreshRequest predicate (created via
page.waitForResponse) is too broad and matches any POST /graphql mentioning
"devices"; update the predicate in refreshRequest to parse
response.request().postData() as JSON and match the specific GraphQL operation
name or unique query string used by the refresh action (e.g. check
parsed.operationName === "<RefreshOperationName>" or that parsed.query contains
a unique fragment of the refresh query). This ensures refreshRequest only
resolves for the click-triggered RefreshDevices request rather than other
background devices queries.
- Around line 30-35: The test currently clicks the dialog locator (dialog) which
is unstable if the role="dialog" shifts to the panel; update the test "modal
closes on backdrop click" to target a dedicated backdrop locator instead of
dialog (for example a data-test/data-testid attribute like
data-testid="backdrop" or a semantic role such as role="presentation") and
replace the dialog.click call with a click on that backdrop locator so the test
always clicks the backdrop regardless of where role="dialog" lives.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ba90765c-1dfd-4a35-97e8-e8bd2d202247

📥 Commits

Reviewing files that changed from the base of the PR and between e1fa0f0 and 818236a.

📒 Files selected for processing (8)

• .github/workflows/chaos.yml
• .github/workflows/ci.yml
• .github/workflows/e2e.yml
• apps/gateway/src/middleware/planEnforcement.ts
• tests/e2e/auth.spec.ts
• tests/e2e/billing.spec.ts
• tests/e2e/devices.spec.ts
• tests/e2e/fixtures/mockAuth.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Serialize chaos runs per namespace.

Because the allowlist currently collapses every run onto grainguard-dev, a scheduled run can overlap with a manual dispatch against the same resources. For destructive experiments, that means cross-contaminated results and longer outages. Add a namespace-scoped concurrency guard here.

Suggested fix

   chaos:
     name: Chaos — ${{ github.event.inputs.experiment || 'all' }}
     runs-on: ubuntu-latest
     timeout-minutes: 30
+    concurrency:
+      group: chaos-${{ github.event.inputs.namespace || 'grainguard-dev' }}
+      cancel-in-progress: false

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/chaos.yml around lines 29 - 33, The chaos job is currently
unguarded and can run overlapping experiments; add a GitHub Actions concurrency
block to the "chaos" job to serialize runs per namespace by grouping on the
namespace input (falling back to the default allowlist namespace) so only one
run per namespace executes at a time; add a concurrency key under the chaos job
that uses a group like "chaos-${{ github.event.inputs.namespace ||
'grainguard-dev' }}" and set cancel-in-progress as appropriate (usually false to
queue rather than cancel) so the serialization is namespace-scoped.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

all mode runs projection-lag with weaker assertions.

tests/chaos/run-all.sh calls tests/chaos/projection-lag.sh, and that script falls back to STRICT_ALERT_CHECK=0 when the env var is absent. This step omits it, while the dedicated projection-lag step sets "1", so the scheduled/full-suite path can pass an alert regression that the single-experiment path would fail.

Suggested fix

       - name: Run — all experiments
         if: ${{ github.event.inputs.experiment == 'all' || github.event_name == 'schedule' }}
         env:
           NAMESPACE: ${{ env.NAMESPACE }}
           KAFKA_BOOTSTRAP: kafka:9092
           GATEWAY_URL: ${{ secrets.CHAOS_GATEWAY_URL }}
           PROMETHEUS_URL: ${{ secrets.CHAOS_PROMETHEUS_URL }}
           TEST_JWT: ${{ secrets.CHAOS_TEST_JWT }}
+          STRICT_ALERT_CHECK: "1"
         run: bash tests/chaos/run-all.sh

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/chaos.yml around lines 71 - 79, The "Run — all
experiments" step runs tests/chaos/run-all.sh which invokes
tests/chaos/projection-lag.sh that defaults STRICT_ALERT_CHECK=0 when the env
var is absent; to make the full-suite/scheduled path use the same strict
assertions as the dedicated projection-lag step, add STRICT_ALERT_CHECK: "1" to
the env block of the "Run — all experiments" step (the step that runs
run-all.sh) so projection-lag.sh receives the same setting.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Move quota reads onto the primary transaction path.

apps/gateway/src/database/db.ts defines pool as the read DB, and getDeviceCount() adds a 60-second Redis cache on top. enforceDeviceQuota(), enforceBulkDeviceQuota(), and enforceAlertRuleQuota() use these values to admit mutations, so replica lag or stale cache can undercount recent writes and let tenants exceed their plan. Read the authoritative count with the same primary transaction client that performs the mutation, or enforce the limit with a DB-side lock/constraint. As per coding guidelines, "Verify every mutating route uses a real transaction client instead of pool-level BEGIN/COMMIT."

Also applies to: 143-149

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/middleware/planEnforcement.ts` around lines 118 - 140,
getDeviceCount currently reads from the read-only pool and a short-lived Redis
cache which can undercount concurrent writes; change the quota checks
(enforceDeviceQuota, enforceBulkDeviceQuota, enforceAlertRuleQuota) to read the
authoritative count using the same primary transaction client used for the
mutation instead of pool.query (i.e., accept/propagate a transaction
client/PoolClient into getDeviceCount and call client.query for the COUNT), and
avoid relying on the Redis cache inside the transaction path (either bypass the
cache for the transactional read or only update the cache after committing);
ensure call sites pass the active transaction client to getDeviceCount so the
mutation and quota read are on the same primary transaction client.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

past_due currently fails open when currentPeriodEnd is missing or invalid.

apps/telemetry-service/migrations/000007_saas_columns.up.sql makes tenants.current_period_end nullable, and this branch just falls through to next() when the value is null or unparsable. That turns any billing sync/backfill gap into indefinite access for past_due subscriptions.

🛠️ Fail closed when the grace window cannot be computed

     if (tenant.subscriptionStatus === "past_due") {
       const periodEnd = tenant.currentPeriodEnd ? new Date(tenant.currentPeriodEnd) : null;
-      const gracePeriodEnd = periodEnd
-        ? new Date(periodEnd.getTime() + 7 * 24 * 60 * 60 * 1000)
-        : null;
+      if (!periodEnd || Number.isNaN(periodEnd.getTime())) {
+        return res.status(403).json({
+          error: "subscription_past_due",
+          message: "Payment is past due. Unable to verify the grace period.",
+          upgradeUrl: "/billing",
+        });
+      }
+      const gracePeriodEnd = new Date(
+        periodEnd.getTime() + 7 * 24 * 60 * 60 * 1000
+      );

       if (gracePeriodEnd && new Date() > gracePeriodEnd) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/middleware/planEnforcement.ts` around lines 196 - 209, When
tenant.subscriptionStatus === "past_due" in the planEnforcement middleware,
treat a missing or invalid tenant.currentPeriodEnd as a failure (closed) instead
of falling through: if currentPeriodEnd is null/unparsable, immediately return
the 403 subscription_past_due response (same shape as the existing response);
otherwise compute periodEnd and gracePeriodEnd as currently done and only allow
through if now <= gracePeriodEnd, otherwise return the 403. Update the logic
around periodEnd/gracePeriodEnd (variables periodEnd, gracePeriodEnd) so the
absence or invalid date does not call next() but returns the error response.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Include the incoming bulk size in the quota check.

This only blocks tenants who are already at the cap. A tenant at 4/5 devices can still import 100 rows here because the check never adds the requested batch size. Compare currentCount + incomingCount against limits.devices, ideally in the same transaction as the insert.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/middleware/planEnforcement.ts` around lines 264 - 275, The
quota check in the plan enforcement uses only currentCount from
getDeviceCount(tenantId) and ignores the incoming batch size, so change the
check in the block around limits.devices to compute incomingCount (the number of
devices being created/imported from the request payload) and compare
currentCount + incomingCount against limits.devices (e.g., use req.body length
or the parsed import rows as incomingCount); ideally perform this check and the
insertion inside the same transaction/atomic operation to avoid race conditions
— update the logic that references limits.devices, currentCount and
getDeviceCount(tenantId) to include incomingCount and return the same 403
payload when currentCount + incomingCount > limits.devices.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "All direct requireFeature call sites:"
rg -nP --type=ts '\brequireFeature\s*\('

echo
echo "Potential numeric-key usages:"
rg -nP --type=ts '\brequireFeature\s*\(\s*["'"'"'](devices|alertRules|apiRateLimit)["'"'"']\s*\)'

Repository: pahuldeepp/GrainGuard-

Length of output: 245

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find and display the PlanLimits type definition
echo "=== PlanLimits type definition ==="
fd -e ts -e tsx | xargs rg -l "type PlanLimits|interface PlanLimits" | head -5

# Show the definition
echo ""
rg "type PlanLimits|interface PlanLimits" -A 20 --type=ts

Repository: pahuldeepp/GrainGuard-

Length of output: 1563

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check if requireFeature is imported or used in route files
echo "=== Imports of requireFeature ==="
rg "requireFeature" --type=ts -B 2 -A 2

# Check the planEnforcement.ts file itself for exports
echo ""
echo "=== Contents of planEnforcement.ts (first 50 lines and around line 312) ==="
head -50 apps/gateway/src/middleware/planEnforcement.ts
echo ""
echo "--- Lines around 312 ---"
sed -n '300,330p' apps/gateway/src/middleware/planEnforcement.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 2901

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search for route configurations that might mount this middleware
echo "=== Looking for route mounting patterns ==="
fd -e ts -e tsx "route" | xargs grep -l "middleware\|planEnforcement" 2>/dev/null || echo "No direct route files found"

echo ""
echo "=== Checking app startup/server files ==="
fd -e ts -e tsx "server|index|app" apps/gateway/src | head -10 | xargs grep -l "middleware" 2>/dev/null || echo "No matches"

Repository: pahuldeepp/GrainGuard-

Length of output: 207

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check server.ts for requireFeature usage
echo "=== requireFeature references in server.ts ==="
grep -n "requireFeature" apps/gateway/src/server.ts || echo "No matches in server.ts"

# Check all route/handler files for usage
echo ""
echo "=== All TypeScript files that mention requireFeature ==="
fd -e ts -e tsx | xargs grep -l "requireFeature" 2>/dev/null

# Look at complete server.ts to understand middleware mounting
echo ""
echo "=== Contents of server.ts ==="
wc -l apps/gateway/src/server.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 303

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# List all files in routes directory if it exists
echo "=== Files in routes/handlers directories ==="
fd -e ts -e tsx "route|handler" apps/gateway/src | head -20 || echo "No route/handler files found"

# Check server.ts to see what middleware IS being mounted
echo ""
echo "=== Middleware references in server.ts ==="
grep -n "middleware\|planEnforcement\|require\|import" apps/gateway/src/server.ts | head -20

# Check if planEnforcement.ts exports anything else
echo ""
echo "=== All exports from planEnforcement.ts ==="
grep "^export" apps/gateway/src/middleware/planEnforcement.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 1644

---

Remove unused requireFeature middleware or fix its type if intended for future use.

requireFeature is exported but never imported or mounted anywhere in the codebase—it's dead code. If this middleware is intended for future use, the type parameter needs narrowing: keyof PlanLimits includes numeric keys (devices, alertRules, apiRateLimit), but the logic only checks value === false. Passing a numeric key would silently bypass the check. Either remove the function or restrict the parameter to boolean-valued keys only:

+type BooleanPlanFeature = {
+  [K in keyof PlanLimits]: PlanLimits[K] extends boolean ? K : never;
+}[keyof PlanLimits];
+
-export function requireFeature(feature: keyof PlanLimits) {
+export function requireFeature(feature: BooleanPlanFeature) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/middleware/planEnforcement.ts` around lines 312 - 322, This
exported requireFeature middleware is unused; either remove it or narrow its
parameter to only boolean-valued plan keys to avoid numeric keys slipping
through. If removing, delete the requireFeature export and any tests; if
keeping, change the signature on requireFeature to accept only PlanLimits keys
whose value type is boolean (use a conditional/mapped type to extract
boolean-valued keys), keep the runtime check (value === false) and retain
references to getTenantPlan, PLAN_LIMITS and PlanLimits as they are used to
evaluate the feature flag.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (1)

.github/workflows/ci.yml (1)

81-87: ⚠️ Potential issue | 🟠 Major

This issue was previously flagged and remains unresolved.

The conditional lint check will silently skip linting for apps without a lint script. Based on the provided context, gateway and jobs-worker lack lint scripts, meaning they'll be silently exempted from linting—a regression in CI coverage.

The previous review correctly identified this as a major issue and suggested either:

1. Failing the job when no lint script exists, or
2. Explicitly modeling exemptions in the matrix configuration

This concern remains valid and should be addressed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 81 - 87, The ESLint job currently
skips linting silently when an app has no "lint" npm script; update the ESLint
step (the job named "ESLint" that runs the shell check using npm run | grep -qE
'^[[:space:]]+lint') to fail instead of skipping by exiting non‑zero if no lint
script is found, or alternatively change the workflow matrix to explicitly model
lint exemptions (e.g., add a matrix field like lint: true/false and guard the
ESLint job to run only when matrix.lint is true); modify either the shell
snippet to call "echo ... && exit 1" on missing lint or update the matrix and
the job condition so apps without lint are explicit and do not silently bypass
linting.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/bff/src/resolvers.ts`:
- Around line 12-15: toIsoDate currently assumes inputs are valid and will throw
RangeError on malformed date-like strings; update to validate inputs and return
a safe ISO string or a defined fallback (e.g., null or an empty string) instead
of throwing. Inside toIsoDate, check for null/undefined, reject
non-string/non-number/non-Date values, and for string/number attempt to
construct Date and verify !isNaN(date.getTime()) before calling toISOString; if
validation fails, return the chosen fallback. Keep the function signature and
replace/remove redundant guards at callsites (the mixed ternary guards) so
callers simply call toIsoDate(value) without additional checks. Make sure to
reference the toIsoDate function where you centralize validation and update all
callers to rely on its consistent behavior.

In `@apps/dashboard/src/e2e/auth0Mock.tsx`:
- Around line 110-118: The mocked loginWithRedirect should persist the shared
test token and update the in-memory token state so isAuthenticated becomes true;
before calling navigateTo(returnTo) and onRedirectCallback, load the canonical
fake token from the e2e fixture (tests/e2e/fixtures/mockAuth.ts) and store it
under TOKEN_KEY in localStorage and set the module's token variable (or call the
existing setter used by this mock) so subsequent checks read the authenticated
token; then proceed to navigateTo(returnTo) and call onRedirectCallback as
before.
- Around line 119-124: The logout(options) mock currently calls
onRedirectCallback, which diverges from real `@auth0/auth0-react` behavior; remove
the onRedirectCallback?.({ returnTo }) invocation from the logout function
(identified as logout(options) in this file) so logout only clears TOKEN_KEY via
window.localStorage.removeItem, calls setToken(null), computes returnTo with
toSameOriginPath, and performs navigateTo(returnTo, "replace") without invoking
onRedirectCallback; leave other logic (TOKEN_KEY, setToken, toSameOriginPath,
navigateTo) unchanged.

---

Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 81-87: The ESLint job currently skips linting silently when an app
has no "lint" npm script; update the ESLint step (the job named "ESLint" that
runs the shell check using npm run | grep -qE '^[[:space:]]+lint') to fail
instead of skipping by exiting non‑zero if no lint script is found, or
alternatively change the workflow matrix to explicitly model lint exemptions
(e.g., add a matrix field like lint: true/false and guard the ESLint job to run
only when matrix.lint is true); modify either the shell snippet to call "echo
... && exit 1" on missing lint or update the matrix and the job condition so
apps without lint are explicit and do not silently bypass linting.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e8a1bc3a-2bfc-4689-a949-80297a2137eb

📥 Commits

Reviewing files that changed from the base of the PR and between 818236a and 9e98fa6.

📒 Files selected for processing (4)

• .github/workflows/ci.yml
• apps/bff/src/resolvers.ts
• apps/dashboard/src/e2e/auth0Mock.tsx
• apps/dashboard/src/features/tenancy/TenantContext.tsx

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Resolver callsites using toIsoDate =="
rg -n -C2 'toIsoDate\(' apps/bff/src/resolvers.ts

echo
echo "== Postgres/Cassandra datasource contracts for timestamp fields =="
fd -i 'postgres' apps/bff/src | xargs -r rg -n -C2 'getDeviceWithTelemetry|getDeviceTelemetry|getAllTelemetry|getTelemetryHistory|created_at|updated_at|recorded_at'
fd -i 'cassandra' apps/bff/src | xargs -r rg -n -C2 'getTelemetryHistoryFromCassandra|recordedAt|recorded_at'

echo
echo "== SQL schema/migrations nullability for timestamp columns =="
fd -e sql | xargs -r rg -n -C2 'created_at|updated_at|recorded_at|NOT NULL'

Repository: pahuldeepp/GrainGuard-

Length of output: 44934

---

🏁 Script executed:

# Check context around line 218 to understand the camelCase field mapping
sed -n '210,225p' apps/bff/src/resolvers.ts

Repository: pahuldeepp/GrainGuard-

Length of output: 736

---

Add input validation to toIsoDate to prevent resolver failures on malformed timestamps.

Line 13–14 can throw RangeError: Invalid time value if upstream data contains invalid date-like strings. While database schema enforces NOT NULL on all timestamp columns, defensive validation prevents edge cases from crashing resolvers.

Note: Mixed guarding patterns across callsites (lines 45, 70 use ternary guards; line 42 does not) despite all columns being NOT NULL indicate inconsistent safety practices. Centralize validation in toIsoDate and remove redundant guards.

🔧 Suggested fix

 function toIsoDate(value: unknown): string {
-  if (value instanceof Date) return value.toISOString();
-  return new Date(value as string | number).toISOString();
+  if (value === null || value === undefined) {
+    throw new Error("Expected date value but received null/undefined");
+  }
+  const date = value instanceof Date ? value : new Date(value as string | number);
+  if (Number.isNaN(date.getTime())) {
+    throw new Error(`Invalid date value: ${String(value)}`);
+  }
+  return date.toISOString();
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	function toIsoDate(value: unknown): string {
	if (value instanceof Date) return value.toISOString();
	return new Date(value as string | number).toISOString();
	}
	function toIsoDate(value: unknown): string {
	if (value === null || value === undefined) {
	throw new Error("Expected date value but received null/undefined");
	}
	const date = value instanceof Date ? value : new Date(value as string | number);
	if (Number.isNaN(date.getTime())) {
	throw new Error(`Invalid date value: ${String(value)}`);
	}
	return date.toISOString();
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/bff/src/resolvers.ts` around lines 12 - 15, toIsoDate currently assumes
inputs are valid and will throw RangeError on malformed date-like strings;
update to validate inputs and return a safe ISO string or a defined fallback
(e.g., null or an empty string) instead of throwing. Inside toIsoDate, check for
null/undefined, reject non-string/non-number/non-Date values, and for
string/number attempt to construct Date and verify !isNaN(date.getTime()) before
calling toISOString; if validation fails, return the chosen fallback. Keep the
function signature and replace/remove redundant guards at callsites (the mixed
ternary guards) so callers simply call toIsoDate(value) without additional
checks. Make sure to reference the toIsoDate function where you centralize
validation and update all callers to rely on its consistent behavior.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Make mocked login establish an authenticated session.

loginWithRedirect() changes the route, but it never persists TOKEN_KEY or updates token, so isAuthenticated stays false after a login click. In the aliased E2E build, that leaves guards and auth-dependent pages behaving as logged out even though the redirect completed. There is already a canonical fake token in tests/e2e/fixtures/mockAuth.ts:15-62; this mock should load the same fixture token (or another shared test token) before navigating.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/dashboard/src/e2e/auth0Mock.tsx` around lines 110 - 118, The mocked
loginWithRedirect should persist the shared test token and update the in-memory
token state so isAuthenticated becomes true; before calling navigateTo(returnTo)
and onRedirectCallback, load the canonical fake token from the e2e fixture
(tests/e2e/fixtures/mockAuth.ts) and store it under TOKEN_KEY in localStorage
and set the module's token variable (or call the existing setter used by this
mock) so subsequent checks read the authenticated token; then proceed to
navigateTo(returnTo) and call onRedirectCallback as before.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In @auth0/auth0-react, when is Auth0Provider's onRedirectCallbackinvoked, and is it ever called bylogout()?

💡 Result:

Auth0Provider's onRedirectCallback is invoked when the application detects Auth0 authorization code and state parameters in the current URL (via hasAuthParams), typically after Auth0 redirects back to the app following a successful loginWithRedirect flow. It processes the callback using the underlying Auth0Client.handleRedirectCallback, retrieves the user if applicable, executes the provided callback function (default removes code/state params via history.replaceState), and clears loading state. It is not called by logout. The logout method constructs a logout URL (with optional returnTo) and performs a direct window.location.replace to Auth0's /v2/logout endpoint, which handles logout and redirects according to its own parameters without returning through the app's callback URL or invoking onRedirectCallback.

Citations:

• 1: https://auth0.github.io/auth0-react/interfaces/Auth0ProviderOptions.html
• 2: https://github.com/auth0/auth0-react/blob/main/src/auth0-provider.tsx
• 3: https://github.com/auth0/auth0-react/blob/master/EXAMPLES.md
• 4: https://auth0.com/docs/libraries/auth0-react
• 5: https://auth0.github.io/auth0-react/interfaces/LogoutOptions.html

---

🏁 Script executed:

# Check if the file exists and examine the logout function
cat -n apps/dashboard/src/e2e/auth0Mock.tsx | sed -n '110,135p'

Repository: pahuldeepp/GrainGuard-

Length of output: 1140

---

🏁 Script executed:

# Search for any dependencies on this logout callback behavior
rg "onRedirectCallback" apps/dashboard/src/e2e/ -A 2 -B 2

Repository: pahuldeepp/GrainGuard-

Length of output: 1760

---

🏁 Script executed:

# Check if there are tests that rely on the callback being fired during logout
rg "logout" apps/dashboard/src/e2e/ -A 3 -B 1 | head -50

Repository: pahuldeepp/GrainGuard-

Length of output: 1312

---

Remove onRedirectCallback invocation from logout().

onRedirectCallback is Auth0's post-login redirect hook. The real @auth0/auth0-react SDK does not invoke it during logout—only after a successful loginWithRedirect flow. Calling it in the mock creates test-only side effects that diverge from production behavior, risking E2E tests passing when logout would fail in production.

Fix

      logout(options) {
        window.localStorage.removeItem(TOKEN_KEY);
        setToken(null);
        const returnTo = toSameOriginPath(options?.logoutParams?.returnTo ?? "/");
        navigateTo(returnTo, "replace");
-       onRedirectCallback?.({ returnTo });
      },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/dashboard/src/e2e/auth0Mock.tsx` around lines 119 - 124, The
logout(options) mock currently calls onRedirectCallback, which diverges from
real `@auth0/auth0-react` behavior; remove the onRedirectCallback?.({ returnTo })
invocation from the logout function (identified as logout(options) in this file)
so logout only clears TOKEN_KEY via window.localStorage.removeItem, calls
setToken(null), computes returnTo with toSameOriginPath, and performs
navigateTo(returnTo, "replace") without invoking onRedirectCallback; leave other
logic (TOKEN_KEY, setToken, toSameOriginPath, navigateTo) unchanged.