feat: introduce @llmgateway/logger package to replace logs - #684
Conversation
- Create new @llmgateway/logger package with pino-based structured logging - Add configurable log levels for development vs production environments - Replace console.log/warn/error statements in critical files: - packages/models/src/provider-api.ts (including line 915 validation log) - packages/auth/src/auth.ts (email verification and Brevo contact creation) - packages/db/src/migrate.ts and db.ts (database operations) - apps/gateway/src/serve.ts (server lifecycle and shutdown) - apps/gateway/src/index.ts (HTTP exceptions and health checks) - Add specialized logging methods for common use cases - Configure pretty printing for development and JSON format for production - Add comprehensive test coverage for logging functionality - Improve error handling with proper Error type checking 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds a new @llmgateway/logger package (pino-based) and replaces ad-hoc console.* logging with the centralized logger across many apps/packages, updates workspace dependencies, adds build/test configs for the logger, and tightens ESLint no-console rules with test overrides. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Component as App Component
participant Logger as @llmgateway/logger
participant Pino as pino
participant Out as stdout/transport
Component->>Logger: logger.info/warn/error(message, context|Error)
activate Logger
Logger->>Pino: pino[level]({...context, err?}, message)
activate Pino
Pino-->>Out: formatted log (JSON or pretty)
deactivate Pino
deactivate Logger
rect rgb(245,250,255)
note over Component,Logger: Error normalization for non-Error values
Component->>Logger: logger.error("Unhandled error", error instanceof Error ? error : new Error(String(error)))
end
sequenceDiagram
autonumber
participant CLI as migrate.ts
participant Logger as @llmgateway/logger
participant DB as Drizzle Migration
CLI->>Logger: info("Starting database migrations")
CLI->>DB: migrate({ connection: databaseUrl })
alt success
DB-->>CLI: done
CLI->>Logger: info("Database migrations completed")
else failure
DB-->>CLI: throw Error
CLI->>Logger: error("Database migration failed", Error)
CLI-->>CLI: rethrow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Removed the unused consoleSpy variable from the logger.spec.ts test file to clean up the test setup and avoid unnecessary variable declaration. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
steebchen
changed the title
- Remove unnecessary specific logging methods (httpRequest, modelRequest, etc.) - Keep only core logging methods (trace, debug, info, warn, error, fatal) - Update provider-api.ts to use generic debug method instead of validation method - Simplify test cases to match new logger interface - Maintain same functionality with cleaner, more flexible API 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/package.json (1)
20-33: Replace all console. calls with the centralized @llmgateway/logger*
console.log/info/warn/error calls remain throughout apps/api/src (e.g. stripe.ts, serve.ts, routes, lib), and there are no imports of @llmgateway/logger. Import and use the shared logger in place of console.* to complete the structured-logging migration.packages/models/src/provider-api.ts (1)
31-33: btoa usage will break in Node environmentsbtoa isn’t available in Node. Use Buffer when present; fall back to btoa in browsers.
- const isBase64 = url.includes(";base64,"); - const base64Data = isBase64 ? data : btoa(data); + const isBase64 = url.includes(";base64,"); + const base64Data = isBase64 + ? data + : (typeof btoa === "function" + ? btoa(data) + : Buffer.from(data, "utf-8").toString("base64"));- const uint8Array = new Uint8Array(arrayBuffer); - const binaryString = Array.from(uint8Array, (byte) => - String.fromCharCode(byte), - ).join(""); - const base64 = btoa(binaryString); + const uint8Array = new Uint8Array(arrayBuffer); + // Prefer Buffer in Node; fall back to btoa in the browser + const base64 = + typeof Buffer !== "undefined" + ? Buffer.from(uint8Array).toString("base64") + : btoa(Array.from(uint8Array, (b) => String.fromCharCode(b)).join(""));Also applies to: 93-99
🧹 Nitpick comments (19)
packages/logger/tsconfig.json (1)
3-6: Consider enabling declaration-only builds and project references for faster, cleaner outputs.If you run tsc alongside tsup, emitting only d.ts and marking the project composite improves incremental builds and avoids duplicate JS outputs.
"compilerOptions": { "rootDir": "./src", - "outDir": "./dist" + "outDir": "./dist", + "declaration": true, + "emitDeclarationOnly": true, + "composite": true },packages/logger/tsup.config.ts (1)
3-10: Add platform/target to tsup.config.tsThe
externalarray is correct—bothpino(^9.4.0) andpino-pretty(^11.3.0) are listed as dependencies. For predictable, Node-only bundles, add:export default defineConfig({ entry: ["src/index.ts"], format: ["esm", "cjs"], dts: true, sourcemap: true, clean: true, external: ["pino", "pino-pretty"], + platform: "node", + target: "node18", });apps/gateway/src/serve.ts (1)
81-84: Avoid logging Promise objects in unhandledRejection.
Serializing a Promise can be noisy or circular; log the reason only.Apply:
-process.on("unhandledRejection", (reason, promise) => { - logger.fatal("Unhandled rejection", { promise, reason }); +process.on("unhandledRejection", (reason) => { + logger.fatal( + "Unhandled rejection", + reason instanceof Error ? reason : { reason: String(reason) }, + ); process.exit(1); -}); +});packages/db/src/db.ts (1)
12-12: Optionally handle connect errors explicitly.
Top-level connect can fail silently; log and surface it early.-void client.connect(); +await client.connect().catch((err) => { + logger.error( + "Error connecting to database", + err instanceof Error ? err : new Error(String(err)), + ); + throw err; +});packages/logger/package.json (1)
19-22: Consider making pino-pretty dev-only with lazy load.
If pretty printing is used only in development, move pino-pretty to devDependencies and lazy-import in dev to reduce prod footprint. Optional.packages/db/src/migrate.ts (1)
1-1: Prefer a child logger for scoped contextUsing a child logger adds stable context (component) to every line and simplifies downstream filtering.
import { logger } from "@llmgateway/logger"; +const log = logger.child({ component: "db:migrate" }); - logger.info("Starting database migrations"); + log.info("Starting database migrations"); - logger.info("Database migrations completed successfully"); + log.info("Database migrations completed successfully"); - logger.error( + log.error( "Database migration failed", error instanceof Error ? error : new Error(String(error)), );Also applies to: 13-14, 25-30
packages/logger/src/logger.spec.ts (1)
1-1: Silence logger output in tests (console mocks won’t affect pino)Pino writes to stdout/stderr, so mocking console.* won’t stop noise. Force a silent level for the test run.
-import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, beforeAll, afterAll } from "vitest"; describe("LLMGateway Logger", () => { + beforeAll(() => { + // Ensure no logger output during tests + process.env.LOG_LEVEL = "silent"; + }); + afterAll(() => { + delete process.env.LOG_LEVEL; + }); beforeEach(() => { // Mock console methods to avoid actual output during tests vi.spyOn(console, "log").mockImplementation(() => {}); vi.spyOn(console, "warn").mockImplementation(() => {}); vi.spyOn(console, "error").mockImplementation(() => {}); });Also applies to: 6-11
packages/auth/src/auth.ts (2)
57-58: Avoid logging PII at info levelEmail addresses are PII. Consider logging domain only or moving this to debug.
- logger.info("Successfully created Brevo contact", { email }); + // Avoid PII leakage in logs + const emailDomain = email.includes("@") ? email.split("@")[1] : "unknown"; + logger.debug("Successfully created Brevo contact", { emailDomain });
31-50: Add request timeout to Brevo API callPrevent hangs on network issues.
- const response = await fetch("https://api.brevo.com/v3/contacts", { + const response = await fetch("https://api.brevo.com/v3/contacts", { method: "POST", headers: { "Content-Type": "application/json", "api-key": brevoApiKey, }, + signal: AbortSignal.timeout(10_000), body: JSON.stringify({apps/gateway/src/index.ts (3)
65-68: Include request context in HTTP exception logsMethod and path make incidents traceable without relying on upstream logs.
- logger.error("HTTP 500 exception", error); + logger.error("HTTP 500 exception", { + err: error, + status, + method: c.req.method, + path: c.req.path, + }); } else { - logger.warn("HTTP client error", { status, message: error.message }); + logger.warn("HTTP client error", { + status, + message: error.message, + method: c.req.method, + path: c.req.path, + });
82-85: Add request context to unhandled error logsKeeps logs consistent and actionable.
- logger.error( - "Unhandled error", - error instanceof Error ? error : new Error(String(error)), - ); + logger.error("Unhandled error", { + err: error instanceof Error ? error : new Error(String(error)), + method: c.req.method, + path: c.req.path, + });
158-161: Add request context to healthcheck failure logsHelps correlate failures with callers.
- logger.error( - "Redis healthcheck failed", - error instanceof Error ? error : new Error(String(error)), - ); + logger.error("Redis healthcheck failed", { + err: error instanceof Error ? error : new Error(String(error)), + method: c.req.method, + path: c.req.path, + });- logger.error( - "Database healthcheck failed", - error instanceof Error ? error : new Error(String(error)), - ); + logger.error("Database healthcheck failed", { + err: error instanceof Error ? error : new Error(String(error)), + method: c.req.method, + path: c.req.path, + });Also applies to: 174-177
packages/models/src/provider-api.ts (1)
954-958: Add timeout to validation fetchPrevents hung key validations on provider slowness.
- const response = await fetch(endpoint, { + const response = await fetch(endpoint, { method: "POST", headers, + signal: AbortSignal.timeout(10_000), body: JSON.stringify(payload), });packages/logger/src/index.ts (6)
1-1: Honor LOG_LEVEL and include 'silent' for tests; tighten LogLevel typing.Read LOG_LEVEL when set and prefer pino’s LevelWithSilent (adds "silent"). This makes test runs noise-free and gives ops an override.
-import pino, { type Logger } from "pino"; +import pino, { type Logger, type Bindings, type LevelWithSilent } from "pino"; -export type LogLevel = "trace" | "debug" | "info" | "warn" | "error" | "fatal"; +export type LogLevel = LevelWithSilent; - private getDefaultLevel(): LogLevel { - const nodeEnv = process.env.NODE_ENV; - if (nodeEnv === "test") { - return "warn"; - } - if (nodeEnv === "production") { - return "info"; - } - return "debug"; - } + private getDefaultLevel(): LogLevel { + const env = process.env.LOG_LEVEL as LogLevel | undefined; + if (env) return env; + const nodeEnv = process.env.NODE_ENV; + if (nodeEnv === "test") return "silent"; + if (nodeEnv === "production") return "info"; + return "debug"; + }Also applies to: 3-3, 37-46
48-52: Normalize LOG_PRETTY parsing.Handle case-insensitive values to avoid surprises from "True"/"FALSE".
- const nodeEnv = process.env.NODE_ENV; - const forcePretty = process.env.LOG_PRETTY === "true"; - const forceJson = process.env.LOG_PRETTY === "false"; + const nodeEnv = process.env.NODE_ENV; + const prettyEnv = process.env.LOG_PRETTY?.toLowerCase(); + const forcePretty = prettyEnv === "true"; + const forceJson = prettyEnv === "false";
64-79: Don’t pass undefined as the first arg; tighten extra typing.Avoids odd frames and improves TS safety.
- trace(message: string, extra?: object): void { - this.logger.trace(extra, message); - } + trace(message: string, extra?: Record<string, unknown>): void { + extra ? this.logger.trace(extra, message) : this.logger.trace(message); + } - debug(message: string, extra?: object): void { - this.logger.debug(extra, message); - } + debug(message: string, extra?: Record<string, unknown>): void { + extra ? this.logger.debug(extra, message) : this.logger.debug(message); + } - info(message: string, extra?: object): void { - this.logger.info(extra, message); - } + info(message: string, extra?: Record<string, unknown>): void { + extra ? this.logger.info(extra, message) : this.logger.info(message); + } - warn(message: string, extra?: object): void { - this.logger.warn(extra, message); - } + warn(message: string, extra?: Record<string, unknown>): void { + extra ? this.logger.warn(extra, message) : this.logger.warn(message); + }
81-95: Error/fatal: accept unknown and handle undefined cleanly.Prevents logging “undefined” frames and keeps stacks intact.
- error(message: string, error?: Error | object): void { + error(message: string, error?: unknown): void { if (error instanceof Error) { this.logger.error({ err: error }, message); - } else { - this.logger.error(error, message); + } else if (error && typeof error === "object") { + this.logger.error(error as Record<string, unknown>, message); + } else { + this.logger.error(message); } } - fatal(message: string, error?: Error | object): void { + fatal(message: string, error?: unknown): void { if (error instanceof Error) { this.logger.fatal({ err: error }, message); - } else { - this.logger.fatal(error, message); + } else if (error && typeof error === "object") { + this.logger.fatal(error as Record<string, unknown>, message); + } else { + this.logger.fatal(message); } }
97-103: Type child bindings with pino.Bindings.Improves discoverability and correctness when adding fields.
- child(bindings: object): LLMGatewayLogger { + child(bindings: Bindings): LLMGatewayLogger { const childPino = this.logger.child(bindings); const childLogger = Object.create(LLMGatewayLogger.prototype); childLogger.logger = childPino; return childLogger; }
110-112: Make createLogger options optional.Saves call sites from passing empty objects.
-export function createLogger(options: LoggerOptions): LLMGatewayLogger { +export function createLogger(options: LoggerOptions = {}): LLMGatewayLogger { return new LLMGatewayLogger(options); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/api/package.json(1 hunks)apps/gateway/package.json(1 hunks)apps/gateway/src/index.ts(5 hunks)apps/gateway/src/serve.ts(3 hunks)packages/auth/package.json(1 hunks)packages/auth/src/auth.ts(5 hunks)packages/db/package.json(1 hunks)packages/db/src/db.ts(2 hunks)packages/db/src/migrate.ts(3 hunks)packages/logger/package.json(1 hunks)packages/logger/src/index.ts(1 hunks)packages/logger/src/logger.spec.ts(1 hunks)packages/logger/tsconfig.json(1 hunks)packages/logger/tsup.config.ts(1 hunks)packages/models/package.json(1 hunks)packages/models/src/provider-api.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
packages/logger/tsup.config.tspackages/logger/src/logger.spec.tsapps/gateway/src/index.tspackages/db/src/db.tspackages/models/src/provider-api.tspackages/db/src/migrate.tspackages/auth/src/auth.tsapps/gateway/src/serve.tspackages/logger/src/index.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
packages/logger/tsup.config.tspackages/logger/src/logger.spec.tsapps/gateway/src/index.tspackages/db/src/db.tspackages/models/src/provider-api.tspackages/db/src/migrate.tspackages/auth/src/auth.tsapps/gateway/src/serve.tspackages/logger/src/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
packages/logger/tsup.config.tspackages/logger/src/logger.spec.tsapps/gateway/src/index.tspackages/db/src/db.tspackages/models/src/provider-api.tspackages/db/src/migrate.tspackages/auth/src/auth.tsapps/gateway/src/serve.tspackages/logger/src/index.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Name unit test files with the .spec.ts suffix
Files:
packages/logger/src/logger.spec.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware.findMany() or db().query.
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle with the latest object syntax for database operations
For read queries, use db().query..findFirst() Files:
apps/gateway/src/index.tsapps/gateway/src/serve.tsapps/gateway/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafety
Files:
apps/gateway/src/index.tsapps/gateway/src/serve.ts🧠 Learnings (4)
📚 Learning: 2025-08-29T15:31:07.044Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.044Z Learning: Applies to apps/gateway/**/*.{ts,tsx} : In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafetyApplied to files:
apps/gateway/package.jsonapps/api/package.json📚 Learning: 2025-08-29T02:12:34.132Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-08-29T02:12:34.132Z Learning: Applies to apps/{api,gateway}/**/*.{ts,tsx} : Use Zod schemas for validation in backend servicesApplied to files:
apps/api/package.json📚 Learning: 2025-08-29T15:31:07.044Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.044Z Learning: Applies to apps/{api,gateway}/**/*.{ts,tsx} : Use Drizzle with the latest object syntax for database operationsApplied to files:
packages/db/src/db.tspackages/db/src/migrate.tspackages/auth/src/auth.ts📚 Learning: 2025-07-23T19:33:40.639Z
Learnt from: CR PR: theopenco/llmgateway#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-07-23T19:33:40.639Z Learning: Applies to **/*.{js,ts} : Use drizzle with the latest object syntax for database operationsApplied to files:
packages/db/src/migrate.ts🧬 Code graph analysis (7)
packages/logger/src/logger.spec.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)createLogger(110-112)apps/gateway/src/index.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)packages/db/src/db.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)packages/models/src/provider-api.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)packages/db/src/migrate.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)packages/auth/src/auth.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/gateway/src/serve.ts (3)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/gateway/src/worker.ts (1)
stopWorker(465-490)packages/db/src/db.ts (1)
closeDatabase(20-31)🪛 GitHub Actions: ci
packages/logger/package.json
[error] 1-1: Lockfile specifiers don't match package.json: 1 dependency added (vitest@^3.2.3). Run 'pnpm install' to synchronize the lockfile.
🔇 Additional comments (13)
packages/auth/package.json (1)
19-24: Approve code changes — logger dependency validatedNo remaining
console.*calls inpackages/auth/src;@llmgateway/loggeris imported inpackages/auth/src/auth.ts(line 2).packages/models/package.json (1)
22-24: LGTM: models now depends on the centralized logger.
This aligns with the PR goal and keeps workspace versions consistent.apps/gateway/src/serve.ts (2)
41-63: Nice structured, stepwise shutdown logs.
Readable, ordered, and with context fields where applicable. This will help postmortems.
77-78: No action needed:logger.fatalsignature mirrorslogger.errorand usage is correct.packages/db/package.json (1)
28-28: LGTM: db package depends on @llmgateway/logger.
Keeps observability consistent across data layer.packages/db/src/db.ts (1)
23-28: Good: standardized DB close logging (success and error paths).
The error branch preserves the original error and adds context; rethrowing is correct.packages/logger/package.json (1)
7-13: Verify exports vs build outputs (ESM/CJS).
Both "import" and "require" point to index.js. Ensure tsup emits compatible formats or export separate files to avoid module interop issues.Suggested:
"exports": { ".": { - "types": "./dist/index.d.ts", - "import": "./dist/index.js", - "require": "./dist/index.js" + "types": "./dist/index.d.ts", + "import": "./dist/index.mjs", + "require": "./dist/index.cjs" } }, "scripts": { - "build": "tsup", - "dev": "tsup --watch", + "build": "tsup --format esm,cjs --dts", + "dev": "tsup --watch --format esm,cjs --dts", "test": "vitest run" }packages/db/src/migrate.ts (1)
13-13: Structured logs look goodReplacements with the centralized logger are consistent and keep messages actionable.
Also applies to: 25-25
packages/auth/src/auth.ts (1)
59-62: Good error normalization with structured loggingPassing a real Error ensures consistent logger formatting and stack traces.
apps/gateway/src/index.ts (1)
60-94: Overall error handling/logging upgrade looks solidCentralized logger usage is consistent and respects severity levels.
packages/models/src/provider-api.ts (2)
106-109: Good use of structured error payloadPassing a plain object with err and url preserves both stack and context.
908-911: Validation model debug log is helpfulThe context keys are clear and low-noise at debug level.
packages/logger/src/index.ts (1)
28-31: translateTime format is validDateformat (used by pino-pretty) supports HH (24-hour), MM (minutes), ss (seconds) and Z (timezone), so
"HH:MM:ss Z"is correct and needs no change.
| "@llmgateway/db": "workspace:*", | ||
| "@llmgateway/logger": "workspace:*", | ||
| "@llmgateway/models": "workspace:*", | ||
| "gpt-tokenizer": "3.0.1", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Adopting centralized logger in gateway — LGTM.
Dependency aligns the app with the new logging stack.
Quick check that console.* calls are gone and logger is used:
🏁 Script executed:
#!/bin/bash
rg -n --type=ts --no-ignore -S $'\\bconsole\\.(log|info|warn|error|debug)\\(' apps/gateway/src || true
rg -n --type=ts --no-ignore -S 'from\\s+["\\\']@llmgateway/logger["\\\']' apps/gateway/srcLength of output: 7802
Remove all remaining console. calls and use @llmgateway/logger*
Search revealed console.log/warn/error calls still present across apps/gateway/src; import { logger } from @llmgateway/logger and replace all console.* usages with logger.[level](…).
🤖 Prompt for AI Agents
In apps/gateway/package.json around lines 21 to 24, the review asks to remove
console.* calls across apps/gateway/src and use @llmgateway/logger instead;
update each file under apps/gateway/src that currently uses console.log /
console.warn / console.error / console.info to import { logger } from
'@llmgateway/logger' at the top, then replace console.log(...) ->
logger.info(...), console.warn(...) -> logger.warn(...), console.error(...) ->
logger.error(...), and console.info(...) -> logger.info(...); ensure there are
no remaining console.* usages, adjust any message formatting to accept
structured args if required, and run tests/lint to confirm imports and usage are
correct.
| logger.info("Server starting", { port }); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Quick follow-up: ensure all console. are gone repo-wide.*
worker.ts still shows console usage in the snippet. Run this to find stragglers and I can auto-fix:
🏁 Script executed:
#!/bin/bash
# Find remaining console calls
rg -nP '\bconsole\.(log|warn|error|info|debug)\s*\(' -g '!**/node_modules/**'Length of output: 17156
Eliminate all remaining console. calls repository-wide.*
Run:
rg -nP '\bconsole\.(log|warn|error|info|debug)\s*\(' -g '!**/node_modules/**'and replace every console.log, console.warn, console.error, and console.info with the centralized logger methods (or remove if purely for local debugging). This cleanup spans dozens of files (e.g. apps/gateway/src/worker.ts, apps/gateway/src/chat, apps/ui/src/**, etc.) and is required before merging.
🤖 Prompt for AI Agents
In apps/gateway/src/serve.ts around lines 11 to 12 there is a plain console.*
usage remaining (e.g., console.log/info) that must be removed; replace any
console calls with the centralized logger methods
(logger.info/logger.warn/logger.error/logger.debug) or remove them if they were
only for local debugging, ensuring you pass the same message and structured
metadata (like { port }) to logger and run the repository-wide search/replace
command rg -nP '\bconsole\.(log|warn|error|info|debug)\s*\(' -g
'!**/node_modules/**' to find and update all other occurrences across the
codebase before merging.
| logger.fatal("Uncaught exception", error); | ||
| process.exit(1); | ||
| }); | ||
|
|
||
| process.on("unhandledRejection", (reason, promise) => { | ||
| console.error("Unhandled rejection at:", promise, "reason:", reason); | ||
| logger.fatal("Unhandled rejection", { promise, reason }); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure logs flush before process.exit on fatal paths.
Without draining, the last logs can be dropped. Consider exposing a flush/final handler in @llmgateway/logger (using pino.final) and calling it here before exit.
🤖 Prompt for AI Agents
In apps/gateway/src/serve.ts around lines 77 to 84, the fatal-path handlers call
process.exit immediately which can drop the last logs; update these handlers to
invoke the logger's flush/final routine (exposed from @llmgateway/logger, e.g. a
pino.final-based function) and wait for it to complete before calling
process.exit. Specifically, import and call the exposed final/flush handler with
the error/reason and a callback (or await a returned promise) to ensure the log
is written, then call process.exit with the appropriate code inside that
completion callback or after awaiting the flush.
| if (!smtpHost || !smtpUser || !smtpPass) { | ||
| console.log(`email verification link: ${url}`); | ||
| console.error( | ||
| logger.info("Email verification link generated", { url }); | ||
| logger.error( | ||
| "SMTP configuration is not set. Email verification will not work.", | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Do not log verification tokens; downgrade severity for config issue
This logs a full verification URL (includes token). That’s sensitive. Also, missing SMTP is a configuration warning, not an error.
- if (!smtpHost || !smtpUser || !smtpPass) {
- logger.info("Email verification link generated", { url });
- logger.error(
- "SMTP configuration is not set. Email verification will not work.",
- );
+ if (!smtpHost || !smtpUser || !smtpPass) {
+ const safeUrl = url.replace(/([?&]token=)[^&]+/, "$1[redacted]");
+ logger.debug("Email verification link generated", { url: safeUrl });
+ logger.warn(
+ "SMTP configuration is not set. Email verification will not work.",
+ );
return;
}📝 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.
| if (!smtpHost || !smtpUser || !smtpPass) { | |
| console.log(`email verification link: ${url}`); | |
| console.error( | |
| logger.info("Email verification link generated", { url }); | |
| logger.error( | |
| "SMTP configuration is not set. Email verification will not work.", | |
| ); | |
| return; | |
| if (!smtpHost || !smtpUser || !smtpPass) { | |
| const safeUrl = url.replace(/([?&]token=)[^&]+/, "$1[redacted]"); | |
| logger.debug("Email verification link generated", { url: safeUrl }); | |
| logger.warn( | |
| "SMTP configuration is not set. Email verification will not work.", | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In packages/auth/src/auth.ts around lines 111 to 116, the code currently logs
the full verification URL (which contains a sensitive token) and emits an error
when SMTP config is missing; remove any logging of the verification URL or token
and replace it with a non-sensitive, generic message (e.g., "Email verification
link generated" without URL) or omit entirely, and change the logger.error call
to logger.warn (or equivalent warning level) to reflect a configuration warning
rather than an error; ensure no sensitive token is output and that the log
message gives clear non-sensitive context.
| { | ||
| "name": "@llmgateway/logger", | ||
| "version": "0.1.0", | ||
| "private": true, | ||
| "main": "./dist/index.js", | ||
| "types": "./dist/index.d.ts", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/index.js", | ||
| "require": "./dist/index.js" | ||
| } | ||
| }, | ||
| "scripts": { | ||
| "build": "tsup", | ||
| "dev": "tsup --watch", | ||
| "test": "vitest run" | ||
| }, | ||
| "dependencies": { | ||
| "pino": "^9.4.0", | ||
| "pino-pretty": "^11.3.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^22.9.0", | ||
| "tsup": "8.4.0", | ||
| "typescript": "5.8.3", | ||
| "vitest": "^3.2.3" | ||
| } | ||
| } |
There was a problem hiding this comment.
Pipeline is red: lockfile out of sync.
GitHub Actions error: “Lockfile specifiers don't match package.json: 1 dependency added (vitest@^3.2.3).” Run pnpm install at the workspace root and commit the updated lockfile.
#!/bin/bash
# From repo root
pnpm install -w
git add pnpm-lock.yaml
git commit -m "chore: sync lockfile after logger package changes"🧰 Tools
🪛 GitHub Actions: ci
[error] 1-1: Lockfile specifiers don't match package.json: 1 dependency added (vitest@^3.2.3). Run 'pnpm install' to synchronize the lockfile.
🤖 Prompt for AI Agents
In packages/logger/package.json lines 1-29 the package.json was changed (vitest
added) but the workspace lockfile is out of sync; run pnpm install -w from the
repository root to update pnpm-lock.yaml, stage and commit the updated
pnpm-lock.yaml (e.g., git add pnpm-lock.yaml && git commit -m "chore: sync
lockfile after logger package changes"), then push the commit so CI picks up the
updated lockfile.
| "build": "tsup", | ||
| "dev": "tsup --watch", | ||
| "test": "vitest run" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm .d.ts are emitted.
"types" points to dist/index.d.ts; ensure tsup (or tsup.config.ts) generates declarations.
🏁 Script executed:
#!/bin/bash
# After build, verify types exist
pnpm -w --filter @llmgateway/logger build
test -f packages/logger/dist/index.d.ts && echo "types OK" || (echo "missing index.d.ts"; exit 1)Length of output: 542
🏁 Script executed:
#!/bin/bash
pnpm install
pnpm -w --filter @llmgateway/logger run build
if test -f packages/logger/dist/index.d.ts; then
echo "types OK"
else
echo "missing index.d.ts"; exit 1
fiLength of output: 1068
Install tsup and configure declaration output Add tsup to devDependencies in packages/logger/package.json and enable declaration file generation (e.g. CLI flag --dts or tsup.config.ts dts: true), update the lockfile (pnpm install --no-frozen-lockfile), then verify pnpm -w --filter @llmgateway/logger build produces dist/index.d.ts.
🤖 Prompt for AI Agents
In packages/logger/package.json around lines 15 to 17, the package scripts
reference tsup but tsup is not added to devDependencies and declaration (.d.ts)
files are not enabled; add "tsup" to devDependencies, enable declaration output
by adding the CLI flag --dts to the build script (or set dts: true in
tsup.config.ts), run pnpm install --no-frozen-lockfile to update the lockfile,
and verify by running pnpm -w --filter @llmgateway/logger build that
dist/index.d.ts is generated.
| this.logger = pino({ | ||
| name, | ||
| level, | ||
| ...(prettyPrint && { | ||
| transport: { | ||
| target: "pino-pretty", | ||
| options: { | ||
| colorize: true, | ||
| translateTime: "HH:MM:ss Z", | ||
| ignore: "pid,hostname", | ||
| }, | ||
| }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default redaction to avoid leaking secrets.
Without redaction, headers/credentials can hit logs (especially in JSON mode). Add safe defaults and allow extension via LOG_REDACT.
this.logger = pino({
name,
level,
+ // Redact common secret fields; extend via LOG_REDACT="a,b,c"
+ redact: {
+ paths:
+ process.env.LOG_REDACT?.split(",").map((s) => s.trim()).filter(Boolean) ??
+ [
+ "req.headers.authorization",
+ "req.headers.cookie",
+ "headers.authorization",
+ "headers.cookie",
+ "password",
+ "*.password",
+ "token",
+ "*.token",
+ "apiKey",
+ "*.apiKey",
+ "secret",
+ "*.secret",
+ ],
+ censor: "[REDACTED]",
+ },
...(prettyPrint && {
transport: {
target: "pino-pretty",
options:
{
colorize: true,
- translateTime: "HH:MM:ss Z",
+ translateTime: "SYS:standard",
ignore: "pid,hostname",
},
},
}),
});📝 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.
| this.logger = pino({ | |
| name, | |
| level, | |
| ...(prettyPrint && { | |
| transport: { | |
| target: "pino-pretty", | |
| options: { | |
| colorize: true, | |
| translateTime: "HH:MM:ss Z", | |
| ignore: "pid,hostname", | |
| }, | |
| }, | |
| }), | |
| }); | |
| this.logger = pino({ | |
| name, | |
| level, | |
| // Redact common secret fields; extend via LOG_REDACT="a,b,c" | |
| redact: { | |
| paths: | |
| process.env.LOG_REDACT?.split(",").map((s) => s.trim()).filter(Boolean) ?? | |
| [ | |
| "req.headers.authorization", | |
| "req.headers.cookie", | |
| "headers.authorization", | |
| "headers.cookie", | |
| "password", | |
| "*.password", | |
| "token", | |
| "*.token", | |
| "apiKey", | |
| "*.apiKey", | |
| "secret", | |
| "*.secret", | |
| ], | |
| censor: "[REDACTED]", | |
| }, | |
| ...(prettyPrint && { | |
| transport: { | |
| target: "pino-pretty", | |
| options: { | |
| colorize: true, | |
| translateTime: "SYS:standard", | |
| ignore: "pid,hostname", | |
| }, | |
| }, | |
| }), | |
| }); |
| logger.error(`Failed to fetch image ${part.image_url.url}`, { | ||
| err: error instanceof Error ? error : new Error(String(error)), | ||
| }); |
There was a problem hiding this comment.
Sanitize image URLs in error logs
This logs full URLs, which can include tokens or private paths. Align with earlier truncation/sanitization.
- logger.error(`Failed to fetch image ${part.image_url.url}`, {
- err: error instanceof Error ? error : new Error(String(error)),
- });
+ logger.error("Failed to fetch image", {
+ err: error instanceof Error ? error : new Error(String(error)),
+ url: part.image_url.url.startsWith("data:")
+ ? "data:[truncated]"
+ : part.image_url.url.substring(0, 50) + "...",
+ });📝 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.
| logger.error(`Failed to fetch image ${part.image_url.url}`, { | |
| err: error instanceof Error ? error : new Error(String(error)), | |
| }); | |
| logger.error("Failed to fetch image", { | |
| err: error instanceof Error ? error : new Error(String(error)), | |
| url: part.image_url.url.startsWith("data:") | |
| ? "data:[truncated]" | |
| : part.image_url.url.substring(0, 50) + "...", | |
| }); |
🤖 Prompt for AI Agents
In packages/models/src/provider-api.ts around lines 210 to 212, the error log
includes the full image URL (which may contain tokens/private paths); replace
the direct usage of part.image_url.url with the same sanitization/truncation
used elsewhere (e.g., call the existing sanitize/truncate helper or strip
query/auth parts and limit length) and log only the sanitized/truncated URL in
the logger.error call while keeping the error object as-is.
- Resolve import conflicts in provider-api.ts keeping logger import - Keep validation logging with debug method - Regenerate pnpm-lock.yaml to resolve dependency conflicts - Preserve all logging infrastructure changes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…atements - Add eslint no-console rule enforced for backend code (apps/api, apps/gateway, packages/) - Allow console statements in test files and UI components (apps/ui, apps/docs) - Systematically replace all console statements in backend services with structured logger calls: - apps/gateway/src/worker.ts: 19+ console replacements - apps/gateway/src/chat/chat.ts: 15+ console replacements - apps/api/src/stripe.ts: 40+ console replacements - All other backend service files updated - Maintain console statements in test files for test output - All packages build successfully with new logging infrastructure - Production logging now fully structured and professional 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
steebchen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (18)
apps/gateway/src/lib/redis.ts (1)
38-39: Use RPOP instead of LPOP for FIFO
In apps/gateway/src/lib/redis.ts (line 38), change the pop call to RPOP so LPUSH+RPOP yields FIFO semantics:- const result = await redisClient.lpop(queue, 10); + const result = await redisClient.rpop(queue, 10);apps/gateway/src/lib/costs.ts (1)
60-66: Fix falsy checks for token counts (0 is valid) — use nullish checks.Zero tokens are legitimate; current falsy checks mis-treat 0 as “missing,” causing incorrect estimation and null costs.
- if ((!promptTokens || !completionTokens) && fullOutput) { + if ((promptTokens == null || completionTokens == null) && fullOutput) { - if (!promptTokens && fullOutput) { + if (promptTokens == null && fullOutput) { - if (!completionTokens && fullOutput && fullOutput.completion) { + if (completionTokens == null && fullOutput && fullOutput.completion) { - if (!calculatedPromptTokens || !calculatedCompletionTokens) { + if (calculatedPromptTokens == null || calculatedCompletionTokens == null) {Also applies to: 87-96, 98-111
apps/gateway/src/models/models.ts (1)
262-265: Removeas any; use a precise type extension.Avoids violating TS guideline and keeps type safety.
- const supportedParameters = (provider as any)?.supportedParameters as - | string[] - | undefined; + type ProviderWithSupported = ProviderModelMapping & { + supportedParameters?: string[]; + }; + const supportedParameters = + (provider as ProviderWithSupported).supportedParameters;apps/gateway/src/scripts/generate-openapi.ts (1)
14-15: Eliminateas anyon securitySchemes.Keep strict typing; merge objects instead of casting.
- spec.components.securitySchemes = config.components.securitySchemes as any; + spec.components.securitySchemes = { + ...(spec.components.securitySchemes ?? {}), + ...(config.components.securitySchemes ?? {}), + };apps/api/src/stripe.ts (1)
559-576: Removeas anyon Stripe types; use exact fields.Use Stripe’s typed properties and extract IDs safely.
- const subscription = (invoice as any).subscription; + const subscription = invoice.subscription; // Extract subscription ID from line items if not directly available - let subscriptionId = - typeof subscription === "string" ? subscription : subscription?.id; + let subscriptionId = + typeof subscription === "string" ? subscription : subscription?.id ?? null; @@ - stripePaymentIntentId: (invoice as any).payment_intent, + stripePaymentIntentId: + typeof invoice.payment_intent === "string" + ? invoice.payment_intent + : invoice.payment_intent?.id ?? undefined,Also applies to: 615-616
packages/models/src/provider-api.ts (5)
260-264: Avoid echoing raw URLs in fallback user-visible text.This text can reach external providers and logs. Replace with a generic placeholder.
Apply:
- return { - type: "text", - text: `[Image failed to load: ${part.image_url.url}]`, - } as TextContent; + return { + type: "text", + text: "[Image failed to load]", + } as TextContent;
55-56: btoa is not reliable in Node runtimes; use Buffer for base64.Ensures compatibility across Node/browser.
Apply:
- const base64Data = isBase64 ? data : btoa(data); + const base64Data = isBase64 ? data : Buffer.from(data, "utf8").toString("base64");- const base64 = btoa(binaryString); + const base64 = Buffer.from(binaryString, "binary").toString("base64");Also applies to: 121-122
213-223: Remove any: type messages precisely.Aligns with TS guideline “never use any”.
Apply:
-function transformMessagesForNoSystemRole(messages: any[]): any[] { +function transformMessagesForNoSystemRole(messages: BaseMessage[]): BaseMessage[] {
415-419: Avoid requestBody: any.Use a broad but typed record to keep flexibility without any.
Apply:
- const requestBody: any = { + const requestBody: Record<string, unknown> = {
1062-1066: Add timeout to provider validation request.Prevents API key validation from stalling callers under network issues.
Apply:
- const response = await fetch(endpoint, { + const response = await fetch(endpoint, { method: "POST", headers, body: JSON.stringify(payload), - }); + signal: AbortSignal.timeout(10_000), + });apps/api/src/routes/beacon.ts (2)
48-48: Remove any: type context explicitly.Use Hono Context to satisfy “no any” guideline.
Apply:
+import type { Context } from "hono";-function extractClientIP(c: any): string | null { +function extractClientIP(c: Context): string | null {
75-75: Remove any: type context explicitly (region helper).Apply:
-function extractRegionInfo(c: any): { country?: string; region?: string } { +function extractRegionInfo(c: Context): { country?: string; region?: string } {apps/api/src/routes/chat.ts (1)
75-88: Replaceas anywith proper StatusCode typing.Meets “no any” rule and preserves typesafety.
Apply:
+import type { StatusCode } from "hono/utils/http-status";- return c.json( - { error: "gateway returned: " + errorJson.message }, - response.status as any, - ); + return c.json( + { error: "gateway returned: " + errorJson.message }, + response.status as StatusCode, + );- return c.json( - { error: `Failed to get chat completion: ${errorText}` }, - response.status as any, - ); + return c.json( + { error: `Failed to get chat completion: ${errorText}` }, + response.status as StatusCode, + );- return c.json( - { error: `Failed to get chat completion: ${err}` }, - response.status as any, - ); + return c.json( + { error: `Failed to get chat completion: ${String(err)}` }, + response.status as StatusCode, + );apps/api/src/serve.ts (1)
38-41: Replaceanywith precise types for server and signalRepository guideline forbids
: any. Tighten types to Node’shttp.ServerandNodeJS.Signals. Also type the close callback error.+import type { Server } from "http"; @@ -const closeServer = (server: any): Promise<void> => { +const closeServer = (server: Server): Promise<void> => { return new Promise((resolve, reject) => { - server.close((error: any) => { + server.close((error?: Error) => { if (error) { reject(error); } else { resolve(); } }); }); }; @@ -const gracefulShutdown = async (signal: string, server: any) => { +const gracefulShutdown = async (signal: NodeJS.Signals, server: Server) => {Also applies to: 50-50
apps/gateway/src/worker.ts (4)
58-66: Removeas anyand harden PG unique-violation detection in lock acquisition.Avoids forbidden
as any, improves type-safety, and keeps behavior identical.Apply this diff:
- if ( - typeof (error as any)?.code === "string" && - (error as any).code === "23505" - ) { - return false; - } + if (isPgUniqueViolation(error)) { + return false; + }Add this helper once (e.g., above
acquireLock):function isPgUniqueViolation(err: unknown): err is { code: "23505" } { return ( typeof err === "object" && err !== null && "code" in err && (err as Record<string, unknown>).code === "23505" ); }
153-154: Validate and clamptopUpAmountto a sane positive number.Prevents NaN/zero/negative amounts reaching Stripe and DB.
- const topUpAmount = Number(org.autoTopUpAmount || "10"); + const rawAmount = Number(org.autoTopUpAmount ?? 10); + const topUpAmount = Number.isFinite(rawAmount) && rawAmount > 0 ? rawAmount : 10;
399-406: Eliminateas anyon log inserts by normalizing payload shape.Keep types consistent by nulling large fields when retention is “none”.
- if (organization?.retentionLevel === "none") { - const { - messages: _messages, - content: _content, - ...metadataOnly - } = data; - return metadataOnly; - } - return data; + if (organization?.retentionLevel === "none") { + return { ...data, messages: null, content: null }; + } + return data;- await db.insert(log).values(processedLogData as any); + await db.insert(log).values(processedLogData);Also applies to: 413-413, 415-418
18-19: Avoid cross-app imports for payments/fees; move to shared package.Importing from
../../api/src/...couples gateway worker to API internals and can break builds/deploys. Extract Stripe client init and fee calculator into shared packages (e.g.,@llmgateway/paymentsand@llmgateway/fees) and depend on those here and in the API.Also applies to: 195-197, 228-239, 250-255
♻️ Duplicate comments (1)
packages/models/src/provider-api.ts (1)
256-258: Do not log full image URLs (sanitize/truncate).Leak risk: tokens/paths may be embedded. Log a sanitized/truncated variant and keep the error object structured.
Apply:
- logger.error(`Failed to fetch image ${part.image_url.url}`, { - err: error instanceof Error ? error : new Error(String(error)), - }); + logger.error("Failed to fetch image", { + err: error instanceof Error ? error : new Error(String(error)), + url: + part.image_url.url.startsWith("data:") + ? "data:[truncated]" + : part.image_url.url.split("?")[0].substring(0, 50) + "...", + });
🧹 Nitpick comments (39)
apps/gateway/src/lib/redis.ts (3)
10-15: Use a Redis-scoped child logger for consistent context.Attach a module-scoped child logger (e.g., { component: "redis" }) so every log line is searchable by component. Then use it in handlers here and below.
Example (outside selected lines):
const log = logger.child({ component: "redis" }); // ... redisClient.on("error", (err) => log.error("Redis Client Error", err instanceof Error ? err : new Error(String(err))) );
26-29: Add queue context to the error log payload.Include the queue name as structured data to aid debugging.
- logger.error( - "Error publishing to queue", - error instanceof Error ? error : new Error(String(error)), - ); + logger.error( + "Error publishing to queue", + { err: (error instanceof Error ? error : new Error(String(error))), queue }, + );Optional robustness (outside selected lines): avoid JSON.stringify pitfalls (BigInt, etc.).
const payload = typeof message === "string" ? message : JSON.stringify(message, (_k, v) => (typeof v === "bigint" ? v.toString() : v)); await redisClient.lpush(queue, payload);
46-49: Add queue context to consumer error logs.Mirror producer logs with structured context.
- logger.error( - "Error consuming from queue", - error instanceof Error ? error : new Error(String(error)), - ); + logger.error( + "Error consuming from queue", + { err: (error instanceof Error ? error : new Error(String(error))), queue }, + );apps/gateway/src/lib/cache.ts (3)
2-2: Prefer a child logger for scoped context.Attach consistent metadata (module scope) once and reuse.
-import { logger } from "@llmgateway/logger"; +import { logger } from "@llmgateway/logger"; +const log = logger.child({ module: "cache" });
29-30: Type-safe catch + structured context in logs.
- Use unknown in catch, then narrow to Error.
- Include operation context (keys/ids) so logs are actionable.
- Use the child logger above for consistency.
- } catch (error) { - logger.error("Error setting cache:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("cache.set failed", { ...meta, key, expirationSeconds }); } - } catch (error) { - logger.error("Error getting cache:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("cache.get failed", { ...meta, key }); return null; } - } catch (error) { - logger.error("Error checking if caching is enabled:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("cache.config failed", { ...meta, projectId, key: `project_cache_config:${projectId}` }); throw error; } - } catch (error) { - logger.error("Error fetching project:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("project fetch failed", { ...meta, projectId, key: `project:${projectId}` }); throw error; } - } catch (error) { - logger.error("Error fetching organization:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("organization fetch failed", { ...meta, organizationId, key: `organization:${organizationId}` }); throw error; } - } catch (error) { - logger.error("Error fetching provider key:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("providerKey fetch failed", { ...meta, organizationId, provider }); throw error; } - } catch (error) { - logger.error("Error fetching custom provider key:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("customProviderKey fetch failed", { ...meta, organizationId, customName }); throw error; } - } catch (error) { - logger.error("Error checking if custom provider exists:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("customProvider.exists check failed", { ...meta, organizationId, providerCandidate }); throw error; } - } catch (error) { - logger.error("Error setting streaming cache:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("streamingCache.set failed", { ...meta, key, expirationSeconds }); } - } catch (error) { - logger.error("Error getting streaming cache:", error); + } catch (error: unknown) { + const meta = error instanceof Error ? { err: error } : { error }; + log.error("streamingCache.get failed", { ...meta, key }); return null; }Also applies to: 41-43, 78-80, 106-108, 134-136, 171-173, 211-213, 250-252, 294-296, 308-311
16-21: Remove remaining any by making cache helpers generic.Aligns with “no : any” TS guideline and improves type safety.
-export async function setCache( - key: string, - value: any, - expirationSeconds: number, -): Promise<void> { +export async function setCache<T>( + key: string, + value: T, + expirationSeconds: number, +): Promise<void> { -export async function getCache(key: string): Promise<any | null> { +export async function getCache<T>(key: string): Promise<T | null> { try { const cachedValue = await redisClient.get(key); if (!cachedValue) { return null; } - return JSON.parse(cachedValue); + return JSON.parse(cachedValue) as T;Optionally specialize call sites for stronger types, e.g.:
const cachedProject = await getCache<InferSelectModel<typeof tables.project>>(projectCacheKey);Also applies to: 33-34
apps/gateway/src/lib/costs.ts (1)
139-141: Minor: prefer nullish check for cachedTokens.- const cachedInputCost = cachedTokens ? cachedTokens * cachedInputPrice : 0; + const cachedInputCost = cachedTokens != null ? cachedTokens * cachedInputPrice : 0;packages/models/src/provider-api.ts (1)
72-75: Strip query strings in logged URLs.Truncate and drop ?query to avoid leaking tokens.
Apply:
- logger.warn("Non-HTTPS URL provided for image fetch in production", { - url: url.substring(0, 20) + "...", + logger.warn("Non-HTTPS URL provided for image fetch in production", { + url: url.split("?")[0].substring(0, 20) + "...", });- logger.warn(`Failed to fetch image from URL (${response.status})`, { - url: url.substring(0, 50) + "...", + logger.warn(`Failed to fetch image from URL (${response.status})`, { + url: url.split("?")[0].substring(0, 50) + "...", });- logger.warn("Invalid content type for image URL", { - contentType, - url: url.substring(0, 50) + "...", + logger.warn("Invalid content type for image URL", { + contentType, + url: url.split("?")[0].substring(0, 50) + "...", });- logger.error("Error processing image URL", { + logger.error("Error processing image URL", { err: error instanceof Error ? error : new Error(String(error)), - url: url.substring(0, 50) + "...", + url: url.split("?")[0].substring(0, 50) + "...", });Also applies to: 82-85, 99-103, 129-132
apps/api/src/routes/beacon.ts (1)
130-136: Minimize PII in info logs.Consider hashing UUID and redacting IP in logs.
Apply:
- logger.info("Received installation beacon", { - uuid: beaconData.uuid, - type: beaconData.type, - clientIP, - country: regionInfo.country, - cloudProvider, - }); + logger.info("Received installation beacon", { + uuid: beaconData.uuid.substring(0, 8) + "...", + type: beaconData.type, + clientIP: clientIP ? "[redacted]" : null, + country: regionInfo.country, + cloudProvider, + });apps/api/src/routes/chat.ts (1)
52-68: Optional: add fetch timeout.Prevents indefinite waits to the gateway in failure scenarios.
Apply:
- const response = await fetch( + const response = await fetch( process.env.NODE_ENV === "production" ? "https://api.llmgateway.io/v1/chat/completions" : "http://localhost:4001/v1/chat/completions", { method: "POST", headers: { "Content-Type": "application/json", Authorization: `Bearer ${authToken}`, }, body: JSON.stringify({ model, messages, stream, }), + signal: AbortSignal.timeout(15_000), }, );apps/api/src/lib/beacon.ts (2)
43-45: Fix log message punctuation/clarity.Apply:
- logger.info( - "Sending installation beacon (for anonymous tracking of self-hosted installs. To disable, set TELEMETRY_ACTIVE=false in your environment variables.", - ); + logger.info( + "Sending installation beacon for anonymous tracking of self-hosted installs. To disable, set TELEMETRY_ACTIVE=false.", + );
76-81: Consistent error field name.Consider
err(noterror) to align with logger.error structure and keep parity across levels.Apply:
- logger.warn("Failed to send installation beacon", { - error: error instanceof Error ? error : new Error(String(error)), + logger.warn("Failed to send installation beacon", { + err: error instanceof Error ? error : new Error(String(error)), });apps/api/src/routes/subscriptions.ts (1)
139-140: Avoid leaking error details to clients.The error is already logged; client message should be generic.
Apply:
- throw new HTTPException(500, { - message: `Failed to create checkout session: ${error}`, - }); + throw new HTTPException(500, { + message: "Failed to create checkout session", + });apps/api/src/index.ts (3)
49-49: Include request context in 5xx HTTPException logsAdd method/path to aid triage without grepping separate access logs.
- logger.error("HTTPException", error); + logger.error("HTTPException", { + err: error, + status, + method: c.req.method, + path: c.req.path, + });
64-67: Enrich “Unhandled error” logs with request contextKeeps parity with HTTPException logging for faster debugging.
-logger.error( - "Unhandled error", - error instanceof Error ? error : new Error(String(error)), -); +logger.error("Unhandled error", { + err: error instanceof Error ? error : new Error(String(error)), + method: c.req.method, + path: c.req.path, +});
119-122: Refactor logger.error to use object payload
Use{ err }to attach the error (with stack) and allow adding fields later.-logger.error( - "Database healthcheck failed", - error instanceof Error ? error : new Error(String(error)), -); +logger.error("Database healthcheck failed", { + err: error instanceof Error ? error : new Error(String(error)), +});apps/api/src/serve.ts (1)
28-33: Nit: log “listening” after server startsMinor ordering tweak to avoid a misleading “listening” log if
serve()throws.- logger.info("Server listening", { port }); - - return serve({ + const server = serve({ port, fetch: app.fetch, }); + logger.info("Server listening", { port }); + return server;apps/gateway/src/worker.ts (5)
228-236: Usewarnforrequires_actionStripe status.Action required likely indicates a degraded path for off-session payments.
- logger.info( + logger.warn( `Auto top-up requires action for organization ${org.id}: ${paymentIntent.status}`, );
361-361: Mask API key identifiers in logs.Prevents leaking full internal identifiers in logs while keeping them traceable.
- logger.info(`Added ${totalCost} usage to API key ${apiKeyId}`); + logger.info(`Added ${totalCost} usage to API key ${maskId(apiKeyId)}`);Add helper (once near the top):
const maskId = (id: string) => (id?.length > 8 ? `${id.slice(0, 4)}…${id.slice(-4)}` : id);
24-26: Lock TTL may expire mid-work; consider a heartbeat.With a fixed 5-minute TTL and no
updatedAtrefresh, long-running batches can have their locks stolen by another worker. Mitigate by periodically bumpingupdatedAtwhile processing or extend TTL to worst-case duration.Also applies to: 278-283
341-348: Optional: guard against negative credits.If business rules disallow negative balances, clamp at zero or add a DB CHECK constraint; otherwise, ignore.
431-435: Optional: use a child logger with module context.Gives every message consistent context without repeating identifiers.
Example:
const logw = logger.child({ module: "gateway-worker" }); // then use logw.info/error/...apps/gateway/src/chat/chat.ts (17)
2077-2088: Use warn (not error) for expected client validation failuresThis path throws a 400 for an invalid parameter. Log as warn to avoid polluting error budgets with client mistakes.
- logger.error( + logger.warn( `Reasoning effort specified for non-reasoning model: ${requestedModel}`, { requestedModel, requestedProvider, reasoning_effort, modelProviders: modelInfo.providers.map((p) => ({ providerId: p.providerId, reasoning: (p as any).reasoning, })), }, );
2713-2715: Ensure stack capture in warn logsPass the Error under the conventional “err” key so the logger (and pino tooling) can serialize stacks consistently.
- logger.warn("Failed to parse cached chunk", { - error: e instanceof Error ? e : new Error(String(e)), - }); + logger.warn("Failed to parse cached chunk", { + err: e instanceof Error ? e : new Error(String(e)), + });
3087-3091: Add correlation context to provider error logsInclude requestId/provider/model to make incidents traceable without scanning DB logs.
- logger.error("Provider error", { - status: res.status, - errorText: errorResponseText, - }); + logger.error("Provider error", { + status: res.status, + errorText: errorResponseText, + requestId, + provider: usedProvider, + model: usedModel, + });
3247-3249: Attach requestId to buffer overflow warningHelps correlate with the same request’s DB log entry and upstream events.
- logger.warn( - "Buffer size exceeded 10MB, clearing buffer to prevent memory exhaustion", - ); + logger.warn( + "Buffer size exceeded 10MB, clearing buffer to prevent memory exhaustion", + { requestId } + );
3389-3397: Prefer “err” + include requestId in streaming JSON-parse warningThis preserves the stack and improves correlation.
- logger.warn("Event data contains SSE field", { + logger.warn("Event data contains SSE field", { eventData: eventData.substring(0, 200) + (eventData.length > 200 ? "..." : ""), dataIndex, eventEnd, bufferLength: bufferCopy.length, provider: usedProvider, + requestId, });
3492-3501: Promote error details to “err” field and add requestIdEnsures stack capture and correlation.
- logger.warn("Failed to parse streaming JSON", { - error: e instanceof Error ? e.message : String(e), + logger.warn("Failed to parse streaming JSON", { + err: e instanceof Error ? e : new Error(String(e)), eventData: eventData.substring(0, 200) + (eventData.length > 200 ? "..." : ""), provider: usedProvider, eventLength: eventData.length, bufferEnd: eventEnd, bufferLength: bufferCopy.length, + requestId, });
3749-3751: Attach requestId to stream read errorMinor, but helpful for cross-referencing.
- logger.error( - "Error reading stream", - error instanceof Error ? error : new Error(String(error)), - ); + logger.error( + "Error reading stream", + error instanceof Error ? error : new Error(String(error)), + ); + logger.error("Error reading stream context", { requestId, provider: usedProvider, model: usedModel });
3776-3780: Attach requestId to SSE send errorSame traceability rationale.
- logger.error( + logger.error( "Failed to send error SSE", sseError instanceof Error ? sseError : new Error(String(sseError)), - ); + ); + logger.error("Failed to send error SSE context", { requestId });
3827-3829: Good: streamed prompt token encoding error is normalizedConsider adding requestId for correlation.
- logger.error( + logger.error( "Failed to encode chat messages in streaming", error instanceof Error ? error : new Error(String(error)), ); + logger.debug("Encoding failure context", { requestId });
3842-3844: Add requestId to completion encoding errorConsistent with other error logs.
- logger.error( + logger.error( "Failed to encode completion text in streaming", error instanceof Error ? error : new Error(String(error)), ); + logger.debug("Encoding failure context", { requestId });
3923-3925: Add requestId to final usage chunk send errorMinor correlation aid.
- logger.error( + logger.error( "Error sending final usage chunk", error instanceof Error ? error : new Error(String(error)), ); + logger.debug("Final usage chunk context", { requestId });
4033-4035: Add requestId to streaming cache save errorAids cache troubleshooting.
- logger.error( + logger.error( "Error saving streaming cache", error instanceof Error ? error : new Error(String(error)), ); + logger.debug("Streaming cache context", { requestId, streamingCacheKey });
4148-4151: Add correlation context to non-streaming provider errorsMirror the streaming branch improvement.
- logger.error("Provider error", { - status: res.status, - errorText: errorResponseText, - }); + logger.error("Provider error", { + status: res.status, + errorText: errorResponseText, + requestId, + provider: usedProvider, + model: usedModel, + });
4263-4265: Gate heavy debug logging behind x-debug and summarizeAvoid dumping full provider responses into logs; log a compact summary when x-debug=true.
- if (process.env.NODE_ENV !== "production") { - logger.debug("API response", { response: json }); - } + if (process.env.NODE_ENV !== "production" && debugMode) { + logger.debug("API response (summary)", { + id: (json as any)?.id, + model: (json as any)?.model, + choicesLength: Array.isArray((json as any)?.choices) ? (json as any).choices.length : undefined, + status: (json as any)?.status, + requestId, + }); + }
4283-4286: Don’t log base64 image payloads; log metadata insteadThese can be huge and sensitive. Log count/types only.
- logger.debug("Gateway - parseProviderResponse extracted images", { images }); - logger.debug("Gateway - Used provider", { usedProvider }); - logger.debug("Gateway - Used model", { usedModel }); + logger.debug("Gateway response metadata", { + usedProvider, + usedModel, + imageCount: images?.length ?? 0, + imageTypes: (images ?? []).slice(0, 5).map((i) => i.image_url?.url?.split(';')?.[0]?.replace('data:', '') || 'unknown'), + requestId, + });
558-576: DRY up repeated “tokenizer encode failed” loggingThe same pattern appears in multiple catch blocks. Consider a tiny helper to reduce duplication and ensure consistent context.
Example helper (placed near other utils in this file):
function logEncodeFailure(context: string, err: unknown, extra?: object) { const error = err instanceof Error ? err : new Error(String(err)); logger.error(context, error); if (extra) logger.debug(`${context} context`, extra); }Usage:
logEncodeFailure("Failed to encode completion text", error, { requestId });Also applies to: 3825-3846
1860-1866: Consider a per-request child loggerCreating a child logger early (e.g., after computing requestId) avoids repeating correlation fields and keeps logs consistent.
Example:
const reqLog = logger.child({ requestId, route: "chat.completions" }); // Then use reqLog.*() throughout this handler
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
apps/api/package.json(1 hunks)apps/api/src/index.ts(4 hunks)apps/api/src/lib/beacon.ts(4 hunks)apps/api/src/routes/beacon.ts(2 hunks)apps/api/src/routes/chat.ts(3 hunks)apps/api/src/routes/subscriptions.ts(6 hunks)apps/api/src/scripts/generate-openapi.ts(2 hunks)apps/api/src/serve.ts(5 hunks)apps/api/src/stripe.ts(35 hunks)apps/gateway/package.json(1 hunks)apps/gateway/src/chat/chat.ts(18 hunks)apps/gateway/src/lib/cache.ts(11 hunks)apps/gateway/src/lib/costs.ts(3 hunks)apps/gateway/src/lib/redis.ts(4 hunks)apps/gateway/src/models/models.ts(2 hunks)apps/gateway/src/scripts/generate-openapi.ts(2 hunks)apps/gateway/src/worker.ts(16 hunks)apps/ui/src/lib/server-api.ts(1 hunks)eslint.config.mjs(2 hunks)packages/models/src/provider-api.ts(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/ui/src/lib/server-api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/gateway/package.json
- apps/api/package.json
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/api/src/routes/subscriptions.tsapps/gateway/src/lib/costs.tsapps/gateway/src/models/models.tsapps/api/src/routes/chat.tsapps/api/src/lib/beacon.tsapps/api/src/stripe.tsapps/api/src/scripts/generate-openapi.tsapps/api/src/index.tsapps/gateway/src/lib/cache.tsapps/gateway/src/worker.tsapps/gateway/src/chat/chat.tsapps/gateway/src/scripts/generate-openapi.tsapps/api/src/routes/beacon.tsapps/gateway/src/lib/redis.tsapps/api/src/serve.tspackages/models/src/provider-api.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/api/src/routes/subscriptions.tsapps/gateway/src/lib/costs.tsapps/gateway/src/models/models.tsapps/api/src/routes/chat.tsapps/api/src/lib/beacon.tsapps/api/src/stripe.tsapps/api/src/scripts/generate-openapi.tsapps/api/src/index.tsapps/gateway/src/lib/cache.tsapps/gateway/src/worker.tsapps/gateway/src/chat/chat.tsapps/gateway/src/scripts/generate-openapi.tsapps/api/src/routes/beacon.tsapps/gateway/src/lib/redis.tsapps/api/src/serve.tspackages/models/src/provider-api.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
apps/api/src/routes/subscriptions.tsapps/gateway/src/lib/costs.tsapps/gateway/src/models/models.tsapps/api/src/routes/chat.tsapps/api/src/lib/beacon.tsapps/api/src/stripe.tsapps/api/src/scripts/generate-openapi.tsapps/api/src/index.tsapps/gateway/src/lib/cache.tsapps/gateway/src/worker.tsapps/gateway/src/chat/chat.tsapps/gateway/src/scripts/generate-openapi.tsapps/api/src/routes/beacon.tsapps/gateway/src/lib/redis.tsapps/api/src/serve.tspackages/models/src/provider-api.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware.findMany() or db().query.
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle with the latest object syntax for database operations
For read queries, use db().query..findFirst() Files:
apps/api/src/routes/subscriptions.tsapps/gateway/src/lib/costs.tsapps/gateway/src/models/models.tsapps/api/src/routes/chat.tsapps/api/src/lib/beacon.tsapps/api/src/stripe.tsapps/api/src/scripts/generate-openapi.tsapps/api/src/index.tsapps/gateway/src/lib/cache.tsapps/gateway/src/worker.tsapps/gateway/src/chat/chat.tsapps/gateway/src/scripts/generate-openapi.tsapps/api/src/routes/beacon.tsapps/gateway/src/lib/redis.tsapps/api/src/serve.tsapps/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/api (Hono), always use Hono + Zod + OpenAPI for validation and typesafety
Files:
apps/api/src/routes/subscriptions.tsapps/api/src/routes/chat.tsapps/api/src/lib/beacon.tsapps/api/src/stripe.tsapps/api/src/scripts/generate-openapi.tsapps/api/src/index.tsapps/api/src/routes/beacon.tsapps/api/src/serve.tsapps/gateway/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafety
Files:
apps/gateway/src/lib/costs.tsapps/gateway/src/models/models.tsapps/gateway/src/lib/cache.tsapps/gateway/src/worker.tsapps/gateway/src/chat/chat.tsapps/gateway/src/scripts/generate-openapi.tsapps/gateway/src/lib/redis.ts🧠 Learnings (4)
📚 Learning: 2025-08-29T02:12:34.132Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-08-29T02:12:34.132Z Learning: Applies to apps/{api,gateway}/**/*.{ts,tsx} : Use Hono as the web framework for backend routes and middlewareApplied to files:
apps/api/src/routes/subscriptions.tsapps/gateway/src/models/models.tsapps/api/src/routes/chat.tsapps/api/src/stripe.tsapps/api/src/index.tsapps/api/src/routes/beacon.ts📚 Learning: 2025-08-29T15:31:07.044Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.044Z Learning: Applies to apps/gateway/**/*.{ts,tsx} : In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafetyApplied to files:
apps/gateway/src/models/models.tsapps/api/src/routes/chat.tsapps/api/src/routes/beacon.ts📚 Learning: 2025-08-29T15:31:07.044Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.044Z Learning: Applies to apps/api/**/*.{ts,tsx} : In apps/api (Hono), always use Hono + Zod + OpenAPI for validation and typesafetyApplied to files:
apps/gateway/src/models/models.tsapps/api/src/routes/chat.tsapps/api/src/routes/beacon.ts📚 Learning: 2025-08-29T02:12:34.132Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-08-29T02:12:34.132Z Learning: Applies to **/*.spec.ts : Name unit test files with the .spec.ts suffixApplied to files:
eslint.config.mjs🧬 Code graph analysis (16)
apps/api/src/routes/subscriptions.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/gateway/src/lib/costs.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/gateway/src/models/models.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/api/src/routes/chat.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/api/src/lib/beacon.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/api/src/stripe.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/api/src/scripts/generate-openapi.ts (1)
packages/logger/src/index.ts (1)
logger(107-107)apps/api/src/index.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/gateway/src/lib/cache.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/gateway/src/worker.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/gateway/src/chat/chat.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/gateway/src/scripts/generate-openapi.ts (1)
packages/logger/src/index.ts (1)
logger(107-107)apps/api/src/routes/beacon.ts (1)
packages/logger/src/index.ts (1)
logger(107-107)apps/gateway/src/lib/redis.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)apps/api/src/serve.ts (2)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)packages/db/src/db.ts (1)
closeDatabase(20-31)packages/models/src/provider-api.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)🔇 Additional comments (28)
eslint.config.mjs (2)
14-14: Enforcing no-console aligns with the new logger—LGTM.This complements the migration to @llmgateway/logger and will prevent regressions.
28-42: Override split confirmed
Validation shows noconsole.*usage outside the allowed test patterns and test-utils, and the UI/docs override correctly enforces console restrictions.apps/gateway/src/lib/redis.ts (1)
1-1: LGTM: centralized logger import.Importing the shared logger via a named export matches the package API. No issues.
apps/api/src/scripts/generate-openapi.ts (1)
10-10: LGTM on logger adoption.Structured logging replaces console calls correctly; error path preserves the Error instance.
Also applies to: 15-18
apps/gateway/src/models/models.ts (1)
237-240: LGTM on error logging.Good use of
logger.error(message, Error)to keep stack/context.apps/gateway/src/scripts/generate-openapi.ts (1)
18-18: LGTM on logger usage.Success and error paths now use structured logs.
Also applies to: 23-26
packages/models/src/provider-api.ts (2)
1017-1020: Good: scoped debug with model + provider.Helpful for triaging validation flows without leaking secrets.
1-1: Nice migration to central logger.Consistent structured logging across this module.
apps/api/src/routes/beacon.ts (1)
2-2: LGTM: central logger adoption.apps/api/src/routes/chat.ts (2)
127-130: Good: unified error logging shape for streaming failures.
161-164: Good: unified error logging for handler failures.apps/api/src/lib/beacon.ts (1)
67-67: LGTM: structured info on record creation.apps/api/src/routes/subscriptions.ts (5)
134-137: LGTM: error logging migration.
209-213: LGTM: error logging migration (cancel).
295-299: LGTM: error logging migration (resume).
395-399: LGTM: error logging migration (upgrade).
472-475: LGTM: error logging migration (status fetch).apps/api/src/index.ts (1)
4-4: LGTM: centralized logger importGood replacement for console.* with the shared logger.
apps/api/src/serve.ts (5)
3-3: LGTM: centralized logger importConsistent with the new logging standard.
16-19: LGTM: migration failure logging with safe error wrappingClear message and proper exit on failure.
52-69: LGTM: comprehensive, ordered shutdown logsGood sequencing and clarity for ops.
71-74: LGTM: error logging during shutdown with safe wrappingCovers non-Error cases correctly.
96-99: LGTM: startup failure logging with safe error wrappingClear message and correct termination.
apps/gateway/src/worker.ts (2)
13-13: Structured logger migration looks good.Nice, consistent replacement of console.* with the centralized logger, proper error normalization, and clear lifecycle messages.
Also applies to: 120-131, 147-151, 195-197, 228-236, 238-248, 250-255, 266-269, 309-312, 345-347, 361-361, 373-373, 376-379, 427-427, 433-433, 461-464, 474-474, 479-479, 483-483, 493-493, 501-501
283-303: Confirm guideline exception for read query with row locks.Coding guideline says reads should use
db().query.<table>.*, but here aSELECT … FOR UPDATE SKIP LOCKEDis used. That’s reasonable, but please confirm this exception is acceptable under team conventions.apps/gateway/src/chat/chat.ts (3)
10-10: Good move: centralized logger importAdopting the shared logger here aligns with the PR’s goals and improves observability.
558-561: Nice: error is properly normalized to an Error instanceThis ensures stacks are emitted by the logger. Looks good.
573-576: Consistent error normalizationSame as above—good practice to guarantee stack traces.
| process.on("uncaughtException", (error) => { | ||
| console.error("Uncaught exception:", error); | ||
| logger.error("Uncaught exception", error); | ||
| process.exit(1); | ||
| }); | ||
|
|
||
| process.on("unhandledRejection", (reason, promise) => { | ||
| console.error("Unhandled rejection at:", promise, "reason:", reason); | ||
| logger.error("Unhandled rejection", { promise, reason }); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type event handler params; avoid implicit any
Strengthens typing and aligns with repo rules.
-process.on("uncaughtException", (error) => {
+process.on("uncaughtException", (error: Error) => {
logger.error("Uncaught exception", error);
process.exit(1);
});
-process.on("unhandledRejection", (reason, promise) => {
- logger.error("Unhandled rejection", { promise, reason });
+process.on(
+ "unhandledRejection",
+ (reason: unknown, promise: Promise<unknown>) => {
+ logger.error("Unhandled rejection", { promise, reason });
+ process.exit(1);
+ },
+);
- process.exit(1);
-});📝 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.
| process.on("uncaughtException", (error) => { | |
| console.error("Uncaught exception:", error); | |
| logger.error("Uncaught exception", error); | |
| process.exit(1); | |
| }); | |
| process.on("unhandledRejection", (reason, promise) => { | |
| console.error("Unhandled rejection at:", promise, "reason:", reason); | |
| logger.error("Unhandled rejection", { promise, reason }); | |
| process.exit(1); | |
| }); | |
| process.on("uncaughtException", (error: Error) => { | |
| logger.error("Uncaught exception", error); | |
| process.exit(1); | |
| }); | |
| process.on( | |
| "unhandledRejection", | |
| (reason: unknown, promise: Promise<unknown>) => { | |
| logger.error("Unhandled rejection", { promise, reason }); | |
| process.exit(1); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In apps/api/src/serve.ts around lines 85 to 93, the process event handler
parameters are implicitly any; add explicit types: declare the uncaughtException
handler parameter as (error: Error) and the unhandledRejection handler
parameters as (reason: unknown, promise: Promise<unknown>), and update the
logger calls if needed (e.g., pass error directly and serialize/inspect reason)
so TypeScript no longer reports implicit any and typings align with repo rules.
| logger.info(JSON.stringify({ kind: "stripe-event", payload: event })); | ||
|
|
There was a problem hiding this comment.
Avoid logging full Stripe payloads; risk of PII and noisy logs.
Log minimal, structured fields; emit full payload only at debug in non-prod.
- logger.info(JSON.stringify({ kind: "stripe-event", payload: event }));
+ if (process.env.NODE_ENV !== "production") {
+ logger.debug("Stripe event received", {
+ id: event.id,
+ type: event.type,
+ livemode: (event as any).livemode, // keep minimal; remove if unsure
+ });
+ } else {
+ logger.info("Stripe event received", { id: event.id, type: event.type });
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/stripe.ts around lines 178 to 179, the current code logs the
entire Stripe event payload which can leak PII and makes logs noisy; change the
logging to emit a minimal structured object (e.g., event.id, event.type,
event.created, event.livemode, event.data.object?.id or customer) via
logger.info, and only log the full JSON payload at logger.debug guarded by an
environment check (non-production) to avoid exposing sensitive data in
production logs.
| logger.info( | ||
| `Successfully upgraded organization ${organizationId} to pro plan via checkout. Updated rows:`, | ||
| result.length, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Second arg to logger should be an object; don’t pass a bare number.
Wrap counts in an object for structured logs.
- logger.info(
- `Successfully upgraded organization ${organizationId} to pro plan via checkout. Updated rows:`,
- result.length,
- );
+ logger.info(
+ `Successfully upgraded organization ${organizationId} to pro plan via checkout.`,
+ { updatedRows: result.length },
+ );
- logger.info(
- `Successfully upgraded organization ${organizationId} to pro plan. Updated rows:`,
- result.length,
- );
- logger.info(
- `Verification - organization plan is now: ${result && result[0]?.plan}`,
- );
+ logger.info(
+ `Successfully upgraded organization ${organizationId} to pro plan.`,
+ { updatedRows: result.length, newPlan: result?.[0]?.plan },
+ );Also applies to: 631-639
🤖 Prompt for AI Agents
In apps/api/src/stripe.ts around lines 271-274 (and similarly at 631-639), the
logger.info call passes a bare number as the second argument which breaks
structured logging; change the call to pass an object containing the count (e.g.
{ updatedRows: result.length }) instead of the raw number so the log entry
remains structured and readable.
| logger.error(`Failed to encode chat messages in costs: ${error}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log structured errors to retain stack traces.
Pass the Error object as the second argument; avoid string interpolation.
- logger.error(`Failed to encode chat messages in costs: ${error}`);
+ logger.error("Failed to encode chat messages in costs", error instanceof Error ? error : new Error(String(error)));
- logger.error(`Failed to encode prompt text: ${error}`);
+ logger.error("Failed to encode prompt text", error instanceof Error ? error : new Error(String(error)));
- logger.error(`Failed to encode completion text: ${error}`);
+ logger.error("Failed to encode completion text", error instanceof Error ? error : new Error(String(error)));Also applies to: 82-83, 93-94
🤖 Prompt for AI Agents
In apps/gateway/src/lib/costs.ts around lines 74-75 (and similarly at 82-83 and
93-94), the review points out that logger.error currently interpolates the Error
into a string which drops structured error data and stack traces; change those
logger.error calls to pass the Error object as a separate argument (e.g.,
logger.error("Failed to encode chat messages in costs", error)) so the logger
can retain the error stack and structured fields while keeping a clear
descriptive message.
| @@ -1,3 +1,5 @@ | |||
| import { logger } from "@llmgateway/logger"; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find other spots that may log raw image URLs
rg -nP -C2 '(logger\.(error|warn)\s*\(|`\s*[^`]*\$\{[^}]*image_url\.url[^}]*\}\s*`)' --type=tsLength of output: 32285
Sanitize raw image_url.url in logs: In packages/models/src/provider-api.ts at line 256, you’re logging the full part.image_url.url; truncate or mask it (e.g. use substring(…,50)+"…") instead of logging the raw URL.
🤖 Prompt for AI Agents
In packages/models/src/provider-api.ts around line 256, the code currently logs
the full part.image_url.url; update the logging to sanitize the value by
truncating or masking it (e.g., take a safe substring of the URL like
url.substring(0,50) + "…" or replace the middle with "...") and ensure you
null-check part.image_url and part.image_url.url before accessing; use the
sanitized variable in the logger call instead of the raw URL so full URLs are
never emitted in logs.
| } | ||
|
|
||
| try { | ||
| const response = await fetch(url); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a timeout to external fetch.
Prevents hangs on slow/unreachable hosts.
Apply:
- const response = await fetch(url);
+ const response = await fetch(url, { signal: AbortSignal.timeout(10_000) });📝 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.
| const response = await fetch(url); | |
| const response = await fetch(url, { signal: AbortSignal.timeout(10_000) }); |
🤖 Prompt for AI Agents
In packages/models/src/provider-api.ts around line 79, the external fetch call
(const response = await fetch(url);) lacks a timeout and can hang; wrap the
fetch with an AbortController, start a timer (e.g. config-driven default like
5000ms) that calls controller.abort() on expiry, pass signal: controller.signal
into fetch, clear the timer after fetch completes, and catch the abort/timeout
error (handle as a failed request with an appropriate error or retry logic) so
slow or unreachable hosts don't block execution.
Deleted unnecessary `no-console` overrides for test and UI files in the ESLint config.
- Transitioned package to a dual-module setup (`module` for ESM, `main` for CJS) for broader compatibility. - Replaced local `tsup.config.ts` with a shared configuration export. - Updated `scripts.build` to use both `tsc` and `tsup` for type checks and bundling. - Adjusted TypeScript paths for cleaner dependencies resolution.
- Replace generic `error` parameter with `error as Error` type annotations in logger calls across `stripe.ts` and `cache.ts`. - Improve logging clarity and help with TypeScript type safety.
This reverts commit a68b2dd.
- Explicitly declare public access modifiers for all class methods in `LLMGatewayLogger`. - Improves code readability and aligns with TypeScript best practices.
- Add `pathname` dependency to `useEffect` for accurate onboarding redirect behavior. - Use `void` with `invalidateQueries` calls to comply with TypeScript standards.
- Eliminate redundant onSuccess props and logic in `UpgradeToProDialog` and related components. - Update dependency arrays in `useEffect` for accuracy and consistency. - Add void usage to `invalidateQueries` for TypeScript compliance.
- Eliminate unused `_err` variable in `provider-api.ts` for cleaner code.
steebchen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/ui/src/app/playground/playground-client.tsx (2)
152-160: Revert dependency array change to avoid state oscillation.Including
selectedModelcauses this effect to run beforesearchParamsreflect the navigation, briefly reverting the user’s selection from stale URL params (race/back/forward). Keep this effect driven solely by URL changes.- }, [searchParams, selectedModel]); + }, [searchParams]);
296-336: Eliminateanytypes for images.Project guideline forbids
: any. Type images explicitly to matchMessage["images"].type ImagePart = { type: "image_url"; image_url: { url: string } }; let fullContent = ""; let finalImages: ImagePart[] = []; let hasReceivedImages = false; // … let imagesToSet: ImagePart[] | undefined;apps/ui/src/components/shared/upgrade-to-pro-dialog.tsx (2)
67-69: Don’t use window.location for navigation in apps/uiPer guidelines, use Next.js router for programmatic navigation.
Apply:
+ "use client"; + import { useRouter } from "next/navigation"; @@ function UpgradeDialogContent({ @@ }) { + const router = useRouter(); @@ - // Redirect to Stripe Checkout - window.location.href = checkoutUrl; + // Redirect to Stripe Checkout + router.push(checkoutUrl);Note: This file already uses React hooks; the
"use client"directive is required at the top if not present elsewhere.
1-1: Add"use client"directive at the top of this component
Insert the line"use client";above the existing imports in
apps/ui/src/components/shared/upgrade-to-pro-dialog.tsx, since it usesuseStateand must be a client component in the Next.js App Router.
♻️ Duplicate comments (1)
packages/logger/src/index.ts (1)
21-34: Add security-focused log redaction configurationThe current implementation doesn't include redaction for sensitive data, which could lead to credentials or PII being logged, especially in production JSON logs.
🧹 Nitpick comments (5)
apps/ui/src/app/playground/playground-client.tsx (2)
136-151: Guard router.replace and drop searchParams from deps to prevent replace loops.Avoid unnecessary replaces and effect thrash by comparing URLs and depending only on
selectedModel(androuter).useEffect(() => { const current = window.location.pathname + window.location.search; const params = new URLSearchParams(window.location.search); if (selectedModel !== "gpt-4o-mini") params.set("model", selectedModel); else params.delete("model"); const next = params.toString() ? `${window.location.pathname}?${params.toString()}` : window.location.pathname; if (next !== current) router.replace(next); }, [selectedModel, router]);
97-99: Align with logger migration (or confirm UI exemption).Multiple
console.*calls remain. If UI is in scope for @llmgateway/logger, switch to it; otherwise confirm ESLint overrides allow console in apps/ui.Also applies to: 195-197, 366-367, 401-402, 441-442, 448-451
apps/ui/src/hooks/useUser.ts (1)
118-121: Batch the invalidateQueries callsFunctionality is fine and the
voidprefix is correct. Minor optimization: batch the two invalidations to schedule a single microtask.Apply:
- onSuccess: () => { - void queryClient.invalidateQueries({ queryKey: ["user"] }); - void queryClient.invalidateQueries({ queryKey: ["session"] }); - }, + onSuccess: () => { + void Promise.all([ + queryClient.invalidateQueries({ queryKey: ["user"] }), + queryClient.invalidateQueries({ queryKey: ["session"] }), + ]); + },apps/ui/src/components/settings/caching-settings.tsx (1)
70-76: Align with thevoidinvalidate pattern used elsewhereThe callback looks good. For consistency with the rest of the PR (and to silence potential no-floating-promises rules), prefix the invalidation with
void.- queryClient.invalidateQueries({ queryKey }); + void queryClient.invalidateQueries({ queryKey });packages/logger/src/index.ts (1)
98-103: Consider using a more robust child logger instantiationThe current child logger creation uses
Object.create()and manual property assignment, which bypasses the constructor and its validation logic. This could lead to inconsistent behavior if the constructor logic changes.Apply this diff for a more maintainable approach:
- public child(bindings: object): LLMGatewayLogger { - const childPino = this.logger.child(bindings); - const childLogger = Object.create(LLMGatewayLogger.prototype); - childLogger.logger = childPino; - return childLogger; - } + public child(bindings: object): LLMGatewayLogger { + const childPino = this.logger.child(bindings); + const childLogger = new LLMGatewayLogger({}); + childLogger.logger = childPino; + return childLogger; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
apps/api/src/stripe.ts(35 hunks)apps/gateway/src/lib/cache.ts(11 hunks)apps/ui/src/app/playground/playground-client.tsx(1 hunks)apps/ui/src/components/landing/pricing-plans.tsx(0 hunks)apps/ui/src/components/settings/caching-settings.tsx(1 hunks)apps/ui/src/components/settings/project-mode-settings.tsx(1 hunks)apps/ui/src/components/shared/upgrade-to-pro-dialog.tsx(1 hunks)apps/ui/src/hooks/useUser.ts(2 hunks)packages/logger/package.json(1 hunks)packages/logger/src/index.ts(1 hunks)packages/logger/tsconfig.json(1 hunks)packages/logger/tsup.config.ts(1 hunks)packages/models/src/provider-api.ts(8 hunks)
💤 Files with no reviewable changes (1)
- apps/ui/src/components/landing/pricing-plans.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/logger/tsconfig.json
- apps/api/src/stripe.ts
- packages/models/src/provider-api.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/ui/src/app/playground/playground-client.tsxapps/gateway/src/lib/cache.tsapps/ui/src/hooks/useUser.tsapps/ui/src/components/settings/project-mode-settings.tsxapps/ui/src/components/settings/caching-settings.tsxpackages/logger/src/index.tsapps/ui/src/components/shared/upgrade-to-pro-dialog.tsxpackages/logger/tsup.config.ts
apps/ui/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In apps/ui (a tanstack router project), always use navigate() for navigation
Files:
apps/ui/src/app/playground/playground-client.tsxapps/ui/src/hooks/useUser.tsapps/ui/src/components/settings/project-mode-settings.tsxapps/ui/src/components/settings/caching-settings.tsxapps/ui/src/components/shared/upgrade-to-pro-dialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
apps/ui/src/app/playground/playground-client.tsxapps/gateway/src/lib/cache.tsapps/ui/src/hooks/useUser.tsapps/ui/src/components/settings/project-mode-settings.tsxapps/ui/src/components/settings/caching-settings.tsxpackages/logger/src/index.tsapps/ui/src/components/shared/upgrade-to-pro-dialog.tsxpackages/logger/tsup.config.ts
apps/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/ui/**/*.{ts,tsx}: In the Next.js UI, use next/link for links and next/navigation’s router for programmatic navigation
Use localStorage (not cookies) for client-side data persistence in the UI
Use TanStack Query for client-side state management in the UI
Use Radix UI with Tailwind CSS for UI components and styling in the frontend
Files:
apps/ui/src/app/playground/playground-client.tsxapps/ui/src/hooks/useUser.tsapps/ui/src/components/settings/project-mode-settings.tsxapps/ui/src/components/settings/caching-settings.tsxapps/ui/src/components/shared/upgrade-to-pro-dialog.tsx
apps/{ui,docs}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/ui/src/app/playground/playground-client.tsxapps/ui/src/hooks/useUser.tsapps/ui/src/components/settings/project-mode-settings.tsxapps/ui/src/components/settings/caching-settings.tsxapps/ui/src/components/shared/upgrade-to-pro-dialog.tsx
apps/ui/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/ui/**/*.{ts,tsx,js,jsx}: In apps/ui (Next.js App Router), use next/link for links
In apps/ui, use next/navigation router.push/replace or redirect() for programmatic navigation
In apps/ui, never use window.location for navigation
Files:
apps/ui/src/app/playground/playground-client.tsxapps/ui/src/hooks/useUser.tsapps/ui/src/components/settings/project-mode-settings.tsxapps/ui/src/components/settings/caching-settings.tsxapps/ui/src/components/shared/upgrade-to-pro-dialog.tsx
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/gateway/src/lib/cache.tsapps/ui/src/hooks/useUser.tspackages/logger/src/index.tspackages/logger/tsup.config.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware.findMany() or db().query.
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle with the latest object syntax for database operations
For read queries, use db().query..findFirst() Files:
apps/gateway/src/lib/cache.tsapps/gateway/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafety
Files:
apps/gateway/src/lib/cache.ts🧬 Code graph analysis (1)
apps/gateway/src/lib/cache.ts (1)
packages/logger/src/index.ts (2)
logger(107-107)error(81-87)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e / run
- GitHub Check: build / run
🔇 Additional comments (9)
apps/ui/src/hooks/useUser.ts (1)
49-71: Good call: include pathname in the onboarding redirect effect depsRe-running the redirect check on route changes avoids stale decisions when users navigate client-side.
apps/ui/src/components/settings/project-mode-settings.tsx (1)
60-66: LGTM: onSuccess signature + void invalidationMatches the new pattern and keeps cache refresh predictable.
apps/ui/src/components/shared/upgrade-to-pro-dialog.tsx (1)
30-37: Public API change: onSuccess prop removed and no remaining call-sites
Verified that no<UpgradeToProDialog>usage passes anonSuccessprop.packages/logger/tsup.config.ts (1)
1-3: LGTM - Clean configuration reuseThe configuration correctly re-exports the shared tsup configuration from the root, maintaining consistency across the monorepo while allowing for potential future customization if needed.
packages/logger/package.json (1)
1-29: Package configuration looks solid with proper ESM/CJS dual exportsThe package.json correctly configures:
- Dual ESM/CJS exports with proper type definitions
- Appropriate dependencies (pino ecosystem)
- Build toolchain with TypeScript and tsup
- Test framework integration
The structure follows modern Node.js package standards and should integrate well with the monorepo architecture.
packages/logger/src/index.ts (2)
37-46: LGTM - Appropriate environment-based log level defaultsThe log level configuration properly handles different environments:
- Test:
warnto reduce noise during testing- Production:
infofor operational visibility without debug clutter- Development:
debugfor detailed troubleshooting
81-95: Excellent Error object handlingThe error and fatal methods properly handle both Error instances and plain objects, using pino's
errfield convention for Error objects to enable proper stack trace serialization.apps/gateway/src/lib/cache.ts (2)
2-2: LGTM - Clean migration to centralized loggingThe import of the centralized logger aligns with the PR's objective to replace ad-hoc console logging across the codebase.
29-29: Consistent error logging migration with proper Error castingAll catch blocks have been consistently updated to use
logger.error()with proper Error type casting. This provides structured logging while maintaining the existing error handling semantics and control flow.The type casting
as Erroris appropriate here since these catch blocks are expected to receive Error objects from the underlying operations.Also applies to: 41-41, 78-78, 106-106, 134-134, 171-171, 211-211, 250-250, 294-294, 308-308
1 participant
Summary
This PR introduces a new
@llmgateway/loggerpackage that provides a comprehensive structured logging solution using the pino logger. It replaces debug console.log, console.error, console.warn, and console.info statements throughout the codebase with this proper logging infrastructure.Key Changes Made:
@llmgateway/loggerpackage with pino-based structured loggingpackages/models/src/provider-api.ts(including line 915 validation log)packages/auth/src/auth.ts(email verification and Brevo contact creation)packages/db/src/migrate.tsanddb.ts(database operations)apps/gateway/src/serve.tsandworker.ts(server lifecycle, shutdown, worker logs)apps/gateway/src/index.ts(HTTP exceptions and health checks)apps/api/src/index.ts,lib/beacon.ts,routes/beacon.ts,routes/chat.ts,routes/subscriptions.ts,scripts/generate-openapi.ts,serve.ts,stripe.ts(various API and webhook logs)apps/gateway/src/chat/chat.ts,lib/cache.ts,lib/costs.ts,lib/redis.ts,models/models.ts,scripts/generate-openapi.ts(gateway logs and cache)apps/ui/src/lib/server-api.ts(server API error logging)@llmgateway/loggerdependencyLogging Features:
Technical Implementation:
Performance Impact:
Test Plan
🤖 Generated with Claude Code
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/d9e82a8a-9f9e-4f59-b7bd-75723f3ee45b
Summary by CodeRabbit
New Features
Refactor
Chores
Tests