[logging] inspector: add typed event foundation#1930
Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
dosubot
Bot
added
size:L
Internal previewPreview URL: https://mcp-inspector-pr-1930.up.railway.app |
Contributor
|
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 a typed structured-logging system and a recursive log scrubber. Introduces 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
mcpjam-inspector/server/app.ts (1)
156-163: Auth/security failures bypass the new request log context.Mounting
requestLogContextMiddlewareaftersessionAuthMiddleware(line 157) means 401s from unauthenticated requests, 403s from origin validation, and the hosted-mode 410 partition responses never producehttp.request.completed/failedevents — exactly the requests an SRE most wants to see when triaging an outage or attack. If that omission is intentional for this PR, no action needed; if not, consider mounting the context middleware just before the security stack so it can emit even when downstream layers short-circuit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/app.ts` around lines 156 - 163, The requestLogContextMiddleware is mounted after sessionAuthMiddleware, so authentication/authorization failures are bypassing the request logging; move the requestLogContextMiddleware to run before the security stack by registering it prior to app.use("*", sessionAuthMiddleware) (i.e., ensure app.use("/api/*", requestLogContextMiddleware) is called before sessionAuthMiddleware) so http.request.completed/failed events are emitted even when sessionAuthMiddleware or origin/host checks short-circuit.mcpjam-inspector/server/middleware/request-log-context.ts (2)
62-64: Streaming/SSE routes never emit completion telemetry.When
isStreaming(c)is true and no error was thrown, the middleware returns silently — nohttp.request.completedevent, no duration, no status. SSE endpoints in Inspector are precisely the long-lived connections you want visibility into (drop rates, p99 lifetime, byte counts), and right now they will be invisible in Axiom dashboards. If the intent was only to defer logging until the stream closes, that work needs to be wired here; otherwise consider emitting a dedicatedhttp.stream.openedevent at this point so streaming routes are at least counted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/middleware/request-log-context.ts` around lines 62 - 64, The middleware currently returns early when isStreaming(c) is true and no error was thrown, which prevents any telemetry; instead, either emit a dedicated stream-opened telemetry event at that branch (e.g., send an "http.stream.opened" event with context like method, path, headers and timestamp) or wire completion logging by attaching a close/end listener to the streaming response/connection (use isStreaming(c) branch to register a 'close'/'finish' handler that will emit the usual http.request.completed event with duration, status and byte counts when the stream actually ends); update the code around isStreaming(c) and thrown so streaming routes are counted and their completion is emitted.
35-36: Usec.header()for idiomatic response header management.In Hono middleware,
c.header()is the recommended API for setting response headers. Whilec.res.headers.set()works beforeawait next()and headers are preserved through downstream handlers,c.header()is the higher-level, idiomatic approach. It integrates seamlessly with Hono's response helpers and is safer across all contexts.Suggested change
const requestId = c.req.header("x-request-id") ?? randomUUID(); - c.res.headers.set("x-request-id", requestId); + c.header("x-request-id", requestId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/middleware/request-log-context.ts` around lines 35 - 36, The middleware currently reads the request ID using c.req.header and sets the response header using c.res.headers.set; replace the low-level call with Hono's idiomatic c.header API by calling c.header("x-request-id", requestId) after computing requestId (from c.req.header("x-request-id") ?? randomUUID()), so the requestId variable remains the same and response header management uses c.header rather than c.res.headers.set.mcpjam-inspector/server/utils/logger.ts (2)
133-135: Useconsole.errorfor*.failedevents.
shouldLog()echoes every event throughconsole.log, including failures. In dev/verbose runs this hides them from terminals/IDEs that key off stderr (and from log shippers that split streams). Routing failed events toconsole.errorkeeps console output severity-faithful with what's already going to Sentry.Also applies to: 166-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/utils/logger.ts` around lines 133 - 135, The logger currently sends every event to stdout via console.log inside the shouldLog() block; change it so that when the eventName endsWith('.failed') it is routed to stderr using console.error instead of console.log (use the eventName and fullPayload variables to build the message), and leave non-failure events using console.log; apply the same change to the other occurrence that similarly uses shouldLog() with eventName/fullPayload so failed events consistently go to stderr.
105-169:eventandsystemEventare near-identical twins — extract a shared emitter.Aside from the context type, the two methods march in lockstep: scrub → ingest to Axiom → conditional Sentry on
.failed→ console echo. A small private helper keyed on(eventName, base, payload, options)would collapse ~30 lines of duplication and ensure future tweaks (e.g., sampling, async ingestion, breadcrumb capture) stay in one place.♻️ Sketch of a shared emitter
+function emitEvent( + eventName: LogEventName, + base: BaseLogContext, + payload: Record<string, unknown>, + options?: SentryOptions, +): void { + const fullPayload = scrubLogPayload({ + ...base, + ...payload, + event: eventName, + timestamp: new Date().toISOString(), + }); + + if (axiom) axiom.ingest(dataset, [fullPayload]); + + if (shouldSendToSentry(eventName) && options?.sentry !== false) { + const extra = fullPayload as Record<string, unknown>; + if (options?.error instanceof Error) { + Sentry.captureException(options.error, { extra }); + } else { + Sentry.captureMessage(eventName, { level: "error", extra }); + } + } + + if (shouldLog()) console.log(`[event] ${eventName}`, fullPayload); +}(Both
eventandsystemEventthen become thin typed façades overemitEvent.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/utils/logger.ts` around lines 105 - 169, The two near-identical functions event and systemEvent should be collapsed into a single private helper (e.g., emitEvent) that takes generics for the event key, a generic base/context, payload, and SentryOptions; move the scrubLogPayload call, axiom.ingest, the shouldSendToSentry + Sentry.captureException/captureMessage logic, and the console log into that helper and have event and systemEvent become thin typed wrappers that call emitEvent with the appropriate context type so behavior and checks (shouldSendToSentry, shouldLog, options?.sentry/error) are preserved.mcpjam-inspector/server/utils/error-classify.ts (1)
17-19: Brittle substring sniffing onreadResource.Matching by
includes("readResource")couples the classifier to upstream message wording — a benign refactor of any caller that happens to mentionreadResourcein unrelated text would silently re-bucket the error. Ahintis the safer signal here; consider keeping this branch only as a last resort and documenting the assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/utils/error-classify.ts` around lines 17 - 19, The current brittle check uses error.message.includes("readResource") in the classifier; change it to first look for a structured hint (e.g., check error.hint or a custom property like error.code === "read_resource_failed" or error.hint?.includes("readResource")) inside the same function/classifier in error-classify.ts, and only fall back to the message substring check as a last resort (keep the message check but add a comment documenting the assumption and/or emit a debug log when relying on the fallback). Update references to the branch that currently reads error.message.includes("readResource") so callers relying on structured hints (hint, code) are preferred.mcpjam-inspector/server/utils/request-logger.ts (2)
50-52: Silent no-op when context is absent.
setRequestLogContextquietly returns when there is no existing context, which means a typo in middleware ordering (or a caller outside/api/*) drops the enrichment with zero signal. Alogger.debug(...)here — or, in dev, a thrown error — would make the misconfiguration self-announcing without changing prod behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/utils/request-logger.ts` around lines 50 - 52, The function setRequestLogContext currently silently returns when c.var.requestLogContext is missing; update setRequestLogContext to emit a debug log (using the existing logger) indicating the missing context and include identifying info (e.g., the partial payload and request id/path if available) so middleware-ordering mistakes are visible, and in non-production (process.env.NODE_ENV !== 'production') also throw a descriptive Error to fail fast during development; reference c.var.requestLogContext and the setRequestLogContext function so you add the logger.debug(...) before the early return and the dev-only throw right after.
17-21: Theas RequestLogContextcast hides a missing-middleware footgun.If
getRequestLogger(c, ...)is ever called from a route that wasn't covered byrequestLogContextMiddleware(the middleware is mounted only on/api/*),c.var.requestLogContextisundefined. Spreadingundefinedis a runtime no-op, so we'd merrily emit an event whosebasehas norequestId,route,method,environment, orrelease— exactly the fields downstream dashboards key on. A defensive guard surfaces the misuse loudly during development instead of poisoning telemetry silently.🛡️ Guard the missing context
- const base: RequestLogContext = { - ...(c.var.requestLogContext as RequestLogContext), - component, - }; + const ctx = c.var.requestLogContext as RequestLogContext | undefined; + if (!ctx) { + throw new Error( + "getRequestLogger called without requestLogContextMiddleware in scope", + ); + } + const base: RequestLogContext = { ...ctx, component }; logger.event(eventName, base, payload, options);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/utils/request-logger.ts` around lines 17 - 21, The code is silently spreading an undefined c.var.requestLogContext via the cast "as RequestLogContext" which masks missing request context from requestLogContextMiddleware; update getRequestLogger to assert or guard that c.var.requestLogContext is present before spreading (e.g., if (!c.var.requestLogContext) throw new Error or call a dev-only logger), remove the unsafe "as RequestLogContext" cast and narrow the type after the check, and ensure logger.event is called with a constructed base that includes component plus the validated requestLogContext so missing middleware is surfaced immediately; reference c.var.requestLogContext, getRequestLogger, requestLogContextMiddleware, RequestLogContext, and logger.event when making the change.mcpjam-inspector/server/middleware/__tests__/request-log-context.middleware.test.ts (1)
45-45:as anyundermines the typed-event contract this PR introduces.The whole point of
RequestLogContext/RequestEventMapis to let the type system catch payload drift. Replacing the casts with the actual types (or a smallpickEventCall<E>()helper) keeps the tests honest the next time a payload field is renamed or removed.Also applies to: 93-93, 102-104, 136-136, 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/middleware/__tests__/request-log-context.middleware.test.ts` at line 45, The test undermines the new typed-event contract by declaring capturedCtx as any; change the loose cast to the correct typed shape using RequestLogContext and the specific event entry from RequestEventMap (e.g., declare capturedCtx as RequestEventMap['yourEventName'] or use a small helper pickEventCall<E>() to infer the payload type), and update the other test occurrences that use "as any" (the other captured/spy variables at the noted locations) to use the concrete typed event signatures so the compiler will catch payload drift; reference RequestLogContext, RequestEventMap, and introduce pickEventCall<E>() if helpful to obtain the correct call argument type for assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/server/index.ts`:
- Around line 51-63: The structured event
"mcp.connection.closed_with_pending_requests" currently drops the original
rejection reason; update the sysLogger.event call in the isMcpConnectionClosed
branch to include the original rejection diagnostics (e.g., reason.message and
reason.name or the full reason/error object) in the event payload or
options.error so the event metadata preserves the distinguishing details; locate
the sysLogger.event invocation inside the isMcpConnectionClosed branch and add
fields like reason, reasonMessage, and reasonName (or attach the reason object)
to the second or third argument as supported by the SystemEventMap for that
event.
In
`@mcpjam-inspector/server/middleware/__tests__/request-log-context.middleware.test.ts`:
- Around line 128-140: The test currently guards with `if (completedCalls.length
> 0)` which lets it pass vacuously; instead assert the contract explicitly by
requiring `completedCalls.length` to be > 0 and then assert on the first call's
`base.route` (derived from `completedCalls[0][1]`) that it does not contain
"nonexistent" (and/or equals "unmatched" or a pattern like `/api/*`); update
assertions around the mocked `logger.event`/`completedCalls` so the test fails
if no event was emitted or if the raw URL is used as the route.
In `@mcpjam-inspector/server/utils/log-events.ts`:
- Around line 137-142: The code in resolveEnvironment() currently calls
console.warn on every invocation which both violates server logging rules and
floods requests; change this by removing console.* usage and either (a) write a
one-time message to process.stderr via process.stderr.write(...) or (b) move
environment resolution into logger.ts to avoid the import cycle, and ensure the
warning is emitted only once by memoizing the resolved value or adding a
module-level boolean (e.g., warned) so the warning is printed a single time;
reference the resolveEnvironment() function and logger.ts to locate where to
invert or adjust the dependency and implement the one-time stderr write.
In `@mcpjam-inspector/server/utils/log-scrubber.ts`:
- Around line 44-59: scrubValue currently treats any non-primitive object the
same and can both lose useful data for special objects (Error, Date, Buffer,
Map, Set, class instances) and infinitely recurse on circular structures; update
scrubValue to first guard against cycles by accepting/creating a WeakSet "seen"
and return "[circular]" when re-entering an object, then detect and handle
common non-plain types explicitly (e.g., for Error return an object with
name/message/stack, for Date return toISOString(), for Buffer return a safe
string or length, for Map/Set serialize entries to arrays, and for objects
constructed via class preserve enumerable properties) while still applying
isForbiddenKey checks and recursively scrubbing values; ensure the API
propagates the seen-tracker through recursive calls (or make a private helper
scrubValueInner(value, seen)) so cycles are prevented.
- Around line 25-30: The key-matching in isForbiddenKey is too narrow (only
exact "email" or keys that endWith("email")) and lets many email-like keys
through; update isForbiddenKey to treat any key that contains the substring
"email" (after lowercasing and after checking ALLOWLISTED_KEYS) as forbidden
(e.g., use lower.includes("email") or a regex that catches email, email_address,
emailVerified, recipient_email, etc.), and update ALLOWLISTED_KEYS to explicitly
list any legitimate exceptions so only intentional fields bypass the rule;
reference isForbiddenKey, ALLOWLISTED_KEYS, and FORBIDDEN_KEY_SUBSTRINGS when
making the change.
In `@mcpjam-inspector/server/utils/logger.ts`:
- Around line 122-131: The code path in shouldSendToSentry handling Sentry
reporting silently drops non-Error options.error values; update the block in
logger.ts (the branch that currently calls Sentry.captureException when
options.error instanceof Error and Sentry.captureMessage otherwise) to either
coerce non-Error values into an Error (e.g., new Error(String(options.error)))
before calling Sentry.captureException, or attach the original non-Error value
into the event extra payload under a known key (e.g., extra.originalError) so
the original value is sent; apply the same change to the similar branch around
the capture at the 155-164 area to ensure no non-Error error is lost.
---
Nitpick comments:
In `@mcpjam-inspector/server/app.ts`:
- Around line 156-163: The requestLogContextMiddleware is mounted after
sessionAuthMiddleware, so authentication/authorization failures are bypassing
the request logging; move the requestLogContextMiddleware to run before the
security stack by registering it prior to app.use("*", sessionAuthMiddleware)
(i.e., ensure app.use("/api/*", requestLogContextMiddleware) is called before
sessionAuthMiddleware) so http.request.completed/failed events are emitted even
when sessionAuthMiddleware or origin/host checks short-circuit.
In
`@mcpjam-inspector/server/middleware/__tests__/request-log-context.middleware.test.ts`:
- Line 45: The test undermines the new typed-event contract by declaring
capturedCtx as any; change the loose cast to the correct typed shape using
RequestLogContext and the specific event entry from RequestEventMap (e.g.,
declare capturedCtx as RequestEventMap['yourEventName'] or use a small helper
pickEventCall<E>() to infer the payload type), and update the other test
occurrences that use "as any" (the other captured/spy variables at the noted
locations) to use the concrete typed event signatures so the compiler will catch
payload drift; reference RequestLogContext, RequestEventMap, and introduce
pickEventCall<E>() if helpful to obtain the correct call argument type for
assertions.
In `@mcpjam-inspector/server/middleware/request-log-context.ts`:
- Around line 62-64: The middleware currently returns early when isStreaming(c)
is true and no error was thrown, which prevents any telemetry; instead, either
emit a dedicated stream-opened telemetry event at that branch (e.g., send an
"http.stream.opened" event with context like method, path, headers and
timestamp) or wire completion logging by attaching a close/end listener to the
streaming response/connection (use isStreaming(c) branch to register a
'close'/'finish' handler that will emit the usual http.request.completed event
with duration, status and byte counts when the stream actually ends); update the
code around isStreaming(c) and thrown so streaming routes are counted and their
completion is emitted.
- Around line 35-36: The middleware currently reads the request ID using
c.req.header and sets the response header using c.res.headers.set; replace the
low-level call with Hono's idiomatic c.header API by calling
c.header("x-request-id", requestId) after computing requestId (from
c.req.header("x-request-id") ?? randomUUID()), so the requestId variable remains
the same and response header management uses c.header rather than
c.res.headers.set.
In `@mcpjam-inspector/server/utils/error-classify.ts`:
- Around line 17-19: The current brittle check uses
error.message.includes("readResource") in the classifier; change it to first
look for a structured hint (e.g., check error.hint or a custom property like
error.code === "read_resource_failed" or error.hint?.includes("readResource"))
inside the same function/classifier in error-classify.ts, and only fall back to
the message substring check as a last resort (keep the message check but add a
comment documenting the assumption and/or emit a debug log when relying on the
fallback). Update references to the branch that currently reads
error.message.includes("readResource") so callers relying on structured hints
(hint, code) are preferred.
In `@mcpjam-inspector/server/utils/logger.ts`:
- Around line 133-135: The logger currently sends every event to stdout via
console.log inside the shouldLog() block; change it so that when the eventName
endsWith('.failed') it is routed to stderr using console.error instead of
console.log (use the eventName and fullPayload variables to build the message),
and leave non-failure events using console.log; apply the same change to the
other occurrence that similarly uses shouldLog() with eventName/fullPayload so
failed events consistently go to stderr.
- Around line 105-169: The two near-identical functions event and systemEvent
should be collapsed into a single private helper (e.g., emitEvent) that takes
generics for the event key, a generic base/context, payload, and SentryOptions;
move the scrubLogPayload call, axiom.ingest, the shouldSendToSentry +
Sentry.captureException/captureMessage logic, and the console log into that
helper and have event and systemEvent become thin typed wrappers that call
emitEvent with the appropriate context type so behavior and checks
(shouldSendToSentry, shouldLog, options?.sentry/error) are preserved.
In `@mcpjam-inspector/server/utils/request-logger.ts`:
- Around line 50-52: The function setRequestLogContext currently silently
returns when c.var.requestLogContext is missing; update setRequestLogContext to
emit a debug log (using the existing logger) indicating the missing context and
include identifying info (e.g., the partial payload and request id/path if
available) so middleware-ordering mistakes are visible, and in non-production
(process.env.NODE_ENV !== 'production') also throw a descriptive Error to fail
fast during development; reference c.var.requestLogContext and the
setRequestLogContext function so you add the logger.debug(...) before the early
return and the dev-only throw right after.
- Around line 17-21: The code is silently spreading an undefined
c.var.requestLogContext via the cast "as RequestLogContext" which masks missing
request context from requestLogContextMiddleware; update getRequestLogger to
assert or guard that c.var.requestLogContext is present before spreading (e.g.,
if (!c.var.requestLogContext) throw new Error or call a dev-only logger), remove
the unsafe "as RequestLogContext" cast and narrow the type after the check, and
ensure logger.event is called with a constructed base that includes component
plus the validated requestLogContext so missing middleware is surfaced
immediately; reference c.var.requestLogContext, getRequestLogger,
requestLogContextMiddleware, RequestLogContext, and logger.event when making the
change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d60fb94c-ecb9-41b3-9bba-0adcf8fb93f6
📒 Files selected for processing (13)
mcpjam-inspector/server/app.tsmcpjam-inspector/server/index.tsmcpjam-inspector/server/middleware/__tests__/request-log-context.middleware.test.tsmcpjam-inspector/server/middleware/request-log-context.tsmcpjam-inspector/server/types/hono.tsmcpjam-inspector/server/utils/__tests__/log-scrubber.test.tsmcpjam-inspector/server/utils/__tests__/logger.test.tsmcpjam-inspector/server/utils/__tests__/request-logger.test.tsmcpjam-inspector/server/utils/error-classify.tsmcpjam-inspector/server/utils/log-events.tsmcpjam-inspector/server/utils/log-scrubber.tsmcpjam-inspector/server/utils/logger.tsmcpjam-inspector/server/utils/request-logger.ts
Collaborator
Author
This was referenced Apr 26, 2026
Vu-John
force-pushed
the
logging-inspector-typed-foundation
branch
from
10298d1 to
b74394b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mcpjam-inspector/server/utils/logger.ts (1)
105-169: Two siblings, one soul — extract the shared envelope/emit pipeline.
eventandsystemEventare byte-for-byte identical apart from their type parameters: same scrub, same Axiom ingest, same Sentry branch, same console line. Future tweaks (e.g., the past suggestion to coerce non-Erroroptions.error, or adding rate limiting / sampling) will have to be applied in two places — and inevitably will drift.♻️ One private emitter, two thin wrappers
+function emitEvent( + eventName: LogEventName, + base: RequestLogContext | SystemLogContext, + payload: Record<string, unknown>, + options?: SentryOptions, +): void { + const fullPayload = scrubLogPayload({ + ...base, + ...payload, + event: eventName, + timestamp: new Date().toISOString(), + }); + + if (axiom) { + axiom.ingest(dataset, [fullPayload]); + } + + if (shouldSendToSentry(eventName) && options?.sentry !== false) { + const extra = fullPayload as Record<string, unknown>; + if (options?.error instanceof Error) { + Sentry.captureException(options.error, { extra }); + } else { + Sentry.captureMessage(eventName, { level: "error", extra }); + } + } + + if (shouldLog()) { + console.log(`[event] ${eventName}`, fullPayload); + } +} + event<E extends keyof RequestEventMap>( eventName: E, base: RequestLogContext, payload: RequestEventMap[E], options?: SentryOptions, ): void { - const fullPayload = scrubLogPayload({ ... }); - // ...30 lines duplicated below... + emitEvent(eventName, base, payload as Record<string, unknown>, options); }, systemEvent<E extends keyof SystemEventMap>( eventName: E, base: SystemLogContext, payload: SystemEventMap[E], options?: SentryOptions, ): void { - const fullPayload = scrubLogPayload({ ... }); - // ...same 30 lines... + emitEvent(eventName, base, payload as Record<string, unknown>, options); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/utils/logger.ts` around lines 105 - 169, event and systemEvent are identical; extract their shared logic into a single private helper (e.g., emitLog or emitEvent) that takes generic type params for the event map, plus parameters (eventName, base, payload, options) and performs scrubLogPayload, axiom.ingest, Sentry capture/extra, and console.log; then make the existing exported event and systemEvent functions thin wrappers that call this helper with the correct type arguments (RequestEventMap/SystemEventMap and RequestLogContext/SystemLogContext) so future changes apply in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/server/middleware/request-log-context.ts`:
- Around line 79-90: The middleware is causing duplicate Sentry captures when it
emits reqLogger.event("http.request.failed") and then re-throws the same error
which app.onError's appLogger.error also captures; fix this by adding a
lightweight marker to errors in the request-log middleware (set a property like
__capturedByRequestLog on thrown before calling reqLogger.event in
request-log-context.ts) and update the global error handler in server/index.ts
(the app.onError / appLogger.error path) to check that marker and skip Sentry
capture when present (log without invoking Sentry.captureException), ensuring
only one Sentry report is created.
---
Nitpick comments:
In `@mcpjam-inspector/server/utils/logger.ts`:
- Around line 105-169: event and systemEvent are identical; extract their shared
logic into a single private helper (e.g., emitLog or emitEvent) that takes
generic type params for the event map, plus parameters (eventName, base,
payload, options) and performs scrubLogPayload, axiom.ingest, Sentry
capture/extra, and console.log; then make the existing exported event and
systemEvent functions thin wrappers that call this helper with the correct type
arguments (RequestEventMap/SystemEventMap and
RequestLogContext/SystemLogContext) so future changes apply in one place.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20320502-24de-46c2-96ab-f0457b5da2b5
📒 Files selected for processing (13)
mcpjam-inspector/server/app.tsmcpjam-inspector/server/index.tsmcpjam-inspector/server/middleware/__tests__/request-log-context.middleware.test.tsmcpjam-inspector/server/middleware/request-log-context.tsmcpjam-inspector/server/types/hono.tsmcpjam-inspector/server/utils/__tests__/log-scrubber.test.tsmcpjam-inspector/server/utils/__tests__/logger.test.tsmcpjam-inspector/server/utils/__tests__/request-logger.test.tsmcpjam-inspector/server/utils/error-classify.tsmcpjam-inspector/server/utils/log-events.tsmcpjam-inspector/server/utils/log-scrubber.tsmcpjam-inspector/server/utils/logger.tsmcpjam-inspector/server/utils/request-logger.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- mcpjam-inspector/server/types/hono.ts
- mcpjam-inspector/server/app.ts
- mcpjam-inspector/server/utils/tests/log-scrubber.test.ts
- mcpjam-inspector/server/utils/request-logger.ts
- mcpjam-inspector/server/utils/log-scrubber.ts
- mcpjam-inspector/server/utils/log-events.ts
This was referenced Apr 26, 2026
Vu-John
commented
Apr 26, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mcpjam-inspector/server/middleware/request-log-context.ts (1)
14-18:/healthinHEALTH_PATHSis unreachable here.Per
server/app.ts, this middleware is mounted on/api/*, so a bare/healthrequest never traverses it. Either drop the entry to keep the set honest, or move the bypass list closer to where the middleware is registered if a top-level/healthroute exists elsewhere.🧹 Tidy-up
const HEALTH_PATHS = new Set([ "/api/mcp/health", "/api/apps/health", - "/health", ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/middleware/request-log-context.ts` around lines 14 - 18, HEALTH_PATHS contains "/health" which is unreachable because this middleware is mounted under /api/* in server/app.ts; remove the "/health" entry from the HEALTH_PATHS set to keep it accurate, or alternatively relocate the bypass list logic to where the middleware is registered in server/app.ts so it can include top-level routes—update the constant HEALTH_PATHS (or move the check) accordingly and ensure tests or route docs reflect the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/server/middleware/request-log-context.ts`:
- Around line 79-90: The middleware currently hardcodes statusCode: 500 when
thrown is truthy; instead derive the real status from the thrown error if
available (e.g., use thrown.getStatus() if it exists, or thrown.status property)
so client errors like HTTPException(400/401/404) are logged with their actual
status; update the reqLogger.event call in request-log-context.ts to compute a
thrownStatus variable (falling back to 500) and use that for statusCode while
keeping the existing classifyError(thrown) logic for errorCode.
---
Nitpick comments:
In `@mcpjam-inspector/server/middleware/request-log-context.ts`:
- Around line 14-18: HEALTH_PATHS contains "/health" which is unreachable
because this middleware is mounted under /api/* in server/app.ts; remove the
"/health" entry from the HEALTH_PATHS set to keep it accurate, or alternatively
relocate the bypass list logic to where the middleware is registered in
server/app.ts so it can include top-level routes—update the constant
HEALTH_PATHS (or move the check) accordingly and ensure tests or route docs
reflect the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d3fde7a-d23e-48ed-95a8-57fcdbc24bc9
📒 Files selected for processing (1)
mcpjam-inspector/server/middleware/request-log-context.ts
dosubot
Bot
added
size:XL
Contributor
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 `@mcpjam-inspector/server/middleware/request-log-context.ts`:
- Around line 36-37: The inbound x-request-id should be validated before echoing
or logging: replace the direct use of c.req.header("x-request-id") when
assigning requestId with a sanitizer that trims the header and accepts it only
if it matches a safe pattern (e.g., /^[A-Za-z0-9_-]{8,128}$/); if it fails
validation, generate a fresh randomUUID(); then use the validated requestId for
c.res.headers.set("x-request-id") and any logging/storage. Update the logic
around the requestId variable assignment in the request-log-context middleware
so only a validated token is reflected and emitted.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c2bfca2-e786-4d68-bf4e-59fb52cc92f6
📒 Files selected for processing (3)
mcpjam-inspector/server/middleware/request-log-context.tsmcpjam-inspector/server/utils/log-events.tsmcpjam-inspector/server/utils/log-scrubber.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/server/utils/log-scrubber.ts
Implements Inspector PR1 from LOGGING_IMPLEMENTATION_PLAN.md §8.1. Adds typed log-events, recursive log-scrubber, request-scoped context middleware (mounted only on /api/*), logger.event/systemEvent methods, and the unhandledRejection system event. No route handlers converted yet. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e extra When options.error is not an Error instance (string, object, Response), coerce it to rawError: String(...) in the captureMessage extra so it's not silently dropped during incident triage. Applies to both event() and systemEvent(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…capture Pass sentry: false when emitting http.request.failed from the middleware so app.onError remains the single Sentry capture source for thrown exceptions. Without this, every 5xx-by-exception produced two Sentry issues with overlapping fingerprints. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix HTTPException status hardcoding: derive effectiveStatus from
HTTPException.status so thrown 4xx errors emit http.request.completed
(not http.request.failed with statusCode 500)
- Memoize resolveEnvironment() and replace console.warn with
process.stderr.write so the warning fires once, not per-call
- Harden scrubValue() to handle Date (→ ISO string), Error (→ {name,
message, stack}), and Buffer (→ "[buffer]") without silently dropping
their content
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Thread a WeakSet through scrubValue recursion so circular object references return "[circular]" instead of causing a stack overflow. WeakSet is allocated once per scrubLogPayload call and shared across the full recursive traversal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Mount requestLogContextMiddleware before security stack so 401/403/410 short-circuits are observed in Axiom (previously invisible). - Emit http.stream.opened on SSE detection and http.stream.closed via a TransformStream wrapper so streaming routes generate telemetry instead of being silently dropped. - getSystemLogger now auto-fills the system envelope; callers pass only the payload. Removes the null-envelope boilerplate footgun. - Sentry capture is now opt-in (sentry: true) for all events; the prior ".failed" auto-capture caused double-capture with route error handlers. - Extract shared emit() helper to dedupe logger.event/systemEvent. - getRequestLogger throws if requestLogContext is missing, surfacing wiring bugs instead of emitting malformed envelopes. - Broaden health-path check to */health, */healthz, and trailing-slash variants; switch to idiomatic c.header() for x-request-id. - Resolve ENVIRONMENT per call (no module-load cache) for test isolation. - Route *.failed console echoes to console.error. - Convert generic unhandledRejection branch to a typed process.unhandled_rejection system event with opt-in Sentry capture. - Tests: add cycle-protection coverage for the scrubber, upstream short-circuit (4xx/5xx) coverage for the middleware, broadened health-path coverage, and stream open/close event coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Vu-John
force-pushed
the
logging-inspector-typed-foundation
branch
from
ba938fd to
6feebad
Compare
Reject inbound x-request-id values that don't match a conservative shape
(/^[A-Za-z0-9_-]{8,128}$/) to prevent log forging via control chars,
cardinality blowup from oversized values, and downstream query injection.
Falls back to a fresh UUID when the header is invalid.
Addresses CodeRabbit review feedback on PR #1930.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Vu-John
added a commit
that referenced
this pull request
Apr 26, 2026
Two CodeRabbit review fixes: 1. Declare @typescript-eslint/parser in mcpjam-inspector/devDependencies. Previously resolved only via workspace hoisting from sdk; would break if the sdk dep is bumped or this workspace is installed standalone. 2. Replace @deprecated JSDoc tag on logger.error/warn/info with a NOTE. The @deprecated tag is not territorial — it paints a strikethrough on every call site across server/CLI/tests, contradicting the route-handler-scoped policy in LOGGING.md and the ESLint rule. Keep the guidance text, drop the tag. Two CI test regressions caused by the strict-throw added to getRequestLogger in PR #1930: - chat-ingestion.test.ts constructed a Hono Context with empty `var`, causing getRequestLogger(c).event() to throw. Updated the test fixture to mount requestLogContext, mirroring real production wiring. - createWebTestApp didn't mount requestLogContextMiddleware, so any route handler that emitted typed telemetry from a catch block threw and converted real error statuses (400/502) into 500. Mounted the middleware in the helper to match production. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the typed event foundation for structured, safe, queryable logging across the inspector. Adds
logger.event()andlogger.systemEvent(), a recursive scrubber, request-scoped log context middleware (mounted only on/api/*), and theunhandledRejectionsystem event. No route handlers are converted in this PR; that work begins in Inspector PR3 once the backend authorize endpoints returninternalLogContext.Changes
server/utils/log-events.ts— typed event map,RequestLogContext,SystemLogContext,resolveEnvironment(),resolveRelease()server/utils/log-scrubber.ts— recursive scrubber that redacts forbidden keys and string patterns (tokens, emails, secrets, JWTs)server/utils/request-logger.ts—getRequestLogger(),getSystemLogger(),setRequestLogContext()helpersserver/utils/error-classify.ts—classifyError(),classifyWidgetError(),classifyTunnelError()helpersserver/middleware/request-log-context.ts— auto-emit middleware forhttp.request.completed/failed, skips health endpoints and SSE streamsserver/utils/logger.ts— addedlogger.event()andlogger.systemEvent()with Axiom + Sentry routingserver/types/hono.ts— addedrequestLogContext?: RequestLogContexttoContextVariableMapserver/app.ts— mountrequestLogContextMiddlewareon/api/*onlyserver/index.ts— replace MCP-closedunhandledRejectionbranch with typed system eventlog-scrubber.test.ts,request-logger.test.ts,request-log-context.middleware.test.tslogger.test.tswithlogger.eventandlogger.systemEventcoverageOut of scope
internalLogContextconsumption (PR2)logger.error/warn/info/debugcallsitesTest plan
Automated
log-scrubber.test.ts— forbidden keys, allowlistedemailDomain, recursion, string-pattern scrubbing (18 tests)request-logger.test.ts—getRequestLogger,setRequestLogContext,getSystemLoggershape (4 tests)request-log-context.middleware.test.ts— happy path, 5xx, health skip, SSE skip, exception re-throw, x-request-id header (10 tests)logger.test.ts(extended) —logger.eventAxiom shape, Sentry routing,logger.systemEventsystem envelope (6 new tests)Manual
x-request-idappears in response headershttp.request.failedis emitted in Axiommcp.connection.closed_with_pending_requestsis emitted withauthType: "system"Risk and rollback
/api/*request; scrubber is called on every eventCross-repo dependencies
logging-inspector-authorize-wrapper)