[Draft] Enhance debug settings with dual-purpose flag and granular control#2908
[Draft] Enhance debug settings with dual-purpose flag and granular control#2908
Conversation
Add starlette_debug setting for Starlette-specific debug control and enhance the debug flag to set both log level and Starlette debug mode. - Add starlette_debug setting for granular control over Starlette debug mode - Update debug flag to set log_level=DEBUG and enable Starlette debug - Update HTTP app creators to accept both debug and starlette_debug parameters - Document both settings in server.mdx with clear usage guidelines - Add comprehensive tests for new settings behavior - Update existing test to reflect new debug behavior Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
strawgate
changed the title
marvin-context-protocol
bot
added
enhancement
Test Failure AnalysisSummary: The static analysis workflow failed due to an unused variable on line 47 in the new test file . Root Cause: In the test , the variable is assigned on line 47 but never used afterward. The test creates a instance with and , which triggers logging reconfiguration as a side effect during initialization, but ruff's linter (F841) correctly identifies that the variable itself is never referenced. Suggested Solution: Add an underscore prefix to indicate the variable is intentionally unused: def test_debug_reconfigures_logging(self):
"""Test that enabling debug reconfigures logging."""
# Create a settings instance with debug enabled
_settings = Settings(debug=True, log_enabled=True)
# Verify logging was reconfigured
logger = get_logger("test")
assert logger.getEffectiveLevel() == logging.DEBUGThis follows the Python convention of prefixing unused variables with underscore to signal they're intentionally unused (in this case, the initialization side effect is what matters). Detailed AnalysisThe ruff linter reported: The hook auto-fixed 3 other issues (removing unused imports) but this one requires manual intervention or the flag. Related Files
|
Test Failure AnalysisSummary: The static analysis workflow failed due to an unused variable Root Cause: In the test Suggested Solution: Add an underscore prefix to indicate the variable is intentionally unused: def test_debug_reconfigures_logging(self):
"""Test that enabling debug reconfigures logging."""
# Create a settings instance with debug enabled
_settings = Settings(debug=True, log_enabled=True)
# Verify logging was reconfigured
logger = get_logger("test")
assert logger.getEffectiveLevel() == logging.DEBUGThis follows the Python convention of prefixing unused variables with underscore to signal they're intentionally unused (in this case, the initialization side effect is what matters). Detailed AnalysisThe ruff linter reported: The hook auto-fixed 3 other issues (removing unused imports) but this one requires manual intervention or the Related Files
|
![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: b2fbe1a1bb
ℹ️ 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".
src/fastmcp/settings.py
Outdated
| if self.log_enabled: | ||
| from fastmcp.utilities.logging import configure_logging | ||
|
|
||
| configure_logging( | ||
| level=self.log_level, |
There was a problem hiding this comment.
When FASTMCP_DEBUG=true, Settings() is constructed during fastmcp/__init__.py before the module-level settings attribute is assigned. model_post_init calls configure_logging(...) here, but configure_logging dereferences fastmcp.settings (see fastmcp/utilities/logging.py:44), which is still unset at this point. That can raise AttributeError and prevent import fastmcp whenever debug is enabled via env. Consider deferring logging configuration until after fastmcp.settings is bound (e.g., keep it in __init__.py) or guard inside configure_logging against missing fastmcp.settings.
Useful? React with 👍 / 👎.
WalkthroughThis PR introduces a new Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
docs/servers/server.mdx
Outdated
| - **`debug`**: Global debug mode that sets log level to DEBUG and enables Starlette debug tracebacks for HTTP/SSE transports, set with `FASTMCP_DEBUG`. Provides a convenient way to enable comprehensive debugging. For granular control, use `log_level` and `starlette_debug` separately | ||
| - **`starlette_debug`**: Enable Starlette debug mode for HTTP/SSE transports only, set with `FASTMCP_STARLETTE_DEBUG`. When enabled, detailed error tracebacks are returned in HTTP responses. Only affects HTTP/SSE transports; has no effect on stdio transport |
There was a problem hiding this comment.
Add a production-safety warning for debug tracebacks.
These options expose detailed tracebacks in HTTP responses; please add a clear warning that you should only enable them in development to avoid leaking internals. As per coding guidelines.
src/fastmcp/settings.py
Outdated
| @field_validator("debug") | ||
| @classmethod | ||
| def _update_log_level_for_debug(cls, v: bool, info) -> bool: | ||
| """When debug is enabled, set log_level to DEBUG.""" | ||
| if v: | ||
| # When debug is True, we need to ensure log_level is set to DEBUG | ||
| # This is checked in model_post_init | ||
| pass | ||
| return v | ||
|
|
||
| def model_post_init(self, __context) -> None: | ||
| """Post-initialization hook to handle debug mode.""" | ||
| if self.debug and self.log_level != "DEBUG": | ||
| # When debug is enabled, force log_level to DEBUG | ||
| self.log_level = "DEBUG" | ||
| # Reconfigure logging if it's enabled | ||
| if self.log_enabled: | ||
| from fastmcp.utilities.logging import configure_logging | ||
|
|
||
| configure_logging( | ||
| level=self.log_level, | ||
| enable_rich_tracebacks=self.enable_rich_tracebacks, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the relevant lines
cat -n src/fastmcp/settings.py | sed -n '285,320p'Repository: jlowin/fastmcp
Length of output: 1531
🏁 Script executed:
# Check imports at the top of the file to see what's available
head -50 src/fastmcp/settings.pyRepository: jlowin/fastmcp
Length of output: 1237
🏁 Script executed:
# Search for the _update_log_level_for_debug method to see its exact current state
rg -A 15 "_update_log_level_for_debug" src/fastmcp/settings.pyRepository: jlowin/fastmcp
Length of output: 777
🏁 Script executed:
# Check if Any is imported and the overall type annotation coverage
rg "from typing import|import typing|from __future__ import annotations" src/fastmcp/settings.pyRepository: jlowin/fastmcp
Length of output: 151
Remove the unused info parameter and add missing type annotation for __context.
The _update_log_level_for_debug validator is a no-op and triggers Ruff ARG003 due to the unused info parameter. Additionally, model_post_init lacks a type annotation for __context, violating the full-annotations guideline. Simplify the validator by removing the unused parameter and add Any as the type for __context.
🛠️ Proposed fix
- `@field_validator`("debug")
- `@classmethod`
- def _update_log_level_for_debug(cls, v: bool, info) -> bool:
- """When debug is enabled, set log_level to DEBUG."""
- if v:
- # When debug is True, we need to ensure log_level is set to DEBUG
- # This is checked in model_post_init
- pass
- return v
+ `@field_validator`("debug")
+ `@classmethod`
+ def _update_log_level_for_debug(cls, v: bool) -> bool:
+ """When debug is enabled, set log_level to DEBUG."""
+ return v
@@
- def model_post_init(self, __context) -> None:
+ def model_post_init(self, __context: Any) -> None:🧰 Tools
🪛 Ruff (0.14.13)
293-293: Unused class method argument: info
(ARG003)
|
/marvin can you address feedback and fix the failing static analysis checks |
|
Claude finished @strawgate's task in 4m 31s —— View job Addressed Feedback and Fixed Static Analysis ✅All issues identified in the reviews have been fixed:
Changes Madesrc/fastmcp/settings.py:
tests/test_debug_settings.py:
docs/servers/server.mdx:
All static analysis checks now pass and the full test suite runs successfully. --- | |
- Fix unused variable in test (settings → _settings) - Add type annotation for __context parameter - Remove unused info parameter from validator - Fix circular import with configure_logging - Add production safety warnings to documentation Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
|
I think the starlette debug thing is an accident (or LLM enthusiasm) -- I personally mainly use this feature just to have a way of detecting if I'm in a debugging session, not for any FastMCP functionality. |
|
@jlowin should we just remove the debug flag then? Or what do you think we should do here |
2 participants
The existing
debugsetting was confusing because it only affected Starlette's debug tracebacks for HTTP/SSE transports, not FastMCP's logging. Users naturally expectedFASTMCP_DEBUGto enable FastMCP debugging, leading to confusion.This PR implements a dual approach:
The
debugflag now does BOTH:log_levelto DEBUGNew
starlette_debugflag for granular control:This provides both convenience (debug) and precision (starlette_debug).
Example usage:
Closes #2860
Generated with Claude Code