feat: Context-Aware AI Suggestion Engine Across MCP Domains - #354
feat: Context-Aware AI Suggestion Engine Across MCP Domains#354KAishwarya2429 wants to merge 19 commits into
Conversation
…l as per review feedback
…l as per review feedback
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a suggestion engine, domain-specific suggestion helpers, an input validation module, and an action-mapper. MCP server responses were modified to include generated suggestions for some tools and a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server as MCP Server
participant Validation as Validation Engine
participant Tool as Tool Logic
participant Suggestion as Suggestion Engine
participant Registry as Domain Registry
Client->>MCP_Server: Invoke tool (e.g., get_loan)
MCP_Server->>Validation: validate_input(tool_name, params)
alt validation OK
Validation-->>MCP_Server: OK
MCP_Server->>Tool: Execute domain tool
Tool-->>MCP_Server: Result (data)
MCP_Server->>Suggestion: generate_suggestions(intent, data)
Suggestion->>Registry: (optional) resolve domain helpers
Registry-->>Suggestion: suggestions list
Suggestion-->>MCP_Server: suggestions
MCP_Server-->>Client: Response { data, suggestions }
else validation fails
Validation-->>MCP_Server: ValidationError
MCP_Server-->>Client: Structured error (safe: True)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/mcp_server.py (1)
298-330:⚠️ Potential issue | 🟠 MajorPreserve domain error responses before building the enhanced loan payload.
If
get_loan_details.func()returns an error dict, this code converts it into a successful"data"response with mostlyNonevalues. Return error/status payloads before constructing suggestions.Proposed change
if not isinstance(data, dict): return data + + status_code = data.get("httpStatusCode") + if "error" in data or (isinstance(status_code, int) and status_code >= 400): + return data tl = data.get("timeline", {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/mcp_server.py` around lines 298 - 330, The handler currently calls get_loan_details.func(loanId) and proceeds to build a success "data" response even when the function returned an error dict; move the error check earlier and return the original error/status payload immediately if get_loan_details.func(...) indicates an error (i.e., not the expected success dict) before constructing response or calling generate_suggestions; specifically, in the block around get_loan_details.func, verify the return type/value and return that error payload unchanged (instead of assigning to response and running generate_suggestions), so functions like generate_suggestions and the response dict are only executed for valid loan detail dicts.
🤖 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/core/suggestion_engine.py`:
- Around line 1-9: The generate_suggestions function in
python/core/suggestion_engine.py currently imports and delegates only to
generate_loan_suggestions, violating the core folder guideline; move the
aggregator out of python/core (e.g., into a tools/application layer) or invert
the dependency by implementing a provider registry (register_domain_providers)
and have generate_suggestions resolve and call all registered providers (not
just generate_loan_suggestions) so client and savings domains are included;
update references to generate_suggestions and remove direct domain imports from
the core module, and register generate_loan_suggestions and other domain
providers with the new registry.
In `@python/core/validation_engine.py`:
- Around line 7-16: The validators _require_positive_int and
_require_positive_number must explicitly reject booleans and non-finite numeric
values: in _require_positive_int ensure the value is an int but not a bool
(e.g., isinstance(value, int) and not isinstance(value, bool)) and still > 0; in
_require_positive_number ensure the value is an int/float but not a bool and is
finite (use math.isfinite) and > 0; update the ValidationError messages
accordingly so safe_tool calls (e.g., get_loan, make_repayment) cannot accept
True/False, NaN, or infinities.
In `@python/mcp_server.py`:
- Around line 444-457: The suggestions are generated from the raw result before
you ensure it matches the normalized shape, so fallback responses get
wrong/empty suggestions; fix by normalizing the payload first and then calling
generate_suggestions with that normalized object (i.e., if result is already a
dict use it, otherwise build payload = {"overdueLoans": result or []}), then
compute suggestions = generate_suggestions("get_overdue_loans", payload) and
return the payload merged with "suggestions"; reference generate_suggestions,
result, and the "overdueLoans" wrapper when making the change.
- Around line 459-487: safe_tool is never applied to MCP tools so validate_input
is not run and the wrapper's broad "except Exception" remains; update the tool
registration so each function decorated with `@mcp.tool`(...) is wrapped by
safe_tool (use safe_tool(tool_name) on each tool function or integrate safe_tool
inside the `@mcp.tool` registration path) and replace the broad except in
safe_tool.wrapper with explicit except ValidationError as e returning the safe
error payload and an except Exception as e that logs the unexpected exception
(use logging.exception or process logger) before returning the non-safe error
payload; reference the safe_tool decorator, validate_input, ValidationError,
wrapper, and the `@mcp.tool-decorated` tool functions when making changes.
In `@python/README.md`:
- Around line 383-391: The docs claim suggestions are not part of MCP tool
responses but the API has been changed: update the README section for Suggestion
Engine (and any related text) to reflect that get_loan and
get_overdue_loans_for_client now include a "suggestions" field in the MCP
response contract; explicitly state the new payload shape (mention the
"suggestions" field), note whether that field is optional/experimental, and
advise clients to handle the presence or absence of "suggestions" when parsing
responses and integrating suggestion_engine.py.
In `@python/tools/domains/loan_suggestions.py`:
- Around line 13-17: The code appends user-facing suggestions even when
loan.get("loanId") is missing, producing messages like "loan None"; before
appending to suggestions, guard on loan_id (the result of loan.get("loanId"))
and skip the entry if it's falsy or None. Locate the use of loan_id and the
suggestions list in the function that builds suggestions and only call
suggestions.append(...) when loan_id is present (i.e., check loan_id and
return/continue to the next loan if it's not).
- Around line 23-32: Normalize and validate the status value before using it:
ensure the `status` variable (in python/tools/domains/loan_suggestions.py) is
converted to a simple string safely (handle dict inputs by extracting a likely
key such as "value" or "name", fall back to str(status) for other types, then
.strip().lower()), and replace substring checks like `"active" in status` with
exact equality checks (e.g., status == "active", status == "pending", status ==
"submitted") when appending suggestions for `loan_id` to the `suggestions` list
so inactive vs active aren’t misclassified and AttributeError from .lower() on
non-strings is avoided.
---
Outside diff comments:
In `@python/mcp_server.py`:
- Around line 298-330: The handler currently calls get_loan_details.func(loanId)
and proceeds to build a success "data" response even when the function returned
an error dict; move the error check earlier and return the original error/status
payload immediately if get_loan_details.func(...) indicates an error (i.e., not
the expected success dict) before constructing response or calling
generate_suggestions; specifically, in the block around get_loan_details.func,
verify the return type/value and return that error payload unchanged (instead of
assigning to response and running generate_suggestions), so functions like
generate_suggestions and the response dict are only executed for valid loan
detail dicts.
🪄 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: 75c84d10-5db8-4be4-ab89-2d3ce838eecc
📒 Files selected for processing (6)
python/README.mdpython/core/suggestion_engine.pypython/core/validation_engine.pypython/mcp_server.pypython/tools/domains/loan_suggestions.pypython/tools/registry.py
| from typing import Any, List | ||
| from tools.domains.loan_suggestions import generate_loan_suggestions | ||
|
|
||
| def generate_suggestions(intent: str, data: Any) -> List[str]: | ||
| suggestions: List[str] = [] | ||
|
|
||
| suggestions.extend(generate_loan_suggestions(intent, data)) | ||
|
|
||
| return suggestions No newline at end of file |
There was a problem hiding this comment.
Move the suggestion aggregator out of python/core and register all domain providers.
This core module imports business-domain suggestion logic and only delegates to loans, so client and savings suggestion scenarios return nothing. Put the aggregator in a tools/application layer or invert the dependency via a provider registry.
As per coding guidelines, python/core/**/*.py: “Code in this folder must handle the MCP protocol, stream parsing, and connection lifecycles.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/core/suggestion_engine.py` around lines 1 - 9, The
generate_suggestions function in python/core/suggestion_engine.py currently
imports and delegates only to generate_loan_suggestions, violating the core
folder guideline; move the aggregator out of python/core (e.g., into a
tools/application layer) or invert the dependency by implementing a provider
registry (register_domain_providers) and have generate_suggestions resolve and
call all registered providers (not just generate_loan_suggestions) so client and
savings domains are included; update references to generate_suggestions and
remove direct domain imports from the core module, and register
generate_loan_suggestions and other domain providers with the new registry.
| def _require_positive_int(params: Dict[str, Any], name: str) -> None: | ||
| value = params.get(name) | ||
| if not isinstance(value, int) or value <= 0: | ||
| raise ValidationError(f"{name} must be a positive integer") | ||
|
|
||
|
|
||
| def _require_positive_number(params: Dict[str, Any], name: str) -> None: | ||
| value = params.get(name) | ||
| if not isinstance(value, (int, float)) or value <= 0: | ||
| raise ValidationError(f"{name} must be positive") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expected: True is an int, and NaN does not compare as <= 0.
python - <<'PY'
nan = float("nan")
print("bool_is_int:", isinstance(True, int))
print("nan_lte_zero:", nan <= 0)
PYRepository: openMF/mcp-mifosx
Length of output: 98
🏁 Script executed:
fd -type f -name "validation_engine.py" | head -20Repository: openMF/mcp-mifosx
Length of output: 231
🏁 Script executed:
fd "validation_engine.py"Repository: openMF/mcp-mifosx
Length of output: 93
🏁 Script executed:
cat -n python/core/validation_engine.py | head -30Repository: openMF/mcp-mifosx
Length of output: 1034
🏁 Script executed:
rg "_require_positive" python/core/validation_engine.py -A 2 -B 2Repository: openMF/mcp-mifosx
Length of output: 664
🏁 Script executed:
rg "validate_input" python/core/ -A 3 -B 3Repository: openMF/mcp-mifosx
Length of output: 500
🏁 Script executed:
rg "validate_input\(" --type py -B 2 -A 2Repository: openMF/mcp-mifosx
Length of output: 634
🏁 Script executed:
cat -n python/mcp_server.py | grep -A 10 -B 10 "validate_input"Repository: openMF/mcp-mifosx
Length of output: 1913
Add guards to reject booleans and non-finite numeric values in validation functions.
In _require_positive_int, True passes validation because isinstance(True, int) is True. In _require_positive_number, float("nan") passes validation because nan <= 0 is False. These validators are called in the safe_tool decorator for all external tool invocations, including financial operations (get_loan loanId, make_repayment amount). Accepting booleans or NaN values can corrupt financial data.
Proposed change
-from typing import Dict, Any
+from math import isfinite
+from typing import Dict, Any
@@
def _require_positive_int(params: Dict[str, Any], name: str) -> None:
value = params.get(name)
- if not isinstance(value, int) or value <= 0:
+ if isinstance(value, bool) or not isinstance(value, int) or value <= 0:
raise ValidationError(f"{name} must be a positive integer")
@@
def _require_positive_number(params: Dict[str, Any], name: str) -> None:
value = params.get(name)
- if not isinstance(value, (int, float)) or value <= 0:
+ if isinstance(value, bool) or not isinstance(value, (int, float)) or not isfinite(value) or value <= 0:
raise ValidationError(f"{name} must be positive")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/core/validation_engine.py` around lines 7 - 16, The validators
_require_positive_int and _require_positive_number must explicitly reject
booleans and non-finite numeric values: in _require_positive_int ensure the
value is an int but not a bool (e.g., isinstance(value, int) and not
isinstance(value, bool)) and still > 0; in _require_positive_number ensure the
value is an int/float but not a bool and is finite (use math.isfinite) and > 0;
update the ValidationError messages accordingly so safe_tool calls (e.g.,
get_loan, make_repayment) cannot accept True/False, NaN, or infinities.
| def safe_tool(tool_name: str) -> Callable: | ||
| def decorator(func: Callable) -> Callable: | ||
| @wraps(func) | ||
| def wrapper(*args: Any, **kwargs: Any) -> Dict[str, Any]: | ||
| try: | ||
| # 🔹 Bind args + kwargs properly | ||
| bound = signature(func).bind_partial(*args, **kwargs) | ||
| bound.apply_defaults() | ||
|
|
||
| validate_input(tool_name, dict(bound.arguments)) | ||
|
|
||
| return func(*args, **kwargs) | ||
|
|
||
| except ValidationError as e: | ||
| return { | ||
| "error": str(e), | ||
| "safe": True, | ||
| "tool": tool_name | ||
| } | ||
|
|
||
| except Exception: | ||
| return { | ||
| "error": "Internal tool execution error", | ||
| "safe": False, | ||
| "tool": tool_name | ||
| } | ||
|
|
||
| return wrapper | ||
| return decorator |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expected: validated tools should show `@safe_tool` near their `@mcp.tool` registrations after the fix.
rg -n -C2 '@safe_tool|def safe_tool|def (get_loan|make_repayment)\(' python/mcp_server.pyRepository: openMF/mcp-mifosx
Length of output: 542
🏁 Script executed:
# Search for all uses of `@safe_tool` in the codebase
rg -n '@safe_tool' python/Repository: openMF/mcp-mifosx
Length of output: 43
🏁 Script executed:
# Find all `@mcp.tool` decorated functions in python/mcp_server.py
rg -n -B1 'def (get_loan|make_repayment|[a-z_]+)\(' python/mcp_server.py | rg -B1 '@mcp.tool'Repository: openMF/mcp-mifosx
Length of output: 1575
🏁 Script executed:
# Verify the validate_input function and how it's being used
rg -n 'validate_input' python/mcp_server.py -A3 -B3Repository: openMF/mcp-mifosx
Length of output: 611
🏁 Script executed:
# Search for custom exceptions in the codebase
rg -n 'class.*Error|class.*Exception' python/ --type pyRepository: openMF/mcp-mifosx
Length of output: 129
🏁 Script executed:
# Check logging setup in mcp_server.py
rg -n 'import.*logging|logger|log\.' python/mcp_server.py | head -20Repository: openMF/mcp-mifosx
Length of output: 193
🏁 Script executed:
# Examine ValidationError specifically
rg -n 'class ValidationError|ValidationError' python/core/ -A3 -B1Repository: openMF/mcp-mifosx
Length of output: 1024
🏁 Script executed:
# Check what exceptions validate_input can raise
rg -n 'raise' python/core/validation_engine.py -B2 -A1Repository: openMF/mcp-mifosx
Length of output: 378
🏁 Script executed:
# Check for any other custom exceptions in core module
rg -n 'raise.*Error|raise.*Exception' python/core/ | grep -v ValidationErrorRepository: openMF/mcp-mifosx
Length of output: 266
Wire @safe_tool into registered MCP tools and replace the broad exception handler.
safe_tool is currently unused: the decorator is never applied to any of the 50+ tools registered with @mcp.tool() (lines 114–770), so validate_input() is never invoked. Additionally, the broad except Exception handler at lines 475–479 violates the Python coding standard; replace it with explicit handling of ValidationError (which validate_input() raises) plus logging for unexpected errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/mcp_server.py` around lines 459 - 487, safe_tool is never applied to
MCP tools so validate_input is not run and the wrapper's broad "except
Exception" remains; update the tool registration so each function decorated with
`@mcp.tool`(...) is wrapped by safe_tool (use safe_tool(tool_name) on each tool
function or integrate safe_tool inside the `@mcp.tool` registration path) and
replace the broad except in safe_tool.wrapper with explicit except
ValidationError as e returning the safe error payload and an except Exception as
e that logs the unexpected exception (use logging.exception or process logger)
before returning the non-safe error payload; reference the safe_tool decorator,
validate_input, ValidationError, wrapper, and the `@mcp.tool-decorated` tool
functions when making changes.
| ## Suggestion Engine (Experimental) | ||
|
|
||
| This repository includes an experimental `suggestion_engine.py` module that demonstrates how context-aware next-step suggestions can be generated based on MCP tool outputs. | ||
|
|
||
| ⚠️ Important: | ||
|
|
||
| - The suggestion engine is NOT integrated into MCP tool responses. | ||
| - MCP server remains a pure execution layer (no decision-making logic). | ||
| - Suggestions are intended to be generated at the client/LLM layer. |
There was a problem hiding this comment.
Align the Suggestion Engine docs with the actual MCP response contract.
This section says suggestions are not integrated into MCP tool responses, but get_loan and get_overdue_loans_for_client now return a "suggestions" field. Update the docs so clients do not build against the wrong payload shape.
Proposed change
- The suggestion engine is NOT integrated into MCP tool responses.
- MCP server remains a pure execution layer (no decision-making logic).
- Suggestions are intended to be generated at the client/LLM layer.
+ The suggestion engine is integrated into selected MCP tool responses as post-processing metadata.
+ MCP server remains the execution layer; suggestions provide guidance but do not execute decisions.
+ Clients may also generate additional suggestions at the client/LLM layer.📝 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.
| ## Suggestion Engine (Experimental) | |
| This repository includes an experimental `suggestion_engine.py` module that demonstrates how context-aware next-step suggestions can be generated based on MCP tool outputs. | |
| ⚠️ Important: | |
| - The suggestion engine is NOT integrated into MCP tool responses. | |
| - MCP server remains a pure execution layer (no decision-making logic). | |
| - Suggestions are intended to be generated at the client/LLM layer. | |
| ## Suggestion Engine (Experimental) | |
| This repository includes an experimental `suggestion_engine.py` module that demonstrates how context-aware next-step suggestions can be generated based on MCP tool outputs. | |
| ⚠️ Important: | |
| - The suggestion engine is integrated into selected MCP tool responses as post-processing metadata. | |
| - MCP server remains the execution layer; suggestions provide guidance but do not execute decisions. | |
| - Clients may also generate additional suggestions at the client/LLM layer. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/README.md` around lines 383 - 391, The docs claim suggestions are not
part of MCP tool responses but the API has been changed: update the README
section for Suggestion Engine (and any related text) to reflect that get_loan
and get_overdue_loans_for_client now include a "suggestions" field in the MCP
response contract; explicitly state the new payload shape (mention the
"suggestions" field), note whether that field is optional/experimental, and
advise clients to handle the presence or absence of "suggestions" when parsing
responses and integrating suggestion_engine.py.
| loan_id = loan.get("loanId") | ||
|
|
||
| suggestions.append(f"Apply a late fee to loan {loan_id}") | ||
| suggestions.append(f"View repayment schedule for loan {loan_id}") | ||
| suggestions.append(f"Send a repayment reminder for loan {loan_id}") |
There was a problem hiding this comment.
Skip entries without a valid loan identifier.
If loanId is missing, the function emits actionable-looking suggestions such as loan None. Guard before generating user-facing actions.
Proposed change
loan_id = loan.get("loanId")
+ if not isinstance(loan_id, int) or loan_id <= 0:
+ continue
suggestions.append(f"Apply a late fee to loan {loan_id}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/tools/domains/loan_suggestions.py` around lines 13 - 17, The code
appends user-facing suggestions even when loan.get("loanId") is missing,
producing messages like "loan None"; before appending to suggestions, guard on
loan_id (the result of loan.get("loanId")) and skip the entry if it's falsy or
None. Locate the use of loan_id and the suggestions list in the function that
builds suggestions and only call suggestions.append(...) when loan_id is present
(i.e., check loan_id and return/continue to the next loan if it's not).
| loan_id = data.get("loanId") | ||
| status = (data.get("status") or "").lower() | ||
|
|
||
| if "active" in status: | ||
| suggestions.append(f"Make a repayment for loan {loan_id}") | ||
| suggestions.append(f"View repayment schedule for loan {loan_id}") | ||
|
|
||
| if "pending" in status or "submitted" in status: | ||
| suggestions.append(f"Approve loan {loan_id}") | ||
| suggestions.append(f"Reject loan {loan_id}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expected: demonstrates that dict status raises on `.lower()` and substring matching misclassifies "inactive".
python - <<'PY'
for raw_status in [{"value": "Active"}, "inactive"]:
try:
status = (raw_status or "").lower()
print(raw_status, status, "active" in status)
except Exception as exc:
print(raw_status, type(exc).__name__)
PYRepository: openMF/mcp-mifosx
Length of output: 118
🏁 Script executed:
# Read the actual file to verify the code snippet
cat -n python/tools/domains/loan_suggestions.py | head -50Repository: openMF/mcp-mifosx
Length of output: 1491
🏁 Script executed:
# Search for any schema validation or type hints in the file
rg -A 5 -B 5 "loan_suggestions" python/tools/domains/Repository: openMF/mcp-mifosx
Length of output: 637
🏁 Script executed:
# Check if there are tests for this function to understand expected behavior
fd -e test -e spec python/tools/domains/ | xargs rg -l "loan_suggestions"Repository: openMF/mcp-mifosx
Length of output: 508
🏁 Script executed:
# Find callers of generate_loan_suggestions
rg "generate_loan_suggestions" python/Repository: openMF/mcp-mifosx
Length of output: 169
🏁 Script executed:
# Look for any schema or type definition files
fd -e json -e yaml -e pydantic python/tools/domains/ | xargs cat 2>/dev/null || echo "No schema files found"Repository: openMF/mcp-mifosx
Length of output: 450
🏁 Script executed:
# Check if there are any data fixtures or examples that show status format
rg "status" python/tools/domains/ -A 2 -B 2 | head -40Repository: openMF/mcp-mifosx
Length of output: 2629
🏁 Script executed:
# Check all places where data is constructed before passing to generate_loan_suggestions
rg -B 10 "generate_loan_suggestions" python/ | grep -A 10 -B 10 "status"Repository: openMF/mcp-mifosx
Length of output: 43
🏁 Script executed:
# Look at loans.py more carefully to see if status structure is consistent
sed -n '40,80p' python/tools/domains/loans.pyRepository: openMF/mcp-mifosx
Length of output: 1746
Normalize status safely and avoid substring-based state checks.
The code does not validate the status field type. If status is a dict (as it is in the raw API response), calling .lower() raises AttributeError. Additionally, substring matching causes false positives: "active" in "inactive" evaluates to True, misclassifying inactive loans. Normalize the status to a string, handle both dict and string inputs, and use exact equality checks instead of substring matching.
Proposed change
loan_id = data.get("loanId")
- status = (data.get("status") or "").lower()
+ raw_status = data.get("status")
+ if isinstance(raw_status, dict):
+ status = str(raw_status.get("value", "")).strip().lower()
+ else:
+ status = str(raw_status or "").strip().lower()
- if "active" in status:
+ if status == "active":
suggestions.append(f"Make a repayment for loan {loan_id}")
suggestions.append(f"View repayment schedule for loan {loan_id}")
- if "pending" in status or "submitted" in status:
+ if status in {"pending", "submitted", "submitted and pending approval"}:
suggestions.append(f"Approve loan {loan_id}")
suggestions.append(f"Reject loan {loan_id}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/tools/domains/loan_suggestions.py` around lines 23 - 32, Normalize and
validate the status value before using it: ensure the `status` variable (in
python/tools/domains/loan_suggestions.py) is converted to a simple string safely
(handle dict inputs by extracting a likely key such as "value" or "name", fall
back to str(status) for other types, then .strip().lower()), and replace
substring checks like `"active" in status` with exact equality checks (e.g.,
status == "active", status == "pending", status == "submitted") when appending
suggestions for `loan_id` to the `suggestions` list so inactive vs active aren’t
misclassified and AttributeError from .lower() on non-strings is avoided.
|
Thanks for the detailed review — really helpful insights. I’ve resolved the merge conflicts with upstream and ensured compatibility with the latest main branch. I’ll now address the review comments, including:
Will push updates shortly. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/core/action_mapper.py (1)
4-21: Avoid exposing mutable action lists from the centralized mapping.
get_actions_for_statusreturns the list stored inACTIONS_BY_STATUS, so any caller can mutate the shared rules accidentally. Use immutable values internally and return a copy/list at the boundary. Also add an explicit type annotation for the mapping.Proposed fix
-from typing import List +from typing import Mapping # Centralized mapping: status → allowed actions -ACTIONS_BY_STATUS = { - "active": ["repay", "view_schedule"], - "pending": ["approve", "reject"], - "submitted": ["approve", "reject"], - "inactive": ["activate"], - "closed": [], -} +ACTIONS_BY_STATUS: Mapping[str, tuple[str, ...]] = { + "active": ("repay", "view_schedule"), + "pending": ("approve", "reject"), + "submitted": ("approve", "reject"), + "inactive": ("activate",), + "closed": (), +} -def get_actions_for_status(status: str) -> List[str]: +def get_actions_for_status(status: str) -> list[str]: @@ - return ACTIONS_BY_STATUS.get(status, []) + return list(ACTIONS_BY_STATUS.get(status, ()))As per coding guidelines, “Flag any new function signatures, complex variable assignments, or class attributes that are missing
mypy-compatible type hints” and “Suggest replacing standard classes with@dataclass(frozen=True)or Pydantic models when defining data structures.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/core/action_mapper.py` around lines 4 - 21, ACTIONS_BY_STATUS is exposing mutable lists and lacks an explicit type annotation; change the mapping to use an immutable value type (e.g., tuple or frozenset) and add a mypy-compatible annotation like Dict[str, Tuple[str, ...]] or Mapping[str, Tuple[str, ...]] for ACTIONS_BY_STATUS, and update get_actions_for_status(status: str) to return a fresh list copy (e.g., list(...)) of the immutable entry so callers cannot mutate shared state; reference ACTIONS_BY_STATUS and get_actions_for_status when making these changes.
🤖 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/core/action_mapper.py`:
- Around line 4-10: ACTIONS_BY_STATUS is too coarse; change it to map actions by
entity/domain and status (e.g., ACTIONS_BY_DOMAIN_STATUS or ACTIONS_BY_ENTITY
with nested dicts keyed first by domain/tool such as "loan" or "savings" then by
status like "active"/"pending") so an "active" loan and an "active" savings
account can have different action lists; update any code that reads
ACTIONS_BY_STATUS to use the new structure (look up by domain then status or
fall back to a safe default) and rename the constant reference sites to the new
identifier to avoid regressions.
In `@python/tools/registry.py`:
- Around line 129-181: The route_intent function uses raw substring checks
(any(w in query ...)) which causes false positives; replace those checks with a
token/phrase-aware matcher (e.g., an existing helper like
is_token_or_phrase_in_query or implement one that checks whole-word boundaries
and multi-word phrases) and call it for every trigger group inside route_intent
(the loans, clients, groups, savings, staff, accounting, reports, products,
charges, codetables branches) so matches only fire on standalone words/phrases;
keep the active_domains accumulation, domain_map iteration, seen/result logic
unchanged.
---
Nitpick comments:
In `@python/core/action_mapper.py`:
- Around line 4-21: ACTIONS_BY_STATUS is exposing mutable lists and lacks an
explicit type annotation; change the mapping to use an immutable value type
(e.g., tuple or frozenset) and add a mypy-compatible annotation like Dict[str,
Tuple[str, ...]] or Mapping[str, Tuple[str, ...]] for ACTIONS_BY_STATUS, and
update get_actions_for_status(status: str) to return a fresh list copy (e.g.,
list(...)) of the immutable entry so callers cannot mutate shared state;
reference ACTIONS_BY_STATUS and get_actions_for_status when making these
changes.
🪄 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: 7305ef6e-5cff-4450-9f52-f1f3225fad3a
📒 Files selected for processing (3)
python/core/action_mapper.pypython/core/suggestion_engine.pypython/tools/registry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/core/suggestion_engine.py
| ACTIONS_BY_STATUS = { | ||
| "active": ["repay", "view_schedule"], | ||
| "pending": ["approve", "reject"], | ||
| "submitted": ["approve", "reject"], | ||
| "inactive": ["activate"], | ||
| "closed": [], | ||
| } |
There was a problem hiding this comment.
Include entity/domain context in the action mapping.
A status-only key is ambiguous across MCP domains. For example, an active loan maps to repayment actions, while an active savings account should map to deposit/withdraw actions. This will either omit valid savings suggestions or make a future validation layer authorize the wrong actions. Consider keying by domain/tool plus status.
Proposed direction
-ACTIONS_BY_STATUS = {
- "active": ["repay", "view_schedule"],
- "pending": ["approve", "reject"],
- "submitted": ["approve", "reject"],
- "inactive": ["activate"],
- "closed": [],
-}
+ACTIONS_BY_ENTITY_STATUS: dict[str, dict[str, tuple[str, ...]]] = {
+ "loan": {
+ "active": ("repay", "view_schedule"),
+ "pending": ("approve", "reject"),
+ "submitted": ("approve", "reject"),
+ "closed": (),
+ },
+ "client": {
+ "inactive": ("activate",),
+ },
+ "savings": {
+ "active": ("deposit", "withdraw"),
+ },
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/core/action_mapper.py` around lines 4 - 10, ACTIONS_BY_STATUS is too
coarse; change it to map actions by entity/domain and status (e.g.,
ACTIONS_BY_DOMAIN_STATUS or ACTIONS_BY_ENTITY with nested dicts keyed first by
domain/tool such as "loan" or "savings" then by status like "active"/"pending")
so an "active" loan and an "active" savings account can have different action
lists; update any code that reads ACTIONS_BY_STATUS to use the new structure
(look up by domain then status or fall back to a safe default) and rename the
constant reference sites to the new identifier to avoid regressions.
|
Addressed review feedback:
Intent routing is now more precise and production-safe. |
2 participants
Overview
This PR extends the MCP Suggestion Engine into a domain-agnostic, reusable system that generates context-aware next-step recommendations across multiple MCP tools.
Building on the initial response augmentation layer, this PR introduces a centralized action-mapping strategy and expands intelligent suggestions beyond loans to clients and savings domains.
Problem
Current implementation:
This leads to:
Solution
1. Centralized Action Mapping
Introduced a shared mapping layer:
2. Multi-Domain Suggestion Engine
Extended
generate_suggestions()to support:Examples:
3. Cleaner Architecture
Impact
Testing
Future Work
Summary by CodeRabbit
New Features
Documentation