[FIX][API]: extract resolve_root_path into shared utils/paths module by prashantgupta24 · Pull Request #3337 · IBM/mcp-context-forge

prashantgupta24 · 2026-02-27T21:35:24Z

🔗 Related Issue

📝 Summary

All 12 request.scope.get("root_path", "") call sites across 5 files now use the shared resolve_root_path() utility with proper settings.app_root_path fallback.

New file created:

mcpgateway/utils/paths.py — canonical resolve_root_path(request) helper (mirrors the logic from admin.py's _resolve_root_path, with 17 passing doctests)

Files updated (Option 1 — DRY approach):

mcpgateway/admin.py — replaced local _resolve_root_path() definition with from mcpgateway.utils.paths import resolve_root_path as _resolve_root_path; all existing call sites unchanged
mcpgateway/main.py:1614 — 2 call sites fixed (lines 1614, 1704)
mcpgateway/routers/sso.py:328 — 1 call site fixed (line 328)
mcpgateway/routers/oauth_router.py:447 — 1 call site fixed (line 447)
mcpgateway/routers/llm_admin_router.py:113 — 5 call sites fixed (lines 113, 212, 262, 385, 483)
mcpgateway/utils/verify_credentials.py:1156 — 3 call sites fixed (lines 1156, 1173, 1203)

🏷️ Type of Change

🧪 Verification

Check	Command	Status
Lint suite	`make lint`	✅
Unit tests	`make test`	✅
Coverage ≥ 80%	`make coverage`	✅

✅ Checklist

Code formatted (make black isort pre-commit)
Tests added/updated for changes
Documentation updated (if applicable)
No secrets or credentials committed

📓 Notes (optional)

Screenshots, design decisions, or additional context.

Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

crivetimihai · 2026-02-27T23:15:30Z

Thanks @prashantgupta24. This is a clean DRY refactoring — the shared resolve_root_path() utility correctly centralizes the 12 call sites and the doctests provide good coverage. Two minor items: (1) please remove the # Made with Bob comment at the end of paths.py, (2) the checklist notes tests weren't added/updated — consider adding a unit test beyond the doctests. Otherwise looks good.

Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

Resolved import conflicts in oauth_router.py and sso.py by keeping both imports (resolve_root_path and sanitize_for_log) as both are needed. Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

rakdutta · 2026-03-23T05:34:45Z

@prashantgupta24 Can you please rebase your branch.
Here are the items that need attention before we can merge:

✅ Required Fixes

Complete Verification Checklist

The PR description shows empty verification status. Please run and confirm all pass:
make lint
make test
make coverage # Verify ≥ 80%
Then update the PR checklist with results.

Fix Docstring Claims in mcpgateway/utils/paths.py

Line 12-13: References "PR #3297" but this is PR #3337 fixing issue #3298. Please correct or clarify.

Suggested fix:

Old:

that was previously private to `mcpgateway/admin.py` (PR #3297).

(mirrors the logic from `admin.py`'s `_resolve_root_path`, with 17 passing doctests)

New:

that was previously private to `mcpgateway/admin.py` (issue #3298).

💡 Optional Improvements (Not Blocking)

Consider adding integration test for middleware behavior with settings.app_root_path set
The fallback parameter is unused by any caller—consider if it's needed

📝 Summary

Code quality: Excellent
Tests: Comprehensive
Blocking issues: Just verification completion + docstring corrections

Once these are addressed, this is ready to merge! 🚀

Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

prashantgupta24 · 2026-03-24T20:58:37Z

@rakdutta done ✅

rakdutta · 2026-03-25T06:15:58Z

Overall: Excellent refactoring that consolidates root path resolution into a shared utility. Well-tested and follows all project conventions.

Strengths

Comprehensive test coverage (151 lines covering all edge cases)
Minimal disruption through smart import aliasing in admin.py
Clean API design with proper type hints and documentation
Correctly placed in mcpgateway/utils/paths.py

Minor Suggestions

Clarify doctest count: PR description mentions "17 passing doctests" but function docstring shows 3 examples. Please clarify what this refers to.
Document enhancement: The new fallback parameter is a nice addition not present in the original admin.py function. Consider mentioning this as a bonus enhancement in the PR description.
Optional: Add note in docstring that request must not be None (though existing call sites already guard for this).

Risk Assessment

Low - Pure structural refactoring with no logic changes beyond consolidation. Excellent test coverage ensures correctness.

@prashantgupta24 Could you please fix the failing test cases before proceeding with the merge?

After setting APP_ROOT_PATH="/proxy/mcp" and running pytest, the following tests are failing due to a mismatch in the expected redirect paths:

FAILED tests/unit/mcpgateway/test_admin.py::TestAdminServerRoutes::test_admin_set_server_state_activate - AssertionError: assert '/proxy/mcp/admin#catalog' == '/admin#catalog'
FAILED tests/unit/mcpgateway/test_admin.py::TestAdminServerRoutes::test_admin_set_server_state_deactivate - AssertionError: assert '/proxy/mcp/admin#catalog' == '/admin#catalog'
FAILED tests/unit/mcpgateway/test_admin.py::TestAdminServerRoutes::test_admin_set_server_state_with_inactive_checked - AssertionError: assert '/proxy/mcp/a...=true#catalog' == '/admin/?incl...=true#catalog'
FAILED tests/unit/mcpgateway/test_admin.py::TestAdminServerRoutes::test_admin_delete_server_success_inactive_unchecked_redirect - AssertionError: assert '/proxy/mcp/admin#catalog' == '/admin#catalog'
FAILED tests/unit/mcpgateway/test_admin.py::TestAdminServerRoutes::test_admin_delete_server_ignores_invalid_team_id - AssertionError: assert '/proxy/mcp/admin#catalog' == '/admin#catalog'
FAILED tests/unit/mcpgateway/test_admin.py::TestA2AAgentManagement::test_admin_delete_a2a_agent_disabled_redirects - AssertionError: assert '/proxy/mcp/admin#a2a-agents' == '/admin#a2a-agents'
FAILED tests/unit/mcpgateway/test_admin.py::test_admin_delete_gateway_success_inactive_checked_redirect - AssertionError: assert '/proxy/mcp/a...true#gateways' == '/admin/?incl...true#gateways'
FAILED tests/unit/mcpgateway/test_admin.py::test_admin_delete_prompt_success_inactive_checked_redirect - AssertionError: assert '/proxy/mcp/a...=true#prompts' == '/admin/?incl...=true#prompts'
FAILED tests/unit/mcpgateway/test_admin.py::test_admin_set_resource_state_success_inactive_checked_redirect - AssertionError: assert '/proxy/mcp/a...rue#resources' == '/admin/?incl...rue#resources'
FAILED tests/unit/mcpgateway/test_admin.py::test_admin_set_prompt_state_success_inactive_checked_redirect - AssertionError: assert '/proxy/mcp/a...=true#prompts' == '/admin/?incl...=true#prompts'
10 failed, 1064 passed, 12 warnings in 10.08s

[FIX][API]: extract resolve_root_path into shared utils/paths module

994b64a

Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

prashantgupta24 requested a review from crivetimihai as a code owner February 27, 2026 21:35

crivetimihai added bug Something isn't working SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release api REST API Related item labels Feb 27, 2026

crivetimihai added this to the Release 1.0.0-RC2 milestone Feb 27, 2026

prashantgupta24 added 2 commits February 27, 2026 16:09

chore: remove trailing comment from paths module

777a496

Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

test(root_path_fix): add unit tests for resolve_root_path helper

c2b5424

Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

prashantgupta24 requested review from kevalmahajan and madhav165 as code owners February 28, 2026 00:15

prashantgupta24 and others added 4 commits February 27, 2026 16:21

Merge branch 'main' into root_path_fix

391068b

docs: remove doctest examples from paths module docstring

332891f

Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

Merge branch 'main' into root_path_fix

8b90a12

Merge branch 'main' into root_path_fix

73684a1

crivetimihai modified the milestones: Release 1.0.0-RC2, Release 1.0.0, Release 1.1.0 Mar 7, 2026

prashantgupta24 and others added 4 commits March 9, 2026 09:32

Merge upstream/main into root_path_fix

33c73fd

Resolved import conflicts in oauth_router.py and sso.py by keeping both imports (resolve_root_path and sanitize_for_log) as both are needed. Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

Merge branch 'main' into root_path_fix

0db7497

Merge branch 'main' into root_path_fix

7dfd8cf

Merge branch 'main' into root_path_fix

ad6d340

crivetimihai assigned rakdutta Mar 20, 2026

Merge branch 'main' into root_path_fix

7cb709c

prashantgupta24 added 2 commits March 24, 2026 12:54

Merge branch 'main' into root_path_fix

0859cb3

docs: update issue reference in docstring

d2aa1d9

Signed-off-by: Prashant Gupta <prashantgupta@us.ibm.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX][API]: extract resolve_root_path into shared utils/paths module#3337

[FIX][API]: extract resolve_root_path into shared utils/paths module#3337
prashantgupta24 wants to merge 14 commits intoIBM:mainfrom
prashantgupta24:root_path_fix

prashantgupta24 commented Feb 27, 2026 •

edited

Loading

Uh oh!

crivetimihai commented Feb 27, 2026

Uh oh!

rakdutta commented Mar 23, 2026

Uh oh!

prashantgupta24 commented Mar 24, 2026

Uh oh!

rakdutta commented Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

prashantgupta24 commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Related Issue

📝 Summary

🏷️ Type of Change

🧪 Verification

✅ Checklist

📓 Notes (optional)

Uh oh!

crivetimihai commented Feb 27, 2026

Uh oh!

rakdutta commented Mar 23, 2026

Old:

that was previously private to mcpgateway/admin.py (PR #3297).

(mirrors the logic from admin.py's _resolve_root_path, with 17 passing doctests)

New:

that was previously private to mcpgateway/admin.py (issue #3298).

Uh oh!

prashantgupta24 commented Mar 24, 2026

Uh oh!

rakdutta commented Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prashantgupta24 commented Feb 27, 2026 •

edited

Loading

that was previously private to `mcpgateway/admin.py` (PR #3297).

(mirrors the logic from `admin.py`'s `_resolve_root_path`, with 17 passing doctests)

that was previously private to `mcpgateway/admin.py` (issue #3298).