feat(instrumentation): add tracing and improve logging - #785
Conversation
Removed unused `/test-trace` endpoints and related OpenTelemetry imports from `api` and `gateway`. Streamlined codebase by eliminating unused tracing logic.
Introduced force tracing via HTTP header `X-Force-Trace`. Added a custom `HeaderBasedForceSampler` to allow sampling override when the header is present. Updated middleware to capture and propagate the header as span attributes, ensuring trace context is included in responses. Added comprehensive tests and documentation.
Updated the `tsup.config.ts` to simplify the exclusion pattern for entry files. Consolidated multiple patterns into a single regex for improved clarity and maintainability.
Moved instrumentation initialization directly into `serve.ts` files for both API and Gateway services. Removed unused dedicated `instrumentation.ts` files, reducing redundancy and streamlining the initialization process. Updated logger to handle absent `pid` and `hostname` by default.
WalkthroughAdds a new @llmgateway/instrumentation package (OpenTelemetry bootstrap, middleware, sampler, tests, docs), integrates tracing into API and Gateway (middleware, startup init/shutdown, trace-aware HTTP clients), augments logger and auth types with trace context, enables source maps, updates Docker build contexts, and adds a multi-arch build script and minor config changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant GW as Gateway (Hono)
participant TM as Tracing Middleware
participant OT as OpenTelemetry
participant S as Upstream Service
rect rgba(230,245,255,0.5)
note right of GW: Startup
GW->>OT: initializeInstrumentation(serviceName="llmgateway-gateway")
end
C->>GW: HTTP Request
activate GW
GW->>TM: invoke tracingMiddleware (extract context, start SERVER span)
TM->>OT: extract & start span
TM-->>GW: next()
GW->>S: httpClient(...) (inject trace headers, CLIENT span)
S-->>GW: Response
GW->>OT: record client span attrs & end
GW->>TM: finish (set status, add x-trace-id, end SERVER span)
deactivate GW
GW-->>C: Response (+trace headers)
sequenceDiagram
autonumber
participant C as Client
participant API as API (Hono)
participant TM as Tracing Middleware
participant OT as OpenTelemetry
participant DB as Downstream
rect rgba(230,245,255,0.5)
note right of API: Startup
API->>OT: initializeInstrumentation(serviceName="llmgateway-api")
end
C->>API: HTTP Request
API->>TM: tracingMiddleware (start SERVER span)
TM->>OT: extract & start span
TM-->>API: next()
API->>DB: httpClient (CLIENT span, inject)
DB-->>API: Response
API->>OT: end client span
API->>TM: finalize (status, headers, end SERVER span)
API-->>C: Response (+trace headers)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
Introduced a `level` option to the logger, allowing default log level configuration. Ensures greater flexibility and control over logging behavior. Removed fallback for deprecated `GCP_PROJECT` environment variable.
Removed unused `typescript` and `tsup` from relevant project packages. Updated `pnpm-lock.yaml` to reflect these changes, ensuring a cleaner dependency graph.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/serve.ts (1)
45-46: Avoid any: type the server instance.Use Node’s http.Server to satisfy the “no any” guideline.
+import type { Server } from "http"; @@ -const closeServer = (server: any): Promise<void> => { +const closeServer = (server: Server): Promise<void> => { @@ -const gracefulShutdown = async (signal: string, server: any) => { +const gracefulShutdown = async (signal: string, server: Server) => {Also applies to: 57-57
🧹 Nitpick comments (28)
.claude/settings.json (1)
3-16: Prefer gating format/build to tool activity; or add timeouts to avoid long-running Stop hooks.Running
pnpm formatandpnpm buildon every Stop can be slow and fire even when no files changed. Consider moving these toPostToolUsewithmatcher: "Edit|MultiEdit|Write"so they only run after code modifications, or at minimum add per-commandtimeoutfields. Also, Stop hooks can influence control flow; guard against accidental loops if you later switch to scripts. (docs.anthropic.com)Option A — gate by tool activity:
- "Stop": [ - { - "hooks": [ - { "type": "command", "command": "pnpm format" }, - { "type": "command", "command": "pnpm build" } - ] - } - ] + "PostToolUse": [ + { + "matcher": "Edit|MultiEdit|Write", + "hooks": [ + { "type": "command", "command": "pnpm format", "timeout": 60 }, + { "type": "command", "command": "pnpm build", "timeout": 300 } + ] + } + ]Option B — keep Stop, add timeouts:
- "Stop": [ + "Stop": [ { "hooks": [ - { "type": "command", "command": "pnpm format" }, - { "type": "command", "command": "pnpm build" } + { "type": "command", "command": "pnpm format", "timeout": 60 }, + { "type": "command", "command": "pnpm build", "timeout": 300 } ] } ]CLAUDE.md (1)
106-106: Prefer top-levelimport; allow vetted dynamicimport()Repo already uses dynamic imports for legitimate cases (SSR/code-splitting and test-only mocks). Update the guideline to permit
import()with justification.Found examples: packages/instrumentation/src/index.spec.ts (await import("./index.js")), apps/ui/src/components/providers.tsx (import("crisp-sdk-web")), apps/ui/src/app/blog/page.tsx & apps/ui/src/app/blog/category/[category]/page.tsx (await import("content-collections")), apps/gateway/src/lib/rate-limit.spec.ts (await import("./redis"), await import("./cache")).
-- Always use top-level `import`, never use require or dynamic imports +- Prefer top-level `import`. Do not use CommonJS `require`. +- Dynamic `import()` is allowed only for code-splitting or conditional runtime/platform loading (e.g., Next.js, optional deps, heavy instrumentation), + and must include a brief comment explaining why.apps/api/src/stripe.ts (1)
209-209: Use structured logging for unhandled Stripe eventsPrefer structured fields over string interpolation to aid searchability and to include the Stripe event id.
- logger.warn(`Unhandled event type: ${event.type}`); + logger.warn("Unhandled Stripe event", { type: event.type, id: event.id });tsconfig.json (1)
19-23: Consider embedding original sources in sourcemapsWith
--enable-source-mapsin production, addinginlineSourceshelps map stack traces without needing the original files on disk."esModuleInterop": true, "sourceMap": true, + "inlineSources": true, "noFallthroughCasesInSwitch": true,packages/auth/src/auth.ts (1)
198-203: Reuse shared TraceContext type instead of duplicating fieldsKeep types consistent with the logger package and reduce drift by nesting a
traceobject.+import type { TraceContext } from "@llmgateway/logger"; export interface Variables { user: typeof auth.$Infer.Session.user | null; session: typeof auth.$Infer.Session.session | null; - traceId?: string; - spanId?: string; + trace?: TraceContext; }apps/gateway/src/chat/chat.ts (1)
66-66: Prefer warn + add context to “User not found” logThis is a validation outcome, not a server fault. Log as warn and include model to aid triage.
- logger.error("User not found", { organizationId }); + logger.warn("User not found for organization", { organizationId, requestedModel });apps/ui/next.config.ts (1)
6-6: Consider gating production source maps to avoid shipping sources by defaultEnabling browser source maps in prod can leak source and increase bundle size. Suggest env-gating and using hidden-source-map on server for safer defaults.
Apply:
- productionBrowserSourceMaps: true, + productionBrowserSourceMaps: process.env.ENABLE_SOURCEMAPS === "true", @@ - webpack: (config, { isServer }) => { - if (isServer) { - config.devtool = "source-map"; - } - return config; - }, + webpack: (config, { isServer }) => { + if (isServer && process.env.ENABLE_SOURCEMAPS === "true") { + config.devtool = "hidden-source-map"; + } + return config; + },Also applies to: 16-21
packages/instrumentation/tsup.config.ts (1)
1-9: Solid minimal build config; add node platform/target and sourcemaps for DXBuilding a lib: set platform/target and emit source maps for debugging while keeping deps external.
export default defineConfig({ entry: ["src/index.ts"], format: ["esm", "cjs"], dts: true, clean: true, - external: ["@google-cloud/opentelemetry-cloud-trace-exporter"], + sourcemap: true, + platform: "node", + target: "node20", + external: ["@google-cloud/opentelemetry-cloud-trace-exporter"], });apps/docs/next.config.mjs (1)
8-8: Mirror UI: gate prod source mapsSame concern as in apps/ui; recommend env gate and hidden-source-map on server.
- productionBrowserSourceMaps: true, + productionBrowserSourceMaps: process.env.ENABLE_SOURCEMAPS === "true", @@ - webpack: (config, { isServer }) => { - if (isServer) { - config.devtool = "source-map"; - } - return config; - }, + webpack: (config, { isServer }) => { + if (isServer && process.env.ENABLE_SOURCEMAPS === "true") { + config.devtool = "hidden-source-map"; + } + return config; + },Also applies to: 12-17
packages/instrumentation/package.json (1)
1-37: Pinset looks current; add engines and guard semconv/SDK compatibilityVersions appear valid as of Sep 13, 2025 (e.g., @opentelemetry/core 2.1.0, sdk-trace-base 2.1.0, @Google-Cloud exporter 3.0.0, propagator 0.21.0). Consider adding an engines constraint to match OTel 2.x minimum Node and prevent accidental installs on older runtimes.
- Verification refs: core 2.1.0 and sdk-trace-base 2.1.0 exist; exporter 3.0.0 and propagator 0.21.0 published 3 days ago. (npmjs.com)
Add:
"types": "./dist/index.d.ts", "scripts": { "build": "tsc && tsup" }, + "engines": { + "node": ">=18.19.0 || >=20.6.0" + },packages/instrumentation/README.md (2)
47-52: Clarify header parsing and case-sensitivitySpecify whether X-Force-Trace value matching is case-insensitive and trimmed (e.g., "True", " TRUE "). If code enforces exact string equality, call it out here to avoid confusion.
55-58: Document Node.js version requirementAdd minimum Node version to align with OpenTelemetry 2.x requirements to prevent runtime surprises.
apps/gateway/src/lib/http-client.ts (1)
21-25: Headers and body handling: avoid forcing JSON and update semconv keys
- Don’t unconditionally set Content-Type to application/json; only set when sending JSON.
- Accept standard HeadersInit/BodyInit types for flexibility.
- Consider semconv updates (http.response.status_code, http.response.body.size) and record numeric sizes.
- const fetchHeaders = { - "Content-Type": "application/json", - ...headers, - ...traceHeaders, - }; + const fetchHeaders: HeadersInit = + headers ? { ...headers, ...traceHeaders } : traceHeaders; @@ - if (body) { - fetchOptions.body = - typeof body === "string" ? body : JSON.stringify(body); - } + if (body !== undefined) { + if (typeof body === "string" || body instanceof Uint8Array) { + fetchOptions.body = body as BodyInit; + } else { + fetchOptions.body = JSON.stringify(body); + (fetchOptions.headers as Record<string, string>)["Content-Type"] ??= + "application/json"; + } + } @@ - "http.status_code": response.status, - "http.response.size": response.headers.get("content-length") || "", + "http.response.status_code": response.status, + "http.response.body.size": Number(response.headers.get("content-length") || 0),Also applies to: 33-35, 54-59
apps/api/src/lib/http-client.ts (3)
38-38: Avoid new URL() in the span name expression.If url is invalid, this throws before try/catch and you won’t end a span. Parse first (as in the diff above) or catch and fall back to the raw url.
49-57: Do not mark 4xx as OK and emit numeric sizes.
- Setting status OK for 4xx is misleading; leave UNSET for 4xx and set ERROR only for 5xx.
- Emit response size as a number.
- span.setAttributes({ - "http.status_code": response.status, - "http.response.size": response.headers.get("content-length") || "", - }); + span.setAttributes({ + "http.status_code": response.status, + "http.response.size": Number(response.headers.get("content-length") || 0), + }); @@ - if (!response.ok) { - span.setStatus({ - code: response.status >= 500 ? 2 : 1, // ERROR : OK - message: `HTTP ${response.status}`, - }); - } + if (response.status >= 500) { + span.setStatus({ code: 2, message: `HTTP ${response.status}` }); + } else if (response.ok) { + span.setStatus({ code: 1 }); + }
3-8: Broaden body typing and set Content-Type conditionally.Support BodyInit (FormData/Blob/URLSearchParams) and only set JSON content-type when stringifying objects.
-export interface HttpClientOptions { +export interface HttpClientOptions { method?: string; headers?: Record<string, string>; - body?: string | object; + body?: BodyInit | object; timeout?: number; } @@ - const fetchHeaders = { - "Content-Type": "application/json", - ...headers, - ...traceHeaders, - }; + const fetchHeaders: Record<string, string> = { + ...headers, + ...traceHeaders, + }; @@ - if (body) { - fetchOptions.body = typeof body === "string" ? body : JSON.stringify(body); - } + if (body !== undefined) { + if (typeof body === "string" || (body as any) instanceof ReadableStream || body instanceof FormData || body instanceof Blob || body instanceof URLSearchParams || body instanceof ArrayBuffer || ArrayBuffer.isView(body)) { + fetchOptions.body = body as BodyInit; + } else { + fetchOptions.body = JSON.stringify(body); + fetchHeaders["Content-Type"] ||= "application/json"; + } + }Also applies to: 33-35, 21-26
packages/logger/src/index.ts (1)
92-121: Type nit: clarify getTraceContext return type.It returns Google Cloud fields plus ids, not TraceContext. Consider returning Record<string, unknown> and exporting a separate type if you need to surface it.
apps/api/src/serve.ts (1)
12-16: Optional: gate telemetry initialization with an env flag.You already set TELEMETRY_ACTIVE in the Docker stage; consider skipping init when it’s explicitly false to reduce local overhead.
- // Initialize tracing for API service - initializeInstrumentation({ - serviceName: process.env.OTEL_SERVICE_NAME || "llmgateway-api", - projectId: process.env.GOOGLE_CLOUD_PROJECT, - }); + // Initialize tracing for API service + if (process.env.TELEMETRY_ACTIVE !== "false") { + initializeInstrumentation({ + serviceName: process.env.OTEL_SERVICE_NAME || "llmgateway-api", + projectId: process.env.GOOGLE_CLOUD_PROJECT, + }); + }apps/gateway/src/index.ts (1)
11-11: Tracing middleware placement: good; consider documenting trace headersRegistering tracing first is correct. Consider adding OpenAPI docs (global note or per-response headers) for x-trace-id and x-cloud-trace-context so clients can correlate requests.
Also applies to: 47-49
apps/api/src/index.ts (1)
11-11: Global tracing added in the right spotLGTM. Tracing runs before CORS and routes. Optionally document emitted trace headers (x-trace-id, x-cloud-trace-context) in OpenAPI for discoverability.
Also applies to: 33-35
apps/api/src/middleware/tracing.ts (1)
1-5: Avoid hard-coding service name; read from env with fallbackKeeps environments consistent with initializeInstrumentation in serve.ts.
-export const tracingMiddleware = createTracingMiddleware({ - serviceName: "llmgateway-api", -}); +export const tracingMiddleware = createTracingMiddleware({ + serviceName: process.env.OTEL_SERVICE_NAME || "llmgateway-api", +});packages/instrumentation/src/middleware.spec.ts (1)
51-53: Restore spies, not just clear callsUse vi.restoreAllMocks() to avoid leaked mocks across suites.
- afterEach(() => { - vi.clearAllMocks(); - }); + afterEach(() => { + vi.clearAllMocks(); + vi.restoreAllMocks(); + });packages/instrumentation/src/index.spec.ts (2)
12-21: Fix linter warnings by adding accessibility modifiers on mock methodsMinor TS style fix.
- NodeSDK: class MockNodeSDK { - start() { + NodeSDK: class MockNodeSDK { + public start() { return Promise.resolve(); - } - shutdown() { + } + public shutdown() { return Promise.resolve(); } },
31-41: Add accessibility modifier to mock BatchSpanProcessor.forceFlush- BatchSpanProcessor: class MockBatchSpanProcessor { - forceFlush() { + BatchSpanProcessor: class MockBatchSpanProcessor { + public forceFlush() { return Promise.resolve(); } },packages/instrumentation/src/index.ts (1)
23-67: Consider exportingHeaderBasedForceSamplerif tests or apps import it.If you intend to test or reuse it externally, add
exportto the class or re-export from index.-class HeaderBasedForceSampler implements Sampler { +export class HeaderBasedForceSampler implements Sampler {scripts/build-images.sh (3)
201-207: Address ShellCheck SC2155: avoid masking return values with inline assignment.Declare and assign separately when reading result files.
- if [[ -f "/tmp/build_result_$app" ]]; then - local result=$(cat "/tmp/build_result_$app") + if [[ -f "/tmp/build_result_$app" ]]; then + local result + result="$(cat "/tmp/build_result_$app")" @@ - if [[ -f "/tmp/build_result_unified" ]]; then - local result=$(cat "/tmp/build_result_unified") + if [[ -f "/tmp/build_result_unified" ]]; then + local result + result="$(cat "/tmp/build_result_unified")"Also applies to: 211-217
118-120: Graceful fallback when not in a git repo.Avoid hard failure if
gitmetadata is unavailable.- SHORT_SHA=$(git rev-parse --short HEAD) - IMAGE_TAG="v0.0.0-${SHORT_SHA}" + if SHORT_SHA="$(git rev-parse --short HEAD 2>/dev/null)"; then + IMAGE_TAG="v0.0.0-${SHORT_SHA}" + else + IMAGE_TAG="v0.0.0-local" + print_warning "Git SHA not found; using tag ${IMAGE_TAG}" + fi
229-231: Pre-checkpnpmavailability.Fail fast with a clearer message if pnpm is missing.
-pnpm build +if ! command -v pnpm >/dev/null 2>&1; then + print_error "pnpm is required but not installed" + exit 1 +fi +pnpm build
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
.claude/settings.json(1 hunks)CLAUDE.md(1 hunks)apps/api/package.json(2 hunks)apps/api/src/index.ts(2 hunks)apps/api/src/lib/http-client.ts(1 hunks)apps/api/src/middleware/tracing.ts(1 hunks)apps/api/src/serve.ts(2 hunks)apps/api/src/stripe.ts(1 hunks)apps/docs/next.config.mjs(1 hunks)apps/gateway/package.json(1 hunks)apps/gateway/src/chat/chat.ts(1 hunks)apps/gateway/src/index.ts(2 hunks)apps/gateway/src/lib/http-client.ts(1 hunks)apps/gateway/src/middleware/tracing.ts(1 hunks)apps/gateway/src/serve.ts(2 hunks)apps/ui/next.config.ts(2 hunks)infra/split.dockerfile(1 hunks)infra/unified.dockerfile(1 hunks)packages/auth/src/auth.ts(1 hunks)packages/instrumentation/README.md(1 hunks)packages/instrumentation/package.json(1 hunks)packages/instrumentation/src/index.spec.ts(1 hunks)packages/instrumentation/src/index.ts(1 hunks)packages/instrumentation/src/middleware.spec.ts(1 hunks)packages/instrumentation/src/middleware.ts(1 hunks)packages/instrumentation/tsconfig.json(1 hunks)packages/instrumentation/tsup.config.ts(1 hunks)packages/logger/package.json(1 hunks)packages/logger/src/index.ts(2 hunks)scripts/build-images.sh(1 hunks)tsconfig.json(1 hunks)tsup.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/ui/next.config.tsapps/gateway/src/middleware/tracing.tsapps/gateway/src/lib/http-client.tsapps/gateway/src/chat/chat.tsapps/api/src/stripe.tsapps/gateway/src/index.tspackages/instrumentation/src/middleware.spec.tspackages/instrumentation/tsup.config.tspackages/auth/src/auth.tspackages/instrumentation/src/index.tstsup.config.tsapps/api/src/middleware/tracing.tsapps/api/src/lib/http-client.tsapps/api/src/index.tsapps/gateway/src/serve.tspackages/logger/src/index.tspackages/instrumentation/src/middleware.tspackages/instrumentation/src/index.spec.tsapps/api/src/serve.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/ui/next.config.tsapps/gateway/src/middleware/tracing.tsapps/gateway/src/lib/http-client.tsapps/gateway/src/chat/chat.tsapps/api/src/stripe.tsapps/gateway/src/index.tspackages/instrumentation/src/middleware.spec.tspackages/instrumentation/tsup.config.tspackages/auth/src/auth.tspackages/instrumentation/src/index.tstsup.config.tsapps/api/src/middleware/tracing.tsapps/api/src/lib/http-client.tsapps/api/src/index.tsapps/gateway/src/serve.tspackages/logger/src/index.tspackages/instrumentation/src/middleware.tspackages/instrumentation/src/index.spec.tsapps/api/src/serve.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/next.config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
apps/ui/next.config.tsapps/gateway/src/middleware/tracing.tsapps/gateway/src/lib/http-client.tsapps/gateway/src/chat/chat.tsapps/api/src/stripe.tsapps/gateway/src/index.tspackages/instrumentation/src/middleware.spec.tspackages/instrumentation/tsup.config.tspackages/auth/src/auth.tspackages/instrumentation/src/index.tstsup.config.tsapps/api/src/middleware/tracing.tsapps/api/src/lib/http-client.tsapps/api/src/index.tsapps/gateway/src/serve.tspackages/logger/src/index.tspackages/instrumentation/src/middleware.tspackages/instrumentation/src/index.spec.tsapps/api/src/serve.ts
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/next.config.ts
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/next.config.ts
apps/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/ui/**/*.{ts,tsx}: Use Next.js App Router (React Server Components) for the UI
Use next/link for links and next/navigation's router for programmatic navigation
Use TanStack Query for client/state data fetching and caching in the UI
Use Radix UI components with Tailwind CSS for UI building and styling
Prefer localStorage over cookies for client-side data persistence
Files:
apps/ui/next.config.ts
**/tsconfig*.json
📄 CodeRabbit inference engine (CLAUDE.md)
Enable and keep TypeScript strict mode in all tsconfig files
Files:
packages/instrumentation/tsconfig.jsontsconfig.json
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
.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()
apps/{api,gateway}/**/*.{ts,tsx}: Use the Hono framework for backend HTTP services in apps/api and apps/gateway
Use Zod for request/response validation in backend routes and handlers
Maintain OpenAPI/Swagger documentation for backend APIsFiles:
apps/gateway/src/middleware/tracing.tsapps/gateway/src/lib/http-client.tsapps/gateway/src/chat/chat.tsapps/api/src/stripe.tsapps/gateway/src/index.tsapps/api/src/middleware/tracing.tsapps/api/src/lib/http-client.tsapps/api/src/index.tsapps/gateway/src/serve.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/middleware/tracing.tsapps/gateway/src/lib/http-client.tsapps/gateway/src/chat/chat.tsapps/gateway/src/index.tsapps/gateway/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/stripe.tsapps/api/src/middleware/tracing.tsapps/api/src/lib/http-client.tsapps/api/src/index.tsapps/api/src/serve.ts**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Name unit tests with the .spec.ts suffix (run by pnpm test:unit)
Files:
packages/instrumentation/src/middleware.spec.tspackages/instrumentation/src/index.spec.ts**/*.{spec,e2e}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vitest as the test framework for unit and E2E tests
Files:
packages/instrumentation/src/middleware.spec.tspackages/instrumentation/src/index.spec.ts🧠 Learnings (17)
📚 Learning: 2025-09-02T00:39:46.758Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-02T00:39:46.758Z Learning: Applies to **/tsconfig*.json : Enable and keep TypeScript strict mode in all tsconfig filesApplied to files:
packages/instrumentation/tsconfig.jsontsconfig.json📚 Learning: 2025-08-29T15:31:07.077Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.077Z 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/index.tsapps/api/src/index.tsapps/gateway/package.json📚 Learning: 2025-09-02T00:39:46.758Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-02T00:39:46.758Z Learning: Applies to apps/{api,gateway}/**/*.{ts,tsx} : Use the Hono framework for backend HTTP services in apps/api and apps/gatewayApplied to files:
apps/gateway/src/index.tsapps/api/src/index.ts📚 Learning: 2025-08-29T15:31:07.077Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.077Z 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/index.tsapps/api/src/index.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: For smoke testing apps, build with 'pnpm build', run 'pnpm --filter=[app] --prod deploy dist/[app]' for each package, then run start.sh to verify 'pnpm start' worksApplied to files:
apps/api/package.jsonapps/gateway/package.json📚 Learning: 2025-08-29T15:31:07.077Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.077Z Learning: For smoke testing apps: build with pnpm build, deploy each package with pnpm --filter=[app] --prod deploy dist/[app], then run start.sh to verify pnpm startApplied to files:
apps/api/package.json📚 Learning: 2025-09-02T00:39:46.758Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-02T00:39:46.758Z Learning: Applies to apps/{api,gateway}/**/*.{ts,tsx} : Use Zod for request/response validation in backend routes and handlersApplied to files:
apps/api/src/index.ts📚 Learning: 2025-09-02T00:39:46.758Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-02T00:39:46.758Z Learning: Applies to apps/{api,gateway}/**/*.{ts,tsx} : Maintain OpenAPI/Swagger documentation for backend APIsApplied to files:
apps/gateway/package.json📚 Learning: 2025-08-03T13:11:33.041Z
Learnt from: CR PR: theopenco/llmgateway#0 File: .cursor/rules/general.mdc:0-0 Timestamp: 2025-08-03T13:11:33.041Z Learning: Before committing, run `pnpm format`.Applied to files:
CLAUDE.md📚 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: After adding features or editing files, make sure to format the code and ensure no linting errors using `pnpm format`Applied to files:
CLAUDE.md📚 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: Always use pnpm for installing or managing dependencies, and running scriptsApplied to files:
CLAUDE.md📚 Learning: 2025-08-29T15:31:07.077Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.077Z Learning: Always use pnpm for installing/managing dependencies and running scriptsApplied to files:
CLAUDE.md📚 Learning: 2025-07-23T19:34:11.203Z
Learnt from: CR PR: theopenco/llmgateway#0 File: .cursor/rules/package-manager.mdc:0-0 Timestamp: 2025-07-23T19:34:11.203Z Learning: Always use pnpm commands when working with dependencies or running scripts in any part of the monorepo. Do not use npm or yarn.Applied to files:
CLAUDE.md📚 Learning: 2025-08-03T13:11:33.041Z
Learnt from: CR PR: theopenco/llmgateway#0 File: .cursor/rules/general.mdc:0-0 Timestamp: 2025-08-03T13:11:33.041Z Learning: Always run `pnpm build` to ensure there are no errors before committing.Applied to files:
CLAUDE.md📚 Learning: 2025-09-02T00:39:46.758Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-02T00:39:46.758Z Learning: Use conventional commits and limit commit/PR titles to a maximum of 50 charactersApplied to files:
CLAUDE.md📚 Learning: 2025-08-03T13:11:33.041Z
Learnt from: CR PR: theopenco/llmgateway#0 File: .cursor/rules/general.mdc:0-0 Timestamp: 2025-08-03T13:11:33.041Z Learning: When writing commit messages, always use the conventional commit messages format.Applied to files:
CLAUDE.md📚 Learning: 2025-08-29T15:31:07.077Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.077Z Learning: After adding or editing files, format and ensure no linting errors using pnpm formatApplied to files:
CLAUDE.md🧬 Code graph analysis (16)
apps/ui/next.config.ts (1)
apps/docs/next.config.mjs (1)
config(6-18)apps/gateway/src/middleware/tracing.ts (2)
apps/api/src/middleware/tracing.ts (1)
tracingMiddleware(3-5)packages/instrumentation/src/middleware.ts (1)
createTracingMiddleware(15-120)apps/gateway/src/lib/http-client.ts (1)
apps/api/src/lib/http-client.ts (2)
HttpClientOptions(3-8)httpClient(10-74)apps/gateway/src/chat/chat.ts (1)
packages/logger/src/index.ts (1)
logger(172-172)apps/api/src/stripe.ts (1)
packages/logger/src/index.ts (1)
logger(172-172)apps/gateway/src/index.ts (1)
apps/gateway/src/middleware/tracing.ts (1)
tracingMiddleware(3-5)packages/instrumentation/src/middleware.spec.ts (1)
packages/instrumentation/src/middleware.ts (1)
createTracingMiddleware(15-120)packages/instrumentation/src/index.ts (1)
packages/logger/src/index.ts (3)
logger(172-172)createLogger(175-177)error(144-151)apps/api/src/middleware/tracing.ts (2)
apps/gateway/src/middleware/tracing.ts (1)
tracingMiddleware(3-5)packages/instrumentation/src/middleware.ts (1)
createTracingMiddleware(15-120)apps/api/src/lib/http-client.ts (1)
apps/gateway/src/lib/http-client.ts (2)
HttpClientOptions(3-8)httpClient(10-74)apps/api/src/index.ts (2)
apps/api/src/middleware/tracing.ts (1)
tracingMiddleware(3-5)apps/gateway/src/middleware/tracing.ts (1)
tracingMiddleware(3-5)apps/gateway/src/serve.ts (1)
packages/instrumentation/src/index.ts (1)
initializeInstrumentation(113-193)packages/logger/src/index.ts (1)
packages/instrumentation/src/index.ts (1)
trace(196-196)packages/instrumentation/src/middleware.ts (1)
packages/logger/src/index.ts (3)
trace(124-127)error(144-151)logger(172-172)packages/instrumentation/src/index.spec.ts (1)
packages/instrumentation/src/index.ts (1)
initializeInstrumentation(113-193)apps/api/src/serve.ts (1)
packages/instrumentation/src/index.ts (1)
initializeInstrumentation(113-193)🪛 GitHub Actions: e2e
packages/instrumentation/src/index.ts
[error] 4-4: Vite import-analysis failed: Failed to resolve entry for package '@llmgateway/logger'. The package may have incorrect main/module/exports specified in its package.json. (during 'pnpm test:e2e')
🪛 Shellcheck (0.10.0)
scripts/build-images.sh
[warning] 202-202: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 212-212: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 GitHub Check: lint / run
packages/instrumentation/src/index.spec.ts
[warning] 36-36:
Missing accessibility modifier on method definition forceFlush
[warning] 17-17:
Missing accessibility modifier on method definition shutdown
[warning] 14-14:
Missing accessibility modifier on method definition start⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build / run
- GitHub Check: test-build-split (docs)
- GitHub Check: test-build-split (gateway)
- GitHub Check: test-build-split (ui)
- GitHub Check: test-build-split (api)
🔇 Additional comments (14)
.claude/settings.json (2)
3-16: Valid use of Stop hook; dropping matcher is correct."Stop" is a supported event and does not use matchers; running simple command hooks here is valid. ✔️ (docs.anthropic.com)
7-13: Verified — 'pnpm format' and 'pnpm build' existRoot package.json defines both "format" and "build"; "build" is present in all workspace package.json files scanned (apps/* and packages/*). Hooks run from the repo root will succeed.
packages/instrumentation/tsconfig.json (1)
1-9: LGTMExtends root config; outputs to dist; aligns with strict settings via the root.
packages/logger/package.json (1)
21-21: Verify single @opentelemetry/api version across workspaceSearch in this environment failed (rg returned "No files were searched" and jq errored), so I couldn't confirm other packages; packages/logger/package.json contains "@opentelemetry/api": "1.9.0". Run these locally and align versions if mismatched:
# quick search for occurrences rg -n --hidden --no-ignore -g '!**/node_modules/**' '@opentelemetry/api' -S # precise package.json scan (requires python3) python3 - <<'PY' import json,glob for p in glob.glob('**/package.json', recursive=True): try: j=json.load(open(p)) except: continue for k in ('dependencies','devDependencies','peerDependencies'): v=j.get(k,{}).get('@opentelemetry/api') if v: print(f"{p}: {v}") PYAlign all package.json entries to the same @opentelemetry/api version (or adopt a peerDependency strategy) if any mismatches are found.
tsup.config.ts (1)
11-11: Glob consolidation LGTMThe negative extglob pattern correctly excludes both .spec and .e2e entries. Nothing else to change here.
infra/split.dockerfile (1)
75-76: Include instrumentation workspace for better install-layer caching.Good addition; ensures pnpm can resolve the local package during the frozen install and maximizes cache hits.
apps/gateway/src/middleware/tracing.ts (1)
3-5: LGTM — consistent service name and middleware wiring.Matches the API counterpart and instrumentation package API.
apps/gateway/package.json (2)
15-15: Source maps enabled on start: goodThis helps stack traces in prod.
23-27: Align OpenTelemetry versions across the monorepoapps/api, apps/gateway, packages/instrumentation declare @opentelemetry/api@1.9.0 and @opentelemetry/sdk-node@0.205.0; packages/logger has @opentelemetry/api@1.9.0 but omits @opentelemetry/sdk-node. Add "@opentelemetry/sdk-node": "0.205.0" to packages/logger/package.json or confirm the omission is intentional and that workspace hoisting/dedupes a compatible sdk-node version.
apps/api/package.json (2)
14-14: Source maps enabled on start: goodConsistent with gateway.
23-26: Align @opentelemetry versions across workspaceapps/api, apps/gateway, packages/instrumentation declare @opentelemetry/api@1.9.0 and @opentelemetry/sdk-node@0.205.0; packages/logger declares @opentelemetry/api@1.9.0 but is missing @opentelemetry/sdk-node — add @opentelemetry/sdk-node@0.205.0 to packages/logger/package.json or confirm omission.
packages/instrumentation/src/middleware.ts (2)
30-37: Header normalization/extraction looks good.Lowercasing headers and supporting
x-cloud-trace-contextimproves interop with the configured propagators.
87-96: Cloud Trace header formatting is correct.Converting hex spanId to decimal and setting
osampling flag matches GCP’s expected format.packages/instrumentation/src/index.ts (1)
137-149: Useresource(service.name) and the correctspanProcessoroption.
- NodeSDK expects
resourcewithservice.nameinstead ofserviceNameoption.- The option is
spanProcessor(singular), notspanProcessors.This impacts whether spans carry the service name and whether your custom processor is used.
+import { Resource } from "@opentelemetry/resources"; +import { SemanticResourceAttributes } from "@opentelemetry/semantic-conventions"; @@ - const sdk = new NodeSDK({ - spanProcessors: [spanProcessor], - sampler, - serviceName: config.serviceName, - textMapPropagator: propagator, - instrumentations: [ + const resource = new Resource({ + [SemanticResourceAttributes.SERVICE_NAME]: config.serviceName, + }); + + const sdk = new NodeSDK({ + spanProcessor: spanProcessor, + sampler, + resource, + textMapPropagator: propagator, + instrumentations: [ getNodeAutoInstrumentations({ "@opentelemetry/instrumentation-fs": { enabled: false, }, }), ], });Likely an incorrect or invalid review comment.
|
|
||
| import type { Sampler, SamplingResult } from "@opentelemetry/sdk-trace-base"; | ||
|
|
||
| const logger = createLogger({ name: "instrumentation" }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Logger import ok, but CI shows resolver failure for @llmgateway/logger.
The e2e pipeline can’t resolve the package entry. Validate the logger package “exports”/“main” and that this package consumes the right subpath.
Run to inspect the logger package config and call-sites:
🏁 Script executed:
#!/bin/bash
fd -a package.json packages/logger | xargs -I{} sh -c 'echo "== {} =="; jq -r ".name,.type,.main,.module,.exports" < {}'
rg -n "@llmgateway/logger" -g '!**/dist/**' -C2Length of output: 379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/logger/package.json =="
jq -r '.name, .type, .main, .module, .exports' packages/logger/package.json || true
echo
echo "== Search repo for '@llmgateway/logger' (no ignores) =="
rg -n --hidden --no-ignore --no-ignore-vcs "@llmgateway/logger" -S -C2 || true
echo
echo "== Search for createLogger symbol =="
rg -n --hidden --no-ignore --no-ignore-vcs "createLogger" -S -C2 || true
echo
echo "== packages/instrumentation/package.json =="
if [ -f packages/instrumentation/package.json ]; then
jq -r '.name, .type, .main, .module, .exports' packages/instrumentation/package.json || true
else
echo "no package.json"
fi
echo
echo "== packages/instrumentation/src/index.ts (if exists) =="
if [ -f packages/instrumentation/src/index.ts ]; then
sed -n '1,240p' packages/instrumentation/src/index.ts || true
else
echo "not found"
fi
echo
echo "== list src dir =="
ls -la packages/instrumentation/src || trueLength of output: 163784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/logger/package.json =="
cat packages/logger/package.json || true
echo
echo "== ls packages/logger/dist =="
ls -la packages/logger/dist || true
echo
echo "== show start of dist/index.js =="
if [ -f packages/logger/dist/index.js ]; then
sed -n '1,200p' packages/logger/dist/index.js || true
else
echo "packages/logger/dist/index.js not found"
fi
echo
echo "== show start of dist/index.cjs =="
if [ -f packages/logger/dist/index.cjs ]; then
sed -n '1,200p' packages/logger/dist/index.cjs || true
else
echo "packages/logger/dist/index.cjs not found"
fi
echo
echo "== grep for 'createLogger' in dist =="
rg -n "createLogger" packages/logger/dist -S || trueLength of output: 1005
CI resolver failure: @llmgateway/logger exports point to missing ./dist
packages/logger/package.json exports (import: ./dist/index.js, require: ./dist/index.cjs, types: ./dist/index.d.ts) but packages/logger/dist/ is absent; consumers (e.g. packages/instrumentation/src/index.ts) import createLogger from "@llmgateway/logger".
- Action: either produce/commit the built artifacts for packages/logger (run the package build and ensure CI builds workspace packages before e2e) or change package.json exports/main to a runtime-resolvable entry (and update CI to build or handle TS source).
- Files to update/check: packages/logger/package.json and CI workflow that runs the e2e pipeline.
🤖 Prompt for AI Agents
In packages/instrumentation/src/index.ts at line 21 the import createLogger from
"@llmgateway/logger" fails in CI because packages/logger/package.json exports
point to ./dist/* which is not present; fix by either (A) building and
committing the logger package artifacts (run the logger package build, ensure
packages/logger/dist/ contains index.js/.cjs/.d.ts and update the monorepo CI to
run the workspace build step before e2e), or (B) change
packages/logger/package.json exports/main to a runtime-resolvable source (e.g.,
point to src/index.ts or a compiled JS entry under your build pipeline) and then
update CI to either build the package during pipeline or allow TypeScript-aware
resolution; update the CI workflow to build workspace packages in order (or add
a pre-e2e build job) and ensure package.json and CI are consistent so consumers
like packages/instrumentation can resolve @llmgateway/logger at runtime.
| const attributes: Record<string, string> = { | ||
| "http.method": method, | ||
| "http.url": c.req.url, | ||
| "http.route": path, | ||
| "http.user_agent": c.req.header("user-agent") || "", | ||
| "http.remote_addr": getClientIp(c), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate any and use proper types; align attribute names with OTel semconv.
- Replace
: anywith Hono’sContext. - Type attributes as
SpanAttributes. - Prefer
http.client_ipoverhttp.remote_addr.
@@
-import {
- trace,
- SpanKind,
- SpanStatusCode,
- context,
- propagation,
-} from "@opentelemetry/api";
+import {
+ trace,
+ SpanKind,
+ SpanStatusCode,
+ context,
+ propagation,
+ type SpanAttributes,
+} from "@opentelemetry/api";
+import type { Context } from "hono";
@@
- const attributes: Record<string, string> = {
+ const attributes: SpanAttributes = {
"http.method": method,
"http.url": c.req.url,
"http.route": path,
"http.user_agent": c.req.header("user-agent") || "",
- "http.remote_addr": getClientIp(c),
+ "http.client_ip": getClientIp(c),
};
@@
-function getClientIp(c: any): string {
+function getClientIp(c: Context): string {Also applies to: 122-130
🤖 Prompt for AI Agents
In packages/instrumentation/src/middleware.ts around lines 41 to 47 (and
likewise update lines 122 to 130), the Context was typed as any and attributes
typed as Record<string,string> with a non-OTel key; change the function
parameter type to Hono's Context, import and use SpanAttributes for the
attributes variable type, and rename the http.remote_addr key to the OTel
semantic convention http.client_ip; ensure values conform to SpanAttributes
value types (string | number | boolean | string[]) and update any other
attribute declarations in the referenced block at lines 122-130 similarly.
| const status = c.res.status; | ||
| span.setAttributes({ | ||
| "http.status_code": status, | ||
| "http.response.size": c.res.headers.get("content-length") || "", | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Record numeric response size; avoid string attribute for http.response.size.
Parse content-length and only set the attribute if numeric.
- span.setAttributes({
- "http.status_code": status,
- "http.response.size": c.res.headers.get("content-length") || "",
- });
+ span.setAttributes({ "http.status_code": status });
+ const cl = c.res.headers.get("content-length");
+ const size = cl ? Number(cl) : undefined;
+ if (Number.isFinite(size)) {
+ span.setAttribute("http.response.size", size as number);
+ }📝 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 status = c.res.status; | |
| span.setAttributes({ | |
| "http.status_code": status, | |
| "http.response.size": c.res.headers.get("content-length") || "", | |
| }); | |
| const status = c.res.status; | |
| span.setAttributes({ "http.status_code": status }); | |
| const cl = c.res.headers.get("content-length"); | |
| const size = cl ? Number(cl) : undefined; | |
| if (Number.isFinite(size)) { | |
| span.setAttribute("http.response.size", size as number); | |
| } |
🤖 Prompt for AI Agents
In packages/instrumentation/src/middleware.ts around lines 71 to 76, the
middleware sets "http.response.size" as a string from headers; change this to
parse the content-length header into a number and only set the attribute when
the parsed value is a valid numeric value (e.g., parseInt/Number and check
isFinite/!isNaN), keeping "http.status_code" as the numeric status; update
span.setAttributes to include "http.response.size" only when the parsed numeric
size is valid.
| --repo REPO Repository base name (default: steebchen/llmgateway/llmgateway) | ||
| --platforms PLATFORMS Platforms to build for (default: linux/amd64,linux/arm64) | ||
| --split-only Build only split images | ||
| --unified-only Build only unified images | ||
| --tag TAG Custom tag (default: v0.0.0-{short-sha}) | ||
| --help Show this help message |
There was a problem hiding this comment.
Fix help text defaults to match actual defaults.
--repo default in help doesn’t match REPOSITORY_BASE="theopenco/llmgateway", and the tag default differs.
- --repo REPO Repository base name (default: steebchen/llmgateway/llmgateway)
+ --repo REPO Repository base name (default: theopenco/llmgateway)
@@
- --tag TAG Custom tag (default: v0.0.0-{short-sha})
+ --tag TAG Custom tag (default: v0.0.0-{short-sha})📝 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.
| --repo REPO Repository base name (default: steebchen/llmgateway/llmgateway) | |
| --platforms PLATFORMS Platforms to build for (default: linux/amd64,linux/arm64) | |
| --split-only Build only split images | |
| --unified-only Build only unified images | |
| --tag TAG Custom tag (default: v0.0.0-{short-sha}) | |
| --help Show this help message | |
| --repo REPO Repository base name (default: theopenco/llmgateway) | |
| --platforms PLATFORMS Platforms to build for (default: linux/amd64,linux/arm64) | |
| --split-only Build only split images | |
| --unified-only Build only unified images | |
| --tag TAG Custom tag (default: v0.0.0-{short-sha}) | |
| --help Show this help message |
🤖 Prompt for AI Agents
In scripts/build-images.sh around lines 45-50, the printed help defaults are
incorrect: update the --repo default to the actual REPOSITORY_BASE value
("theopenco/llmgateway") and update the --tag default text to exactly match the
tag default pattern used by the script (use the same variable/format the script
sets for the tag), so the help output reflects the real defaults.
Revised `tsconfig.json` to add path alias `@llmgateway/*` and adjusted include rules. Simplified `tsup.config.ts` by importing shared configuration from the root.
Removed unnecessary `@types/node` declarations from individual package.json files. Centralized the dependency in the root package.json to streamline maintenance and avoid duplication. Updated `pnpm-lock.yaml` accordingly.
Added error handling for tracing initialization to allow service startup without instrumentation if it fails. Introduced graceful shutdown of OpenTelemetry SDK to ensure all spans are flushed during shutdown. Updated HTTP clients to use correct span and context injection for improved trace propagation.
Changed the default repository in `build-images.sh` from `steebchen/llmgateway/llmgateway` to `theopenco/llmgateway`. Also updated `Context` type usage in middleware to improve typing accuracy. Enhanced client IP detection logic in middleware for better coverage.
Replaced `Record<string, string>` with `Attributes` type for span attributes in middleware. Ensures compliance with OpenTelemetry standards and improves type accuracy.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/db/package.json (1)
25-27: Broken scripts: tsx removed but still referenced
resetandseedstill calltsx, but this package no longer depends on it. Runningpnpm --filter db reset/seedwill likely fail under pnpm’s isolated bins.Two fixes; pick one:
- Re-add
tsxhere, or- Use the workspace root tool via
pnpm -w exec."scripts": { "build": "tsc && tsup", "generate": "pnpm drizzle-kit generate --config drizzle.config.ts", "migrate": "pnpm drizzle-kit migrate --config drizzle.config.ts", "push": "pnpm drizzle-kit push --force --config drizzle.config.ts", - "reset": "tsx reset.ts", - "seed": "tsx seed.ts" + "reset": "pnpm -w exec tsx reset.ts", + "seed": "pnpm -w exec tsx seed.ts" },Also applies to: 37-37
apps/docs/package.json (1)
13-19: Build calls tsc but TypeScript was removed from this packageSince
typescriptmoved to the root, callingtschere will fail under pnpm unless you invoke the root binary. Either re-addtypescriptlocally or usepnpm -w exec."scripts": { - "build": "fumadocs-mdx && tsc && next build && cp ../gateway/openapi.json .", + "build": "fumadocs-mdx && pnpm -w exec tsc && next build && cp ../gateway/openapi.json .", "dev": "next dev --turbo --port 3005",Also applies to: 47-48
♻️ Duplicate comments (2)
apps/gateway/src/serve.ts (1)
20-31: Gateway tracing lifecycle wired correctlySDK handle retained and shut down after services stop. Resolves earlier shutdown-handler conflict concerns.
Also applies to: 84-88
packages/instrumentation/src/index.ts (1)
3-3: CI import failure risk: ensure @llmgateway/logger resolves in CIPast CI showed resolver failure for this import due to missing built artifacts/exports in packages/logger. Please verify the logger package builds/exports are consumable before e2e.
Run to inspect exports and built files:
#!/bin/bash set -euo pipefail echo "== logger package.json exports ==" jq -r '.name,.main,.module,.types,.exports' packages/logger/package.json echo echo "== dist presence ==" ls -la packages/logger/dist || true echo echo "== instrumentation import sites ==" rg -n '@llmgateway/logger' -S -g '!**/dist/**' -C2
🧹 Nitpick comments (5)
packages/instrumentation/tsup.config.ts (1)
1-3: Terser wrapper and ESM resolutionOne-line re-export is cleaner, and adding the
.tsextension avoids ESM resolution edge cases when the config is loaded outside ts-node.-import { tsup } from "../../tsup.config"; - -export { tsup }; +export { tsup } from "../../tsup.config.ts";packages/instrumentation/src/index.ts (4)
6-10: Minor: collapse duplicated @opentelemetry/core importsCombine these into a single import for consistency.
-import { CompositePropagator } from "@opentelemetry/core"; -import { - W3CTraceContextPropagator, - W3CBaggagePropagator, -} from "@opentelemetry/core"; +import { + CompositePropagator, + W3CTraceContextPropagator, + W3CBaggagePropagator, +} from "@opentelemetry/core";
31-53: Make forced sampling header check robust (boolean/number/case-insensitive)Currently only "true"/"1" strings are honored. Accept boolean and numeric forms too to reduce surprises from upstream proxies.
- if (attributes && attributes["http.header.x-force-trace"]) { - const forceTrace = attributes["http.header.x-force-trace"]; - if (forceTrace === "true" || forceTrace === "1") { + if (attributes && attributes["http.header.x-force-trace"] !== undefined) { + const v = attributes["http.header.x-force-trace"]; + const forced = + v === true || + v === 1 || + (typeof v === "string" && ["1", "true", "yes", "on"].includes(v.toLowerCase())); + if (forced) { return { decision: SamplingDecision.RECORD_AND_SAMPLED, attributes: { ...attributes, "sampling.forced": true, }, }; } }
70-107: Honor standard OTel sampler env vars (OTEL_TRACES_SAMPLER/_ARG)Support the spec env vars in addition to OTEL_SAMPLE_RATE for better portability.
function getSamplerConfig() { - const sampleRate = process.env.OTEL_SAMPLE_RATE; + const sampleRate = process.env.OTEL_SAMPLE_RATE; + const stdSampler = process.env.OTEL_TRACES_SAMPLER; // e.g., "always_on" | "always_off" | "traceidratio" + const stdArg = process.env.OTEL_TRACES_SAMPLER_ARG; let baseSampler: Sampler; let baseDescription: string; - if (sampleRate === undefined) { + // Prefer spec-compliant vars if present + if (stdSampler) { + switch (stdSampler) { + case "always_on": + baseSampler = new AlwaysOnSampler(); + baseDescription = "100% (OTEL_TRACES_SAMPLER=always_on)"; + break; + case "always_off": + baseSampler = new TraceIdRatioBasedSampler(0); + baseDescription = "0% (OTEL_TRACES_SAMPLER=always_off)"; + break; + case "traceidratio": { + const rate = parseFloat(stdArg ?? ""); + if (isNaN(rate) || rate < 0 || rate > 1) { + logger.warn( + `Invalid OTEL_TRACES_SAMPLER_ARG "${stdArg}" for traceidratio, falling back to 100%`, + ); + baseSampler = new AlwaysOnSampler(); + baseDescription = "100% (invalid OTEL_TRACES_SAMPLER_ARG)"; + } else { + baseSampler = new TraceIdRatioBasedSampler(rate); + baseDescription = `${Math.round(rate * 100)}% (traceidratio)`; + } + break; + } + default: + logger.warn( + `Unsupported OTEL_TRACES_SAMPLER "${stdSampler}", falling back to OTEL_SAMPLE_RATE or 100%`, + ); + // fall through to sampleRate handling below + } + } + + if (!baseSampler && sampleRate === undefined) { baseSampler = new AlwaysOnSampler(); baseDescription = "100% (always on)"; - } else { + } else if (!baseSampler) { const rate = parseFloat(sampleRate); if (isNaN(rate) || rate < 0 || rate > 1) { logger.warn( `Invalid OTEL_SAMPLE_RATE value "${sampleRate}", using 100% sampling`, ); baseSampler = new AlwaysOnSampler(); baseDescription = "100% (always on, invalid rate specified)";
138-150: Prefer NodeSDK’s traceExporter option (simpler, version-stable)Replace the explicit BatchSpanProcessor with NodeSDK's traceExporter (packages/instrumentation/src/index.ts). Repository pins @opentelemetry/sdk-node@0.205.0 and NodeSDK will use the provided exporter with a BatchSpanProcessor automatically. (open-telemetry.github.io)
- spanProcessors: [spanProcessor], + traceExporter: traceExporter,- // Use BatchSpanProcessor as recommended by Google Cloud documentation - const spanProcessor = new BatchSpanProcessor(traceExporter);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/api/package.json(3 hunks)apps/api/src/lib/http-client.ts(1 hunks)apps/api/src/serve.ts(4 hunks)apps/docs/package.json(1 hunks)apps/gateway/package.json(2 hunks)apps/gateway/src/lib/http-client.ts(1 hunks)apps/gateway/src/serve.ts(5 hunks)apps/ui/package.json(0 hunks)infra/unified.dockerfile(1 hunks)package.json(2 hunks)packages/db/package.json(1 hunks)packages/instrumentation/package.json(1 hunks)packages/instrumentation/src/index.ts(1 hunks)packages/instrumentation/tsconfig.json(1 hunks)packages/instrumentation/tsup.config.ts(1 hunks)packages/logger/package.json(1 hunks)packages/logger/src/index.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/ui/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/instrumentation/tsconfig.json
- packages/logger/package.json
- infra/unified.dockerfile
- apps/gateway/package.json
- apps/api/src/lib/http-client.ts
- apps/gateway/src/lib/http-client.ts
- apps/api/package.json
- packages/instrumentation/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:
packages/instrumentation/src/index.tspackages/instrumentation/tsup.config.tsapps/api/src/serve.tspackages/logger/src/index.tsapps/gateway/src/serve.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/instrumentation/src/index.tspackages/instrumentation/tsup.config.tsapps/api/src/serve.tspackages/logger/src/index.tsapps/gateway/src/serve.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
packages/instrumentation/src/index.tspackages/instrumentation/tsup.config.tsapps/api/src/serve.tspackages/logger/src/index.tsapps/gateway/src/serve.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
.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()
apps/{api,gateway}/**/*.{ts,tsx}: Use the Hono framework for backend HTTP services in apps/api and apps/gateway
Use Zod for request/response validation in backend routes and handlers
Maintain OpenAPI/Swagger documentation for backend APIsFiles:
apps/api/src/serve.tsapps/gateway/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/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/serve.ts🧠 Learnings (2)
📚 Learning: 2025-08-29T15:31:07.077Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-08-29T15:31:07.077Z Learning: Applies to apps/{api,gateway}/**/*.{ts,tsx} : Use Drizzle with the latest object syntax for database operationsApplied to files:
packages/db/package.json📚 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/package.json🧬 Code graph analysis (3)
packages/instrumentation/src/index.ts (1)
packages/logger/src/index.ts (3)
logger(175-175)createLogger(178-180)error(147-154)apps/api/src/serve.ts (2)
packages/instrumentation/src/index.ts (2)
initializeInstrumentation(114-181)shutdownInstrumentation(183-192)packages/logger/src/index.ts (2)
error(147-154)logger(175-175)apps/gateway/src/serve.ts (3)
packages/instrumentation/src/index.ts (2)
initializeInstrumentation(114-181)shutdownInstrumentation(183-192)packages/logger/src/index.ts (2)
error(147-154)logger(175-175)apps/gateway/src/worker.ts (1)
startWorker(427-477)⏰ 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). (7)
- GitHub Check: build / run
- GitHub Check: e2e / run
- GitHub Check: test-build-unified
- GitHub Check: test-build-split (ui)
- GitHub Check: test-build-split (api)
- GitHub Check: test-build-split (docs)
- GitHub Check: test-build-split (gateway)
🔇 Additional comments (6)
package.json (1)
38-38: Align @types/node with runtime (and centralize tsx usage)
- Root package.json pins @types/node@24.3.3 while .tool-versions specifies nodejs 24.5.0 — update/align @types/node (or use ^24.x) to match the runtime used in CI/containers.
- Scripts invoke tsx directly; switch to workspace execution (e.g.
pnpm -w exec tsx ...) so the workspace-installed tsx is used. Affected scripts: apps/api (scripts: dev, generate), apps/gateway (dev, generate), packages/db (reset, seed).- CI: .github/workflows/run.yml and .github/workflows/claude.yml use
node-version: ${{ env.nodejs_version }}— ensureenv.nodejs_versionresolves to the same 24.x value (or set an explicit node-version) to keep types/runtime aligned.packages/logger/src/index.ts (1)
31-41: LGTM: log level handling now honors options and sensible defaultsConstructor now respects
options.levelwith a fallback togetDefaultLevel(). Good fix.apps/api/src/serve.ts (1)
19-29: Clean instrumentation lifecycle with graceful shutdownInitialization is guarded; shutdown flushes spans last—good ordering.
Also applies to: 87-91
packages/instrumentation/src/index.ts (3)
152-179: Good fix: await sdk.start() with error handlingSwitching to await ensures failures are caught and logged; the troubleshooting guidance is helpful.
183-192: Graceful shutdown looks goodAwaiting sdk.shutdown() with logging and rethrow is correct.
197-201: Keep the .js re-export — repo is ESM with bundler resolutionRoot tsconfig.json sets module="esnext" and moduleResolution="bundler" (verbatimModuleSyntax true); packages/instrumentation extends it and package.json declares "type":"module" with ESM exports, so keep "./middleware.js" as-is.
Replaced usage of `isSpanContextValid` and updated `traceFlags` logic to reference `TraceFlags.SAMPLED`. Ensures compliance with OpenTelemetry API updates and improves trace context handling.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/build-images.sh (1)
36-51: Align help text for --tag with actual behavior.Use a neutral placeholder to match the computed default precisely.
- --tag TAG Custom tag (default: v0.0.0-{short-sha}) + --tag TAG Custom tag (default: v0.0.0-<short-sha>)
🧹 Nitpick comments (8)
scripts/build-images.sh (3)
114-121: Provide a fallback tag if git is unavailable or executed outside a repo.Prevents hard failure on CI or local environments without .git. Uses a timestamped fallback.
if [[ -n "$CUSTOM_TAG" ]]; then IMAGE_TAG="$CUSTOM_TAG" else - SHORT_SHA=$(git rev-parse --short HEAD) - IMAGE_TAG="v0.0.0-${SHORT_SHA}" + if ! SHORT_SHA="$(git rev-parse --short HEAD 2>/dev/null)"; then + SHORT_SHA="nogit-$(date +%Y%m%d%H%M%S)" + fi + IMAGE_TAG="v0.0.0-${SHORT_SHA}" fi
165-179: Avoid positional coupling in build_args when overriding platform for local --load builds.Replacing
build_args[3]assumes a fixed index, which is brittle if args shift. Rebuild the array for the local case and preserve--targetwhen set.- # Replace the platforms string with current platform only - platforms_str="$current_platform" - build_args[3]="$current_platform" - build_args+=("--load") + # Rebuild args for local single-arch load (preserve target if present) + build_args=( + "buildx" "build" + "--platform" "$current_platform" + "--file" "$dockerfile" + ) + if [[ -n "$target" ]]; then + build_args+=("--target" "$target") + fi + build_args+=( + "--tag" "${image_base}:${IMAGE_TAG}" + "--tag" "${image_base}:latest" + "--build-arg" "APP_VERSION=${IMAGE_TAG}" + "--load" + )Also applies to: 146-160, 152-155
186-187: Add cleanup trap for temporary result files.Prevents stale /tmp/build_result_* files from affecting subsequent runs.
} +trap 'rm -f /tmp/build_result_* /tmp/build_result_unified 2>/dev/null || true' EXIT + # Build all apps pnpm buildpackages/logger/src/index.ts (5)
6-10: Type the trace flags precisely and return a typed context.Use the concrete TraceFlags (number) instead of string, and type getTraceContext to the exported TraceContext plus arbitrary fields.
Apply:
export interface TraceContext { traceId?: string; spanId?: string; - traceFlags?: string; + traceFlags?: TraceFlags; } @@ - private getTraceContext(): object { + private getTraceContext(): TraceContext & Record<string, unknown> { @@ - traceFlags: spanContext.traceFlags.toString(), + traceFlags: spanContext.traceFlags,Also applies to: 96-96, 120-123
107-109: Add env var fallbacks for GCP project ID.Some runtimes expose GCLOUD_PROJECT or GCP_PROJECT instead of GOOGLE_CLOUD_PROJECT.
Apply:
- const projectId = process.env.GOOGLE_CLOUD_PROJECT; + const projectId = + process.env.GOOGLE_CLOUD_PROJECT || + process.env.GCLOUD_PROJECT || + process.env.GCP_PROJECT;
126-145: Tighten param types and reduce repetition.
- Prefer Record<string, unknown> over object in public API.
- Optionally DRY the trace-context merge via a small helper.
Apply:
- public trace(message: string, extra?: object): void { + public trace(message: string, extra?: Record<string, unknown>): void { const traceContext = this.getTraceContext(); this.logger.trace({ ...traceContext, ...extra }, message); } @@ - public debug(message: string, extra?: object): void { + public debug(message: string, extra?: Record<string, unknown>): void { const traceContext = this.getTraceContext(); this.logger.debug({ ...traceContext, ...extra }, message); } @@ - public info(message: string, extra?: object): void { + public info(message: string, extra?: Record<string, unknown>): void { const traceContext = this.getTraceContext(); this.logger.info({ ...traceContext, ...extra }, message); } @@ - public warn(message: string, extra?: object): void { + public warn(message: string, extra?: Record<string, unknown>): void { const traceContext = this.getTraceContext(); this.logger.warn({ ...traceContext, ...extra }, message); }Optionally add:
private mergeTrace(extra?: Record<string, unknown>) { return { ...this.getTraceContext(), ...extra }; }…then call this.logger.(this.mergeTrace(extra), message).
147-163: Error payload typing and usage.Use Record<string, unknown> for non-Error payloads to avoid spreading unknowns; current behavior is fine otherwise.
Apply:
- public error(message: string, error?: Error | object): void { + public error(message: string, error?: Error | Record<string, unknown>): void { const traceContext = this.getTraceContext(); if (error instanceof Error) { this.logger.error({ ...traceContext, err: error }, message); } else { this.logger.error({ ...traceContext, ...error }, message); } } @@ - public fatal(message: string, error?: Error | object): void { + public fatal(message: string, error?: Error | Record<string, unknown>): void { const traceContext = this.getTraceContext(); if (error instanceof Error) { this.logger.fatal({ ...traceContext, err: error }, message); } else { this.logger.fatal({ ...traceContext, ...error }, message); } }
165-171: Improve child() type-safety.Object.create returns any; cast to the class type to avoid leaking any and keep private member access safe.
Apply:
public child(bindings: object): LLMGatewayLogger { const childPino = this.logger.child(bindings); - const childLogger = Object.create(LLMGatewayLogger.prototype); + const childLogger = Object.create( + LLMGatewayLogger.prototype, + ) as LLMGatewayLogger; childLogger.logger = childPino; return childLogger; }Optionally also type the param:
- public child(bindings: object): LLMGatewayLogger { + public child(bindings: Record<string, unknown>): LLMGatewayLogger {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/instrumentation/src/middleware.ts(1 hunks)packages/logger/src/index.ts(3 hunks)scripts/build-images.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/instrumentation/src/middleware.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
packages/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/src/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
packages/logger/src/index.ts
🪛 Shellcheck (0.10.0)
scripts/build-images.sh
[warning] 202-202: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 212-212: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (7)
- GitHub Check: e2e / run
- GitHub Check: build / run
- GitHub Check: test-build-split (gateway)
- GitHub Check: test-build-split (api)
- GitHub Check: test-build-split (docs)
- GitHub Check: test-build-unified
- GitHub Check: test-build-split (ui)
🔇 Additional comments (3)
packages/logger/src/index.ts (3)
1-1: LGTM: Correct OTel API usage and flags.Importing isSpanContextValid and TraceFlags fixes the earlier issue.
18-26: LGTM: Cloud Logging severity mapping is appropriate.
38-66: Keep base: undefined; add messageKey: "message" to surface logs in Google Cloud Logging.Pino v8 omits pid/hostname with base: undefined (not null). Add messageKey: "message" inside the pino config so GCL's Message column displays the log text.
File: packages/logger/src/index.ts (lines 38–66)
Likely an incorrect or invalid review comment.
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| --push) | ||
| PUSH=true | ||
| shift | ||
| ;; | ||
| --registry) | ||
| REGISTRY="$2" | ||
| shift 2 | ||
| ;; | ||
| --repo) | ||
| REPOSITORY_BASE="$2" | ||
| shift 2 | ||
| ;; | ||
| --platforms) | ||
| IFS=',' read -ra PLATFORMS <<< "$2" | ||
| shift 2 | ||
| ;; | ||
| --split-only) | ||
| SPLIT_ONLY=true | ||
| shift | ||
| ;; | ||
| --unified-only) | ||
| UNIFIED_ONLY=true | ||
| shift | ||
| ;; | ||
| --tag) | ||
| CUSTOM_TAG="$2" | ||
| shift 2 | ||
| ;; | ||
| --help) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add value guards for options that require arguments (avoids set -u “unbound variable” on $2).
If a user passes e.g. --tag without a value, the script exits with an unhelpful unbound-variable error. Guard before accessing ${2} using ${2-}.
--registry)
- REGISTRY="$2"
+ if [[ $# -lt 2 || ${2-} == --* ]]; then
+ print_error "--registry requires a value"
+ exit 1
+ fi
+ REGISTRY="$2"
shift 2
;;
--repo)
- REPOSITORY_BASE="$2"
+ if [[ $# -lt 2 || ${2-} == --* ]]; then
+ print_error "--repo requires a value"
+ exit 1
+ fi
+ REPOSITORY_BASE="$2"
shift 2
;;
--platforms)
- IFS=',' read -ra PLATFORMS <<< "$2"
+ if [[ $# < 2 || -z ${2-} ]]; then
+ print_error "--platforms requires a comma-separated value"
+ exit 1
+ fi
+ IFS=',' read -ra PLATFORMS <<< "$2"
shift 2
;;
--tag)
- CUSTOM_TAG="$2"
+ if [[ $# -lt 2 || ${2-} == --* ]]; then
+ print_error "--tag requires a value"
+ exit 1
+ fi
+ CUSTOM_TAG="$2"
shift 2
;;📝 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.
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| --push) | |
| PUSH=true | |
| shift | |
| ;; | |
| --registry) | |
| REGISTRY="$2" | |
| shift 2 | |
| ;; | |
| --repo) | |
| REPOSITORY_BASE="$2" | |
| shift 2 | |
| ;; | |
| --platforms) | |
| IFS=',' read -ra PLATFORMS <<< "$2" | |
| shift 2 | |
| ;; | |
| --split-only) | |
| SPLIT_ONLY=true | |
| shift | |
| ;; | |
| --unified-only) | |
| UNIFIED_ONLY=true | |
| shift | |
| ;; | |
| --tag) | |
| CUSTOM_TAG="$2" | |
| shift 2 | |
| ;; | |
| --help) | |
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| --push) | |
| PUSH=true | |
| shift | |
| ;; | |
| --registry) | |
| if [[ $# -lt 2 || ${2-} == --* ]]; then | |
| print_error "--registry requires a value" | |
| exit 1 | |
| fi | |
| REGISTRY="$2" | |
| shift 2 | |
| ;; | |
| --repo) | |
| if [[ $# -lt 2 || ${2-} == --* ]]; then | |
| print_error "--repo requires a value" | |
| exit 1 | |
| fi | |
| REPOSITORY_BASE="$2" | |
| shift 2 | |
| ;; | |
| --platforms) | |
| if [[ $# -lt 2 || -z ${2-} ]]; then | |
| print_error "--platforms requires a comma-separated value" | |
| exit 1 | |
| fi | |
| IFS=',' read -ra PLATFORMS <<< "$2" | |
| shift 2 | |
| ;; | |
| --split-only) | |
| SPLIT_ONLY=true | |
| shift | |
| ;; | |
| --unified-only) | |
| UNIFIED_ONLY=true | |
| shift | |
| ;; | |
| --tag) | |
| if [[ $# -lt 2 || ${2-} == --* ]]; then | |
| print_error "--tag requires a value" | |
| exit 1 | |
| fi | |
| CUSTOM_TAG="$2" | |
| shift 2 | |
| ;; | |
| --help) |
🤖 Prompt for AI Agents
In scripts/build-images.sh around lines 66 to 96, option branches that access $2
(e.g. --registry, --repo, --platforms, --tag) can trigger "unbound variable"
when invoked without an argument; update each branch to use the safe parameter
expansion ${2-} when reading the value and add a guard that checks if the
resulting value is empty, printing a clear error and exiting (or handling the
absence) instead of blindly using $2; ensure the shifts remain correct (shift 2
only when a value was provided) so the script doesn't misparse subsequent args.
| wait_for_builds() { | ||
| local pids=("$@") | ||
| local failed_builds=() | ||
|
|
||
| print_status "Waiting for ${#pids[@]} build processes to complete..." | ||
|
|
||
| # Wait for all processes | ||
| for pid in "${pids[@]}"; do | ||
| wait "$pid" | ||
| done | ||
|
|
||
| # Check results | ||
| for app in "${SPLIT_APPS[@]}"; do | ||
| if [[ -f "/tmp/build_result_$app" ]]; then | ||
| local result=$(cat "/tmp/build_result_$app") | ||
| if [[ "$result" == "FAILED:$app" ]]; then | ||
| failed_builds+=("$app") | ||
| fi | ||
| rm -f "/tmp/build_result_$app" | ||
| fi | ||
| done | ||
|
|
||
| # Check unified build result if it exists | ||
| if [[ -f "/tmp/build_result_unified" ]]; then | ||
| local result=$(cat "/tmp/build_result_unified") | ||
| if [[ "$result" == "FAILED:unified" ]]; then | ||
| failed_builds+=("unified") | ||
| fi | ||
| rm -f "/tmp/build_result_unified" | ||
| fi | ||
|
|
||
|
|
||
| # Return failure if any builds failed | ||
| if [[ ${#failed_builds[@]} -gt 0 ]]; then | ||
| print_error "The following builds failed: ${failed_builds[*]}" | ||
| return 1 | ||
| fi | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make wait_for_builds robust: honor wait exit codes, handle missing result files, and fix ShellCheck SC2155.
Currently, a background build that exits non‑zero before writing its /tmp result is silently treated as success. Also, SC2155 warns about declaring and assigning with command substitution. Capture wait statuses, treat missing result files as failures, and split declaration/assignment.
wait_for_builds() {
- local pids=("$@")
- local failed_builds=()
-
- print_status "Waiting for ${#pids[@]} build processes to complete..."
-
- # Wait for all processes
- for pid in "${pids[@]}"; do
- wait "$pid"
- done
-
- # Check results
- for app in "${SPLIT_APPS[@]}"; do
- if [[ -f "/tmp/build_result_$app" ]]; then
- local result=$(cat "/tmp/build_result_$app")
- if [[ "$result" == "FAILED:$app" ]]; then
- failed_builds+=("$app")
- fi
- rm -f "/tmp/build_result_$app"
- fi
- done
-
- # Check unified build result if it exists
- if [[ -f "/tmp/build_result_unified" ]]; then
- local result=$(cat "/tmp/build_result_unified")
- if [[ "$result" == "FAILED:unified" ]]; then
- failed_builds+=("unified")
- fi
- rm -f "/tmp/build_result_unified"
- fi
-
-
- # Return failure if any builds failed
- if [[ ${#failed_builds[@]} -gt 0 ]]; then
- print_error "The following builds failed: ${failed_builds[*]}"
- return 1
- fi
-
- return 0
+ local pids=("$@")
+ local failed_builds=()
+ local failed_pids=()
+
+ print_status "Waiting for ${#pids[@]} build processes to complete..."
+
+ # Wait for all processes and capture non-zero statuses
+ for pid in "${pids[@]}"; do
+ if ! wait "$pid"; then
+ failed_pids+=("$pid")
+ fi
+ done
+
+ # Check split build results
+ for app in "${SPLIT_APPS[@]}"; do
+ local result_file="/tmp/build_result_$app"
+ if [[ -f "$result_file" ]]; then
+ local result
+ result="$(<"$result_file")"
+ if [[ "$result" == "FAILED:$app" ]]; then
+ failed_builds+=("$app")
+ fi
+ rm -f "$result_file"
+ elif ((${#pids[@]})); then
+ # Missing result file → treat as failure
+ failed_builds+=("$app")
+ fi
+ done
+
+ # Check unified build result
+ local unified_file="/tmp/build_result_unified"
+ if [[ -f "$unified_file" ]]; then
+ local result
+ result="$(<"$unified_file")"
+ if [[ "$result" == "FAILED:unified" ]]; then
+ failed_builds+=("unified")
+ fi
+ rm -f "$unified_file"
+ elif ((${#pids[@]} == 1)); then
+ failed_builds+=("unified")
+ fi
+
+ if ((${#failed_pids[@]})) || ((${#failed_builds[@]})); then
+ print_error "The following builds failed: ${failed_builds[*]}"
+ return 1
+ fi
+ return 0
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wait_for_builds() { | |
| local pids=("$@") | |
| local failed_builds=() | |
| print_status "Waiting for ${#pids[@]} build processes to complete..." | |
| # Wait for all processes | |
| for pid in "${pids[@]}"; do | |
| wait "$pid" | |
| done | |
| # Check results | |
| for app in "${SPLIT_APPS[@]}"; do | |
| if [[ -f "/tmp/build_result_$app" ]]; then | |
| local result=$(cat "/tmp/build_result_$app") | |
| if [[ "$result" == "FAILED:$app" ]]; then | |
| failed_builds+=("$app") | |
| fi | |
| rm -f "/tmp/build_result_$app" | |
| fi | |
| done | |
| # Check unified build result if it exists | |
| if [[ -f "/tmp/build_result_unified" ]]; then | |
| local result=$(cat "/tmp/build_result_unified") | |
| if [[ "$result" == "FAILED:unified" ]]; then | |
| failed_builds+=("unified") | |
| fi | |
| rm -f "/tmp/build_result_unified" | |
| fi | |
| # Return failure if any builds failed | |
| if [[ ${#failed_builds[@]} -gt 0 ]]; then | |
| print_error "The following builds failed: ${failed_builds[*]}" | |
| return 1 | |
| fi | |
| return 0 | |
| } | |
| wait_for_builds() { | |
| local pids=("$@") | |
| local failed_builds=() | |
| local failed_pids=() | |
| print_status "Waiting for ${#pids[@]} build processes to complete..." | |
| # Wait for all processes and capture non-zero statuses | |
| for pid in "${pids[@]}"; do | |
| if ! wait "$pid"; then | |
| failed_pids+=("$pid") | |
| fi | |
| done | |
| # Check split build results | |
| for app in "${SPLIT_APPS[@]}"; do | |
| local result_file="/tmp/build_result_$app" | |
| if [[ -f "$result_file" ]]; then | |
| local result | |
| result="$(<"$result_file")" | |
| if [[ "$result" == "FAILED:$app" ]]; then | |
| failed_builds+=("$app") | |
| fi | |
| rm -f "$result_file" | |
| elif ((${#pids[@]})); then | |
| # Missing result file → treat as failure | |
| failed_builds+=("$app") | |
| fi | |
| done | |
| # Check unified build result | |
| local unified_file="/tmp/build_result_unified" | |
| if [[ -f "$unified_file" ]]; then | |
| local result | |
| result="$(<"$unified_file")" | |
| if [[ "$result" == "FAILED:unified" ]]; then | |
| failed_builds+=("unified") | |
| fi | |
| rm -f "$unified_file" | |
| elif ((${#pids[@]} == 1)); then | |
| failed_builds+=("unified") | |
| fi | |
| if ((${#failed_pids[@]})) || ((${#failed_builds[@]})); then | |
| print_error "The following builds failed: ${failed_builds[*]}" | |
| return 1 | |
| fi | |
| return 0 | |
| } |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 202-202: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 212-212: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In scripts/build-images.sh around lines 188 to 227, wait_for_builds currently
waits for PIDs but ignores each wait exit status, treats missing /tmp build
result files as success, and uses command substitution in the same line as a
local declaration (triggers SC2155); change the loop to capture each wait exit
code (e.g., local pid; wait "$pid"; local exit_code=$?), record failures when
wait returns non‑zero by adding the corresponding app name to failed_builds,
treat missing /tmp/build_result_$app files as failures (add app to failed_builds
when the file is absent), and fix SC2155 by declaring local variables separately
from assignment for all command substitutions (declare local result;
result="$(cat ...)" or use read). Ensure the unified result follows the same
pattern (separate declaration, check missing file, and consider non‑zero wait
statuses).
1 participant
Summary by CodeRabbit
New Features
Chores
Documentation