Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds request-scoped chat-completion logging: Hono middleware that queues and flushes per-request logs (with fallback synthesis for 4xx client errors), context helpers to build/enqueue logs, test helpers to process pending logs, UI handling for Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as chatCompletionLog<br/>Middleware
participant Handler as Request<br/>Handler
participant Enqueuer as Log<br/>Enqueuer
participant Stream as Stream<br/>Coordinator
participant Flusher as Log<br/>Flusher
participant DB as Database
Client->>Middleware: HTTP request (incl. x-request-id)
Middleware->>Middleware: create ChatCompletionLogState & clone raw request
Middleware->>Handler: invoke downstream handler
Handler->>Enqueuer: enqueueChatLog(...) -> push to pendingLogs
alt streaming
Handler->>Stream: registerStreamCompletion()
Stream->>Client: stream events
Stream->>Middleware: finishStreamCompletion()
end
alt downstream throws
Handler-x Middleware: throws HTTPException -> caughtError stored
end
Middleware->>Flusher: await streamCompletion if present
Middleware->>Flusher: if 4xx & pendingLogs empty -> synthesize client_error log
Flusher->>DB: insertLog(entry) for each pending log
DB-->>Flusher: persisted
Flusher-->>Middleware: flush complete
Middleware-->>Client: finalize response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c9a276ed8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| log.unifiedFinishReason === "client_error", | ||
| ); | ||
|
|
||
| if (status >= 400 && status < 500 && !hasQueuedClientError) { |
There was a problem hiding this comment.
This condition synthesizes a client_error for any 4xx response unless an existing queued row is already tagged client_error, but the handler also queues other intentional 4xx outcomes (for example, non-streaming cancellation queues finishReason: "canceled" before returning HTTP 400). In those cases the middleware writes two rows for the same request (canceled plus synthesized client_error), which inflates counts and misclassifies request outcomes; synthesis should be skipped whenever a log is already queued, not only when a queued log is client_error.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/middleware/chat-completion-log.ts`:
- Around line 44-49: The synthesized log's streamed field is computed only from
baseLogEntry.rawRequest.stream, but rawRequest is only set when debugMode is
true, causing streamed to be false for non-debug streaming failures; update the
logging path that constructs the base log (or the creator of baseLogEntry) to
pass the original request's stream flag explicitly (e.g., add a stream boolean
into the base log options or baseLogEntry) and then change the streamed
computation in chat-completion-log.ts to prefer that explicit flag (reference:
streamed, baseLogEntry.rawRequest, debugMode, base log options).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2331c158-3d95-47ea-9846-1039cb7b0702
📒 Files selected for processing (7)
apps/gateway/src/api-individual.e2e.tsapps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/middleware/chat-completion-log.tsapps/gateway/src/chat/tools/chat-log-context.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/vars.ts
| streamed: | ||
| typeof baseLogEntry.rawRequest === "object" && | ||
| baseLogEntry.rawRequest !== null && | ||
| "stream" in baseLogEntry.rawRequest | ||
| ? Boolean(baseLogEntry.rawRequest.stream) | ||
| : false, |
There was a problem hiding this comment.
streamed field may be incorrectly false when debugMode is disabled.
The streamed detection relies on baseLogEntry.rawRequest.stream, but rawRequest is only populated when debugMode is true. For non-debug streaming requests that fail with a 4xx, the synthesized client error log will have streamed: false regardless of the actual request.
If the synthesized logs are only for early validation failures (before the stream flag is even relevant), this may be acceptable. Otherwise, consider passing the stream flag separately through the base log options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/middleware/chat-completion-log.ts` around lines 44 -
49, The synthesized log's streamed field is computed only from
baseLogEntry.rawRequest.stream, but rawRequest is only set when debugMode is
true, causing streamed to be false for non-debug streaming failures; update the
logging path that constructs the base log (or the creator of baseLogEntry) to
pass the original request's stream flag explicitly (e.g., add a stream boolean
into the base log options or baseLogEntry) and then change the streamed
computation in chat-completion-log.ts to prefer that explicit flag (reference:
streamed, baseLogEntry.rawRequest, debugMode, base log options).
There was a problem hiding this comment.
Pull request overview
Refactors /v1/chat/completions request logging by moving log insertion out of the route handler into a route-scoped post-middleware, with request-scoped log state stored on the Hono context to preserve per-attempt and streaming logging while preventing duplicate log rows.
Changes:
- Introduces a per-request
chatCompletionLogStatecontext variable and helpers to build/enqueue log rows during request processing. - Adds a
/completions-scoped middleware that flushes queued logs post-handler and can synthesizeclient_errorlogs for local 4xx responses. - Updates API/unit/e2e tests to account for async log flushing and adds duplicate-log prevention assertions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| apps/gateway/src/vars.ts | Adds chatCompletionLogState typing to Hono context variables. |
| apps/gateway/src/test-utils/test-helpers.ts | Adds helper to explicitly process pending log queue work during tests. |
| apps/gateway/src/chat/tools/chat-log-context.ts | New request-scoped log state + helpers to build base log entries and enqueue logs. |
| apps/gateway/src/chat/middleware/chat-completion-log.ts | New post-middleware that flushes queued logs asynchronously and synthesizes missing client-error logs. |
| apps/gateway/src/chat/chat.ts | Refactors route logging to enqueue logs into context state; registers/finishes stream completion for post-flush coordination. |
| apps/gateway/src/api.spec.ts | Adjusts tests for async log flushing and adds request-id-based assertions. |
| apps/gateway/src/api-individual.e2e.ts | Adds assertion ensuring a single log row per request id for a client-error scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface ChatCompletionLogState { | ||
| pendingLogs: LogInsertData[]; | ||
| baseLogOptions?: Partial<CreateLogEntryOptions>; | ||
| streamCompletion?: Promise<void>; | ||
| resolveStreamCompletion?: () => void; | ||
| caughtError?: unknown; | ||
| internalContentFilter?: boolean; | ||
| clientErrorSynthesized?: boolean; | ||
| } |
There was a problem hiding this comment.
clientErrorSynthesized is written (set to false initially and later to true) but never read anywhere. This adds state surface area without affecting behavior—consider removing it (and its initialization) or using it to gate duplicate-synthesis logic if that was the intent.
steebchen
force-pushed
the
chat-log-post-hook
branch
from
6c9a276 to
5b087ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/api.spec.ts`:
- Around line 1371-1376: The immediate assertion that logs length is 0 can miss
late async writes; replace the direct expect after calling processPendingLogs()
with a short polling/retry assertion that queries
db.select().from(tables.log).where(eq(tables.log.requestId, requestId)) until it
consistently returns length 0 or a timeout elapses. Update both occurrences (the
block using processPendingLogs() and the later block around lines 1532–1540) to
use the same retry/wait helper (e.g., waitForCondition/ retryUntil/ eventually)
so the test re-queries the DB periodically and only fails after a reasonable
timeout, ensuring stability against delayed writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4856b8a3-a00e-4ee9-b79d-47f7072f7d2b
📒 Files selected for processing (7)
apps/gateway/src/api-individual.e2e.tsapps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/middleware/chat-completion-log.tsapps/gateway/src/chat/tools/chat-log-context.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/vars.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/gateway/src/api-individual.e2e.ts
- apps/gateway/src/vars.ts
- apps/gateway/src/test-utils/test-helpers.ts
- apps/gateway/src/chat/middleware/chat-completion-log.ts
- apps/gateway/src/chat/tools/chat-log-context.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b087ad179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state.pendingLogs.push({ | ||
| ...baseLogEntry, | ||
| ...logFields, | ||
| }); |
There was a problem hiding this comment.
enqueueChatLog only does a shallow spread into pendingLogs, so nested objects (for example routingMetadata) are kept by reference until chatCompletionLogMiddleware flushes at request end. In retry flows, routingMetadata.providerScores is mutated in place later (e.g. rate-limit annotations in chat.ts), which means earlier attempt logs can be written with post-retry state instead of the state at enqueue time, corrupting per-attempt analytics/debug data. Serialize or deep-clone the payload when enqueuing to preserve a point-in-time snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 274f10c99f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| usedModel: initialRequestedModel, | ||
| usedModelMapping: requestedModel, | ||
| usedProvider: requestedProvider ?? "llmgateway", | ||
| requestedModel: initialRequestedModel, |
There was a problem hiding this comment.
This initializes the synthesized-log base with usedProvider/usedModel before provider routing and provider-key checks complete, so any 4xx thrown before the later refresh (for example the API-keys "No provider key set..." path) is logged from stale defaults. In those cases the new middleware-generated client_error row is attributed to llmgateway (and requested-model placeholders) instead of the actual routed provider context, which skews provider-level error analytics and troubleshooting for the exact scenarios this refactor newly logs.
Useful? React with 👍 / 👎.
steebchen
force-pushed
the
chat-log-post-hook
branch
from
274f10c to
dac487f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gateway/src/chat/middleware/chat-completion-log.ts (1)
118-132: Consider snapshottingpendingLogsbefore iteration for defensive safety.The
for...ofloop iterates over the livestate.pendingLogsarray. While timing makes concurrent mutation unlikely (handlers complete before flush for non-streaming; stream completion is awaited for streaming), snapshotting the array prevents edge cases if a misbehaving handler pushes logs afterfinishStreamCompletion.🛡️ Defensive snapshot
- for (const logData of state.pendingLogs) { + const logsToFlush = [...state.pendingLogs]; + state.pendingLogs.length = 0; // Clear to prevent duplicate processing + for (const logData of logsToFlush) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/middleware/chat-completion-log.ts` around lines 118 - 132, The loop currently iterates directly over state.pendingLogs which can be mutated during flush; make a defensive shallow copy first (e.g., const logsToFlush = [...state.pendingLogs]) and iterate over logsToFlush when calling _insertLog so concurrent pushes to state.pendingLogs won't affect the flush; keep the existing internalContentFilter merge logic and error handling in place and perform this change where the for...of over state.pendingLogs is located (the flush loop inside finishStreamCompletion/flush routine).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/gateway/src/chat/middleware/chat-completion-log.ts`:
- Around line 118-132: The loop currently iterates directly over
state.pendingLogs which can be mutated during flush; make a defensive shallow
copy first (e.g., const logsToFlush = [...state.pendingLogs]) and iterate over
logsToFlush when calling _insertLog so concurrent pushes to state.pendingLogs
won't affect the flush; keep the existing internalContentFilter merge logic and
error handling in place and perform this change where the for...of over
state.pendingLogs is located (the flush loop inside finishStreamCompletion/flush
routine).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 34ebfea1-7982-4daa-8791-4318393be59c
📒 Files selected for processing (7)
apps/gateway/src/api-individual.e2e.tsapps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/middleware/chat-completion-log.tsapps/gateway/src/chat/tools/chat-log-context.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/vars.ts
✅ Files skipped from review due to trivial changes (1)
- apps/gateway/src/vars.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/gateway/src/test-utils/test-helpers.ts
- apps/gateway/src/api-individual.e2e.ts
- apps/gateway/src/chat/tools/chat-log-context.ts
- apps/gateway/src/api.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dac487f194
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hasQueuedClientError = state.pendingLogs.some( | ||
| (log) => | ||
| log.finishReason === "client_error" || | ||
| log.unifiedFinishReason === "client_error", | ||
| ); |
There was a problem hiding this comment.
The 4xx synthesis gate only checks for an existing queued client_error, so requests that already queued a different terminal log (for example non-streaming cancellation queues finishReason: "canceled" and returns HTTP 400) will get a second synthesized client_error row. That creates duplicate rows for one request and inflates/misclassifies error analytics; synthesis should be suppressed when pendingLogs is non-empty, not only when a queued entry is already client_error.
Useful? React with 👍 / 👎.
| state.pendingLogs.push({ | ||
| ...baseLogEntry, | ||
| ...logFields, | ||
| }); |
There was a problem hiding this comment.
Queued logs are stored with a shallow spread, so nested references (notably routingMetadata.providerScores) are shared with live request state and can be mutated later during retry routing before flushChatCompletionLogs runs. In multi-attempt flows this causes earlier attempt logs to persist post-mutation metadata instead of the point-in-time values for that attempt, which corrupts retry/debug analytics.
Useful? React with 👍 / 👎.
| usedModel: initialRequestedModel, | ||
| usedModelMapping: requestedModel, | ||
| usedProvider: requestedProvider ?? "llmgateway", | ||
| requestedModel: initialRequestedModel, |
There was a problem hiding this comment.
Base log options are initialized with usedProvider/usedModel before provider key resolution and then reused for synthesized 4xx logs. If a 4xx is thrown before the later refresh (for example No API key set for provider...), the synthesized client_error log is attributed to these provisional defaults (llmgateway/requested placeholders) instead of the resolved provider context, skewing provider-level error reporting for precisely these pre-upstream failures.
Useful? React with 👍 / 👎.
steebchen
force-pushed
the
chat-log-post-hook
branch
from
0df0b65 to
f1f19cd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1f19cde20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| streamed: | ||
| typeof baseLogEntry.rawRequest === "object" && | ||
| baseLogEntry.rawRequest !== null && | ||
| "stream" in baseLogEntry.rawRequest | ||
| ? Boolean(baseLogEntry.rawRequest.stream) | ||
| : false, |
There was a problem hiding this comment.
Using rawRequest.stream to set streamed misclassifies synthesized client_error logs when a request asks for streaming but fails before any SSE response is started (for example, early 4xx validation/provider-key errors). In those cases the API returns a normal JSON error, but this code records streamed: true, which inflates streaming error metrics and makes debugging request behavior harder; the flag should be based on the actual response mode (e.g., emitted SSE) rather than requested mode.
Useful? React with 👍 / 👎.
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/api.spec.ts (1)
1165-1206:⚠️ Potential issue | 🟡 MinorPlease include the repo-standard unit test run in validation.
The PR notes mention selective Vitest runs, but these changes add
*.spec.tscoverage. Please add apnpm test:unitrun before merge so the full unit suite is exercised through the standard entrypoint.As per coding guidelines, "
**/*.spec.ts: Run unit tests withpnpm test:unitafter adding features`."Also applies to: 1363-1388, 1512-1552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/api.spec.ts` around lines 1165 - 1206, The test run validation is missing a repo-standard unit test execution for newly added/spec files (e.g., apps/gateway/**/*.spec.ts); update the CI workflow that validates PRs to run the unit test entrypoint by adding a step to execute "pnpm test:unit" (or the existing test:unit script) before merge/after installing deps and before any integration/e2e steps so all "*.spec.ts" files are exercised; ensure this new step is included alongside other checks referenced in the PR and apply the same change for the other affected ranges (lines noted for additional specs).
♻️ Duplicate comments (1)
apps/gateway/src/chat/middleware/chat-completion-log.ts (1)
44-49:⚠️ Potential issue | 🟡 Minor
streamedcan still be wrong on synthesized 4xx rows.At Lines 44-49, this is inferred from
baseLogEntry.rawRequest.stream. WhenrawRequestis absent, a real streaming validation failure gets recorded as non-streaming. Carry the original request'sstreamflag in the base log state instead of deriving it from raw payload capture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/middleware/chat-completion-log.ts` around lines 44 - 49, The streamed flag is being derived from baseLogEntry.rawRequest which is missing for synthesized 4xx rows, causing real streaming failures to be marked non-streaming; instead propagate the original request's stream flag into the base log state when the log entry is first created and use that stored value for the streamed field (update the code that sets baseLogEntry and the streamed calculation to read the carried originalStream flag rather than inspecting baseLogEntry.rawRequest.stream); modify references around baseLogEntry.rawRequest and the streamed field so synthesized/fallback rows use the preserved original stream flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/tools/chat-log-context.ts`:
- Around line 55-84: buildBaseLogEntry currently returns null until
hasCompleteBaseLogOptions is satisfied, which causes post-auth client errors
(synthesized client_error) to be skipped; fix by either seeding minimal base
fields earlier in getOrCreateChatCompletionLogState (populate requestId, project
and apiKey from Context<ServerTypes> when the state is created) or change
buildBaseLogEntry to accept a synthesized/client-error path: add a
hasMinimalBaseLogOptions helper (checking only the minimal required fields like
requestId/project/apiKey) and use it when patch indicates a synthesized
client_error (or pass an allowPartial flag into buildBaseLogEntry); update
callers (e.g., where updateBaseLogOptions is used) to pass the flag or rely on
the seeded minimal state so synthesized client_error logs get built even before
full updateBaseLogOptions runs.
---
Outside diff comments:
In `@apps/gateway/src/api.spec.ts`:
- Around line 1165-1206: The test run validation is missing a repo-standard unit
test execution for newly added/spec files (e.g., apps/gateway/**/*.spec.ts);
update the CI workflow that validates PRs to run the unit test entrypoint by
adding a step to execute "pnpm test:unit" (or the existing test:unit script)
before merge/after installing deps and before any integration/e2e steps so all
"*.spec.ts" files are exercised; ensure this new step is included alongside
other checks referenced in the PR and apply the same change for the other
affected ranges (lines noted for additional specs).
---
Duplicate comments:
In `@apps/gateway/src/chat/middleware/chat-completion-log.ts`:
- Around line 44-49: The streamed flag is being derived from
baseLogEntry.rawRequest which is missing for synthesized 4xx rows, causing real
streaming failures to be marked non-streaming; instead propagate the original
request's stream flag into the base log state when the log entry is first
created and use that stored value for the streamed field (update the code that
sets baseLogEntry and the streamed calculation to read the carried
originalStream flag rather than inspecting baseLogEntry.rawRequest.stream);
modify references around baseLogEntry.rawRequest and the streamed field so
synthesized/fallback rows use the preserved original stream flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ee593692-8dbe-4428-a316-33c128b5174f
📒 Files selected for processing (7)
apps/gateway/src/api-individual.e2e.tsapps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/middleware/chat-completion-log.tsapps/gateway/src/chat/tools/chat-log-context.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/vars.ts
✅ Files skipped from review due to trivial changes (2)
- apps/gateway/src/vars.ts
- apps/gateway/src/api-individual.e2e.ts
| function hasCompleteBaseLogOptions( | ||
| options?: Partial<CreateLogEntryOptions>, | ||
| ): options is CreateLogEntryOptions { | ||
| return Boolean( | ||
| options && | ||
| typeof options.requestId === "string" && | ||
| options.project && | ||
| options.apiKey && | ||
| typeof options.usedModel === "string" && | ||
| typeof options.usedProvider === "string" && | ||
| typeof options.requestedModel === "string" && | ||
| Array.isArray(options.messages) && | ||
| options.customHeaders !== undefined && | ||
| typeof options.debugMode === "boolean", | ||
| ); | ||
| } | ||
|
|
||
| export function buildBaseLogEntry( | ||
| c: Context<ServerTypes>, | ||
| patch: Partial<CreateLogEntryOptions> = {}, | ||
| ) { | ||
| const state = getOrCreateChatCompletionLogState(c); | ||
| const mergedOptions = { | ||
| ...state.baseLogOptions, | ||
| ...patch, | ||
| }; | ||
|
|
||
| if (!hasCompleteBaseLogOptions(mergedOptions)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This still misses some post-auth client errors.
At Line 82, buildBaseLogEntry() bails out until the full base option set exists. In the provided apps/gateway/src/chat/chat.ts flow, updateBaseLogOptions(c, ...) runs only after checks like assertApiKeyWithinUsageLimits(apiKey) and the archived-project 410 branch, so those local post-auth 4xx/410 responses will still skip synthesized client_error logging. Seed the minimal base fields earlier, or let the synthesized path build from a smaller required subset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/tools/chat-log-context.ts` around lines 55 - 84,
buildBaseLogEntry currently returns null until hasCompleteBaseLogOptions is
satisfied, which causes post-auth client errors (synthesized client_error) to be
skipped; fix by either seeding minimal base fields earlier in
getOrCreateChatCompletionLogState (populate requestId, project and apiKey from
Context<ServerTypes> when the state is created) or change buildBaseLogEntry to
accept a synthesized/client-error path: add a hasMinimalBaseLogOptions helper
(checking only the minimal required fields like requestId/project/apiKey) and
use it when patch indicates a synthesized client_error (or pass an allowPartial
flag into buildBaseLogEntry); update callers (e.g., where updateBaseLogOptions
is used) to pass the flag or rely on the seeded minimal state so synthesized
client_error logs get built even before full updateBaseLogOptions runs.
steebchen
force-pushed
the
chat-log-post-hook
branch
from
f1f19cd to
13ba95e
Compare
This reverts commit e28767c.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6cac672c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rawRequestPreviewPromise: c.req.raw | ||
| .clone() | ||
| .json() | ||
| .catch(() => undefined), |
There was a problem hiding this comment.
rawRequestPreviewPromise starts c.req.raw.clone().json() for every /completions request, even though the preview is only needed when synthesizing a fallback 4xx log. The main handler already parses the body via c.req.json(), so this doubles JSON parsing and body buffering on the hot path; for large chat/image payloads this can add noticeable latency and memory pressure (including OOM risk) in production. Defer this parse until fallback synthesis is actually required.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ui/src/components/dashboard/log-card.tsx (1)
196-210: Consider simplifying the nested ternary for Badge variant.The nested ternary is functional but could be more readable. This is a minor stylistic suggestion:
♻️ Optional refactor
<Badge - variant={ - isClientError - ? "outline" - : log.hasError - ? "destructive" - : log.unifiedFinishReason === "content_filter" - ? "destructive" - : "default" - } - className={ - isClientError - ? "border-orange-300 bg-orange-50 text-orange-600" - : undefined - } + variant={ + isClientError ? "outline" + : log.hasError || log.unifiedFinishReason === "content_filter" ? "destructive" + : "default" + } + className={ + isClientError + ? "border-orange-300 bg-orange-50 text-orange-600" + : undefined + } >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dashboard/log-card.tsx` around lines 196 - 210, The Badge variant prop uses a nested ternary that is hard to read; refactor it in the Badge JSX (the Badge component's variant prop where isClientError, log.hasError and log.unifiedFinishReason are used) into a small, readable expression: compute the variant in a local const (e.g. const badgeVariant = isClientError ? "outline" : log.hasError || log.unifiedFinishReason === "content_filter" ? "destructive" : "default") or equivalent if/else before the return, then pass badgeVariant to <Badge variant={badgeVariant}> to keep intent clear and eliminate deep nesting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ui/src/components/dashboard/log-card.tsx`:
- Around line 196-210: The Badge variant prop uses a nested ternary that is hard
to read; refactor it in the Badge JSX (the Badge component's variant prop where
isClientError, log.hasError and log.unifiedFinishReason are used) into a small,
readable expression: compute the variant in a local const (e.g. const
badgeVariant = isClientError ? "outline" : log.hasError ||
log.unifiedFinishReason === "content_filter" ? "destructive" : "default") or
equivalent if/else before the return, then pass badgeVariant to <Badge
variant={badgeVariant}> to keep intent clear and eliminate deep nesting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e2f6eec-1dee-4664-92d2-341be29d365b
📒 Files selected for processing (12)
apps/gateway/src/api-individual.e2e.tsapps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/middleware/chat-completion-log.spec.tsapps/gateway/src/chat/middleware/chat-completion-log.tsapps/gateway/src/chat/tools/chat-log-context.tsapps/gateway/src/chat/tools/transform-streaming-to-openai.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/vars.tsapps/ui/src/app/dashboard/[orgId]/[projectId]/activity/[logId]/log-detail-client.tsxapps/ui/src/components/dashboard/log-card.tsxapps/worker/src/worker.ts
✅ Files skipped from review due to trivial changes (3)
- apps/gateway/src/api-individual.e2e.ts
- apps/gateway/src/chat/middleware/chat-completion-log.spec.ts
- apps/gateway/src/chat/tools/transform-streaming-to-openai.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/gateway/src/vars.ts
- apps/gateway/src/chat/tools/chat-log-context.ts
- apps/gateway/src/api.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a664ea420a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const state: ChatCompletionLogState = { | ||
| pendingLogs: [], | ||
| clientErrorSynthesized: false, | ||
| rawRequestPreview: c.req.raw.clone(), |
There was a problem hiding this comment.
This unconditionally clones the raw request for every /completions call, even though the clone is only needed for synthesized fallback 4xx logs. Request.clone() tees the body stream, so when the normal handler parses the request, the unread clone branch can accumulate a second buffered copy of large payloads (for example image/base64-heavy prompts), adding avoidable memory pressure and latency on the hot path and increasing OOM risk under load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99397521dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Report key health for environment-based tokens | ||
| // Don't report content_filter as a key error - it's intentional provider behavior | ||
| if (envVarName !== undefined && finishReason !== "content_filter") { | ||
| reportKeyError(envVarName, configIndex, res.status, errorResponseText); |
There was a problem hiding this comment.
This refactor leaves chat error handling reporting health only for env-var keys (envVarName) and no longer records outcomes for DB-backed providerKey entries. In credits/hybrid/provider-key flows envVarName is unset, so tracked-key metrics are never updated and failover selection (which uses tracked-key health in cached-queries.ts) cannot demote bad keys, causing repeated routing to unhealthy provider keys. Please restore reportTrackedKeyError/Success updates in this path.
Useful? React with 👍 / 👎.
|
|
||
| return c.json(responseForCurrentRequest); | ||
| return c.json(cachedResponse); |
There was a problem hiding this comment.
On a non-stream cache hit this now returns cachedResponse verbatim, so request-scoped metadata from the original cached request (notably metadata.request_id) is sent to later callers. That breaks request correlation/debugging because multiple distinct requests can receive the same cached request ID; this path should reapply current-request metadata before returning JSON.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/gateway/src/api.spec.ts (1)
1487-1491:⚠️ Potential issue | 🟠 MajorStabilize async log-count assertions to actually catch late duplicate writes.
These are single-instant checks after async queue processing; delayed inserts can land after the assertion and create false passes on duplicate-prevention behavior.
Suggested stabilization pattern
+async function expectStableLogCountByRequestId( + requestId: string, + expectedCount: number, + windowMs = 500, + intervalMs = 50, +) { + const start = Date.now(); + while (Date.now() - start < windowMs) { + await processPendingLogs(); + const logs = await db + .select() + .from(tables.log) + .where(eq(tables.log.requestId, requestId)); + expect(logs).toHaveLength(expectedCount); + await new Promise((resolve) => setTimeout(resolve, intervalMs)); + } +} ... - await processPendingLogs(); - const logs = await db.select().from(tables.log).where(eq(tables.log.requestId, requestId)); - expect(logs).toHaveLength(0); + await expectStableLogCountByRequestId(requestId, 0); ... - const matchingLogs = await db.select().from(tables.log).where(eq(tables.log.requestId, requestId)); - expect(matchingLogs).toHaveLength(1); + await expectStableLogCountByRequestId(requestId, 1);Also applies to: 1541-1545, 1722-1727, 1887-1891
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/api.spec.ts` around lines 1487 - 1491, The single-instant DB assertions (the matchingLogs query and expect(matchingLogs).toHaveLength(1)) can miss late inserts; replace them with a stabilized polling/assertion that retries the select on tables.log where eq(tables.log.requestId, requestId) until the count becomes 1 or a short timeout elapses. Concretely: remove the direct expect on matchingLogs in the test blocks (the matchingLogs variable and its usage) and instead loop/retry (e.g., a small backoff loop or a test helper like waitFor/retryUntil) re-running db.select().from(tables.log).where(eq(tables.log.requestId, requestId)) until its length is 1 or fail after timeout; apply the same change to the other occurrences mentioned (around lines 1541-1545, 1722-1727, 1887-1891) so duplicate late writes are detected reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/gateway/src/api.spec.ts`:
- Around line 1487-1491: The single-instant DB assertions (the matchingLogs
query and expect(matchingLogs).toHaveLength(1)) can miss late inserts; replace
them with a stabilized polling/assertion that retries the select on tables.log
where eq(tables.log.requestId, requestId) until the count becomes 1 or a short
timeout elapses. Concretely: remove the direct expect on matchingLogs in the
test blocks (the matchingLogs variable and its usage) and instead loop/retry
(e.g., a small backoff loop or a test helper like waitFor/retryUntil) re-running
db.select().from(tables.log).where(eq(tables.log.requestId, requestId)) until
its length is 1 or fail after timeout; apply the same change to the other
occurrences mentioned (around lines 1541-1545, 1722-1727, 1887-1891) so
duplicate late writes are detected reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0c20df34-f1c4-497e-b5c2-75c16d39c2e1
📒 Files selected for processing (6)
apps/gateway/src/api-individual.e2e.tsapps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/transform-streaming-to-openai.tsapps/ui/src/app/dashboard/[orgId]/[projectId]/activity/[logId]/log-detail-client.tsxpackages/shared/src/components/log-card.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/gateway/src/api-individual.e2e.ts
- apps/gateway/src/chat/tools/transform-streaming-to-openai.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a3349c84f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| routingAttempts.length > 0 ? routingAttempts : null, | ||
| requestId, | ||
| usedRegion, | ||
| ); |
There was a problem hiding this comment.
This call no longer passes requestId, but transformResponseToOpenai still expects (…, routing, requestId, usedRegion), so usedRegion is being bound to requestId and usedRegion becomes undefined. As a result, non-streaming transformed responses get incorrect metadata.request_id values (often a region string or empty string) and lose metadata.used_region, which breaks request-level correlation/debugging for successful responses.
Useful? React with 👍 / 👎.
| timeToFirstToken: null, | ||
| timeToFirstReasoningToken: null, | ||
| responseSize, | ||
| content: content, |
There was a problem hiding this comment.
The queued log now writes content directly, but the previous logic intentionally replaced it with joined base64 data: image URLs when image generation returned inline images. For image-generation completions where text content is empty, this drops the only renderable payload from log.content, so the activity detail view can no longer display generated images from logs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb0a26325f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const headers = getProviderHeaders(usedProvider, usedToken, { | ||
| webSearchEnabled: !!webSearchTool, | ||
| }); |
There was a problem hiding this comment.
This call now omits requestId, so upstream requests no longer carry the gateway request ID. getProviderHeaders only injects x-request-id when options.requestId is provided, which means provider-side logs can no longer be correlated with gateway logs for the same request (this regression affects both streaming and non-streaming paths).
Useful? React with 👍 / 👎.
| failedProviderIds.add(providerRetryKey(usedProvider, usedRegion)); | ||
| continue; |
There was a problem hiding this comment.
On fetch failures this marks the entire provider/region as failed via providerRetryKey(...) and immediately excludes it from further selection, but there is no key-level retry path left in this refactor. In projects with multiple keys for one provider, a single bad key now causes avoidable request failures (especially when no other provider is available), even though another key for the same provider could succeed.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adopts main's chat.ts wholesale and rewires its insertLog wrappers to push entries onto the middleware's pendingLogs queue, so the branch gets the immediate-SSE-error retry, same-provider alternate-key retry, buildRoutingAttempt logId/apiKeyHash stamping, and request_id metadata flow without losing the middleware-based 4xx synthesis. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b5c2663f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function getRequestId(c: Context<ServerTypes>): string { | ||
| return c.req.header("x-request-id") ?? shortid(40); | ||
| } |
There was a problem hiding this comment.
For synthesized client_error rows, getRequestId() only checks c.req.header("x-request-id") and otherwise creates a fresh shortid, but many /chat/completions 4xx paths generate the request ID inside chat.ts and set it on the response header (c.header("x-request-id", requestId)) after parsing starts. When callers do not provide x-request-id, post-auth 4xx responses (for example missing provider key) can be logged under a different request ID than the one returned to the client, breaking request-level correlation in logs and debugging workflows.
Useful? React with 👍 / 👎.
# Conflicts: # apps/gateway/src/chat/chat.ts
# Conflicts: # apps/gateway/src/chat/chat.ts
Integrates cache write billing from main (#2171) into chat-log-post-hook refactoring branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Resolved merge conflicts with origin/main. Conflicts: Resolution: Since both branches made large structural changes to the same file, git's diff algorithm produced false-positive conflict matches (anchoring on common text like
TypeScript compilation passes cleanly ( |
Summary
/v1/chat/completionslog insertion out of the route body into a route-scoped post-middlewareclient_errorrows for local 4xx responsesReview fix
finallyValidation
pnpm exec vitest run apps/gateway/src/api.spec.ts -t "streaming request surfaces truncated upstream streams|missing Authorization header|missing provider API key" --no-file-parallelismpnpm exec vitest run apps/gateway/src/fallback.spec.ts -t "streaming 500 error returns error SSE event and logs upstream_error|non-streaming: retries on 500 and succeeds on fallback provider with failed_attempts in metadata" --no-file-parallelismE2E_TEST=true pnpm exec vitest run -c vitest/vitest.e2e.config.mts apps/gateway/src/api-individual.e2e.ts -t "JSON output mode error when 'json' not mentioned in messages" --no-file-parallelismpnpm lintpnpm formatpnpm buildSummary by CodeRabbit
New Features
Bug Fixes
Tests