feat(mcp): add pay slip tools to demo server and LangGraph allowlist - #79
srivilliamsai wants to merge 2 commits into
Conversation
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe change adds two payslip MCP tools, enables them in bridge and Flowise configurations, documents them, and adds a standalone verification script. ChangesPayslip MCP tools
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to The new mock payslip tools match the documented demo security model and are ready for normal merge checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (2 skipped: 2 unsupported.) ✨ 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 |
There was a problem hiding this comment.
🟡 Changes recommended
The verification script does not support configured worker IDs, and Flowise documentation is outdated.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Workday payslip lookup tools to the demo MCP server and exposes them through Flowise and LangGraph.
Changes:
- Added current-user and worker-specific payslip tools.
- Added verification checks and documentation.
- Updated Flowise actions and the LangGraph allowlist.
File summaries
| File | Summary |
|---|---|
mcp-demo-server/README.md |
Documents the new payslip tools. |
mcp-demo-server/main.py |
Implements payslip lookups and filtering. |
mcp-demo-server/check_payslips.py |
Adds payslip verification checks. Moderate (1 vote): assumes the current worker is always WK001. |
flowise/flows/workday-mcp-agent.json |
Registers the new Flowise tools. Nit (3 votes): Flowise documentation still lists 11 tools and omits the new names. |
bridge-service/app/orchestration/langgraph/tools/mcp.py |
Updates the LangGraph default tool allowlist. |
Review details
Suppressed comments (1)
mcp-demo-server/check_payslips.py:50
- This check assumes the current user is always WK001 (and has exactly two CNY slips), but
main.pyreadsCURRENT_USER_WORKER_IDfrom the environment and the demo configuration explicitly allows changing it. With a supported value such as WK002 or WK003,python3 check_payslips.pyfails even though the tool is behaving correctly; set the fixture user explicitly or derive the expected payload from the selected worker.
assert len(all_slips["payslips"]) == 2, f"Expected 2 slips for WK001, got {len(all_slips['payslips'])}"
assert all_slips["payslips"][0]["currency"] == "CNY"
assert all_slips["payslips"][0]["gross_pay"] == 35000
assert all_slips["payslips"][0]["net_pay"] == 25200
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "agentSelectedToolConfig": { | ||
| "mcpServerConfig": "{\n \"key\": \"wd-mcp-server\",\n \"url\": \"https://YOUR_MCP_SERVER_URL/mcp\",\n \"headers\": {\n \"Authorization\": \"Bearer YOUR_MCP_API_KEY\"\n },\n \"approvalPolicy\": \"always\"\n}", | ||
| "mcpActions": "[\"find_employee_id_by_name\",\"get_current_user_info\",\"get_current_user_time_off_balance\",\"get_current_user_time_off_history\",\"get_direct_reports\",\"get_more_employee_data\",\"get_my_time_off_eligibility\",\"get_personal_information\",\"get_today_date_and_day_of_week\",\"request_my_time_off\",\"get_time_off_balance\"]", | ||
| "mcpActions": "[\"find_employee_id_by_name\",\"get_current_user_info\",\"get_current_user_payslips\",\"get_current_user_time_off_balance\",\"get_current_user_time_off_history\",\"get_direct_reports\",\"get_more_employee_data\",\"get_my_time_off_eligibility\",\"get_payslip_by_worker_id\",\"get_personal_information\",\"get_today_date_and_day_of_week\",\"request_my_time_off\",\"get_time_off_balance\"]", |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from main import CURRENT_USER_WORKER_ID, get_current_user_payslips, get_payslip_by_worker_id # noqa: E402 | ||
|
|
||
|
|
||
| def main() -> None: |
There was a problem hiding this comment.
check_payslips.py is outside the tests/check_*.py path that CI runs, so these checks never run on pull requests. Move this check under the CI test path or add a workflow step for it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 5/5
flowise/README.mdis out of sync withflowise/flows/workday-mcp-agent.jsonand the LangGraph allowlist: it still documents 11 Workday MCP tools and omits the two payslip tools, which could mislead users about available functionality. Update the README tool count and list to match the new flow and allowlist.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="flowise/flows/workday-mcp-agent.json">
<violation number="1" location="flowise/flows/workday-mcp-agent.json:732">
P3: flowise/README.md still lists the Workday MCP client as having 11 tools and omits the two new payslip tools, while this change adds them to both the Flowise flow and the LangGraph allowlist. The README's own sync requirement instructs keeping the tool list current. Update flowise/README.md's tool table and the "11 Workday tools" count to include `get_current_user_payslips` and `get_payslip_by_worker_id`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "agentSelectedToolConfig": { | ||
| "mcpServerConfig": "{\n \"key\": \"wd-mcp-server\",\n \"url\": \"https://YOUR_MCP_SERVER_URL/mcp\",\n \"headers\": {\n \"Authorization\": \"Bearer YOUR_MCP_API_KEY\"\n },\n \"approvalPolicy\": \"always\"\n}", | ||
| "mcpActions": "[\"find_employee_id_by_name\",\"get_current_user_info\",\"get_current_user_time_off_balance\",\"get_current_user_time_off_history\",\"get_direct_reports\",\"get_more_employee_data\",\"get_my_time_off_eligibility\",\"get_personal_information\",\"get_today_date_and_day_of_week\",\"request_my_time_off\",\"get_time_off_balance\"]", | ||
| "mcpActions": "[\"find_employee_id_by_name\",\"get_current_user_info\",\"get_current_user_payslips\",\"get_current_user_time_off_balance\",\"get_current_user_time_off_history\",\"get_direct_reports\",\"get_more_employee_data\",\"get_my_time_off_eligibility\",\"get_payslip_by_worker_id\",\"get_personal_information\",\"get_today_date_and_day_of_week\",\"request_my_time_off\",\"get_time_off_balance\"]", |
There was a problem hiding this comment.
P3: flowise/README.md still lists the Workday MCP client as having 11 tools and omits the two new payslip tools, while this change adds them to both the Flowise flow and the LangGraph allowlist. The README's own sync requirement instructs keeping the tool list current. Update flowise/README.md's tool table and the "11 Workday tools" count to include get_current_user_payslips and get_payslip_by_worker_id.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flowise/flows/workday-mcp-agent.json, line 732:
<comment>flowise/README.md still lists the Workday MCP client as having 11 tools and omits the two new payslip tools, while this change adds them to both the Flowise flow and the LangGraph allowlist. The README's own sync requirement instructs keeping the tool list current. Update flowise/README.md's tool table and the "11 Workday tools" count to include `get_current_user_payslips` and `get_payslip_by_worker_id`.</comment>
<file context>
@@ -729,7 +729,7 @@
"agentSelectedToolConfig": {
"mcpServerConfig": "{\n \"key\": \"wd-mcp-server\",\n \"url\": \"https://YOUR_MCP_SERVER_URL/mcp\",\n \"headers\": {\n \"Authorization\": \"Bearer YOUR_MCP_API_KEY\"\n },\n \"approvalPolicy\": \"always\"\n}",
- "mcpActions": "[\"find_employee_id_by_name\",\"get_current_user_info\",\"get_current_user_time_off_balance\",\"get_current_user_time_off_history\",\"get_direct_reports\",\"get_more_employee_data\",\"get_my_time_off_eligibility\",\"get_personal_information\",\"get_today_date_and_day_of_week\",\"request_my_time_off\",\"get_time_off_balance\"]",
+ "mcpActions": "[\"find_employee_id_by_name\",\"get_current_user_info\",\"get_current_user_payslips\",\"get_current_user_time_off_balance\",\"get_current_user_time_off_history\",\"get_direct_reports\",\"get_more_employee_data\",\"get_my_time_off_eligibility\",\"get_payslip_by_worker_id\",\"get_personal_information\",\"get_today_date_and_day_of_week\",\"request_my_time_off\",\"get_time_off_balance\"]",
"agentSelectedTool": "customMCP"
}
</file context>
| # 2. Worker lookup: WK001 specific periods | ||
| feb_slip = get_payslip_by_worker_id("WK001", "2026-02") |
There was a problem hiding this comment.
get_payslip_by_worker_id, while get_current_user_payslips is only called without a period. CI can pass if the current-user period match or missing-period branch breaks. Keep checks for both branches of get_current_user_payslips.
Knowledge Base Used: MCP demo data model
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Confidence score: 5/5
bridge-service/tests/check_payslips.pyno longer callsget_current_user_payslips(period), leaving the period-filter and missing-period error branches inmcp-demo-server/main.pyuntested and allowing regressions in those paths to go unnoticed — restore targeted coverage for both cases.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="bridge-service/tests/check_payslips.py">
<violation number="1" location="bridge-service/tests/check_payslips.py:52">
P3: The latest delta removed all calls to `get_current_user_payslips(period)`, so the period-filter and missing-period error branches of that tool (mcp-demo-server/main.py:223-241) are no longer covered; only the equivalent paths of `get_payslip_by_worker_id` are tested. Add back a period and a missing-period call for the current user (WK via the fixture) so the current-user tool keeps its own branch coverage, and keep at least one exact-value assertion so mock-data corruption is caught.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| all_slips = get_current_user_payslips() | ||
| assert "payslips" in all_slips, f"Expected payslips list, got {all_slips}" | ||
| assert all_slips["worker_id"] == CURRENT_USER_WORKER_ID | ||
| assert len(all_slips["payslips"]) >= 1, f"Expected at least 1 slip for {CURRENT_USER_WORKER_ID}" |
There was a problem hiding this comment.
P3: The latest delta removed all calls to get_current_user_payslips(period), so the period-filter and missing-period error branches of that tool (mcp-demo-server/main.py:223-241) are no longer covered; only the equivalent paths of get_payslip_by_worker_id are tested. Add back a period and a missing-period call for the current user (WK via the fixture) so the current-user tool keeps its own branch coverage, and keep at least one exact-value assertion so mock-data corruption is caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bridge-service/tests/check_payslips.py, line 52:
<comment>The latest delta removed all calls to `get_current_user_payslips(period)`, so the period-filter and missing-period error branches of that tool (mcp-demo-server/main.py:223-241) are no longer covered; only the equivalent paths of `get_payslip_by_worker_id` are tested. Add back a period and a missing-period call for the current user (WK via the fixture) so the current-user tool keeps its own branch coverage, and keep at least one exact-value assertion so mock-data corruption is caught.</comment>
<file context>
@@ -44,42 +49,38 @@ def main() -> None:
- assert all_slips["payslips"][0]["currency"] == "CNY"
- assert all_slips["payslips"][0]["gross_pay"] == 35000
- assert all_slips["payslips"][0]["net_pay"] == 25200
+ assert len(all_slips["payslips"]) >= 1, f"Expected at least 1 slip for {CURRENT_USER_WORKER_ID}"
- # 2. Current user specific period (February 2026)
</file context>
| assert len(all_slips["payslips"]) >= 1, f"Expected at least 1 slip for {CURRENT_USER_WORKER_ID}" | |
| assert len(all_slips["payslips"]) >= 1, f"Expected at least 1 slip for {CURRENT_USER_WORKER_ID}" | |
| # Re-exercise the current-user period filter and missing-period error branches | |
| feb_current = get_current_user_payslips("2026-02") | |
| assert feb_current.get("period") == "2026-02", f"Expected 2026-02, got {feb_current}" | |
| assert feb_current.get("net_pay") == 25200 | |
| missing_current = get_current_user_payslips("2025-12") | |
| assert "error" in missing_current, f"Expected error for 2025-12, got {missing_current}" |
Summary
Adds Workday pay slip lookup tools (
get_current_user_payslipsandget_payslip_by_worker_id) to the demo MCP server backed by the existingmock_data/pay_slips.jsondataset, and registers them in the LangGraph reference allowlist.Component(s) Changed
Type of Change
Testing
python3 mcp-demo-server/check_payslips.py— verified current user history, period filtering, error handling for nonexistent periods/workers, and multi-currency (CNY, JPY, KRW) payloads.ruff check .— all checks passed.DEFAULT_MCP_TOOL_ALLOWLISTinbridge-service/app/orchestration/langgraph/tools/mcp.pycontains all tools.check_async_runner.py,check_idempotency.py,check_lineworks_signature.py,check_llm_config.py,check_prompt_security.py) — all passed.Checklist
Summary by cubic
Adds two pay slip lookup tools to the demo MCP server and registers them in the LangGraph tool allowlist, so agents can now retrieve pay slips by period or worker ID.
get_current_user_payslipsandget_payslip_by_worker_idread from the existingmock_data/pay_slips.jsonand return an error dict for unknown periods or workers.YYYY-MMperiod filter; without it they return all available payslips for the user or worker.bridge-service/tests/check_payslips.pyto verify behavior including multi-currency payloads (CNY, JPY, KRW).Written for commit b817bd5. Summary will update on new commits.
The PR appears safe to merge. One non-blocking test gap remains.
Findings
Summary
Diagram
sequenceDiagram participant U as User participant A as LangGraph or Flowise participant M as MCP demo server participant D as pay_slips.json U->>A: Ask for pay slips A->>M: Call an allowed pay slip tool M->>D: Match current or requested worker and period D-->>M: Return matching fixture records M-->>A: Return a pay slip, history, or error A-->>U: Explain the resultReviews (2) · Last reviewed commit: "docs: update flowise tool list and wire ..."