fix(logging): never write logs to stdout in stdio mode - #571
Conversation
- LOG_JSON=true previously created a bare pino instance that wrote NDJSON log lines to stdout, corrupting the MCP JSON-RPC channel in stdio transport mode; logs are now forced to stderr whenever the server runs on stdio, for both plain and JSON formats - add LOG_DESTINATION=stdout|stderr override for HTTP mode; defaults keep the container convention (pretty to stderr, NDJSON to stdout) - fix LOG_FORMAT rendering: messageFormat crossed the worker-thread transport boundary, duplicating the prefix and printing 'undefined' for the level; format tokens now drive field visibility via ignore list only - document LOG_FORMAT and LOG_DESTINATION plus a destination matrix in the configuration guide Closes #563
Applies every pending Dependabot update in one pass: - @cloudflare/workers-types 4.20260623.1 -> 5.20260808.1 (major) - vitepress 2.0.0-alpha.17 -> 2.0.0-alpha.19 - undici ^8.5.0 -> ^8.10.0 - @modelcontextprotocol/sdk, @clack/prompts, eslint, prettier, @typescript-eslint/*, ts-jest, graphql, picomatch, xstate minor/patch - prisma + @prisma/client 7.8.0 -> 7.9.1 - nx 23.0.0 -> 23.1.1 - lockfile-only transitive bumps: hono 4.13.1, postcss 8.5.26, ip-address 10.4.0, brace-expansion 1.1.18, js-yaml 3.15.1, fast-uri 3.1.5, tar 7.5.22, body-parser 2.3.0 typescript 7 and @types/node 26 majors are intentionally skipped: @types/node major is on the Dependabot ignore list and the native tsc migration is a separate effort. Supersedes Dependabot PRs #543, #544, #545, #546, #551, #556, #557, #559, #560, #561, #566, #568, #569, #570
The list_jobs statuses filter became an array of CiJobStatus in c1b37f9, but the integration schema test still passed a bare string and failed against the current schema.
prettier 3.9 collapses short union types onto one line; reformat the 9 affected files so lint passes on the updated toolchain.
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request routes logger output by transport, updates logging documentation and tests, upgrades workflow actions and package dependencies, and applies formatting-only changes plus a runner schema test fixture update. ChangesLogging and maintenance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant MCPTransport
participant Logger
participant OutputStream
MCPTransport->>Logger: select stdio or HTTP mode
Logger->>Logger: resolve LOG_DESTINATION and LOG_JSON
Logger->>OutputStream: write logs to stdout or stderr
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Test Coverage ReportOverall Coverage: 96.94%
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gitlab-mcp/tests/unit/logger.test.ts`:
- Around line 502-515: Extend the logger tests near the existing
pretty-transport case with an HTTP plain-text scenario: set PORT, leave LOG_JSON
unset, and set LOG_DESTINATION to stdout, then import the logger and assert the
pino transport destination is 1. Reuse the existing pino mock and environment
cleanup patterns.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dd36fd0d-ff93-46f3-9143-a820cea72b25
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
.github/workflows/ci-cd.yml.github/workflows/codeql.yml.github/workflows/coverage-pages.yml.github/workflows/docs.yml.github/workflows/pr-validation.yml.github/workflows/release-please.yml.github/workflows/upstream-monitor.ymlpackage.jsonpackages/gitlab-mcp-db/package.jsonpackages/gitlab-mcp/docs/guide/configuration.mdpackages/gitlab-mcp/package.jsonpackages/gitlab-mcp/src/cli/docker/types.tspackages/gitlab-mcp/src/entities/context/types.tspackages/gitlab-mcp/src/entities/core/index.tspackages/gitlab-mcp/src/entities/files/index.tspackages/gitlab-mcp/src/entities/mrs/index.tspackages/gitlab-mcp/src/entities/wiki/index.tspackages/gitlab-mcp/src/graphql/workItems.tspackages/gitlab-mcp/src/logger.tspackages/gitlab-mcp/src/services/TokenScopeDetector.tspackages/gitlab-mcp/tests/integration/helpers/registry-helper.tspackages/gitlab-mcp/tests/integration/schemas/runners.test.tspackages/gitlab-mcp/tests/unit/logger.test.ts
![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: 36fe5bdf90
ℹ️ 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_FORMAT tokens toggle field visibility; the line always renders in pino-pretty's fixed order, so stop advertising custom layouts in the docstring and configuration guide - add unit test for LOG_DESTINATION=stdout with the pretty transport in HTTP mode - drop issue references from code comments and test names; ticket numbers belong in commits and PRs, not in source
Startup 'Failed to parse JSONRPC message' bursts mean an outdated build (stale npx cache) logging to stdout; document the symptom, cause, and cache-refresh fix with a quick-diagnosis row.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfc89b9639
ℹ️ 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".
The %msg token cannot suppress the message: a log line without its message is useless, so only %time, %level, and %name act as visibility switches. Make the docstring and configuration guide say so explicitly instead of implying %msg is toggleable.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Logging fix (closes #563)
LOG_JSON=truepreviously created a bare pino instance writing NDJSON to stdout, which corrupts the MCP JSON-RPC channel in stdio transport mode. Logs are now forced to stderr whenever the server runs on stdio, for both plain and JSON formats.LOG_DESTINATION=stdout|stderroverride for HTTP mode. Defaults keep the container convention: pretty to stderr, NDJSON to stdout. In stdio mode the override is ignored with a warning.LOG_FORMATrendering: the messageFormat template crossed the worker-thread transport boundary, so the standard prefix was rendered twice and{levelLabel}printed asundefined. Format tokens now drive field visibility via the ignore list only.LOG_FORMAT(visibility switches, fixed line order),LOG_DESTINATION, and a transport/destination matrix in the configuration guide; added a troubleshooting entry for the "Failed to parse JSONRPC message" startup spam (outdated build from a stale npx cache).Note for #563 context: the default (plain) mode already wrote to stderr since the pino migration; the reporter's colored multi-line output on stdout matches a pre-9.x release (stale npx cache). The remaining real leak was the
LOG_JSONpath fixed here.Dependency updates (supersedes all open Dependabot PRs)
@cloudflare/workers-types4 -> 5 (major), vitepress alpha.19, undici 8.10, MCP SDK 1.30, eslint/prettier/ts-eslint/ts-jest/graphql/picomatch/xstate/clack minor+patch, prisma 7.9.1, nx 23.1.1Test fix
runnersintegration schema test still passedstatusesas a bare string after c1b37f9 switched the filter to an array; updated to match the schema.Testing
Closes #563
Summary by CodeRabbit
New Features
Documentation
Tests
Maintenance