fix(ci): golangci-lint v2, SQL injection fix, BFF/dashboard lint cleanup#10
Merged
pahuldeepp merged 9 commits intomasterfrom Mar 28, 2026
Merged
fix(ci): golangci-lint v2, SQL injection fix, BFF/dashboard lint cleanup#10pahuldeepp merged 9 commits intomasterfrom
pahuldeepp merged 9 commits intomasterfrom
Conversation
- Move //nolint:staticcheck inline with bodyPool.Put (was on wrong line, so SA6002 was not suppressed) - Replace dynamic WHERE clause construction in auditLog.ts with static parameterised query using IS NULL OR col = \$N pattern so CodeQL can verify no user data reaches the SQL string Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In v2 the exclude-dirs option belongs under run, not issues. The schema validator was rejecting the config with "additional properties 'exclude-dirs' not allowed" under issues. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- resolvers.ts: _ → unknown, cache.get<any> → Record<string,unknown>, row: any → Record<string,unknown>, resolve payloads → unknown - circuitBreaker.ts: err: any → unknown with type narrowing via cast - redis.ts: cache.set value param any → unknown - metrics.ts: import Express Request/Response types instead of any - server.ts: remove unused postgresCircuitBreaker import, replace payload as any casts with Record<string,unknown>, tighten WS ctx type - WebhooksPage.tsx: catch (e: any) → catch (e) (4 occurrences) - BillingPage.tsx: add setSearchParams to useEffect dependency array - exportCsv.test.ts: as any → as unknown as HTMLElement / typeof Blob Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTightens TypeScript typings across the BFF, updates CI to use the golangci-lint GitHub Action with action-controlled version, replaces dynamic SQL assembly with static parameterized queries, makes tenant-scoped Postgres operations transactional, and standardizes several error-handling and lint-suppression sites. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- .golangci.yml: move linters-settings → linters.settings (v2 schema), remove exclude-dirs from run (not valid in v2); all linter-specific settings are now nested under linters.settings per v2 format - WebhooksPage.tsx: narrow caught error type with instanceof Error check so e.message compiles under useUnknownInCatchVariables: true Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/gateway/src/routes/devicesImport.ts (1)
136-142: 🧹 Nitpick | 🔵 TrivialConsider wrapping job creation and status update in a transaction.
The bulk import job is created with
pool.query(line 136), devices are processed, then job status is updated (line 177). If the process crashes between device creation and status update, the job remains in'running'state indefinitely.As per coding guidelines: "Verify every mutating route uses a real transaction client instead of pool-level BEGIN/COMMIT."
♻️ Suggested approach
Consider using a transaction client for the job lifecycle, or implement a cleanup mechanism for stale
'running'jobs. At minimum, wrap the final UPDATE in error handling to ensure it completes.🤖 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, Wrap the bulk_import_jobs creation and its final status UPDATE in a real DB transaction using a transaction client instead of pool.query: acquire a client via pool.connect(), call BEGIN, run the INSERT into bulk_import_jobs (currently using pool.query that returns jobId) and all subsequent device INSERTs/updates using that client, then COMMIT after the final UPDATE of the job status; ensure you handle errors with a try/catch/finally that issues ROLLBACK on failure and always releases the client so the job can't be left permanently in 'running' state.apps/bff/src/resolvers.ts (5)
37-41:⚠️ Potential issue | 🔴 CriticalPipeline failure:
unknownnot assignable toDateconstructor.The pipeline fails because
row.created_atandrow.recorded_atare typed asunknown(fromRecord<string, unknown>), butnew Date()requiresstring | number | Date.🐛 Fix: Cast fields before Date construction
const result = { deviceId: row.device_id, tenantId: row.tenant_id, serialNumber: row.serial_number, - createdAt: new Date(row.created_at).toISOString(), + createdAt: new Date(row.created_at as string).toISOString(), temperature: row.temperature || null, humidity: row.humidity || null, - recordedAt: row.recorded_at ? new Date(row.recorded_at).toISOString() : null, + recordedAt: row.recorded_at ? new Date(row.recorded_at as string).toISOString() : null, version: row.version || null, };🤖 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 37 - 41, The failure is due to passing unknown-typed fields into new Date; update the mapping in resolvers.ts (the block assigning createdAt and recordedAt from row) to first cast or validate row.created_at and row.recorded_at before constructing Dates (e.g. coerce with String(row.created_at) or check typeof and then use row.created_at as string|number, and preserve null handling for recorded_at); ensure createdAt and recordedAt receive valid string|number|Date inputs or null so new Date(...) no longer receives unknown.
130-137:⚠️ Potential issue | 🔴 CriticalPipeline failure:
allTelemetryresolver needs casts.🐛 Fix
const result = rows.map((row: Record<string,unknown>) => ({ deviceId: row.device_id, temperature: row.temperature, humidity: row.humidity, - recordedAt: new Date(row.recorded_at).toISOString(), - updatedAt: new Date(row.updated_at).toISOString(), + recordedAt: new Date(row.recorded_at as string).toISOString(), + updatedAt: new Date(row.updated_at as string).toISOString(), version: row.version, }));🤖 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 130 - 137, The allTelemetry resolver's rows.map currently assumes correct JS types; explicitly cast each row field: coerce row.device_id to string, temperature and humidity to numbers (e.g., Number(row.temperature)), version to number, and ensure recorded_at/updated_at are converted via new Date(...) before calling toISOString(); update the mapping in the rows.map callback (referencing device_id, temperature, humidity, recorded_at, updated_at, version) so runtime type mismatches are avoided and the resolver returns the correct typed shape.
58-67:⚠️ Potential issue | 🔴 CriticalPipeline failure: Same
unknowntoDateissue indevicesresolver.Apply the same cast fix for
row.created_atandrow.recorded_at.🐛 Fix
const result = rows.map((row: Record<string,unknown>) => ({ deviceId: row.device_id, tenantId: row.tenant_id, serialNumber: row.serial_number, - createdAt: new Date(row.created_at).toISOString(), + createdAt: new Date(row.created_at as string).toISOString(), temperature: row.temperature || null, humidity: row.humidity || null, - recordedAt: row.recorded_at ? new Date(row.recorded_at).toISOString() : null, + recordedAt: row.recorded_at ? new Date(row.recorded_at as string).toISOString() : null, version: row.version || null, }));🤖 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 58 - 67, The devices resolver maps DB rows and currently passes values typed as unknown into new Date (in the rows.map block creating createdAt and recordedAt), causing the same unknown->Date issue; update the mapping to cast row.created_at and row.recorded_at to string before constructing Dates (e.g., use a string cast or String(...) for created_at and conditional cast for recorded_at) so createdAt = new Date(row.created_at as string).toISOString() and recordedAt = row.recorded_at ? new Date(row.recorded_at as string).toISOString() : null; keep the rest of the mapping (deviceId, tenantId, serialNumber, temperature, humidity, version) unchanged and refer to the rows.map mapping function in resolvers.ts.
172-179:⚠️ Potential issue | 🔴 CriticalPipeline failure:
manyDeviceTelemetryresolver needs casts.🐛 Fix
const result = { deviceId: row.device_id, temperature: row.temperature, humidity: row.humidity, - recordedAt: new Date(row.recorded_at).toISOString(), - updatedAt: new Date(row.updated_at).toISOString(), + recordedAt: new Date(row.recorded_at as string).toISOString(), + updatedAt: new Date(row.updated_at as string).toISOString(), version: row.version, };🤖 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 172 - 179, The manyDeviceTelemetry resolver is returning raw DB row fields without proper type casts; update the construction of result in the manyDeviceTelemetry function so numeric fields are explicitly cast (e.g. temperature and humidity via Number(...) or parseFloat(...), version via Number(...) or parseInt(...)) and ensure timestamp fields are converted to ISO strings from the correct input type (use new Date(Number(row.recorded_at)) / new Date(Number(row.updated_at)) if DB returns epoch numbers, or new Date(row.recorded_at) if DB returns ISO strings). Keep deviceId as a string and apply these casts where the result object is built (refer to result, row.temperature, row.humidity, row.version, row.recorded_at, row.updated_at).
100-102:⚠️ Potential issue | 🔴 CriticalPipeline failure:
deviceTelemetryresolver needs casts.🐛 Fix
const result = { deviceId: row.device_id, temperature: row.temperature, humidity: row.humidity, - recordedAt: new Date(row.recorded_at).toISOString(), - updatedAt: new Date(row.updated_at).toISOString(), + recordedAt: new Date(row.recorded_at as string).toISOString(), + updatedAt: new Date(row.updated_at as string).toISOString(), version: row.version, };🤖 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 100 - 102, The deviceTelemetry resolver is constructing recordedAt/updatedAt from row fields without type casts which causes pipeline type errors; update the mapping in the resolver to explicitly cast row.recorded_at and row.updated_at to the correct primitive (e.g., Number(row.recorded_at) or String(...) depending on DB type) before passing into new Date(...), and cast row.version to the expected type (e.g., Number(row.version) or String(row.version)) so recordedAt, updatedAt and version satisfy the resolver's return types (refer to the recordedAt, updatedAt, version properties and the row variable inside the deviceTelemetry resolver).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 12-23: Re-enable the unused and gosimple linters by adding
"unused" and "gosimple" to the enabled list in the .golangci.yml (i.e., include
them alongside errcheck, govet, staticcheck, etc.), or if they were
intentionally disabled, add a short comment in the file explaining the rationale
and where to revisit this decision; reference the existing enable block that
lists errcheck, govet, staticcheck, ineffassign, gocritic, gosec, prealloc,
bodyclose, noctx, exhaustive to locate where to add the two linters or the
explanatory comment.
In `@apps/bff/src/datasources/elasticsearch.ts`:
- Around line 47-49: Replace the manual cast on the es.search call with the
client’s generic typing: call es.search with the generic type parameter for the
hit _source (use ESHit['_source']) instead of casting the function; update the
declaration where you currently do (es.search as (p: typeof params) =>
Promise<...>)(params) to a direct es.search<ESHit['_source']>(params) so the
returned result has proper typed hits for the subsequent result.hits.hits.map
usage.
In `@apps/bff/src/datasources/postgres.ts`:
- Around line 335-336: In createDevice and updateDevice the createdAt field is
built using new Date(result.rows[0].created_at as string).toISOString(), which
is inconsistent with how cursors are handled elsewhere; replace that direct Date
construction with the same cursor-decoding helper used by other cursor logic
(e.g., the project function that converts stored cursor timestamps to ISO such
as parseCursorDate/decodeCursor/convertCursorToISO) and pass
result.rows[0].created_at through that helper so createdAt is produced the same
way as other cursor-derived dates.
- Around line 298-307: The code constructs edges via items.map and
inconsistently handles created_at: the cursor uses (row.created_at as
Date).toISOString() while node.createdAt wraps it in new Date(...), which is
redundant and can fail; update node.createdAt to mirror the cursor by using
(row.created_at as Date).toISOString() and similarly ensure recordedAt handles
row.recorded_at safely by checking if it's a Date then calling .toISOString()
(e.g., recordedAt: row.recorded_at ? (row.recorded_at as Date).toISOString() :
null) so both cursor and node fields consistently treat timestamps as Date
objects.
In `@apps/bff/src/datasources/redis.ts`:
- Line 27: The map callback in getMany still uses (r: any); change it to a
properly typed signature reflecting pipeline results (e.g., (r: string | null))
and propagate a generic type for parsed results (e.g., getMany<T> returning (T |
null)[]). Update the mapping to use (r: string | null) => r ? JSON.parse(r) as T
: null and consider wrapping JSON.parse in a try/catch or a safe parser if
needed; reference the getMany function and the results variable (pipeline
output) when making the change.
In `@apps/bff/src/resolvers.ts`:
- Around line 23-26: Define concrete TypeScript interfaces for DB rows (e.g.,
DeviceRow, TelemetryRow) that match the schema and replace uses of
Record<string, unknown>; update the resolvers (notably the device resolver and
other spots around the device/telemetry handlers) to use
cache.get<DeviceRow>(cacheKey) and typed query/result variables instead of
casting fields from Record<string, unknown>, then adjust return types of
functions like device to return DeviceRow (or a mapped DTO) so field accesses
are type-checked at compile time.
- Around line 209-214: The deviceTelemetryHistory resolver is returning values
from rows with no type casts, causing the pipeline failure; update the rows.map
transform in deviceTelemetryHistory to cast each field to the expected types
(e.g., convert row.deviceId to string, row.temperature and row.humidity to
number, and ensure row.recordedAt is parsed/cast safely to a Date/ISO string)
and handle nullable/undefined values appropriately so the resolver always
returns the correct typed shape.
In `@apps/dashboard/src/features/webhooks/WebhooksPage.tsx`:
- Around line 48-49: Change each catch block in WebhooksPage.tsx to treat the
caught value as unknown and extract a safe message using the same pattern used
elsewhere: use "e instanceof Error ? e.message : 'Failed to load webhooks'" (or
the appropriate fallback per block) instead of accessing e.message directly.
Update the five catch blocks that call toast.error in the functions/renderers
referenced around the catch sites (the try/catch handling in the data fetch and
action handlers within the WebhooksPage component) so they use the instanceof
Error check and provide the existing fallback strings.
In `@apps/gateway/src/routes/auditLog.ts`:
- Line 45: Validate the incoming before string before calling new
Date(before).toISOString(): check with Date.parse(before) or isNaN check (e.g.,
if (!before || isNaN(Date.parse(before))) return a 400 error) or wrap the
conversion in try/catch and return 400 on failure; update the route handler that
reads the before parameter (the code using new Date(before).toISOString()) to
perform this validation and return a clear 400 response for invalid dates
instead of letting RangeError be thrown.
---
Outside diff comments:
In `@apps/bff/src/resolvers.ts`:
- Around line 37-41: The failure is due to passing unknown-typed fields into new
Date; update the mapping in resolvers.ts (the block assigning createdAt and
recordedAt from row) to first cast or validate row.created_at and
row.recorded_at before constructing Dates (e.g. coerce with
String(row.created_at) or check typeof and then use row.created_at as
string|number, and preserve null handling for recorded_at); ensure createdAt and
recordedAt receive valid string|number|Date inputs or null so new Date(...) no
longer receives unknown.
- Around line 130-137: The allTelemetry resolver's rows.map currently assumes
correct JS types; explicitly cast each row field: coerce row.device_id to
string, temperature and humidity to numbers (e.g., Number(row.temperature)),
version to number, and ensure recorded_at/updated_at are converted via new
Date(...) before calling toISOString(); update the mapping in the rows.map
callback (referencing device_id, temperature, humidity, recorded_at, updated_at,
version) so runtime type mismatches are avoided and the resolver returns the
correct typed shape.
- Around line 58-67: The devices resolver maps DB rows and currently passes
values typed as unknown into new Date (in the rows.map block creating createdAt
and recordedAt), causing the same unknown->Date issue; update the mapping to
cast row.created_at and row.recorded_at to string before constructing Dates
(e.g., use a string cast or String(...) for created_at and conditional cast for
recorded_at) so createdAt = new Date(row.created_at as string).toISOString() and
recordedAt = row.recorded_at ? new Date(row.recorded_at as string).toISOString()
: null; keep the rest of the mapping (deviceId, tenantId, serialNumber,
temperature, humidity, version) unchanged and refer to the rows.map mapping
function in resolvers.ts.
- Around line 172-179: The manyDeviceTelemetry resolver is returning raw DB row
fields without proper type casts; update the construction of result in the
manyDeviceTelemetry function so numeric fields are explicitly cast (e.g.
temperature and humidity via Number(...) or parseFloat(...), version via
Number(...) or parseInt(...)) and ensure timestamp fields are converted to ISO
strings from the correct input type (use new Date(Number(row.recorded_at)) / new
Date(Number(row.updated_at)) if DB returns epoch numbers, or new
Date(row.recorded_at) if DB returns ISO strings). Keep deviceId as a string and
apply these casts where the result object is built (refer to result,
row.temperature, row.humidity, row.version, row.recorded_at, row.updated_at).
- Around line 100-102: The deviceTelemetry resolver is constructing
recordedAt/updatedAt from row fields without type casts which causes pipeline
type errors; update the mapping in the resolver to explicitly cast
row.recorded_at and row.updated_at to the correct primitive (e.g.,
Number(row.recorded_at) or String(...) depending on DB type) before passing into
new Date(...), and cast row.version to the expected type (e.g.,
Number(row.version) or String(row.version)) so recordedAt, updatedAt and version
satisfy the resolver's return types (refer to the recordedAt, updatedAt, version
properties and the row variable inside the deviceTelemetry resolver).
In `@apps/gateway/src/routes/devicesImport.ts`:
- Around line 136-142: Wrap the bulk_import_jobs creation and its final status
UPDATE in a real DB transaction using a transaction client instead of
pool.query: acquire a client via pool.connect(), call BEGIN, run the INSERT into
bulk_import_jobs (currently using pool.query that returns jobId) and all
subsequent device INSERTs/updates using that client, then COMMIT after the final
UPDATE of the job status; ensure you handle errors with a try/catch/finally that
issues ROLLBACK on failure and always releases the client so the job can't be
left permanently in 'running' state.
🪄 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: c1b78edb-f1c0-48f1-a222-e79ded7b9cc8
📒 Files selected for processing (13)
.github/workflows/ci.yml.golangci.ymlapps/bff/src/datasources/elasticsearch.tsapps/bff/src/datasources/postgres.tsapps/bff/src/datasources/redis.tsapps/bff/src/lib/circuitBreaker.tsapps/bff/src/observability/metrics.tsapps/bff/src/resolvers.tsapps/bff/src/server.tsapps/dashboard/src/features/webhooks/WebhooksPage.tsxapps/dashboard/src/utils/exportCsv.test.tsapps/gateway/src/routes/auditLog.tsapps/gateway/src/routes/devicesImport.ts
Comment on lines
+12
to
23
| default: none | ||
| enable: | ||
| - errcheck | ||
| - govet | ||
| - staticcheck | ||
| - unused | ||
| - gosimple | ||
| - ineffassign | ||
| - typecheck | ||
| - gocritic | ||
| - gosec | ||
| - prealloc | ||
| - bodyclose | ||
| - noctx | ||
| - exhaustive |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider re-enabling unused and gosimple linters.
Setting default: none and explicitly enabling linters is clean for v2, but unused (dead code detection) and gosimple (code simplifications) provide valuable feedback that was previously enabled. Their removal may allow technical debt to accumulate undetected.
If these were intentionally removed due to noise, consider documenting the rationale in a comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 12 - 23, Re-enable the unused and gosimple
linters by adding "unused" and "gosimple" to the enabled list in the
.golangci.yml (i.e., include them alongside errcheck, govet, staticcheck, etc.),
or if they were intentionally disabled, add a short comment in the file
explaining the rationale and where to revisit this decision; reference the
existing enable block that lists errcheck, govet, staticcheck, ineffassign,
gocritic, gosec, prealloc, bodyclose, noctx, exhaustive to locate where to add
the two linters or the explanatory comment.
Comment on lines
+47
to
+49
| const result = await (es.search as (p: typeof params) => Promise<{ hits: { total: unknown; hits: ESHit[] } }>)(params); | ||
|
|
||
| return result.hits.hits.map((hit: any) => ({ | ||
| return result.hits.hits.map((hit: ESHit) => ({ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
@elastic/elasticsearch typescript generic search response
💡 Result:
In the @elastic/elasticsearch Node.js client, the generic search response type is SearchResponse<TDocument, TAggregations>, imported from '@elastic/elasticsearch/lib/api/types' or via 'estypes'. Key usage: import { Client } from '@elastic/elasticsearch'; interface MyDocument { title: string; content: string; } const result = await client.search({ index: 'my-index', query: { match_all: {} } }); // result is typed as TransportResult<SearchResponse, unknown> or SearchResponse depending on options The SearchResponse<TDocument = unknown, TAggregations = Record<AggregateName, AggregationsAggregate>> interface includes: - took: number - timed_out: boolean - _shards: ShardStatistics - hits: SearchHitsMetadata - aggregations?: TAggregations - etc. For modern "new types", pass the document type directly to search. Official docs confirm first-class TypeScript support with generics for search responses, using snake_case params. Example from docs: interface Document { character: string; quote: string; } const result = await client.search({ index: 'game-of-thrones', query: { match: { quote: 'winter' } } }); Source code shows: export default async function SearchApi<TDocument = unknown, TAggregations = Record<T.AggregateName, T.AggregationsAggregate>>(...): Promise<T.SearchResponse<TDocument, TAggregations>> Legacy types used explicit SearchResponse
Citations:
- 1: https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/7.17/typescript.html
- 2: https://www.elastic.co/docs/reference/elasticsearch/clients/javascript/typescript
- 3: https://github.com/elastic/elasticsearch-js/blob/main/src/api/api/search.ts
- 4: https://tessl.io/registry/tessl/npm-elastic--elasticsearch/files/docs/search-operations.md
- 5: Added Request/Response types definitions elastic/elasticsearch-js#970
- 6: Refactored type definitions elastic/elasticsearch-js#1119
- 7: https://www.npmjs.com/package/@elastic/elasticsearch
🏁 Script executed:
cat -n apps/bff/src/datasources/elasticsearch.ts | head -60Repository: pahuldeepp/GrainGuard-
Length of output: 2220
🏁 Script executed:
cat -n apps/bff/src/datasources/elasticsearch.ts | tail -30Repository: pahuldeepp/GrainGuard-
Length of output: 1275
🏁 Script executed:
rg -l "elasticsearch" apps/bff/src --type ts | head -10Repository: pahuldeepp/GrainGuard-
Length of output: 133
🏁 Script executed:
rg "searchDevices" apps/bff/src/resolvers.ts -B 3 -A 5Repository: pahuldeepp/GrainGuard-
Length of output: 648
🏁 Script executed:
rg "BffContext|tenantFilter" apps/bff/src --type ts -B 2 -A 2 | head -50Repository: pahuldeepp/GrainGuard-
Length of output: 2639
Refactor to use typed generics for better type safety.
The @elastic/elasticsearch client officially supports typed generics for search responses. Replace the manual type cast with:
const result = await es.search<ESHit['_source']>({...params});This eliminates the unconventional cast and provides first-class TypeScript type safety without manual assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/bff/src/datasources/elasticsearch.ts` around lines 47 - 49, Replace the
manual cast on the es.search call with the client’s generic typing: call
es.search with the generic type parameter for the hit _source (use
ESHit['_source']) instead of casting the function; update the declaration where
you currently do (es.search as (p: typeof params) => Promise<...>)(params) to a
direct es.search<ESHit['_source']>(params) so the returned result has proper
typed hits for the subsequent result.hits.hits.map usage.
Comment on lines
+298
to
+307
| const edges = items.map((row: Row) => ({ | ||
| cursor: Buffer.from((row.created_at as Date).toISOString()).toString("base64"), | ||
| node: { | ||
| deviceId: row.device_id, | ||
| tenantId: row.tenant_id, | ||
| serialNumber: row.serial_number, | ||
| createdAt: new Date(row.created_at).toISOString(), | ||
| createdAt: new Date(row.created_at as string).toISOString(), | ||
| temperature: row.temperature ?? null, | ||
| humidity: row.humidity ?? null, | ||
| recordedAt: row.recorded_at ? new Date(row.recorded_at).toISOString() : null, | ||
| recordedAt: row.recorded_at ? new Date(row.recorded_at as string).toISOString() : null, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent Date handling between cursor and node fields.
Line 299 casts row.created_at as Date and calls .toISOString() directly, while line 304 wraps it in new Date(row.created_at as string). The pg driver returns timestamp columns as JavaScript Date objects, so line 299's approach is correct but line 304 is redundant (and would fail if given an actual Date object).
♻️ Suggested consistency fix
cursor: Buffer.from((row.created_at as Date).toISOString()).toString("base64"),
node: {
deviceId: row.device_id,
tenantId: row.tenant_id,
serialNumber: row.serial_number,
- createdAt: new Date(row.created_at as string).toISOString(),
+ createdAt: (row.created_at as Date).toISOString(),
temperature: row.temperature ?? null,
humidity: row.humidity ?? null,
- recordedAt: row.recorded_at ? new Date(row.recorded_at as string).toISOString() : null,
+ recordedAt: row.recorded_at ? (row.recorded_at as Date).toISOString() : null,
version: row.version ?? null,
},📝 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
| const edges = items.map((row: Row) => ({ | |
| cursor: Buffer.from((row.created_at as Date).toISOString()).toString("base64"), | |
| node: { | |
| deviceId: row.device_id, | |
| tenantId: row.tenant_id, | |
| serialNumber: row.serial_number, | |
| createdAt: new Date(row.created_at).toISOString(), | |
| createdAt: new Date(row.created_at as string).toISOString(), | |
| temperature: row.temperature ?? null, | |
| humidity: row.humidity ?? null, | |
| recordedAt: row.recorded_at ? new Date(row.recorded_at).toISOString() : null, | |
| recordedAt: row.recorded_at ? new Date(row.recorded_at as string).toISOString() : null, | |
| const edges = items.map((row: Row) => ({ | |
| cursor: Buffer.from((row.created_at as Date).toISOString()).toString("base64"), | |
| node: { | |
| deviceId: row.device_id, | |
| tenantId: row.tenant_id, | |
| serialNumber: row.serial_number, | |
| createdAt: (row.created_at as Date).toISOString(), | |
| temperature: row.temperature ?? null, | |
| humidity: row.humidity ?? null, | |
| recordedAt: row.recorded_at ? (row.recorded_at as Date).toISOString() : null, |
🤖 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 298 - 307, The code
constructs edges via items.map and inconsistently handles created_at: the cursor
uses (row.created_at as Date).toISOString() while node.createdAt wraps it in new
Date(...), which is redundant and can fail; update node.createdAt to mirror the
cursor by using (row.created_at as Date).toISOString() and similarly ensure
recordedAt handles row.recorded_at safely by checking if it's a Date then
calling .toISOString() (e.g., recordedAt: row.recorded_at ? (row.recorded_at as
Date).toISOString() : null) so both cursor and node fields consistently treat
timestamps as Date objects.
Comment on lines
+335
to
336
| createdAt: new Date(result.rows[0].created_at as string).toISOString(), | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same inconsistency in createDevice and updateDevice.
These also use new Date(result.rows[0].created_at as string) which is inconsistent with the cursor handling.
Also applies to: 351-352
🤖 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 335 - 336, In createDevice
and updateDevice the createdAt field is built using new
Date(result.rows[0].created_at as string).toISOString(), which is inconsistent
with how cursors are handled elsewhere; replace that direct Date construction
with the same cursor-decoding helper used by other cursor logic (e.g., the
project function that converts stored cursor timestamps to ISO such as
parseCursorDate/decodeCursor/convertCursorToISO) and pass
result.rows[0].created_at through that helper so createdAt is produced the same
way as other cursor-derived dates.
| @@ -27,7 +27,7 @@ export const cache = { | |||
| return results.map((r: any) => r ? JSON.parse(r) : null); | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remaining any in getMany callback.
Line 27 still uses (r: any) which is inconsistent with the PR's goal of eliminating explicit any usage. Consider typing the pipeline results properly.
♻️ Proposed fix
- return results.map((r: any) => r ? JSON.parse(r) : null);
+ return results.map((r) => (typeof r === "string" ? JSON.parse(r) : null));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/bff/src/datasources/redis.ts` at line 27, The map callback in getMany
still uses (r: any); change it to a properly typed signature reflecting pipeline
results (e.g., (r: string | null)) and propagate a generic type for parsed
results (e.g., getMany<T> returning (T | null)[]). Update the mapping to use (r:
string | null) => r ? JSON.parse(r) as T : null and consider wrapping JSON.parse
in a try/catch or a safe parser if needed; reference the getMany function and
the results variable (pipeline output) when making the change.
Comment on lines
+23
to
26
| device: async (_: unknown, args: { deviceId: string }, ctx: BffContext) => { | ||
| const cacheKey = `device:full:${ctx.tenantId}:${args.deviceId}`; | ||
| const cached = await cache.get<any>(cacheKey); | ||
| const cached = await cache.get<Record<string,unknown>>(cacheKey); | ||
| if (cached) return cached; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type safety: Consider defining specific row interfaces instead of repeated casts.
Throughout the resolvers, Record<string, unknown> requires casting every field access. Consider defining typed interfaces (e.g., DeviceRow, TelemetryRow) that match the database schema to get compile-time safety without scattered casts.
Also applies to: 48-54, 73-77, 112-118, 146-150
🤖 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 23 - 26, Define concrete TypeScript
interfaces for DB rows (e.g., DeviceRow, TelemetryRow) that match the schema and
replace uses of Record<string, unknown>; update the resolvers (notably the
device resolver and other spots around the device/telemetry handlers) to use
cache.get<DeviceRow>(cacheKey) and typed query/result variables instead of
casting fields from Record<string, unknown>, then adjust return types of
functions like device to return DeviceRow (or a mapped DTO) so field accesses
are type-checked at compile time.
| LIMIT $5`, | ||
| [ | ||
| tenantId, | ||
| before ? new Date(before).toISOString() : null, |
There was a problem hiding this comment.
Validate before date string before conversion.
If before is an invalid date string (e.g., "not-a-date"), new Date(before).toISOString() will throw RangeError: Invalid time value. Consider validating or handling this:
🛡️ Proposed fix
- before ? new Date(before).toISOString() : null,
+ before ? (isNaN(Date.parse(before)) ? null : new Date(before).toISOString()) : null,Or wrap in try-catch and return a 400 error for invalid dates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/routes/auditLog.ts` at line 45, Validate the incoming before
string before calling new Date(before).toISOString(): check with
Date.parse(before) or isNaN check (e.g., if (!before ||
isNaN(Date.parse(before))) return a 400 error) or wrap the conversion in
try/catch and return 400 on failure; update the route handler that reads the
before parameter (the code using new Date(before).toISOString()) to perform this
validation and return a clear 400 response for invalid dates instead of letting
RangeError be thrown.
Cherry-picked commit 4f8ebc9 changed row types from `any` to `Record<string,unknown>`, making date fields typed as `unknown`. Add a toIsoDate() helper and replace all direct new Date(row.x) calls so TypeScript strict mode is satisfied without unsafe casts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- golangci.yml: exclude errcheck for `defer x.Close()` (idiomatic Go cleanup path where error handling is a no-op); add gosec G101 and G118 to excludes (dev-default connection strings and intentional background-context goroutines) - saga-orchestrator/health, libs/health: use `_ = conn.Close()` for TCP health-probe close (non-defer path, errcheck cannot be suppressed via source rule alone) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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: The toIsoDate function must guard against null/undefined
and invalid dates: change its signature to return string | null, then at the top
check if value === null || value === undefined and return null; if value is a
Date return toISOString(); otherwise coerce with new Date(value as
string|number) and if date.toString() === "Invalid Date" return null else return
date.toISOString(); update any callers that currently assume a non-null string
(e.g., places that call toIsoDate directly) to handle the nullable return
appropriately.
- Around line 214-219: The mapping in the rows.map callback in resolvers.ts is
accessing camelCase fields that don't exist; update the property accesses to use
snake_case from the Postgres result (use row.device_id instead of row.deviceId,
and row.recorded_at passed into toIsoDate instead of row.recordedAt) while
leaving temperature and humidity as-is and keeping the toIsoDate call; locate
the rows.map where toIsoDate is used to convert recordedAt and change those two
property names accordingly.
🪄 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: 39e0376f-d87b-4169-992e-8340eea792ed
📒 Files selected for processing (1)
apps/bff/src/resolvers.ts
Comment on lines
+12
to
+15
| function toIsoDate(value: unknown): string { | ||
| if (value instanceof Date) return value.toISOString(); | ||
| return new Date(value as string | number).toISOString(); | ||
| } |
There was a problem hiding this comment.
toIsoDate does not handle null or undefined safely.
new Date(undefined)produces "Invalid Date" →toISOString()throwsRangeErrornew Date(null)produces epoch (1970-01-01) → silently returns incorrect data
Some call sites guard with null checks (lines 45, 70), but others invoke toIsoDate directly (lines 105-106, 139-140, 181-182, 218). If the database ever returns NULL for those columns, this will crash or corrupt data.
🛡️ Proposed fix to add null/undefined guard
function toIsoDate(value: unknown): string {
+ if (value == null) {
+ throw new Error('toIsoDate received null or undefined');
+ }
if (value instanceof Date) return value.toISOString();
return new Date(value as string | number).toISOString();
}Alternatively, return null and change the return type to string | null to align with nullable schema fields.
🤖 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, The toIsoDate function must
guard against null/undefined and invalid dates: change its signature to return
string | null, then at the top check if value === null || value === undefined
and return null; if value is a Date return toISOString(); otherwise coerce with
new Date(value as string|number) and if date.toString() === "Invalid Date"
return null else return date.toISOString(); update any callers that currently
assume a non-null string (e.g., places that call toIsoDate directly) to handle
the nullable return appropriately.
Comment on lines
+214
to
219
| return rows.map((row: Record<string,unknown>) => ({ | ||
| deviceId: row.deviceId, | ||
| temperature: row.temperature, | ||
| humidity: row.humidity, | ||
| recordedAt: new Date(row.recordedAt).toISOString(), | ||
| recordedAt: toIsoDate(row.recordedAt), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check what column names db.getTelemetryHistory returns
# Expected: Either camelCase aliases in the query, or snake_case that should be used here
ast-grep --pattern $'getTelemetryHistory($$$) {
$$$
}'
# Also check for any column aliasing patterns
rg -n -A 10 'getTelemetryHistory' --type tsRepository: pahuldeepp/GrainGuard-
Length of output: 4713
🏁 Script executed:
# Check how other resolvers map Postgres rows
rg -n -B 5 -A 5 'db\.get' apps/bff/src/resolvers.ts | head -60Repository: pahuldeepp/GrainGuard-
Length of output: 2293
Fix property access: use row.device_id and row.recorded_at instead of camelCase variants.
Line 215-218 attempt to access row.deviceId and row.recordedAt, but db.getTelemetryHistory() returns Postgres query results with snake_case columns (device_id, recorded_at). All other resolvers in this file correctly map these snake_case properties. This mismatch will cause undefined values to be returned.
Change:
deviceId: row.device_id,
temperature: row.temperature,
humidity: row.humidity,
recordedAt: toIsoDate(row.recorded_at),🤖 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 214 - 219, The mapping in the
rows.map callback in resolvers.ts is accessing camelCase fields that don't
exist; update the property accesses to use snake_case from the Postgres result
(use row.device_id instead of row.deviceId, and row.recorded_at passed into
toIsoDate instead of row.recordedAt) while leaving temperature and humidity
as-is and keeping the toIsoDate call; locate the rows.map where toIsoDate is
used to convert recordedAt and change those two property names accordingly.
…lures - Remove invalid exclude-rules.source from .golangci.yml (field is not accepted by golangci-lint v2.11.0 config verify, aborting lint run) - Add gosec G101 + G118 exclusions (dev-default DSN literals and intentional background-context goroutines) - Add //nolint:errcheck to all defer x.Close() shutdown paths across 9 service and tool entry-points; closing on shutdown is idiomatic Go and the error is unrecoverable at that point Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 36-41: The global suppression of gosec G101 in the gosec ->
excludes block is too broad and can mask real hardcoded credentials; remove G101
from the global excludes and instead limit suppression to specific files or
directories by adding an issues: exclude-rules entry targeting gosec/G101 for
known dev/default config files (or apply inline //nolint:gosec on those
dev-default lines), referencing the gosec excludes/G101 identifier and using
path-based rules so the linter still flags G101 everywhere else.
- Line 41: The global suppression of G118 in .golangci.yml is too broad; remove
the global "- G118" entry and instead add targeted nolint directives at the
specific intentional uses of context.Background() (for example inside long‑lived
goroutine functions or workers) using "//nolint:gosec" comments on the offending
lines or enclosing functions; keep normal G118 checks active so accidental uses
(e.g., in HTTP handlers or request-scoped functions) still trigger warnings.
🪄 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: 0e0e5bb9-f4bc-4181-bb81-0d3e79241a2d
📒 Files selected for processing (3)
.golangci.ymlapps/saga-orchestrator/internal/health/checker.golibs/health/health.go
Comment on lines
+36
to
+41
| gosec: | ||
| excludes: | ||
| - G104 # unhandled errors in deferred calls (noisy) | ||
| - G404 # math/rand is acceptable here for jitter and synthetic test data | ||
| - G101 # dev-default connection strings are not real credentials | ||
| - G118 # background context in long-lived goroutines is intentional |
There was a problem hiding this comment.
Global suppression of G101 may mask real credential leaks.
Excluding G101 (hardcoded credentials) project-wide because of "dev-default connection strings" is risky. If actual credentials are accidentally committed elsewhere, they won't be flagged.
Consider using //nolint:gosec directives on the specific lines containing dev defaults, or use exclude-rules with a path filter to limit suppression to specific files (e.g., config or test files) rather than a blanket exclusion.
# Example: Limit G101 suppression to specific paths
issues:
exclude-rules:
- path: "internal/config/"
linters: [gosec]
text: "G101"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 36 - 41, The global suppression of gosec G101 in
the gosec -> excludes block is too broad and can mask real hardcoded
credentials; remove G101 from the global excludes and instead limit suppression
to specific files or directories by adding an issues: exclude-rules entry
targeting gosec/G101 for known dev/default config files (or apply inline
//nolint:gosec on those dev-default lines), referencing the gosec excludes/G101
identifier and using path-based rules so the linter still flags G101 everywhere
else.
| - G104 # unhandled errors in deferred calls (noisy) | ||
| - G404 # math/rand is acceptable here for jitter and synthetic test data | ||
| - G101 # dev-default connection strings are not real credentials | ||
| - G118 # background context in long-lived goroutines is intentional |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider scoping G118 suppression more narrowly.
While context.Background() in long-lived goroutines can be intentional, global suppression removes useful signal when background context is used inappropriately (e.g., in request handlers). Consider using //nolint:gosec directives on the specific intentional usages instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml at line 41, The global suppression of G118 in .golangci.yml is
too broad; remove the global "- G118" entry and instead add targeted nolint
directives at the specific intentional uses of context.Background() (for example
inside long‑lived goroutine functions or workers) using "//nolint:gosec"
comments on the offending lines or enclosing functions; keep normal G118 checks
active so accidental uses (e.g., in HTTP handlers or request-scoped functions)
still trigger warnings.
This was referenced Mar 28, 2026
Closed
This was referenced Mar 28, 2026
pahuldeepp
added a commit
that referenced
this pull request
Mar 28, 2026
HIGH PRIORITY: - #9: Fix CSRF timing attack — pad buffers to fixed length before timingSafeEqual - #10: Upgrade circuit breaker to distributed (Redis-backed state sharing across pods) - #8: Fix saga recovery infinite loop — mark corrupted payloads as FAILED instead of retrying MEDIUM PRIORITY: - #11: Add webhook idempotency check (dedup by endpoint_id + event_type) - #12: Assert Stripe webhook body is Buffer before signature verification - #13: Eliminate N+1 query in deviceTelemetry resolver (single JOIN query) - #15: Add ORDER BY + LIMIT to saga FindByCorrelationID for deterministic results - #17: Make critical audit events (auth, admin) throw on failure instead of silent swallow LOW PRIORITY: - #20: Add 10s query timeout to all saga repository DB operations - #23: Add IsValidStatus validator for saga status constants - #24: Set httpServer.timeout (30s) and keepAliveTimeout (65s) on BFF - #25: Add RabbitMQ heartbeat (30s) and connection error/close handlers - #7: Fix remaining saga JSON marshal error check (initialErr) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
golangci-lint-action@v9with proper v2 config format (version: "2",default: none,exclude-dirsunderrun)auditLog.ts— replaces dynamicWHERE ${conditions.join()}with static parameterised query usingIS NULL OR col = $NpatterntenantQueryinpostgres.tswith explicitBEGIN/set_config/COMMITtransactionconsole.logcalls fromelasticsearch.ts@typescript-eslint/no-explicit-anywarnings across BFF and dashboard (resolvers, circuitBreaker, redis, metrics, server, WebhooksPage, BillingPage, exportCsv test)setSearchParamsdependency inBillingPageuseEffectTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores