[AI-206] Add get_loan_details MCP tool with validation and structured errors - #298
[AI-206] Add get_loan_details MCP tool with validation and structured errors#298Kartikey-Malkani wants to merge 2 commits into
Conversation
|
Lgtm for me - What do you think @IOhacker |
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Add this Jira ticket in the PR title https://mifosforge.jira.com/browse/AI-206
Kartikey-Malkani
changed the title
…e cases - Add comprehensive docstring with args, returns, and edge cases - Include example success and error responses in JSON format - Document all common loan status values and response fields - Clarify API requirements and error conditions Closes AI-206
|
Thanks for the feedback! I’ve incorporated the Jira ticket in both the PR title and description, and made the requested updates. Also ensured consistency across validation, logging, and documentation changes. Please let me know if any further refinements are needed. |
|
Added demo screenshot showing MCP tool execution for This demonstrates:
Let me know if you'd like additional examples (error cases, edge cases). |
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes restructure loan-related MCP tools and domain adapters: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
python/mcp_server.py (1)
394-497: Extract the repeated pre-flight loan check into a helper.The four-line pattern — call
get_loan_details_domain.func(loanId), checkisinstance/httpStatusCode == 404, extractstatus, compare against expected states — is duplicated acrossapprove_disburse_loan,reject_loan,make_repayment,apply_loan_fee,waive_loan_interest,undo_approval,undo_disbursal, andreschedule_loan_app. Consolidating into one helper reduces drift risk (e.g., error message wording is already inconsistent betweenapply_loan_feeandundo_approval).Sketch
def _load_loan_or_error(loan_id: int, allowed_states: tuple[str, ...] | None = None) -> tuple[dict | None, dict | None]: """Return (loan_dict, None) on success, or (None, error_dict) if not found or status disallowed.""" check = get_loan_details_domain.func(loan_id) if not isinstance(check, dict) or check.get("httpStatusCode") == 404: return None, {"error": f"Loan ID {loan_id} not found. Check get_client_accts to see valid loanIds."} if allowed_states: status = check.get("status", {}).get("value", "") if not any(s in status.lower() for s in allowed_states): return None, {"error": f"Loan {loan_id} is in status '{status}'. Allowed: {allowed_states}."} return check, None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/mcp_server.py` around lines 394 - 497, Extract the repeated pre-flight loan validation into a helper named _load_loan_or_error(loan_id: int, allowed_states: tuple[str, ...] | None = None) that calls get_loan_details_domain.func(loan_id), returns (loan_dict, None) on success or (None, error_dict) when the loan is missing or its status is not in allowed_states; then update approve_disburse_loan, reject_loan, make_repayment, apply_loan_fee, waive_loan_interest, undo_approval, undo_disbursal, and reschedule_loan_app to call _load_loan_or_error with the appropriate allowed_states and immediately return the error dict if non-None, and ensure the helper standardizes the "Loan ID ... not found. Check get_client_accts..." message and the status-disallowed message format so all callers use the same wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/mcp_server.py`:
- Around line 337-340: The get_loan function currently calls the decorated
get_loan_details Tool object (not the original callable), which will fail at
runtime; extract the actual implementation into a private helper (e.g.,
_get_loan_details_impl) that contains the logic and return type dict, have both
get_loan_details (the `@mcp.tool`() function) and get_loan (the
backward-compatible `@mcp.tool`() alias) simply call and return
_get_loan_details_impl(loanId), and ensure the helper has the same signature and
docstring content so both decorated wrappers delegate to the real implementation
instead of calling the Tool wrapper.
- Around line 289-317: The isinstance check in get_loan_details currently
accepts bool because bool is a subclass of int; change the validation to
explicitly exclude bool (e.g., use "isinstance(loan_id, int) and not
isinstance(loan_id, bool)" or "type(loan_id) is int") and keep the
positive-integer check (loan_id > 0); update the validation branch in
get_loan_details to use this stricter test so True/False are rejected and the
error payload is returned as before.
In `@python/tests/test_loans.py`:
- Around line 19-37: Tests are importing tools.domains.loans.get_loan_details
but asserting behavior implemented in the MCP wrapper; update the tests to
import and call the MCP wrapper in mcp_server (the handler around the domain
layer that performs the validation and HTTP-status mapping) instead of
tools.domains.loans.get_loan_details, so the assertions for 400 and 502 apply;
keep the fineract_client patch/mocking but target the same mocked client used by
the wrapper, and run the same loan_id inputs (0 and 999) against mcp_server's
get_loan_details wrapper to validate the intended behavior.
In `@python/tools/domains/loans.py`:
- Around line 16-74: The docstring promises validation, error envelopes and
httpStatusCode but the function simply prints and calls
fineract_client.execute_get(f"loans/{loan_id}") without validating loan_id or
normalizing responses; update the domain to perform input validation (e.g., add
a Pydantic model or simple guard that ensures loan_id is a positive int before
calling fineract_client.execute_get), catch exceptions/HTTP errors from
fineract_client.execute_get and map them into the documented response envelope
(include httpStatusCode and an "error" field for 4xx/5xx, 400 for invalid
loan_id, 404/401/500 mapping as appropriate), and ensure callers like
get_loan_details_domain.func receive the validated/normalized dict returned by
this function rather than relying on MCP wrappers.
---
Nitpick comments:
In `@python/mcp_server.py`:
- Around line 394-497: Extract the repeated pre-flight loan validation into a
helper named _load_loan_or_error(loan_id: int, allowed_states: tuple[str, ...] |
None = None) that calls get_loan_details_domain.func(loan_id), returns
(loan_dict, None) on success or (None, error_dict) when the loan is missing or
its status is not in allowed_states; then update approve_disburse_loan,
reject_loan, make_repayment, apply_loan_fee, waive_loan_interest, undo_approval,
undo_disbursal, and reschedule_loan_app to call _load_loan_or_error with the
appropriate allowed_states and immediately return the error dict if non-None,
and ensure the helper standardizes the "Loan ID ... not found. Check
get_client_accts..." message and the status-disallowed message format so all
callers use the same wording.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: d4942b98-c700-4ab2-a1b4-7807bd16e7a6
📒 Files selected for processing (3)
python/mcp_server.pypython/tests/test_loans.pypython/tools/domains/loans.py
| @mcp.tool() | ||
| def get_loan(loanId: int) -> dict: | ||
| """Get key details of a specific loan.""" | ||
| data = get_loan_details.func(loanId) | ||
| def get_loan_details(loan_id: int) -> dict: | ||
| """Get key details of a specific loan by loan ID. | ||
|
|
||
| This MCP-native tool validates the loan ID, fetches loan data from Fineract | ||
| via the loan domain adapter, and returns a structured JSON payload. | ||
| """ | ||
| # Fast fail for invalid IDs to avoid unnecessary API round trips. | ||
| if not isinstance(loan_id, int) or loan_id <= 0: | ||
| return { | ||
| "error": "Invalid loan_id. It must be a positive integer.", | ||
| "httpStatusCode": 400, | ||
| "loan_id": loan_id, | ||
| } | ||
|
|
||
| data = get_loan_details_domain.func(loan_id) | ||
| if not isinstance(data, dict): | ||
| return data | ||
| return { | ||
| "error": "Unexpected response type from loan details tool.", | ||
| "httpStatusCode": 502, | ||
| "loan_id": loan_id, | ||
| } | ||
|
|
||
| if "error" in data: | ||
| return { | ||
| "error": data.get("error"), | ||
| "httpStatusCode": data.get("httpStatusCode", 502), | ||
| "loan_id": loan_id, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the mcp_server.py file
find . -name "mcp_server.py" -type fRepository: openMF/mcp-mifosx
Length of output: 83
🏁 Script executed:
# Also check if there's a FastMCP library or decorator definition
find . -name "*.py" -type f | head -20Repository: openMF/mcp-mifosx
Length of output: 668
🏁 Script executed:
# Search for `@mcp.tool` decorator usage
rg "@mcp\.tool" -A 5 -B 2Repository: openMF/mcp-mifosx
Length of output: 27387
🏁 Script executed:
# Read the mcp_server.py file, focusing on lines around 289-340 and beyond
cat -n python/mcp_server.py | sed -n '280,350p'Repository: openMF/mcp-mifosx
Length of output: 3226
🏁 Script executed:
# Also check for the FastMCP library import and decorator definition
head -100 python/mcp_server.py | cat -nRepository: openMF/mcp-mifosx
Length of output: 4688
🏁 Script executed:
# Search for the `@mcp.tool` decorator definition or FastMCP import
rg "from.*mcp|import.*mcp" python/mcp_server.py -B 2 -A 2Repository: openMF/mcp-mifosx
Length of output: 141
🏁 Script executed:
# Check what FastMCP is and how `@mcp.tool`() decorator works
rg "class FastMCP|def tool" --type py -B 2 -A 5Repository: openMF/mcp-mifosx
Length of output: 43
🏁 Script executed:
# Search for repeated validation patterns starting around line 396
cat -n python/mcp_server.py | sed -n '390,510p'Repository: openMF/mcp-mifosx
Length of output: 7494
🏁 Script executed:
# Look for the pattern of checking get_loan_details_domain.func in multiple tools
rg "get_loan_details_domain\.func" python/mcp_server.py -B 2 -A 4Repository: openMF/mcp-mifosx
Length of output: 3978
🏁 Script executed:
# Verify that bool is a subclass of int in Python
python3 << 'EOF'
print("isinstance(True, int):", isinstance(True, int))
print("isinstance(False, int):", isinstance(False, int))
print("issubclass(bool, int):", issubclass(bool, int))
print("True <= 0:", True <= 0)
print("False <= 0:", False <= 0)
EOFRepository: openMF/mcp-mifosx
Length of output: 179
🏁 Script executed:
# Check if FastMCP decorator preserves the original function
rg "fastmcp|FastMCP" --type py -B 1 -A 10 | head -50Repository: openMF/mcp-mifosx
Length of output: 1852
🏁 Script executed:
# Verify how the decorated function get_loan_details is called from get_loan
cat -n python/mcp_server.py | sed -n '337,341p'Repository: openMF/mcp-mifosx
Length of output: 237
Fix the bool subclass bypass in the isinstance check.
In Python, bool is a subclass of int. The check isinstance(loan_id, int) accepts True and False as valid integers. Since True <= 0 is False, the call get_loan_details(True) passes validation and reaches get_loan_details_domain.func(True) → fineract_client.execute_get("loans/True").
Proposed fix
- if not isinstance(loan_id, int) or loan_id <= 0:
+ if not isinstance(loan_id, int) or isinstance(loan_id, bool) or loan_id <= 0:
return {
"error": "Invalid loan_id. It must be a positive integer.",
"httpStatusCode": 400,
"loan_id": loan_id,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/mcp_server.py` around lines 289 - 317, The isinstance check in
get_loan_details currently accepts bool because bool is a subclass of int;
change the validation to explicitly exclude bool (e.g., use "isinstance(loan_id,
int) and not isinstance(loan_id, bool)" or "type(loan_id) is int") and keep the
positive-integer check (loan_id > 0); update the validation branch in
get_loan_details to use this stricter test so True/False are rejected and the
error payload is returned as before.
| @mcp.tool() | ||
| def get_loan(loanId: int) -> dict: | ||
| """Backward-compatible alias for get_loan_details.""" | ||
| return get_loan_details(loanId) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does fastmcp's @mcp.tool() decorator return the original function or a Tool wrapper object? Is the decorated function directly callable as a regular Python function?
💡 Result:
FastMCP's @mcp.tool decorator returns a Tool wrapper object (specifically, a FunctionTool instance), not the original function. The decorated function is no longer directly callable as a regular Python function. This behavior is explicitly documented in the official FastMCP documentation on decorating methods: "FastMCP decorators return objects (Tool, Resource, or Prompt instances) rather than the original function. This means that when you decorate a method directly, the method becomes the returned object and is no longer callable by your code." The documentation warns against direct decoration of methods because "The method will no longer be callable from Python". This applies generally to @mcp.tool on functions as well, as confirmed by source code inspection. The standalone fastmcp.tools.tool decorator returns the original function with metadata attached (making it callable), but mcp.tool (the instance method on FastMCP) immediately registers the tool by creating a Tool.from_function(fn) and adds it to the server's tool manager, replacing the function reference with the non-callable Tool object. For regular functions (not methods), the code compiles and runs (e.g., all quickstart examples use @mcp.tool directly on def add(...)), but the bound name now refers to the Tool object, not a callable function. To keep the original function callable, use the standalone @fastmcp.tools.tool decorator and manually register with mcp.add_tool(decorated_fn).
Citations:
- 1: https://gofastmcp.com/v2/patterns/decorating-methods
- 2: https://gofastmcp.com/v2/servers/tools
- 3: https://gofastmcp.com/python-sdk/fastmcp-tools-function_tool
- 4: https://github.com/jlowin/fastmcp/blob/main/src/fastmcp/server/server.py
- 5: https://github.com/PrefectHQ/fastmcp/blob/main/src/fastmcp/server/server.py
- 6: https://github.com/jlowin/fastmcp/blob/e87ede07/src/fastmcp/tools/function_tool.py
Extract the implementation into a private helper function to avoid calling decorated functions.
When @mcp.tool() decorates a function, it returns a Tool wrapper object, not the original callable function. The current code attempts to call the decorated get_loan_details() function directly from get_loan(), which will fail at runtime because get_loan_details is no longer a callable function—it is a Tool object.
The safe pattern is to extract the body into a private helper that both decorated functions delegate to:
Refactor pattern
+def _get_loan_details_impl(loan_id: int) -> dict:
+ # ... existing body of get_loan_details ...
+
`@mcp.tool`()
def get_loan_details(loan_id: int) -> dict:
- """..."""
- # body
+ """..."""
+ return _get_loan_details_impl(loan_id)
`@mcp.tool`()
def get_loan(loanId: int) -> dict:
"""Backward-compatible alias for get_loan_details."""
- return get_loan_details(loanId)
+ return _get_loan_details_impl(loanId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/mcp_server.py` around lines 337 - 340, The get_loan function currently
calls the decorated get_loan_details Tool object (not the original callable),
which will fail at runtime; extract the actual implementation into a private
helper (e.g., _get_loan_details_impl) that contains the logic and return type
dict, have both get_loan_details (the `@mcp.tool`() function) and get_loan (the
backward-compatible `@mcp.tool`() alias) simply call and return
_get_loan_details_impl(loanId), and ensure the helper has the same signature and
docstring content so both decorated wrappers delegate to the real implementation
instead of calling the Tool wrapper.
| @patch("tools.domains.loans.fineract_client") | ||
| def test_get_loan_details_invalid_loan_id(mock_client): | ||
| from tools.domains.loans import get_loan_details | ||
|
|
||
| result = get_loan_details.func(0) | ||
| assert result["httpStatusCode"] == 400 | ||
| assert "Invalid loan_id" in result["error"] | ||
| mock_client.execute_get.assert_not_called() | ||
|
|
||
|
|
||
| @patch("tools.domains.loans.fineract_client") | ||
| def test_get_loan_details_api_failure(mock_client): | ||
| from tools.domains.loans import get_loan_details | ||
|
|
||
| mock_client.execute_get.return_value = {"error": "Connection failed: timeout"} | ||
| result = get_loan_details.func(999) | ||
| assert result["httpStatusCode"] == 502 | ||
| assert "timeout" in result["error"] | ||
| mock_client.execute_get.assert_called_once_with("loans/999") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the domain function has no validation/status mapping.
fd -t f 'loans.py' python/tools/domains | xargs sed -n '1,80p'
# Show the MCP wrapper for comparison.
fd -t f 'mcp_server.py' python | xargs sed -n '285,320p'Repository: openMF/mcp-mifosx
Length of output: 4283
Tests import from domain layer but validate against MCP wrapper behavior—both tests will fail as written.
The tests on lines 19–37 import get_loan_details from tools.domains.loans, but the domain function (lines 73–74) performs no validation and no status-code mapping. It unconditionally returns the raw Fineract response. The validation (400 for invalid ID) and error mapping (502 for API failure) exist in the MCP wrapper in mcp_server.py (lines 290–317).
Current failures:
- Line 23 (
test_get_loan_details_invalid_loan_id): Calls domain withloan_id=0, which invokesexecute_get("loans/0"). The unconfigured mock returns aMagicMock. Assertionresult["httpStatusCode"] == 400raisesTypeError(MagicMock is not a dict), andassert_not_called()fails (the call was made). - Line 33 (
test_get_loan_details_api_failure): Domain returns the raw error dict{"error": "Connection failed: timeout"}. Accessingresult["httpStatusCode"]raisesKeyError.
Either target the MCP wrapper function (which implements the documented behavior), or move validation and error mapping into the domain function.
Proposed fix—target the MCP wrapper
-@patch("tools.domains.loans.fineract_client")
-def test_get_loan_details_invalid_loan_id(mock_client):
- from tools.domains.loans import get_loan_details
-
- result = get_loan_details.func(0)
- assert result["httpStatusCode"] == 400
- assert "Invalid loan_id" in result["error"]
- mock_client.execute_get.assert_not_called()
-
-
-@patch("tools.domains.loans.fineract_client")
-def test_get_loan_details_api_failure(mock_client):
- from tools.domains.loans import get_loan_details
-
- mock_client.execute_get.return_value = {"error": "Connection failed: timeout"}
- result = get_loan_details.func(999)
- assert result["httpStatusCode"] == 502
- assert "timeout" in result["error"]
- mock_client.execute_get.assert_called_once_with("loans/999")
+@patch("tools.domains.loans.fineract_client")
+def test_get_loan_details_invalid_loan_id(mock_client):
+ from mcp_server import get_loan_details
+
+ result = get_loan_details(0)
+ assert result["httpStatusCode"] == 400
+ assert "Invalid loan_id" in result["error"]
+ mock_client.execute_get.assert_not_called()
+
+
+@patch("tools.domains.loans.fineract_client")
+def test_get_loan_details_api_failure(mock_client):
+ from mcp_server import get_loan_details
+
+ mock_client.execute_get.return_value = {"error": "Connection failed: timeout"}
+ result = get_loan_details(999)
+ assert result["httpStatusCode"] == 502
+ assert "timeout" in result["error"]
+ mock_client.execute_get.assert_called_once_with("loans/999")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/tests/test_loans.py` around lines 19 - 37, Tests are importing
tools.domains.loans.get_loan_details but asserting behavior implemented in the
MCP wrapper; update the tests to import and call the MCP wrapper in mcp_server
(the handler around the domain layer that performs the validation and
HTTP-status mapping) instead of tools.domains.loans.get_loan_details, so the
assertions for 400 and 502 apply; keep the fineract_client patch/mocking but
target the same mocked client used by the wrapper, and run the same loan_id
inputs (0 and 999) against mcp_server's get_loan_details wrapper to validate the
intended behavior.
| """Fetch comprehensive loan account details and current status. | ||
|
|
||
| Answers: 'What is the status and outstanding balance of Loan #12345?' | ||
|
|
||
| This tool retrieves full loan account information including: | ||
| - Current status (Active, Pending, Closed, etc.) | ||
| - Principal amount, interest rate, term length | ||
| - Outstanding balance and arrears information | ||
| - Product name and account number | ||
|
|
||
| Args: | ||
| loan_id (int): The loan account ID (required, must be positive integer) | ||
|
|
||
| Returns: | ||
| dict: Structured response containing: | ||
| - id: The loan ID | ||
| - accountNo: Loan account number | ||
| - status: Current loan status (Active, Pending, Closed, Withdrawn, Rejected, or Write-Off) | ||
| - principal: Loan principal amount | ||
| - interestRatePerPeriod: Interest rate per period (%) | ||
| - termFrequency: Total loan term in months | ||
| - loanProductName: Loan product name | ||
| - disbursedOn: Disbursement date | ||
| - totalRepayment: Total repayments made so far | ||
| - totalOutstanding: Total amount still owed | ||
| - httpStatusCode: 200 on success, 4xx/5xx on error | ||
| - error: Error message if request failed | ||
|
|
||
| Edge Cases: | ||
| - Invalid loan_id (non-positive): Returns 400 with validation error | ||
| - Loan not found (valid ID, doesn't exist): Returns 404 from Fineract | ||
| - Database error: Returns 500 with error details | ||
| - Unauthorized: Returns 401 if API token invalid | ||
|
|
||
| Example Success Response (HTTP 200): | ||
| { | ||
| "id": 12345, | ||
| "accountNo": "000100012-000001", | ||
| "status": {"id": 300, "value": "Active"}, | ||
| "principal": 50000, | ||
| "interestRatePerPeriod": 12.5, | ||
| "termFrequency": 60, | ||
| "loanProductName": "Product - Loan", | ||
| "httpStatusCode": 200 | ||
| } | ||
|
|
||
| Example Validation Error Response (HTTP 400): | ||
| { | ||
| "error": "Invalid loan_id. It must be a positive integer.", | ||
| "httpStatusCode": 400, | ||
| "validation": { | ||
| "field": "loan_id", | ||
| "value": "invalid", | ||
| "expected": "positive integer" | ||
| } | ||
| } | ||
| """ | ||
| print(f"[Tool] Fetching summary details for Loan #{loan_id}...") | ||
| return fineract_client.execute_get(f"loans/{loan_id}") |
There was a problem hiding this comment.
Docstring documents behavior the function does not implement, and the domain endpoint lacks input validation.
The expanded docstring advertises a structured response with httpStatusCode, a 400 validation error for non-positive loan_id, and 401/404/500 error handling. The function body (lines 73-74) is still print(...) + return fineract_client.execute_get(f"loans/{loan_id}") — it performs no validation and does not inject httpStatusCode into responses. The 400/502 mapping lives only in the MCP wrapper in mcp_server.py. Callers that import this domain function directly (the workflow tools in mcp_server.py call get_loan_details_domain.func for pre-flight checks) will never see the documented behavior.
Per coding guidelines: "Enforce that new endpoints added here include strict input schema validation (e.g., through Pydantic) before executing external APIs." This domain endpoint accepts loan_id with no validation.
Either (a) push the validation and error envelope into the domain function so the docstring is accurate and all call sites benefit, or (b) reduce the docstring to describe only what the function actually returns.
Option A — move validation into the domain (preferred per guidelines)
`@tool`
def get_loan_details(loan_id: int):
"""Fetch comprehensive loan account details and current status.
...
"""
+ if not isinstance(loan_id, int) or isinstance(loan_id, bool) or loan_id <= 0:
+ return {
+ "error": "Invalid loan_id. It must be a positive integer.",
+ "httpStatusCode": 400,
+ "loan_id": loan_id,
+ }
print(f"[Tool] Fetching summary details for Loan #{loan_id}...")
return fineract_client.execute_get(f"loans/{loan_id}")As per coding guidelines: "Enforce that new endpoints added here include strict input schema validation (e.g., through Pydantic) before executing external APIs."
📝 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.
| """Fetch comprehensive loan account details and current status. | |
| Answers: 'What is the status and outstanding balance of Loan #12345?' | |
| This tool retrieves full loan account information including: | |
| - Current status (Active, Pending, Closed, etc.) | |
| - Principal amount, interest rate, term length | |
| - Outstanding balance and arrears information | |
| - Product name and account number | |
| Args: | |
| loan_id (int): The loan account ID (required, must be positive integer) | |
| Returns: | |
| dict: Structured response containing: | |
| - id: The loan ID | |
| - accountNo: Loan account number | |
| - status: Current loan status (Active, Pending, Closed, Withdrawn, Rejected, or Write-Off) | |
| - principal: Loan principal amount | |
| - interestRatePerPeriod: Interest rate per period (%) | |
| - termFrequency: Total loan term in months | |
| - loanProductName: Loan product name | |
| - disbursedOn: Disbursement date | |
| - totalRepayment: Total repayments made so far | |
| - totalOutstanding: Total amount still owed | |
| - httpStatusCode: 200 on success, 4xx/5xx on error | |
| - error: Error message if request failed | |
| Edge Cases: | |
| - Invalid loan_id (non-positive): Returns 400 with validation error | |
| - Loan not found (valid ID, doesn't exist): Returns 404 from Fineract | |
| - Database error: Returns 500 with error details | |
| - Unauthorized: Returns 401 if API token invalid | |
| Example Success Response (HTTP 200): | |
| { | |
| "id": 12345, | |
| "accountNo": "000100012-000001", | |
| "status": {"id": 300, "value": "Active"}, | |
| "principal": 50000, | |
| "interestRatePerPeriod": 12.5, | |
| "termFrequency": 60, | |
| "loanProductName": "Product - Loan", | |
| "httpStatusCode": 200 | |
| } | |
| Example Validation Error Response (HTTP 400): | |
| { | |
| "error": "Invalid loan_id. It must be a positive integer.", | |
| "httpStatusCode": 400, | |
| "validation": { | |
| "field": "loan_id", | |
| "value": "invalid", | |
| "expected": "positive integer" | |
| } | |
| } | |
| """ | |
| print(f"[Tool] Fetching summary details for Loan #{loan_id}...") | |
| return fineract_client.execute_get(f"loans/{loan_id}") | |
| """Fetch comprehensive loan account details and current status. | |
| Answers: 'What is the status and outstanding balance of Loan `#12345`?' | |
| This tool retrieves full loan account information including: | |
| - Current status (Active, Pending, Closed, etc.) | |
| - Principal amount, interest rate, term length | |
| - Outstanding balance and arrears information | |
| - Product name and account number | |
| Args: | |
| loan_id (int): The loan account ID (required, must be positive integer) | |
| Returns: | |
| dict: Structured response containing: | |
| - id: The loan ID | |
| - accountNo: Loan account number | |
| - status: Current loan status (Active, Pending, Closed, Withdrawn, Rejected, or Write-Off) | |
| - principal: Loan principal amount | |
| - interestRatePerPeriod: Interest rate per period (%) | |
| - termFrequency: Total loan term in months | |
| - loanProductName: Loan product name | |
| - disbursedOn: Disbursement date | |
| - totalRepayment: Total repayments made so far | |
| - totalOutstanding: Total amount still owed | |
| - httpStatusCode: 200 on success, 4xx/5xx on error | |
| - error: Error message if request failed | |
| Edge Cases: | |
| - Invalid loan_id (non-positive): Returns 400 with validation error | |
| - Loan not found (valid ID, doesn't exist): Returns 404 from Fineract | |
| - Database error: Returns 500 with error details | |
| - Unauthorized: Returns 401 if API token invalid | |
| Example Success Response (HTTP 200): | |
| { | |
| "id": 12345, | |
| "accountNo": "000100012-000001", | |
| "status": {"id": 300, "value": "Active"}, | |
| "principal": 50000, | |
| "interestRatePerPeriod": 12.5, | |
| "termFrequency": 60, | |
| "loanProductName": "Product - Loan", | |
| "httpStatusCode": 200 | |
| } | |
| Example Validation Error Response (HTTP 400): | |
| { | |
| "error": "Invalid loan_id. It must be a positive integer.", | |
| "httpStatusCode": 400, | |
| "validation": { | |
| "field": "loan_id", | |
| "value": "invalid", | |
| "expected": "positive integer" | |
| } | |
| } | |
| """ | |
| if not isinstance(loan_id, int) or isinstance(loan_id, bool) or loan_id <= 0: | |
| return { | |
| "error": "Invalid loan_id. It must be a positive integer.", | |
| "httpStatusCode": 400, | |
| "loan_id": loan_id, | |
| } | |
| print(f"[Tool] Fetching summary details for Loan #{loan_id}...") | |
| return fineract_client.execute_get(f"loans/{loan_id}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/tools/domains/loans.py` around lines 16 - 74, The docstring promises
validation, error envelopes and httpStatusCode but the function simply prints
and calls fineract_client.execute_get(f"loans/{loan_id}") without validating
loan_id or normalizing responses; update the domain to perform input validation
(e.g., add a Pydantic model or simple guard that ensures loan_id is a positive
int before calling fineract_client.execute_get), catch exceptions/HTTP errors
from fineract_client.execute_get and map them into the documented response
envelope (include httpStatusCode and an "error" field for 4xx/5xx, 400 for
invalid loan_id, 404/401/500 mapping as appropriate), and ensure callers like
get_loan_details_domain.func receive the validated/normalized dict returned by
this function rather than relying on MCP wrappers.
3 participants
Jira: https://mifosforge.jira.com/browse/AI-206
Summary - Add domain-level get_loan_details(loan_id) function with input validation - Add centralized MCP-level endpoint in mcp_server.py - Include comprehensive error handling for invalid IDs and failed API calls - Add unit tests covering success, invalid input, and API failure cases ## Changes - python/tools/domains/loans.py: new get_loan_details() with validation - python/mcp_server.py: MCP tool registration + backward-compatible alias - python/tests/test_loans.py: tests for valid/invalid/error cases ## Verification - Tests pass: 12 passed Closes AI-206
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation