feat(observability): structured pino logger for stdio MCP servers - #185
Merged
Conversation
Five of the stdio MCP servers logged via raw console.error (no structure, no
deliveryId correlation, no secret redaction); a console.error(err) on an
Octokit RequestError could dump a ghs_ token verbatim. Only resolve-review-thread
used pino, and via an inline instance.
- Extract the redaction primitives (REDACT_PATHS, errSerializer, scrubStructured,
...) verbatim from src/logger.ts into a new config-free src/utils/log-redaction.ts.
logger.ts imports + re-exports REDACT_PATHS/errSerializer (back-compat). The
config-free split is load-bearing: MCP subprocesses can't import logger.ts
(it reads config at module load).
- Add src/mcp/mcp-logger.ts createMcpLogger(serverName): a stderr pino logger
with the same REDACT_PATHS + errSerializer, binding server + inherited
deliveryId. Redaction parity without pulling in config.
- Convert all 6 servers (comment, inline-comment, github-state, repo-memory,
merge-readiness, resolve-review-thread) to createMcpLogger; secret_redacted
blobs become structured log.warn events; raw console.error(err) becomes a
redacted log.error({ err }).
- registry.ts: DELIVERY_ID moved into sharedEnv so every server inherits it.
- executor.ts: LOG_LEVEL added to the subprocess env allowlist so
LOG_LEVEL=debug propagates to the CLI + MCP subprocesses.
- Tests (mcp-logger bindings/redaction/stderr; resolve-review-thread retargeted)
+ docs (observability.md MCP section, CLAUDE.md note).
Volume reuses LOG_LEVEL, no new env. Pre-existing resolve-review-thread config
coupling via retry.ts is tracked as follow-up #184.
Closes #172
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 32 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
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 |
There was a problem hiding this comment.
Pull request overview
Refactors stdio MCP servers from raw console.error to a shared structured pino logger (createMcpLogger) that writes to stderr and applies the same REDACT_PATHS + errSerializer scrubbing as the main daemon logger. The redaction primitives are extracted verbatim from src/logger.ts into a new config-free src/utils/log-redaction.ts so MCP subprocesses can import them without pulling in config validation. DELIVERY_ID is moved into sharedEnv so every server inherits it for log correlation, and LOG_LEVEL is added to the executor env allowlist so verbosity propagates through the CLI to MCP subprocesses.
Changes:
- New
src/utils/log-redaction.ts(extracted) andsrc/mcp/mcp-logger.tshelper;src/logger.tsre-exports for back-compat. - All 6 stdio MCP servers converted to
createMcpLogger; rawconsole.error(err)→ redactedlog.error({ err }); ad-hocJSON.stringify({...})→ structuredlog.warn. - Registry promotes
DELIVERY_IDintosharedEnv; executor allowlistsLOG_LEVEL; new tests for logger bindings, redaction parity, stderr-only output, andresolve-review-threadshared-helper contract; docs updated.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/log-redaction.ts |
New config-free module holding REDACT_PATHS, errSerializer, and scrubbers extracted from logger.ts. |
src/logger.ts |
Drops inlined redaction code and re-exports errSerializer/REDACT_PATHS from the new module for back-compat. |
src/mcp/mcp-logger.ts |
New createMcpLogger(serverName) helper: stderr pino with shared redaction and server/deliveryId bindings. |
src/mcp/servers/comment.ts |
Switches to createMcpLogger, structured secret_redacted event, process.exit(1) on runServer failure. |
src/mcp/servers/inline-comment.ts |
Same conversion; structured redaction event includes pull_number/path; catch now exits non-zero. |
src/mcp/servers/github-state.ts |
Routes raw err through redacting logger to avoid leaking Octokit auth headers. |
src/mcp/servers/merge-readiness.ts |
Same redacted-error logging change. |
src/mcp/servers/repo-memory.ts |
Replaces fail-fast console.error with structured log.error. |
src/mcp/servers/resolve-review-thread.ts |
Drops inline pino(...) instance in favor of shared createMcpLogger. |
src/mcp/registry.ts |
Moves DELIVERY_ID into sharedEnv; simplifies commentServerDef signature. |
src/core/executor.ts |
Adds LOG_LEVEL to env allowlist so verbosity propagates to CLI + MCP subprocesses. |
test/mcp/mcp-logger.test.ts |
New tests: bindings, redaction parity (err.message, installationToken), and stderr-only runtime check. |
test/mcp/resolve-review-thread.test.ts |
Retargets static contract to assert use of shared createMcpLogger and absence of direct pino(. |
docs/operate/observability.md |
Documents the MCP-server log fields, stderr contract, and LOG_LEVEL propagation. |
CLAUDE.md |
Updates the secret-handling invariant note to reference createMcpLogger + log-redaction.ts. |
chrisleekr
pushed a commit
that referenced
this pull request
Jun 30, 2026
# [1.14.0](v1.13.0...v1.14.0) (2026-06-30) ### Bug Fixes * **agent-sdk:** pass settingSources [] so SDK ignores cloned PR .claude/settings.json ([#195](#195)) ([153fef3](153fef3)) * **check:** derive scoped-executor scan set from filesystem (dead guard) ([#208](#208)) ([8b0e8ef](8b0e8ef)) * **daemon:** sweep full workspace triple on startup and crash exit ([#239](#239)) ([56fa714](56fa714)) * **idempotency:** gate side-effecting handlers with Valkey claim to prevent redelivery duplicates ([#212](#212)) ([68dacdb](68dacdb)) * **infrastructure:** patch krb5 HIGH CVE-2026-40356 in shared Docker base ([#180](#180)) ([1d1bc3b](1d1bc3b)) * **mcp:** redact Octokit error tool-results and widen GitHub token regex ([#238](#238)) ([675d610](675d610)) * **mcp:** wrap GitHub-touching MCP servers + state-fetchers in retryWithBackoff ([#205](#205)) ([319beb9](319beb9)) * **observability:** canonicalise child-logger entity id under entityNumber ([#178](#178)) ([808ca46](808ca46)) * **security:** gate LLM scanner redacted_body to deletion-only ([#206](#206)) ([d52cf78](d52cf78)) * **security:** set strictMcpConfig to block cloned-PR .mcp.json auto-load ([#210](#210)) ([2c58ec1](2c58ec1)) * **testing:** run colocated src/**/*.test.ts in CI + add drift guard ([#204](#204)) ([5990e0d](5990e0d)) ### Features * **agent-sdk:** block destructive Bash at runtime via PreToolUse hook ([#241](#241)) ([f3132f2](f3132f2)) * **observability:** add 12 structured Pino event families with Zod-strict schemas ([#251](#251)) ([eaad36b](eaad36b)) * **observability:** add queue_wait_ms to dispatcher offer/no-daemon logs ([#207](#207)) ([7a5cfb0](7a5cfb0)) * **observability:** add structured retry.* events ([#225](#225)) ([6713cbf](6713cbf)) * **observability:** emit failed_stage and failed_stage_delta_ms on pipeline.failed ([#244](#244)) ([4f2483c](4f2483c)) * **observability:** emit structured idempotency events on all 4 claimDelivery outcomes ([#242](#242)) ([e1e7f9e](e1e7f9e)) * **observability:** installationId on loggers + config-free MCP retry ([#189](#189)) ([ba09f76](ba09f76)), closes [#177](#177) [#184](#184) * **observability:** log + persist SDK token usage on executions ([#209](#209)) ([5407dcd](5407dcd)) * **observability:** log octokit rate-limit headers via hook.after ([#183](#183)) ([30e1715](30e1715)) * **observability:** periodic fleet-state gauge snapshot ([#186](#186)) ([7429460](7429460)) * **observability:** redact crash logs via uncaughtException/unhandledRejection handlers ([#181](#181)) ([d4248f4](d4248f4)) * **observability:** structured dispatcher + heartbeat log events ([#188](#188)) ([c615bc4](c615bc4)) * **observability:** structured pino logger for stdio MCP servers ([#185](#185)) ([5244e8a](5244e8a)) * **observability:** structured pipeline.stage timing events with delta_ms ([#182](#182)) ([4125971](4125971))
Owner
Author
|
🎉 This PR is included in version 1.14.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
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.
What
Resolves #172. Five stdio MCP servers logged via raw
console.error— no structure, nodeliveryIdcorrelation, no secret redaction. Aconsole.error(err)on an OctokitRequestError(github-state.ts,merge-readiness.ts) could dump aghs_…token verbatim. Onlyresolve-review-threadused pino, via an inline instance.Changes
REDACT_PATHS,errSerializer,scrubStructured,scrubString,scrubRequest,scrubResponse, ...) verbatim fromsrc/logger.tsinto a new config-freesrc/utils/log-redaction.ts.logger.tsimports + re-exportsREDACT_PATHS/errSerializer(back-compat). This split is load-bearing: MCP subprocesses can't importlogger.ts(it readsconfigat module load), so parity needs a config-free source.src/mcp/mcp-logger.tscreateMcpLogger(serverName): a stderr pino logger applying the sameREDACT_PATHS+errSerializer, bindingserver+ inheriteddeliveryId.createMcpLogger.secret_redactedJSON blobs → structuredlog.warnevents; rawconsole.error(err)→ redactedlog.error({ err }).registry.ts:DELIVERY_IDmoved intosharedEnvso every server inherits it (dropped the redundantcommentServerDefparam).executor.ts:LOG_LEVELadded to the subprocess env allowlist soLOG_LEVEL=debugpropagates to the CLI + MCP subprocesses.test/mcp/mcp-logger.test.ts: bindings + redaction parity + runtime stderr-vs-stdout;resolve-review-thread.test.tsretargeted to the shared helper). Docs:observability.mdMCP-server section +CLAUDE.mdredaction note.Judgement calls
configinto the subprocess and crash it). Verified the built MCP bundles contain no config-validation marker and the redaction code is inlined.LOG_LEVEL(per the feat(observability): log octokit rate-limit headers via hook.after for per-installation quota visibility #170 precedent).resolve-review-threadconfig coupling viaretry.ts → loggeris unrelated to logging and tracked as follow-up refactor(observability): decouple resolve-review-thread MCP server from config via retry.ts logger import #184 (not made worse here).Verification
typecheck / eslint (0 errors) / prettier / em-dash / docs-citations clean.
bun run build+check:mcp-bundlegreen (7 servers resolve, no config leak, redaction inlined). Tests: logger (16) + mcp (38) pass. Broad regression (mcp+core+utils+webhook) vs clean main: identical pre-existing failures, +5 passing, zero new failures. Senior-review gate: 3 independent passes, converged clean (extraction confirmed byte-identical; the two critical pino contracts verified). CodeRabbit unavailable this run (org out of credits).🤖 Generated with Claude Code