Merged
Conversation
* fix: added build check workflow * chore: typo fixes
📝 WalkthroughWalkthroughThe pull request refactors the application's logging infrastructure to use JSON-formatted output. It replaces FastMCP's logger with Python's standard Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
Prashant-Surya
force-pushed
the
chore/logs
branch
from
3c3bb29 to
1aaec61
Compare
![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
🧹 Nitpick comments (3)
plane_mcp/__main__.py (2)
37-52: Module-level logging configuration runs on import.Calling
configure_json_logging()at module level (line 52) means the logging configuration is applied whenever this module is imported, not just whenmain()is called. This could affect tests or other code that imports from this module.Consider moving the call inside
main()or guarding it:Alternative: configure inside main()
-configure_json_logging() - -logger = logging.getLogger("fastmcp.plane_mcp") +logger = logging.getLogger("fastmcp.plane_mcp") class ServerMode(Enum): ... def main() -> None: """Run the MCP server.""" + configure_json_logging() + server_mode = ServerMode.STDIO🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/__main__.py` around lines 37 - 52, The module currently calls configure_json_logging() at import time which alters the "fastmcp" logger for all importers; move the configure_json_logging() invocation into the main() entry point (or wrap it behind an if __name__ == "__main__": guard) so the logging mutation only happens when the program is executed, not when the module is imported; update references to configure_json_logging and ensure main() invokes it before any FastMCP activity so behavior remains unchanged when run.
19-34: Consider including full stack trace in error logging.The
JSONFormattercaptures only the exception type and message but omits the traceback. For debugging production issues, the full stack trace is often essential.Proposed enhancement to include traceback
+import traceback + class JSONFormatter(logging.Formatter): """JSON log formatter for structured logging (Datadog, ELK, etc.).""" def format(self, record: logging.LogRecord) -> str: log_entry = { "timestamp": datetime.fromtimestamp(record.created, tz=timezone.utc).isoformat(), "level": record.levelname, "logger": record.name, "message": record.getMessage(), } if record.exc_info and record.exc_info[1]: log_entry["error"] = { "type": type(record.exc_info[1]).__name__, "message": str(record.exc_info[1]), + "traceback": traceback.format_exception(*record.exc_info), } return json.dumps(log_entry)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/__main__.py` around lines 19 - 34, The JSONFormatter.format currently stores only exception type and message from record.exc_info; update JSONFormatter.format to include the full traceback by formatting record.exc_info (e.g., call self.formatException(record.exc_info) or use traceback.format_exception) and add that string as an additional field (e.g., log_entry["error"]["traceback"] or "stack") inside the error object; ensure you only attempt to format when record.exc_info is present and that the traceback content is JSON-serializable (convert to a single string)..github/workflows/build_check.yml (1)
14-24: Missing Node.js version setup step.The workflow relies on the runner's default Node.js version, which can vary and cause inconsistent builds. Add a
setup-nodeaction to pin the Node.js version.Proposed fix
- name: Checkout code uses: actions/checkout@v4 - with: - fetch-depth: 0 + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: '20' + cache: 'npm' - name: Install dependencies run: npm ciAlso,
fetch-depth: 0fetches the entire git history, which is unnecessary for a build check and slows down checkout. Remove it unless you specifically need commit history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_check.yml around lines 14 - 24, Add a setup-node step before "Install dependencies" to pin the Node.js version (e.g. use actions/setup-node@v4 with node-version set to your project's supported version or package.json engines) so builds are consistent, and remove the unnecessary fetch-depth: 0 from the "Checkout code" step to avoid fetching full git history during the build check; update the workflow to place the setup-node step between the "Checkout code" and "Install dependencies" steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plane_mcp/server.py`:
- Line 61: The middleware call
oauth_mcp.add_middleware(StructuredLoggingMiddleware(include_payloads=True))
currently logs full tool request/response payloads which may expose sensitive
workspace data; change this to make include_payloads configurable (eg. read a
boolean from an environment variable or app config) and default it to False, or
set include_payloads=False directly if payload logging is not permitted; update
initialization of StructuredLoggingMiddleware and any related config loading so
the value is controlled via env/config and add a short config comment or
docstring next to the oauth_mcp.add_middleware call mentioning the security
implications.
---
Nitpick comments:
In @.github/workflows/build_check.yml:
- Around line 14-24: Add a setup-node step before "Install dependencies" to pin
the Node.js version (e.g. use actions/setup-node@v4 with node-version set to
your project's supported version or package.json engines) so builds are
consistent, and remove the unnecessary fetch-depth: 0 from the "Checkout code"
step to avoid fetching full git history during the build check; update the
workflow to place the setup-node step between the "Checkout code" and "Install
dependencies" steps.
In `@plane_mcp/__main__.py`:
- Around line 37-52: The module currently calls configure_json_logging() at
import time which alters the "fastmcp" logger for all importers; move the
configure_json_logging() invocation into the main() entry point (or wrap it
behind an if __name__ == "__main__": guard) so the logging mutation only happens
when the program is executed, not when the module is imported; update references
to configure_json_logging and ensure main() invokes it before any FastMCP
activity so behavior remains unchanged when run.
- Around line 19-34: The JSONFormatter.format currently stores only exception
type and message from record.exc_info; update JSONFormatter.format to include
the full traceback by formatting record.exc_info (e.g., call
self.formatException(record.exc_info) or use traceback.format_exception) and add
that string as an additional field (e.g., log_entry["error"]["traceback"] or
"stack") inside the error object; ensure you only attempt to format when
record.exc_info is present and that the traceback content is JSON-serializable
(convert to a single string).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd7fd7f1-9e24-49af-b428-bae09b8a7d91
📒 Files selected for processing (4)
.github/workflows/build_check.ymlplane_mcp/__main__.pyplane_mcp/server.pypyproject.toml
| ], | ||
| ), | ||
| ) | ||
| oauth_mcp.add_middleware(StructuredLoggingMiddleware(include_payloads=True)) |
There was a problem hiding this comment.
Consider security implications of include_payloads=True.
Enabling include_payloads=True logs tool request and response payloads. While tokens are stored in request context (not tool arguments), tool payloads may still contain sensitive workspace data such as issue content, project names, or user information. Verify this aligns with your data handling policies.
If sensitive data exposure in logs is a concern, consider:
- Setting
include_payloads=Falseor making it configurable via environment variable - Ensuring log destinations have appropriate access controls
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plane_mcp/server.py` at line 61, The middleware call
oauth_mcp.add_middleware(StructuredLoggingMiddleware(include_payloads=True))
currently logs full tool request/response payloads which may expose sensitive
workspace data; change this to make include_payloads configurable (eg. read a
boolean from an environment variable or app config) and default it to False, or
set include_payloads=False directly if payload logging is not permitted; update
initialization of StructuredLoggingMiddleware and any related config loading so
the value is controlled via env/config and add a short config comment or
docstring next to the oauth_mcp.add_middleware call mentioning the security
implications.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plane_mcp/__main__.py (1)
19-34: Consider including full stack trace in error logging.The
JSONFormattercaptures exception type and message but omits the full traceback. For debugging production issues, the stack trace is often essential.♻️ Proposed enhancement to include traceback
+import traceback + class JSONFormatter(logging.Formatter): """JSON log formatter for structured logging (Datadog, ELK, etc.).""" def format(self, record: logging.LogRecord) -> str: log_entry = { "timestamp": datetime.fromtimestamp(record.created, tz=timezone.utc).isoformat(), "level": record.levelname, "logger": record.name, "message": record.getMessage(), } if record.exc_info and record.exc_info[1]: log_entry["error"] = { "type": type(record.exc_info[1]).__name__, "message": str(record.exc_info[1]), + "traceback": traceback.format_exception(*record.exc_info), } return json.dumps(log_entry)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane_mcp/__main__.py` around lines 19 - 34, The JSONFormatter currently records exception type and message but not the full stack trace; update JSONFormatter.format to capture the traceback when record.exc_info is present (use the record.exc_info tuple with traceback.format_exception or traceback.format_exception_only/format_exception to produce a single string) and add it to the log_entry under error (e.g. error["traceback"]) so structured logs include the full stack trace; ensure you import and use the traceback module and fall back safely if exc_info is absent (optionally include record.stack_info if available).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plane_mcp/__main__.py`:
- Around line 37-54: The configure_json_logging function only sets up the
"fastmcp" logger, leaving "plane_mcp.*" loggers with different formatting;
update configure_json_logging to also configure the "plane_mcp" logger hierarchy
(or accept a list of logger names) by creating the same
StreamHandler/JSONFormatter, removing existing handlers, setting level to
logging.INFO and propagate=False for logging.getLogger("plane_mcp") so that
plane_mcp.server, plane_mcp.client and plane_mcp.auth.* use the same JSON
output; ensure the module-level logger = logging.getLogger("fastmcp.plane_mcp")
remains consistent with this configuration.
---
Nitpick comments:
In `@plane_mcp/__main__.py`:
- Around line 19-34: The JSONFormatter currently records exception type and
message but not the full stack trace; update JSONFormatter.format to capture the
traceback when record.exc_info is present (use the record.exc_info tuple with
traceback.format_exception or traceback.format_exception_only/format_exception
to produce a single string) and add it to the log_entry under error (e.g.
error["traceback"]) so structured logs include the full stack trace; ensure you
import and use the traceback module and fall back safely if exc_info is absent
(optionally include record.stack_info if available).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efa9f8a8-9598-4585-868e-56303c6c573d
📒 Files selected for processing (3)
plane_mcp/__main__.pyplane_mcp/server.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
Comment on lines
+37
to
+54
| def configure_json_logging(): | ||
| """Replace FastMCP's Rich handlers with a JSON formatter on the fastmcp logger.""" | ||
| fastmcp_logger = logging.getLogger("fastmcp") | ||
|
|
||
| # Remove all existing handlers (Rich) | ||
| for handler in fastmcp_logger.handlers[:]: | ||
| fastmcp_logger.removeHandler(handler) | ||
|
|
||
| handler = logging.StreamHandler(sys.stderr) | ||
| handler.setFormatter(JSONFormatter()) | ||
| fastmcp_logger.addHandler(handler) | ||
| fastmcp_logger.setLevel(logging.INFO) | ||
| fastmcp_logger.propagate = False | ||
|
|
||
|
|
||
| configure_json_logging() | ||
|
|
||
| logger = logging.getLogger("fastmcp.plane_mcp") |
There was a problem hiding this comment.
Inconsistent logging configuration across logger hierarchies.
configure_json_logging() only configures the "fastmcp" logger, but other modules in this codebase use loggers under the "plane_mcp" hierarchy:
plane_mcp/server.pyuseslogging.getLogger(__name__)→"plane_mcp.server"plane_mcp/auth/usesget_logger(__name__)→"plane_mcp.auth.*"plane_mcp/client.pyusesget_logger(__name__)→"plane_mcp.client"
These loggers are not children of "fastmcp", so they won't use the JSON formatter and will output in a different format (or propagate to root with default formatting).
Consider also configuring the "plane_mcp" logger hierarchy for consistent JSON output:
🔧 Proposed fix for consistent logging
def configure_json_logging():
"""Replace FastMCP's Rich handlers with a JSON formatter on the fastmcp logger."""
- fastmcp_logger = logging.getLogger("fastmcp")
-
- # Remove all existing handlers (Rich)
- for handler in fastmcp_logger.handlers[:]:
- fastmcp_logger.removeHandler(handler)
-
- handler = logging.StreamHandler(sys.stderr)
- handler.setFormatter(JSONFormatter())
- fastmcp_logger.addHandler(handler)
- fastmcp_logger.setLevel(logging.INFO)
- fastmcp_logger.propagate = False
+ json_handler = logging.StreamHandler(sys.stderr)
+ json_handler.setFormatter(JSONFormatter())
+
+ for logger_name in ("fastmcp", "plane_mcp"):
+ target_logger = logging.getLogger(logger_name)
+ for h in target_logger.handlers[:]:
+ target_logger.removeHandler(h)
+ target_logger.addHandler(json_handler)
+ target_logger.setLevel(logging.INFO)
+ target_logger.propagate = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def configure_json_logging(): | |
| """Replace FastMCP's Rich handlers with a JSON formatter on the fastmcp logger.""" | |
| fastmcp_logger = logging.getLogger("fastmcp") | |
| # Remove all existing handlers (Rich) | |
| for handler in fastmcp_logger.handlers[:]: | |
| fastmcp_logger.removeHandler(handler) | |
| handler = logging.StreamHandler(sys.stderr) | |
| handler.setFormatter(JSONFormatter()) | |
| fastmcp_logger.addHandler(handler) | |
| fastmcp_logger.setLevel(logging.INFO) | |
| fastmcp_logger.propagate = False | |
| configure_json_logging() | |
| logger = logging.getLogger("fastmcp.plane_mcp") | |
| def configure_json_logging(): | |
| """Replace FastMCP's Rich handlers with a JSON formatter on the fastmcp logger.""" | |
| json_handler = logging.StreamHandler(sys.stderr) | |
| json_handler.setFormatter(JSONFormatter()) | |
| for logger_name in ("fastmcp", "plane_mcp"): | |
| target_logger = logging.getLogger(logger_name) | |
| for h in target_logger.handlers[:]: | |
| target_logger.removeHandler(h) | |
| target_logger.addHandler(json_handler) | |
| target_logger.setLevel(logging.INFO) | |
| target_logger.propagate = False | |
| configure_json_logging() | |
| logger = logging.getLogger("fastmcp.plane_mcp") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plane_mcp/__main__.py` around lines 37 - 54, The configure_json_logging
function only sets up the "fastmcp" logger, leaving "plane_mcp.*" loggers with
different formatting; update configure_json_logging to also configure the
"plane_mcp" logger hierarchy (or accept a list of logger names) by creating the
same StreamHandler/JSONFormatter, removing existing handlers, setting level to
logging.INFO and propagate=False for logging.getLogger("plane_mcp") so that
plane_mcp.server, plane_mcp.client and plane_mcp.auth.* use the same JSON
output; ensure the module-level logger = logging.getLogger("fastmcp.plane_mcp")
remains consistent with this configuration.
Saurabhkmr98
approved these changes
Mar 20, 2026
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
3 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.
Description
Type of Change
Testing Details
Summary by CodeRabbit
New Features
Chores