feat: Adds a new tool to use the Django Check command. Updates the list_models tool to support app filtering #10
Conversation
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
Signed-off-by: Rai Siqueira <[email protected]>
Signed-off-by: Rai Siqueira <[email protected]>
Reviewer's GuideImplements a new Django Sequence diagram for the new run_check Django system check toolsequenceDiagram
actor MCPClient
participant FastMCPServer
participant DjangoAIBoostServer
participant run_check_tool
participant DjangoChecks
MCPClient->>FastMCPServer: invoke run_check(app_labels, tags, deploy, fail_level, databases)
FastMCPServer->>DjangoAIBoostServer: dispatch tool call
DjangoAIBoostServer->>run_check_tool: run_check(app_labels, tags, deploy, fail_level, databases)
activate run_check_tool
run_check_tool->>run_check_tool: validate and normalize parameters
run_check_tool->>DjangoChecks: sync_to_async(execute_checks)
activate DjangoChecks
DjangoChecks->>DjangoChecks: map fail_level string to level constant
alt app_labels provided
DjangoChecks->>DjangoChecks: resolve app_configs from app_labels
else no app_labels
DjangoChecks->>DjangoChecks: use all app_configs
end
DjangoChecks->>DjangoChecks: run_checks(app_configs, tags, deploy, databases)
DjangoChecks-->>run_check_tool: list of check messages
deactivate DjangoChecks
run_check_tool->>run_check_tool: categorize messages by level
run_check_tool->>run_check_tool: compute summary counts and success flag
run_check_tool-->>DjangoAIBoostServer: CheckResult dict
deactivate run_check_tool
DjangoAIBoostServer-->>FastMCPServer: serialized CheckResult
FastMCPServer-->>MCPClient: tool response (JSON)
Sequence diagram for the updated list_models tool with app filteringsequenceDiagram
actor MCPClient
participant FastMCPServer
participant DjangoAIBoostServer
participant list_models_tool
participant DjangoApps
MCPClient->>FastMCPServer: invoke list_models(app_labels)
FastMCPServer->>DjangoAIBoostServer: dispatch tool call
DjangoAIBoostServer->>list_models_tool: list_models(app_labels)
activate list_models_tool
list_models_tool->>DjangoApps: apps.get_models()
DjangoApps-->>list_models_tool: all_models
alt app_labels provided
list_models_tool->>list_models_tool: filter all_models by model._meta.app_label in app_labels
else no app_labels
list_models_tool->>list_models_tool: use all_models without filtering
end
loop for each model in filtered models
list_models_tool->>list_models_tool: extract app_label, model_name, fields, relationships
list_models_tool->>list_models_tool: append model info to models_info
end
list_models_tool->>list_models_tool: build response dict {total_count, app_filter, models}
list_models_tool-->>DjangoAIBoostServer: ListModelsResponse dict
deactivate list_models_tool
DjangoAIBoostServer-->>FastMCPServer: serialized ListModelsResponse
FastMCPServer-->>MCPClient: tool response (JSON)
Updated class diagram for list_models and run_check tool response structuresclassDiagram
class ListModelsResponse {
int total_count
list~string~ app_filter
list~ModelInfo~ models
}
class ModelInfo {
string app_label
string model_name
string model_module
list~FieldInfo~ fields
list~RelationshipInfo~ relationships
}
class FieldInfo {
string name
string type
bool null
bool blank
int max_length
bool primary_key
bool unique
string related_model
}
class RelationshipInfo {
string name
string type
string related_model
bool many_to_many
bool one_to_one
bool many_to_one
bool one_to_many
}
class CheckResult {
bool success
string checked_apps
string tags
bool deploy_checks
string fail_level
int total_issues
list~CheckMessage~ critical
list~CheckMessage~ errors
list~CheckMessage~ warnings
list~CheckMessage~ info
list~CheckMessage~ debug
CheckSummary summary
string error
}
class CheckMessage {
string id
int level
string message
string hint
string obj
}
class CheckSummary {
int critical_count
int error_count
int warning_count
int info_count
int debug_count
}
ListModelsResponse "1" o-- "many" ModelInfo
ModelInfo "1" o-- "many" FieldInfo
ModelInfo "1" o-- "many" RelationshipInfo
CheckResult "1" o-- "many" CheckMessage
CheckResult "1" o-- "1" CheckSummary
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
run_check, thefail_levelsemantics don’t fully match the docstring: CRITICAL messages always flipsuccesstoFalseeven whenfail_levelis set higher, whereas the description implies the threshold should uniformly bemsg.level >= check_level; consider basingsuccesssolely on that comparison for all levels. - The
run_checkcall torun_checkspassesdatabases=databases or [], which changes Django’s default behavior of using all configured databases whenNone; prefer passingdatabasesthrough as-is soNonekeeps the framework’s default semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run_check`, the `fail_level` semantics don’t fully match the docstring: CRITICAL messages always flip `success` to `False` even when `fail_level` is set higher, whereas the description implies the threshold should uniformly be `msg.level >= check_level`; consider basing `success` solely on that comparison for all levels.
- The `run_check` call to `run_checks` passes `databases=databases or []`, which changes Django’s default behavior of using all configured databases when `None`; prefer passing `databases` through as-is so `None` keeps the framework’s default semantics.
## Individual Comments
### Comment 1
<location> `src/django_ai_boost/server_fastmcp.py:608-612` </location>
<code_context>
+ app_configs = [apps.get_app_config(label) for label in app_labels]
+
+ # Run the checks
+ messages = run_checks(
+ app_configs=app_configs,
+ tags=tags,
+ include_deployment_checks=deploy,
+ databases=databases or [],
+ )
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing `[]` instead of `None` for `databases` changes Django’s default behavior.
`run_checks` interprets `databases=None` as “all databases” and `[]` as “no databases”. Using `databases or []` means that when `databases` is `None`, no DB checks run. To keep Django’s behavior, pass `databases=databases` directly, or only normalize when it’s already a list (e.g. leave `None` as `None`).
</issue_to_address>
### Comment 2
<location> `src/django_ai_boost/server_fastmcp.py:671-673` </location>
<code_context>
+
+ return result
+
+ except LookupError as e:
+ return {"error": f"App not found: {str(e)}"}
+ except Exception as e:
+ return {"error": f"Error running checks: {str(e)}"}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align error responses with the success-path schema for easier client handling.
The success path returns a rich `result` dict, while the exception handlers return only `{"error": ...}`. This forces clients to special-case error responses if they rely on fields like `success`, `summary`, or the categorized lists. Prefer returning the same schema on errors (e.g., `{"success": False, "error": ..., "critical": [], ...}`) so clients can handle all outcomes uniformly.
Suggested implementation:
```python
except LookupError as e:
error_message = f"App not found: {str(e)}"
return {
"success": False,
"error": error_message,
"critical": [],
"errors": [error_message],
"warnings": [],
"info": [],
"debug": [],
"summary": {
"critical_count": 0,
"error_count": 1,
"warning_count": 0,
"info_count": 0,
"debug_count": 0,
},
}
except Exception as e:
error_message = f"Error running checks: {str(e)}"
return {
"success": False,
"error": error_message,
"critical": [],
"errors": [error_message],
"warnings": [],
"info": [],
"debug": [],
"summary": {
"critical_count": 0,
"error_count": 1,
"warning_count": 0,
"info_count": 0,
"debug_count": 0,
},
}
```
If the success-path `result` dict includes additional keys (e.g., an `app_label`, `checks_run`, or similar metadata), you may want to add those keys to the error responses as well (with `None`, `[]`, or other appropriate defaults) so the schema is perfectly uniform for all outcomes.
</issue_to_address>
### Comment 3
<location> `test_server.py:80` </location>
<code_context>
print("\n3. Testing list_models:")
print("-" * 60)
+ # Test 1: List all models (no filter)
+ print("\nTest 3a: List all models (no filter)")
+ all_models = apps.get_models()
</code_context>
<issue_to_address>
**issue (testing):** test_list_models now mirrors the implementation instead of exercising the MCP tool and asserting behaviour
This version recreates the expected `result_*` structures but never calls `server_fastmcp.list_models.fn` or asserts on its output, so it won’t catch regressions and can easily diverge from the real behaviour.
Instead, have this test:
- Call `server_fastmcp.list_models.fn(app_labels=...)` for the three cases (no filter, single app, multiple apps).
- Assert that the return value is a dict with `total_count`, `app_filter`, and `models`.
- Assert `total_count == len(models)`.
- Assert `app_filter` is `None` (no filter), `['blog']` (single app), and `['blog', 'auth']` (multiple apps).
- Assert the `(app, model)` pairs match those from `apps.get_models()` for each filter.
You can keep the `print` diagnostics, but the test should primarily fail when the tool’s behaviour changes unexpectedly.
</issue_to_address>
### Comment 4
<location> `test_run_check.py:21` </location>
<code_context>
+initialize_django = server_fastmcp.initialize_django
+
+
+async def test_run_check():
+ """Test the run_check tool with various parameters."""
+ print("Testing run_check tool...")
</code_context>
<issue_to_address>
**issue (testing):** test_run_check script lacks assertions and won’t fail CI if run_check regresses
Right now this only prints output and never asserts, so regressions in `run_check` wouldn’t cause the test (or CI) to fail.
If this is meant as an automated test, please add assertions on the returned dict structure and types (e.g., required keys like `"success"`, `"summary"`, `"warnings"` as a list) and verify that `checked_apps`, `tags`, `deploy_checks`, and `fail_level` reflect the parameters passed in each scenario. You may also want to split scenarios into separate test functions for clearer failures.
If it’s intended as a manual helper script instead, consider renaming it (e.g., `manual_run_check_demo.py`) so it isn’t confused with an automated test.
</issue_to_address>
### Comment 5
<location> `src/django_ai_boost/server_fastmcp.py:109` </location>
<code_context>
@mcp.tool()
-async def list_models() -> list[dict[str, Any]]:
+async def list_models(app_labels: list[str] | None = None) -> dict[str, Any]:
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `run_check` to use a top-level sync helper and centralized success/summary logic for clearer, more consistent behavior.
You can simplify `run_check` quite a bit without changing behavior by:
1. Flattening the nested sync function.
2. Centralizing success/summary computation instead of mutating `success` in each branch.
3. Keeping the error response shape consistent.
### 1. Flatten `execute_checks` into a top-level sync helper
The inner `execute_checks` doesn’t really need to be nested. You can move it out and wrap it with `sync_to_async` at the call site:
```python
from asgiref.sync import sync_to_async
def _execute_checks(
app_labels: list[str] | None,
tags: list[str] | None,
deploy: bool,
fail_level: str,
databases: list[str] | None,
) -> dict[str, Any]:
from django.core.checks import run_checks
from django.core.checks.messages import DEBUG, INFO, WARNING, ERROR, CRITICAL
try:
level_map = {
"DEBUG": DEBUG,
"INFO": INFO,
"WARNING": WARNING,
"ERROR": ERROR,
"CRITICAL": CRITICAL,
}
check_level = level_map.get(fail_level.upper(), ERROR)
app_configs = None
if app_labels:
app_configs = [apps.get_app_config(label) for label in app_labels]
messages = run_checks(
app_configs=app_configs,
tags=tags,
include_deployment_checks=deploy,
databases=databases or [],
)
result = {
"success": True,
"checked_apps": app_labels or "all",
"tags": tags or "all",
"deploy_checks": deploy,
"fail_level": fail_level,
"total_issues": len(messages),
"critical": [],
"errors": [],
"warnings": [],
"info": [],
"debug": [],
}
# Collect messages only – no success logic here
for msg in messages:
message_dict = {
"id": msg.id,
"level": msg.level,
"message": msg.msg,
"hint": msg.hint or "",
"obj": str(msg.obj) if msg.obj else "",
}
if msg.level >= CRITICAL:
result["critical"].append(message_dict)
elif msg.level >= ERROR:
result["errors"].append(message_dict)
elif msg.level >= WARNING:
result["warnings"].append(message_dict)
elif msg.level >= INFO:
result["info"].append(message_dict)
else:
result["debug"].append(message_dict)
# Centralized success + summary computation
has_failing = any(m.level >= check_level for m in messages)
result["success"] = not has_failing
result["summary"] = {
"critical_count": len(result["critical"]),
"error_count": len(result["errors"]),
"warning_count": len(result["warnings"]),
"info_count": len(result["info"]),
"debug_count": len(result["debug"]),
}
return result
except LookupError as e:
return {
"success": False,
"error": f"App not found: {str(e)}",
"checked_apps": app_labels or "all",
"tags": tags or "all",
"deploy_checks": deploy,
"fail_level": fail_level,
}
except Exception as e:
return {
"success": False,
"error": f"Error running checks: {str(e)}",
"checked_apps": app_labels or "all",
"tags": tags or "all",
"deploy_checks": deploy,
"fail_level": fail_level,
}
@mcp.tool()
async def run_check(
app_labels: list[str] | None = None,
tags: list[str] | None = None,
deploy: bool = False,
fail_level: str = "ERROR",
databases: list[str] | None = None,
) -> dict[str, Any]:
return await sync_to_async(_execute_checks)(
app_labels=app_labels,
tags=tags,
deploy=deploy,
fail_level=fail_level,
databases=databases,
)
```
Benefits:
- No nested function; the core logic is directly visible in `_execute_checks`.
- Message categorization is only about bucketing; success is computed once based on `check_level`, not mutated in many branches.
- Error paths still include `error` but now also return `success=False` and the same top-level keys (`checked_apps`, `tags`, etc.), which keeps the response shape consistent while remaining backward compatible (existing clients reading only `error` still work).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+608
to
+612
| messages = run_checks( | ||
| app_configs=app_configs, | ||
| tags=tags, | ||
| include_deployment_checks=deploy, | ||
| databases=databases or [], |
There was a problem hiding this comment.
issue (bug_risk): Passing [] instead of None for databases changes Django’s default behavior.
run_checks interprets databases=None as “all databases” and [] as “no databases”. Using databases or [] means that when databases is None, no DB checks run. To keep Django’s behavior, pass databases=databases directly, or only normalize when it’s already a list (e.g. leave None as None).
Comment on lines
+671
to
+673
| except LookupError as e: | ||
| return {"error": f"App not found: {str(e)}"} | ||
| except Exception as e: |
There was a problem hiding this comment.
suggestion (bug_risk): Align error responses with the success-path schema for easier client handling.
The success path returns a rich result dict, while the exception handlers return only {"error": ...}. This forces clients to special-case error responses if they rely on fields like success, summary, or the categorized lists. Prefer returning the same schema on errors (e.g., {"success": False, "error": ..., "critical": [], ...}) so clients can handle all outcomes uniformly.
Suggested implementation:
except LookupError as e:
error_message = f"App not found: {str(e)}"
return {
"success": False,
"error": error_message,
"critical": [],
"errors": [error_message],
"warnings": [],
"info": [],
"debug": [],
"summary": {
"critical_count": 0,
"error_count": 1,
"warning_count": 0,
"info_count": 0,
"debug_count": 0,
},
}
except Exception as e:
error_message = f"Error running checks: {str(e)}"
return {
"success": False,
"error": error_message,
"critical": [],
"errors": [error_message],
"warnings": [],
"info": [],
"debug": [],
"summary": {
"critical_count": 0,
"error_count": 1,
"warning_count": 0,
"info_count": 0,
"debug_count": 0,
},
}If the success-path result dict includes additional keys (e.g., an app_label, checks_run, or similar metadata), you may want to add those keys to the error responses as well (with None, [], or other appropriate defaults) so the schema is perfectly uniform for all outcomes.
| print("\n3. Testing list_models:") | ||
| print("-" * 60) | ||
|
|
||
| # Test 1: List all models (no filter) |
There was a problem hiding this comment.
issue (testing): test_list_models now mirrors the implementation instead of exercising the MCP tool and asserting behaviour
This version recreates the expected result_* structures but never calls server_fastmcp.list_models.fn or asserts on its output, so it won’t catch regressions and can easily diverge from the real behaviour.
Instead, have this test:
- Call
server_fastmcp.list_models.fn(app_labels=...)for the three cases (no filter, single app, multiple apps). - Assert that the return value is a dict with
total_count,app_filter, andmodels. - Assert
total_count == len(models). - Assert
app_filterisNone(no filter),['blog'](single app), and['blog', 'auth'](multiple apps). - Assert the
(app, model)pairs match those fromapps.get_models()for each filter.
You can keep the print diagnostics, but the test should primarily fail when the tool’s behaviour changes unexpectedly.
| initialize_django = server_fastmcp.initialize_django | ||
|
|
||
|
|
||
| async def test_run_check(): |
There was a problem hiding this comment.
issue (testing): test_run_check script lacks assertions and won’t fail CI if run_check regresses
Right now this only prints output and never asserts, so regressions in run_check wouldn’t cause the test (or CI) to fail.
If this is meant as an automated test, please add assertions on the returned dict structure and types (e.g., required keys like "success", "summary", "warnings" as a list) and verify that checked_apps, tags, deploy_checks, and fail_level reflect the parameters passed in each scenario. You may also want to split scenarios into separate test functions for clearer failures.
If it’s intended as a manual helper script instead, consider renaming it (e.g., manual_run_check_demo.py) so it isn’t confused with an automated test.
| return {"error": f"Error retrieving setting: {str(e)}"} | ||
|
|
||
|
|
||
| @mcp.tool() |
There was a problem hiding this comment.
issue (complexity): Consider refactoring run_check to use a top-level sync helper and centralized success/summary logic for clearer, more consistent behavior.
You can simplify run_check quite a bit without changing behavior by:
- Flattening the nested sync function.
- Centralizing success/summary computation instead of mutating
successin each branch. - Keeping the error response shape consistent.
1. Flatten execute_checks into a top-level sync helper
The inner execute_checks doesn’t really need to be nested. You can move it out and wrap it with sync_to_async at the call site:
from asgiref.sync import sync_to_async
def _execute_checks(
app_labels: list[str] | None,
tags: list[str] | None,
deploy: bool,
fail_level: str,
databases: list[str] | None,
) -> dict[str, Any]:
from django.core.checks import run_checks
from django.core.checks.messages import DEBUG, INFO, WARNING, ERROR, CRITICAL
try:
level_map = {
"DEBUG": DEBUG,
"INFO": INFO,
"WARNING": WARNING,
"ERROR": ERROR,
"CRITICAL": CRITICAL,
}
check_level = level_map.get(fail_level.upper(), ERROR)
app_configs = None
if app_labels:
app_configs = [apps.get_app_config(label) for label in app_labels]
messages = run_checks(
app_configs=app_configs,
tags=tags,
include_deployment_checks=deploy,
databases=databases or [],
)
result = {
"success": True,
"checked_apps": app_labels or "all",
"tags": tags or "all",
"deploy_checks": deploy,
"fail_level": fail_level,
"total_issues": len(messages),
"critical": [],
"errors": [],
"warnings": [],
"info": [],
"debug": [],
}
# Collect messages only – no success logic here
for msg in messages:
message_dict = {
"id": msg.id,
"level": msg.level,
"message": msg.msg,
"hint": msg.hint or "",
"obj": str(msg.obj) if msg.obj else "",
}
if msg.level >= CRITICAL:
result["critical"].append(message_dict)
elif msg.level >= ERROR:
result["errors"].append(message_dict)
elif msg.level >= WARNING:
result["warnings"].append(message_dict)
elif msg.level >= INFO:
result["info"].append(message_dict)
else:
result["debug"].append(message_dict)
# Centralized success + summary computation
has_failing = any(m.level >= check_level for m in messages)
result["success"] = not has_failing
result["summary"] = {
"critical_count": len(result["critical"]),
"error_count": len(result["errors"]),
"warning_count": len(result["warnings"]),
"info_count": len(result["info"]),
"debug_count": len(result["debug"]),
}
return result
except LookupError as e:
return {
"success": False,
"error": f"App not found: {str(e)}",
"checked_apps": app_labels or "all",
"tags": tags or "all",
"deploy_checks": deploy,
"fail_level": fail_level,
}
except Exception as e:
return {
"success": False,
"error": f"Error running checks: {str(e)}",
"checked_apps": app_labels or "all",
"tags": tags or "all",
"deploy_checks": deploy,
"fail_level": fail_level,
}
@mcp.tool()
async def run_check(
app_labels: list[str] | None = None,
tags: list[str] | None = None,
deploy: bool = False,
fail_level: str = "ERROR",
databases: list[str] | None = None,
) -> dict[str, Any]:
return await sync_to_async(_execute_checks)(
app_labels=app_labels,
tags=tags,
deploy=deploy,
fail_level=fail_level,
databases=databases,
)Benefits:
- No nested function; the core logic is directly visible in
_execute_checks. - Message categorization is only about bucketing; success is computed once based on
check_level, not mutated in many branches. - Error paths still include
errorbut now also returnsuccess=Falseand the same top-level keys (checked_apps,tags, etc.), which keeps the response shape consistent while remaining backward compatible (existing clients reading onlyerrorstill work).
hugobessa
approved these changes
Dec 15, 2025
Contributor
hugobessa
left a comment
hugobessa
left a comment
There was a problem hiding this comment.
LGTM, only left two nit comments about imports
Comment on lines
+582
to
+589
| from django.core.checks import run_checks | ||
| from django.core.checks.messages import ( | ||
| DEBUG, | ||
| INFO, | ||
| WARNING, | ||
| ERROR, | ||
| CRITICAL, | ||
| ) |
Contributor
There was a problem hiding this comment.
Nit: Any reason for this not to be on top level?
Collaborator
Author
There was a problem hiding this comment.
The imports are intentionally inside the function because the Django check framework requires django.setup() to be called first. Our MCP server initializes Django in run_server(), and if we import these at module level, they'd execute before Django is ready
Signed-off-by: Rai Siqueira <[email protected]>
fjsj
approved these changes
Dec 17, 2025
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.
New features
list_modelstool to receive parameters. Now, the AI can use one or more apps to filter which models will be returned. Partially solves Truncated output when listing models #9.Django Check example output:
list_modelstool filtering by just one app:Summary by Sourcery
Add a configurable Django system check MCP tool and enhance model listing to support app-based filtering and better handle large outputs.
New Features:
Enhancements:
Build:
Tests: