fix: use CredentialStoreAdapter in sap_tool instead of raw os.getenv()#6319
Merged
bryanadenhq merged 3 commits intoApr 1, 2026
Merged
Conversation
Author
|
@TimothyZhang7, could you please merge this. |
Collaborator
|
Hi @KartikPawade, there seems to be a linter error, please run uv run ruff check. I can merge after that. |
Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughregister_tools in the SAP tool now accepts an optional CredentialStoreAdapter and resolves Changes
Sequence Diagram(s)sequenceDiagram
participant Agent
participant Register as Registrar
participant CredStore as CredentialStore (optional)
participant Env as Environment
participant SAP as SAP API
Agent->>Registrar: call register_tools(mcp, credentials?)
alt credentials provided
Registrar->>CredStore: credentials.get('sap_base_url','sap_username','sap_password')
CredStore-->>Registrar: returns values or None
else no credentials
Registrar->>Env: read SAP_BASE_URL/SAP_USERNAME/SAP_PASSWORD
Env-->>Registrar: returns values or empty
end
Registrar->>SAP: make authenticated request using resolved values
SAP-->>Registrar: response
Registrar-->>Agent: register tool functions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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)
Comment |
Author
|
Hi @bryanadenhq , made those ruff formatting, I think we are good now, let me know if anything. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/tests/tools/test_sap_tool.py (1)
240-258: Add a regression test for fallback when adapter is present but missing valuesThis suite only checks env fallback with
credentials=None. The runtime path intools/mcp_server.pypasses an adapter, so a key regression case is: adapter provided, adapter values missing, env vars set, and tool still succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/tests/tools/test_sap_tool.py` around lines 240 - 258, Add a new test (e.g., test_env_fallback_when_adapter_missing_values) that mirrors test_env_fallback_when_no_adapter but supplies an adapter object with missing credential values instead of None; patch os.environ with ENV and patch aden_tools.tools.sap_tool.sap_tool.httpx.get to return the same _mock_resp(data), then call the same tool invocation sap_list_purchase_orders (via tool_fns["sap_list_purchase_orders"]) passing the adapter or arranging the runtime in tools/mcp_server.py so the adapter is present but its credential fields are empty/absent, assert result["count"] == 0 and that mock_get was called with a URL containing "my-tenant-api.s4hana.ondemand.com" to verify env fallback works when adapter exists but lacks values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/src/aden_tools/tools/sap_tool/sap_tool.py`:
- Around line 44-53: The _get_config() logic currently uses credentials directly
and skips environment fallbacks and may let CredentialStoreAdapter.get()
KeyError propagate; update _get_config() to obtain base_url, username, and
password by first attempting safe lookups from credentials using .get(...) (or
catching KeyError when calling CredentialStoreAdapter.get) and falling back to
os.getenv("SAP_BASE_URL"/"SAP_USERNAME"/"SAP_PASSWORD") when credentials are
missing or lookup fails; ensure any KeyError from CredentialStoreAdapter.get is
caught and treated as a missing value so env vars are used instead, referencing
the _get_config function and variables base_url, username, password and the
CredentialStoreAdapter.get method.
In `@tools/tests/tools/test_sap_tool.py`:
- Around line 25-47: Add explicit type hints: annotate _mock_credentials() to
return MagicMock, and annotate tool_fns and tool_fns_with_creds to return
Dict[str, Callable[..., Any]] (or typing.Mapping[str, Callable[..., Any]] if
preferred). Update the function signatures for _mock_credentials, tool_fns, and
tool_fns_with_creds accordingly and ensure necessary typing imports (Dict,
Callable, Any) are present.
---
Nitpick comments:
In `@tools/tests/tools/test_sap_tool.py`:
- Around line 240-258: Add a new test (e.g.,
test_env_fallback_when_adapter_missing_values) that mirrors
test_env_fallback_when_no_adapter but supplies an adapter object with missing
credential values instead of None; patch os.environ with ENV and patch
aden_tools.tools.sap_tool.sap_tool.httpx.get to return the same
_mock_resp(data), then call the same tool invocation sap_list_purchase_orders
(via tool_fns["sap_list_purchase_orders"]) passing the adapter or arranging the
runtime in tools/mcp_server.py so the adapter is present but its credential
fields are empty/absent, assert result["count"] == 0 and that mock_get was
called with a URL containing "my-tenant-api.s4hana.ondemand.com" to verify env
fallback works when adapter exists but lacks values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0067c351-445c-4ab4-b589-e04939854521
📒 Files selected for processing (2)
tools/src/aden_tools/tools/sap_tool/sap_tool.pytools/tests/tools/test_sap_tool.py
Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6318
Description
The
sap_toolaccepted acredentialsparameter but never used it — all credential resolution went throughos.getenv()directly. This fix alignssap_toolwith the standard pattern used by every other tool in the repo, following the n8n tool pattern as suggested by @Antiarin.Type of Change
Related Issues
Fixes #6318
Changes Made
_get_config()insideregister_tools()to access thecredentialsclosureCredentialStoreAdapterresolution withos.getenv()fallback, matching the n8n tool patterncredentials: Anytocredentials: CredentialStoreAdapter | NonewithTYPE_CHECKINGimportTesting
ruff checkclean)Checklist
Summary by CodeRabbit
New Features
Improvements
Tests