feat: server-managed mode — server-side connections, credential stripping, SSO/keyring auth - #2
feat: server-managed mode — server-side connections, credential stripping, SSO/keyring auth#2OstryP wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughTurns the project into an installable package with a CLI and server factory, adds managed configuration and connection caching, refactors tool execution and schema patches, and expands CI, unit tests, and end-to-end SAP lifecycle coverage. Changesmcp-sapcli package restructure
Sequence Diagram(s)sequenceDiagram
participant Client as MCP client
participant Tool as SapcliCommandTool
participant Manager as ConnectionManager
participant Backend as sapcli / ADT / gCTS
Client->>Tool: run(arguments)
Tool->>Manager: get_connection(system, conn_type)
Manager-->>Tool: connection
Tool->>Backend: execute command
alt unauthorized
Tool->>Manager: evict(system, conn_type)
Tool->>Manager: get_connection(system, conn_type)
Manager-->>Tool: fresh connection
Tool->>Backend: retry command
end
Tool-->>Client: OperationResult or error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 15
🧹 Nitpick comments (6)
README.md (2)
96-98: 📐 Maintainability & Code Quality | 🔵 TrivialConfirm basic-auth validation timing description is accurate.
The text states "both
userandpasswordreference fields are required at config load; their resolved values are validated at first connection". The code comment inconfig.pyclarifies that_resolve_basic_credentials()fails fast during connection creation, not config load. The README phrasing is slightly ambiguous—"required at config load" could be read as "must be present in JSON" rather than "validated". Consider rephrasing to "bothuserandpasswordfields must be present in the config; their resolved values are validated at first connection" for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 96 - 98, Clarify the basic-auth timing wording in the README to match `_resolve_basic_credentials()` and the config behavior: state that both `user` and `password` fields must be present in the config, and that their resolved values are validated when the first connection is created, not at config load. Keep the note aligned with the existing basic-auth section so the distinction between field presence and resolved-value validation is unambiguous.
452-454: 📐 Maintainability & Code Quality | 🔵 TrivialHonest documentation of known limitation.
The clear "Currently broken" warning for Server Configuration tools manages user expectations appropriately. Consider adding a tracking issue reference if one exists, so users can monitor when this is fixed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 452 - 454, The README note for the Server Configuration tools already warns that the commands are currently broken, but it should also point to a tracking issue if one exists so users can follow the fix. Update the documentation text near the Server Configuration tools note to preserve the honest limitation message and add a reference to the relevant issue or ticket; keep the guidance tied to the affected tools rather than changing the behavior description..github/workflows/ci.yml (2)
21-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable persisted Git credentials on checkout.
Each job runs PR-controlled code after checkout. Keeping the token in local git config is unnecessary here and increases exposure if a later step reads it.
Suggested fix
- - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + persist-credentials: falseAlso applies to: 50-50, 84-84, 120-120, 259-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 21, The checkout step currently leaves persisted Git credentials enabled, which is unnecessary for these PR jobs and increases token exposure. Update each actions/checkout usage in the workflow to disable persisted credentials by setting the checkout action’s persist-credentials option to false, and apply the same change to all listed job occurrences so no step keeps the token in local git config after checkout.Source: Linters/SAST tools
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the lockfile in CI
These
uv pip installsteps ignoreuv.lock, so the jobs can run against a dependency set different from the one under review. Switch the test jobs touv sync --frozen, and have the dependency-audit job deriverequirements-prod.txtfrom the lockfile instead of re-resolving.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 31 - 35, The CI dependency installation in the workflow is bypassing uv.lock, so update the test job setup to use uv sync --frozen instead of uv pip install in the install step. Also adjust the dependency-audit job to generate requirements-prod.txt from the existing lockfile rather than re-resolving dependencies, using the workflow steps that currently perform installation and audit.tests/test_argparsertool.py (1)
382-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression for
conn_typeinheritance.
add_parser()now propagatesconn_type, and managed-mode execution uses that field to choose the server-side connection. This block only locks downconn_factory, so aconn_typeregression would slip through until runtime.Suggested test
def test_subparser_inherits_conn_factory(self): """Test that subparser inherits parent's conn_factory.""" def mock_factory(): return None parent = ArgParserTool("parent", None, conn_factory=mock_factory) child = parent.add_parser("child") assert child.conn_factory is mock_factory + + def test_subparser_inherits_conn_type(self): + """Test that subparser inherits parent's conn_type.""" + parent = ArgParserTool("parent", None, conn_type="gcts") + + child = parent.add_parser("child") + + assert child.conn_type == "gcts"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_argparsertool.py` around lines 382 - 392, Add a regression test in ArgParserTool coverage to verify that add_parser() propagates conn_type from the parent to the child parser, similar to the existing conn_factory inheritance check. Update the subparser inheritance test around ArgParserTool and add_parser so it asserts the child parser’s conn_type matches the parent’s value, ensuring managed-mode connection selection remains covered.src/sapclimcp/toolpatches.py (1)
121-124: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNarrow this patch to the known file-backed tools.
applies_to()now matches anysource: stringfield, but the wrapper assumes that field is a filesystem path and overwrites it with a tempfile. That is broader than the “single-file string source tools” scope described for this patch and could silently corrupt a future business parameter namedsource.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sapclimcp/toolpatches.py` around lines 121 - 124, The applies_to() filter in toolpatches.py is too broad because it matches any tool with a string-typed source field, even when source is not a filesystem path. Narrow the check so this patch only applies to the known file-backed tools that the wrapper is meant to handle, using the existing applies_to() method as the gate. Update the matching logic to require the specific tool identifiers or schema shape for those file-backed tools, not just source: string, so the tempfile rewrite only happens where intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 152-166: The “Force-uninstall keyring (extras=none)” step in the
CI workflow should not fail if keyring is already missing. Update the
uninstall/check logic so the `uv pip uninstall` step tolerates an absent package
and the validation still only asserts that importing `keyring` fails, using the
same workflow job and `extras == 'none'` branch to locate it.
In `@Makefile`:
- Around line 13-14: The make test target no longer bootstraps the package
import path, so a clean checkout cannot import sapclimcp under the src layout.
Update the Makefile’s test target to either install the package in editable mode
before running pytest or restore the appropriate PYTHONPATH/bootstrap used for
src-based imports, keeping the existing pytest and coverage invocation in the
same target.
In `@src/sapclimcp/argparsertool.py`:
- Around line 143-148: The required-argument detection in the arg parsing logic
is treating action-based flags as required when they have no explicit default.
Update the `argparsertool` flow around the `required` calculation to recognize
`store_true`, `store_false`, `append`, and `count` as optional by inspecting the
action/kwargs before setting `required`, so the behavior matches `argparse` and
avoids `MissingArgument` for these flags.
In `@src/sapclimcp/cli.py`:
- Around line 29-68: The credential commands in _credential_set,
_credential_get, and _credential_delete only handle the missing-import case, but
keyring backend operations can still raise KeyringError subclasses such as
NoKeyringError or InitError. Update the credential path in src/sapclimcp/cli.py
to catch keyring.errors.KeyringError around keyring.set_password,
keyring.get_password, and keyring.delete_password, then print a clear CLI error
and exit cleanly instead of letting a traceback escape.
In `@src/sapclimcp/config.py`:
- Around line 418-425: `get_connection_params()` is resolving `sys_config.user`
unconditionally, which breaks cookie-auth systems even though
`SapcliCommandTool.run()` and `_create_adt_connection()` do not use `user` for
`auth == "cookie"`. Update `get_connection_params()` to skip resolving `user`
for cookie-auth connections and preserve the raw value or omit it in that path,
while keeping the existing resolve behavior for non-cookie auth.
In `@src/sapclimcp/errors.py`:
- Around line 95-100: The basic-auth remediation in the error handling logic
should also guide users to keyring-backed credentials, not just config-file
values or environment variables. Update the action text in the auth error path
so it references the credential resolution flow used by the basic-auth login
code, including `keyring:`-resolved passwords, and point users to the right
place to update or reconfigure those credentials alongside the existing `user`
and `password` guidance.
In `@src/sapclimcp/mcptools.py`:
- Around line 287-296: The dispatch in _execute_command() only handles adt and
gcts, so rfc and odata tools registered by transform_sapcli_commands() become
unusable. Update the command execution flow to either route non-ADT/gCTS tools
through a generic conn_factory-backed path (including the client-side
credentials they need) or prevent transform_sapcli_commands() from registering
unsupported conn_type values when managed connections are enabled. Make the fix
in the dispatch logic and the command registration/helpers so the supported tool
set stays consistent.
In `@start-mcp.py`:
- Around line 35-36: The convenience launcher is forcing experimental mode by
passing the experimental flag when invoking sapclimcp, which bypasses the safe
default allowlist used by create_mcp_server() and the main CLI. Remove that flag
from the launcher so it respects the same verified default surface unless the
user explicitly opts in elsewhere, and keep the wrapper aligned with the
existing CLI entry points.
In `@tests/e2e/conftest.py`:
- Around line 148-174: The package_name fixture currently masks
abap_package_create failures by falling back to "$TMP", which lets
package-specific tests pass without exercising the new package-creation path.
Update package_name in tests/e2e/conftest.py to be strict for package lifecycle
tests: either raise/skip when call_tool_check for "abap_package_create" fails,
or split the current behavior into a dedicated strict package fixture and a
separate fallback fixture for package-agnostic tests. Keep the logging in
package_name, but ensure tests like tests/e2e/test_01_package.py cannot succeed
unless package creation actually works.
- Around line 67-69: The run ID generated by _generate_run_id in conftest.py is
too short for shared package/object naming and can collide across test runs.
Increase the entropy by returning a longer random hex string from
secrets.token_hex, and keep the uppercasing so the new run_id remains compatible
with the existing package and object name formatting used throughout the suite.
In `@tests/e2e/helpers.py`:
- Around line 19-22: The structured_content handling in the helper that parses
tool results should not assume result["result"] is a valid 3-item tuple. Update
the parsing logic in the helper that returns (success, log_msgs, contents) to
validate the shape inside a try block and fall through to the JSON fallback on
any malformed payload or unpacking error, so bad structured_content returns the
documented failure tuple instead of raising early.
In `@tests/e2e/test_00_connectivity.py`:
- Around line 48-52: The gCTS connectivity check in call_tool_check currently
skips tests on any failure, which hides real regressions. Update the skip logic
around abap_gcts_repolist in test_00_connectivity.py to only call pytest.skip
when log_msgs/content clearly indicate gCTS is unavailable or unsupported on the
system, and otherwise let auth, routing, retry, or other unexpected failures
fail the test.
In `@tests/e2e/test_02_program.py`:
- Around line 151-159: The post-delete check in the `test_02_program.py` flow is
too broad because `call_tool_check()` can fail for reasons other than the
program being deleted. Update the `abap_program_read` assertion in the
`test_program`/delete verification path to inspect the returned error/details
and confirm it is specifically the expected “not found” case before treating the
deletion as successful. Use the existing `call_tool_check` result tuple and the
`abap_program_read` call site to locate the check.
In `@tests/e2e/test_03_class.py`:
- Around line 214-226: The ATC test currently treats every failed abap_atc_run
call in test_09_run_atc as “not configured,” which hides real auth, connection,
or tool errors. Update the logic around call_tool_check so it only calls
pytest.skip when the returned logs or error signal explicitly indicate ATC is
unsupported or not configured, and let all other failures surface as test
failures. Use the existing test_09_run_atc and call_tool_check flow to
distinguish the specific unsupported case from genuine runtime errors.
In `@tests/e2e/test_05_functiongroup.py`:
- Around line 77-79: The ABAP source generated by the function module helper
still contains a non-ASCII em dash in the `test_05_functiongroup` fixture, which
makes the E2E test depend on Unicode/source-codepage behavior. Update the string
in the function module generation path (the snippet building the `FUNCTION ...
ENDFUNCTION` source) to use plain ASCII punctuation, replacing the em dash with
a regular hyphen so the generated source stays portable.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 21: The checkout step currently leaves persisted Git credentials enabled,
which is unnecessary for these PR jobs and increases token exposure. Update each
actions/checkout usage in the workflow to disable persisted credentials by
setting the checkout action’s persist-credentials option to false, and apply the
same change to all listed job occurrences so no step keeps the token in local
git config after checkout.
- Around line 31-35: The CI dependency installation in the workflow is bypassing
uv.lock, so update the test job setup to use uv sync --frozen instead of uv pip
install in the install step. Also adjust the dependency-audit job to generate
requirements-prod.txt from the existing lockfile rather than re-resolving
dependencies, using the workflow steps that currently perform installation and
audit.
In `@README.md`:
- Around line 96-98: Clarify the basic-auth timing wording in the README to
match `_resolve_basic_credentials()` and the config behavior: state that both
`user` and `password` fields must be present in the config, and that their
resolved values are validated when the first connection is created, not at
config load. Keep the note aligned with the existing basic-auth section so the
distinction between field presence and resolved-value validation is unambiguous.
- Around line 452-454: The README note for the Server Configuration tools
already warns that the commands are currently broken, but it should also point
to a tracking issue if one exists so users can follow the fix. Update the
documentation text near the Server Configuration tools note to preserve the
honest limitation message and add a reference to the relevant issue or ticket;
keep the guidance tied to the affected tools rather than changing the behavior
description.
In `@src/sapclimcp/toolpatches.py`:
- Around line 121-124: The applies_to() filter in toolpatches.py is too broad
because it matches any tool with a string-typed source field, even when source
is not a filesystem path. Narrow the check so this patch only applies to the
known file-backed tools that the wrapper is meant to handle, using the existing
applies_to() method as the gate. Update the matching logic to require the
specific tool identifiers or schema shape for those file-backed tools, not just
source: string, so the tempfile rewrite only happens where intended.
In `@tests/test_argparsertool.py`:
- Around line 382-392: Add a regression test in ArgParserTool coverage to verify
that add_parser() propagates conn_type from the parent to the child parser,
similar to the existing conn_factory inheritance check. Update the subparser
inheritance test around ArgParserTool and add_parser so it asserts the child
parser’s conn_type matches the parent’s value, ensuring managed-mode connection
selection remains covered.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21685a92-b033-4c56-ab14-80de742f452e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.github/workflows/ci.yml.gitignore.pylintrcMakefileREADME.mddev-requirements.txtpyproject.tomlrequirements.txtsrc/sapcli-mcp-client.pysrc/sapcli-mcp-server.pysrc/sapclimcp/__main__.pysrc/sapclimcp/argparsertool.pysrc/sapclimcp/cli.pysrc/sapclimcp/config.pysrc/sapclimcp/errors.pysrc/sapclimcp/mcptools.pysrc/sapclimcp/server.pysrc/sapclimcp/toolpatches.pystart-mcp.pytests/e2e/__init__.pytests/e2e/conftest.pytests/e2e/helpers.pytests/e2e/test_00_connectivity.pytests/e2e/test_01_package.pytests/e2e/test_02_program.pytests/e2e/test_03_class.pytests/e2e/test_04_interface.pytests/e2e/test_05_functiongroup.pytests/e2e/test_06_ddl.pytests/e2e/test_07_table.pytests/e2e/test_08_structure.pytests/test_argparsertool.pytests/test_cli_and_server.pytests/test_config.pytests/test_errors.pytests/test_sapcli_mcp_server.pytests/test_toolpatches.py
💤 Files with no reviewable changes (2)
- requirements.txt
- dev-requirements.txt
| def _credential_set(args: argparse.Namespace) -> None: | ||
| """Store a credential in the OS keyring.""" | ||
| _require_keyring() | ||
| assert keyring is not None # post-guard invariant for type-checker + readers | ||
| value = args.value if args.value is not None else sys.stdin.readline().rstrip("\r\n") | ||
| if not value: | ||
| print("No value provided (pass as argument or pipe via stdin)", file=sys.stderr) | ||
| sys.exit(1) | ||
| keyring.set_password(KEYRING_SERVICE, args.key, value) | ||
| print(f"Stored credential: {args.key}") | ||
|
|
||
|
|
||
| def _credential_get(args: argparse.Namespace) -> None: | ||
| """Retrieve a credential from the OS keyring.""" | ||
| _require_keyring() | ||
| assert keyring is not None # post-guard invariant for type-checker + readers | ||
| value = keyring.get_password(KEYRING_SERVICE, args.key) | ||
| if value is None: | ||
| print(f"No credential found for key: {args.key}", file=sys.stderr) | ||
| sys.exit(1) | ||
| print(value) | ||
|
|
||
|
|
||
| def _credential_delete(args: argparse.Namespace) -> None: | ||
| """Delete a credential from the OS keyring.""" | ||
| _require_keyring() | ||
| # Make the post-guard invariant machine-checkable: the deferred | ||
| # `from keyring.errors` below assumes `keyring is not None`. | ||
| assert keyring is not None | ||
| # Defer the keyring.errors lookup until after _require_keyring() has | ||
| # confirmed `keyring is not None` — keeps the no-keyring code path | ||
| # free of attribute access on the soft-imported module. | ||
| from keyring.errors import PasswordDeleteError # pylint: disable=import-outside-toplevel | ||
|
|
||
| try: | ||
| keyring.delete_password(KEYRING_SERVICE, args.key) | ||
| print(f"Deleted credential: {args.key}") | ||
| except PasswordDeleteError: | ||
| print(f"No credential found for key: {args.key}", file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file list ---\n'
git ls-files src/sapclimcp/cli.py
printf '\n--- outline ---\n'
ast-grep outline src/sapclimcp/cli.py --view expanded || true
printf '\n--- relevant lines ---\n'
sed -n '1,220p' src/sapclimcp/cli.pyRepository: jfilak/mcp-sapcli
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file list ---'
git ls-files src/sapclimcp/cli.py
echo
echo '--- outline ---'
ast-grep outline src/sapclimcp/cli.py --view expanded || true
echo
echo '--- relevant lines ---'
sed -n '1,240p' src/sapclimcp/cli.pyRepository: jfilak/mcp-sapcli
Length of output: 7233
🌐 Web query:
keyring set_password get_password delete_password KeyringError backend unavailable documentation
💡 Result:
The Python keyring library provides a simple interface to access the system's underlying password/credential storage [1][2]. Key API Methods The core functionality is accessed through three primary methods [1][3]: - keyring.set_password(service, username, password): Stores a password in the active keyring [1][2]. - keyring.get_password(service, username): Retrieves the password for the given service and username. It returns the password as a string, or None if it does not exist [1][3]. - keyring.delete_password(service, username): Deletes the stored password. If the password does not exist, it raises a keyring.errors.PasswordDeleteError [1][2]. Error Handling The library defines a hierarchy of exceptions, all inheriting from the base class keyring.errors.KeyringError [1][3]. Common specific exceptions include: - keyring.errors.InitError: Raised when the keyring cannot be initialized [1][4]. - keyring.errors.PasswordSetError: Raised when a password fails to be stored [1][3]. - keyring.errors.PasswordDeleteError: Raised when a password fails to be deleted [1][4]. - keyring.errors.NoKeyringError: Raised when no suitable or recommended backend is found [5][6]. Backend Unavailable Issues The error NoKeyringError (or RuntimeError) indicating "No recommended backend was available" typically occurs when the library cannot detect or initialize a secure credential store on the host system [7][8][5]. To troubleshoot this: 1. Identify Available Backends: Run the command keyring --list-backends in your terminal to see which backends are detected and why others might be marked as "not viable" [8]. 2. Check Dependencies: Missing system packages are a common cause (e.g., dbus-python on Linux, or specific C extensions) [7][6]. 3. Headless Environments: In headless environments (like servers or Docker containers), many desktop-based backends fail because they cannot access a display or D-Bus session [8]. 4. Alternative Backends: If you cannot configure a recommended secure backend, you may install the keyrings.alt package to use less secure, non-recommended fallbacks [7][8][5]. Note that these should generally be avoided for production-grade security [7].
Citations:
- 1: https://keyring.readthedocs.io/en/latest/?badge=latest
- 2: https://keyring.readthedocs.io/en/latest/index.html
- 3: https://pypi.org/project/keyring/
- 4: https://pypi.org/project/keyring/25.4.0/
- 5: WSL2+Debian: keyring.errors.NoKeyringError: No recommended backend was available. jaraco/keyring#566
- 6: https://stackoverflow.com/questions/73118743/python-python-keyring-package-upgrade-of-python-has-broken-keyring
- 7: RuntimeError: No recommended backend was available. Install the keyrings.alt package if you want to use the non-recommended backends. See README.rst for details. jaraco/keyring#258
- 8: No recommended keyring backend (Linux headless) jaraco/keyring#569
Catch keyring backend errors in the credential path.
_require_keyring() only covers the import case. set_password(), get_password(), and delete_password() can still raise keyring.errors.KeyringError (for example NoKeyringError / InitError) when no backend is available or the OS keychain is unreachable, and this branch bypasses the startup error formatter. Wrap these calls and exit with a clean CLI message instead of a traceback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sapclimcp/cli.py` around lines 29 - 68, The credential commands in
_credential_set, _credential_get, and _credential_delete only handle the
missing-import case, but keyring backend operations can still raise KeyringError
subclasses such as NoKeyringError or InitError. Update the credential path in
src/sapclimcp/cli.py to catch keyring.errors.KeyringError around
keyring.set_password, keyring.get_password, and keyring.delete_password, then
print a clear CLI error and exit cleanly instead of letting a traceback escape.
There was a problem hiding this comment.
Fair — keyring.set/get/delete_password can raise a KeyringError when no backend is available (headless/CI) and leak a traceback past the clean CLI error path. It's low-severity (admin-time setup subcommand, run by a human), but wrapping the three calls in try/except keyring.errors.KeyringError is reasonable polish — noting as a follow-up.
There was a problem hiding this comment.
@OstryP Sounds good — thanks for the clarification. Agreed that this is more polish than blocker-level for this PR, and deferring it to a follow-up is reasonable.
If you want, I can open a GitHub issue to track wrapping the keyring.set_password(), keyring.get_password(), and keyring.delete_password() calls in src/sapclimcp/cli.py with try/except keyring.errors.KeyringError so the credential subcommands fail cleanly when no usable backend is available.
| success, log_msgs, content = await call_tool_check( | ||
| mcp_client, "abap_gcts_repolist", {"system": system_name} | ||
| ) | ||
| if not success: | ||
| pytest.skip(f"gCTS not available on this system: {log_msgs}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only skip gCTS when the system is actually missing gCTS.
This currently skips on any unsuccessful abap_gcts_repolist call, so auth, retry, or routing regressions in the new gCTS path will look like “not available” instead of failing CI. Gate the skip on a specific unavailable/capability signal and fail on everything else.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test_00_connectivity.py` around lines 48 - 52, The gCTS
connectivity check in call_tool_check currently skips tests on any failure,
which hides real regressions. Update the skip logic around abap_gcts_repolist in
test_00_connectivity.py to only call pytest.skip when log_msgs/content clearly
indicate gCTS is unavailable or unsupported on the system, and otherwise let
auth, routing, retry, or other unexpected failures fail the test.
There was a problem hiding this comment.
Acknowledged — call_tool_check can't separate "gCTS not configured" from a routing regression. In practice the earlier lifecycle tests (systeminfo / package_list via call_tool_ok) already validate auth and transport before this point, so the residual gap is a gCTS-routing-specific regression. A precise gate would need the exact "gCTS unavailable" signal, which isn't documented and is brittle across SAP releases — so I'm noting it as a follow-up (this suite is live-only and skips in CI).
There was a problem hiding this comment.
@OstryP, that makes sense — thanks for the clarification.
Given the earlier systeminfo/package_list checks, I agree the remaining exposure here is narrowed to gCTS-specific routing/regression coverage, and deferring a stricter skip gate until there’s a stable unsupported/unavailable signal is reasonable.
If you want, I can open a follow-up GitHub issue to track tightening test_03_gcts_repolist once a reliable gCTS-unavailable signature is known.
(•ᵕ•)🐇
| success, _, _ = await call_tool_check( | ||
| mcp_client, | ||
| "abap_program_read", | ||
| { | ||
| "name": self._prog_name, | ||
| "system": system_name, | ||
| }, | ||
| ) | ||
| assert not success, "Program should not exist after deletion" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't treat every failed read as proof of deletion.
call_tool_check() also reports success=False for auth/transport/tool failures, so this passes even when the post-delete read broke for an unrelated reason. Assert the failure is specifically the expected “not found” case before counting the delete step as successful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test_02_program.py` around lines 151 - 159, The post-delete check
in the `test_02_program.py` flow is too broad because `call_tool_check()` can
fail for reasons other than the program being deleted. Update the
`abap_program_read` assertion in the `test_program`/delete verification path to
inspect the returned error/details and confirm it is specifically the expected
“not found” case before treating the deletion as successful. Use the existing
`call_tool_check` result tuple and the `abap_program_read` call site to locate
the check.
There was a problem hiding this comment.
Lower-risk than the gCTS/ATC skips: the preceding abap_program_delete uses call_tool_ok on the same system, so auth and transport are proven working at the moment of the post-delete read. Asserting a specific "not found" would need the exact (version-dependent) error string and risks flakiness — noting as a follow-up.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sapclimcp/argparsertool.py`:
- Around line 57-67: The boolean flag handling in _argument_spec_to_json_spec()
is missing argparse’s implicit defaults for store_true and store_false, so
omitted flags can resolve incorrectly. Update the logic around the action checks
in _argument_spec_to_json_spec() so store_true emits an implicit False default
and store_false emits an implicit True default before calling
_add_default_if_specified(), while preserving the existing behavior for count,
typ, and explicit defaults.
In `@src/sapclimcp/config.py`:
- Around line 351-352: The ConnectionManager cache is being mutated concurrently
in HTTP mode, so `evict()` and `get_connection()` need synchronized access to
the shared `_cache` dict. Add a lock around all cache reads and writes in the
`ConnectionManager` methods that touch `_cache`, especially the stale-entry
cleanup path in `get_connection()` and the removal logic in `evict()`. Replace
the unsafe direct delete on the stale-entry path with a safe removal under that
lock, such as a guarded pop-style removal, so overlapping eviction/refresh does
not raise a `KeyError`.
In `@src/sapclimcp/server.py`:
- Around line 61-71: The managed-mode prompt text in
MCP_SERVER_INSTRUCTIONS_MANAGED should be generated from
connection_manager.default_system instead of always assuming a fallback exists.
Update the server instruction building logic in server.py to branch on whether
default_system is None: keep the optional-system wording only when a default is
present, and switch to a required-parameter message when no default exists so
ConnectionPatch and the prompt stay consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e2390b2-e030-4d03-a957-54680f54a716
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.github/workflows/ci.yml.gitignore.pylintrcMakefileREADME.mddev-requirements.txtpyproject.tomlrequirements.txtsrc/sapcli-mcp-client.pysrc/sapcli-mcp-server.pysrc/sapclimcp/__main__.pysrc/sapclimcp/argparsertool.pysrc/sapclimcp/cli.pysrc/sapclimcp/config.pysrc/sapclimcp/errors.pysrc/sapclimcp/mcptools.pysrc/sapclimcp/server.pysrc/sapclimcp/toolpatches.pystart-mcp.pytests/e2e/__init__.pytests/e2e/conftest.pytests/e2e/helpers.pytests/e2e/test_00_connectivity.pytests/e2e/test_01_package.pytests/e2e/test_02_program.pytests/e2e/test_03_class.pytests/e2e/test_04_interface.pytests/e2e/test_05_functiongroup.pytests/e2e/test_06_ddl.pytests/e2e/test_07_table.pytests/e2e/test_08_structure.pytests/test_argparsertool.pytests/test_cli_and_server.pytests/test_config.pytests/test_errors.pytests/test_sapcli_mcp_server.pytests/test_toolpatches.py
💤 Files with no reviewable changes (2)
- requirements.txt
- dev-requirements.txt
✅ Files skipped from review due to trivial changes (4)
- .pylintrc
- .gitignore
- src/sapclimcp/main.py
- tests/e2e/init.py
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/e2e/test_01_package.py
- tests/e2e/test_00_connectivity.py
- pyproject.toml
- src/sapcli-mcp-server.py
- Makefile
- tests/e2e/test_04_interface.py
- tests/e2e/test_07_table.py
- tests/e2e/test_08_structure.py
- tests/e2e/test_05_functiongroup.py
- tests/e2e/test_06_ddl.py
- tests/e2e/test_02_program.py
- tests/test_errors.py
- src/sapclimcp/errors.py
- src/sapcli-mcp-client.py
- src/sapclimcp/toolpatches.py
- tests/test_argparsertool.py
- tests/e2e/test_03_class.py
Replace requirements.txt / dev-requirements.txt with a PEP 621 pyproject.toml ([project.dependencies] + [project.optional-dependencies], with keyring as an opt-in [keyring] extra), the pinned sapcli dependency, and the sapcli-mcp console entry point. Add uv.lock for reproducible installs and a `python -m sapclimcp` entry (__main__.py). The existing flake8 + pylint + mypy lint stack is kept as-is; the Makefile lint/check targets and the [dev] extra are just extended to cover the new sapclimcp package. requires-python is >=3.12 to match sapcli's own floor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the server-managed connection layer so the MCP client never handles credentials. config.py introduces named-system configuration, SecretRef (lazy keyring: / $ENV / literal resolution), CookieSessionInitializer for SSO cookie auth via sapcli's HTTPSessionInitializer protocol, and a ConnectionManager that caches connections per (system, conn_type) with TTL, eviction, and retry-on-401. errors.py turns raw tracebacks into actionable messages (connection refused, SSL, expired cookie, missing sapcli) and owns the package exception hierarchy (ConfigError, ToolInputError). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… server Extend the ArgParser-to-tool layer for server-managed mode: ConnectionPatch removes connection/credential parameters from the generated tool schemas (the LLM sees only business params plus an optional `system` selector), SourceDataPatch handles source payloads, and mcptools resolves the connection server-side per call with a single combined error path. argparsertool gains the supporting plumbing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add server.py (create_mcp_server, MCP server instructions, --config / --stdio / --log-level wiring) and cli.py (`sapcli-mcp credential set/get/delete` backed by the OS keyring). Update the server/client scripts to use the package server factory and add start-mcp.py as a convenience launcher. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add tests/e2e covering the full ABAP lifecycle (connectivity, package, program, class, interface, function group, DDL, table, structure). These require a live SAP system (configured via env vars) and skip when it is unset. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add .github/workflows/ci.yml running flake8 + pylint, pytest on
ubuntu/macos/windows, mypy, an install matrix across {none, keyring} extras (the
no-keyring legs uninstall the transitive keyring to exercise the soft-import
path), and a pip-audit dependency scan.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document the server-managed connection model (named systems, credential stripping before the LLM, cookie/keyring auth), configuration via the JSON config file, and the actual tool inventory with sandbox-tested annotations (and known-bug flags, e.g. the upstream datapreview OSQL stdout/encoding issue). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
21-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable checkout credential persistence.
These jobs do not need Git credentials after checkout; set
persist-credentials: falseon eachactions/checkoutstep to reduce token exposure if later steps or artifacts accidentally include repository metadata.🔒 Suggested change
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + persist-credentials: falseAlso applies to: 50-50, 84-84, 120-120, 259-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 21, Disable credential persistence on each actions/checkout step in the CI workflow by adding persist-credentials: false to every checkout use, since these jobs do not need Git credentials after checkout. Update each affected checkout invocation consistently so the workflow still checks out the repository but does not leave tokens available for later steps or artifacts.Source: Linters/SAST tools
tests/test_cli_and_server.py (1)
111-140: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a runtime regression for required
system.This schema test is good, but it would not catch
run()poppingsystembefore validation. Add a managed multi-system/no-default call that suppliessystemand asserts the tool runs instead of raising “missing system”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli_and_server.py` around lines 111 - 140, Add a runtime regression around the managed-mode tool path so `system` is actually validated at execution time: in the test suite near `test_managed_mode_strips_credentials_from_all_schemas`, create a managed multi-system/no-default scenario and invoke the relevant tool through `run()` with an explicit `system` value, then assert it succeeds instead of raising a missing-system error. Use the existing `create_mcp_server`, `list_tools`, and `abap_program_write` flow to verify `run()` is not removing `system` before validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sapclimcp/argparsertool.py`:
- Around line 157-163: `ArgParserTool._argument_spec_to_json_spec()` currently
treats `action="store_const"` as optional but still falls through to
`_builtin_to_spec(None)`, which triggers `Unsupported type`. Update the
`optional_action` handling in `argparsertool.py` so `store_const` is either
removed from that list or explicitly mapped to a JSON spec using
`const`/`default` before calling `_builtin_to_spec`, keeping the behavior
consistent with the other action cases.
In `@src/sapclimcp/mcptools.py`:
- Around line 388-403: `SapcliMcpTool` is removing `system` from the input
before `arg_tool.parse_args()`, which causes managed multi-system calls to miss
a required field and bypass any schema defaults. Update the flow in
`mcptools.py` so `system` is parsed/validated by `parse_args()` first, then
extracted from the parsed arguments for connection lookup in `get_connection`,
keeping the caller-supplied value available to `ConnectionPatch` and other
argument validation.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 21: Disable credential persistence on each actions/checkout step in the
CI workflow by adding persist-credentials: false to every checkout use, since
these jobs do not need Git credentials after checkout. Update each affected
checkout invocation consistently so the workflow still checks out the repository
but does not leave tokens available for later steps or artifacts.
In `@tests/test_cli_and_server.py`:
- Around line 111-140: Add a runtime regression around the managed-mode tool
path so `system` is actually validated at execution time: in the test suite near
`test_managed_mode_strips_credentials_from_all_schemas`, create a managed
multi-system/no-default scenario and invoke the relevant tool through `run()`
with an explicit `system` value, then assert it succeeds instead of raising a
missing-system error. Use the existing `create_mcp_server`, `list_tools`, and
`abap_program_write` flow to verify `run()` is not removing `system` before
validation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4372b940-47c8-4b41-8b1c-6c8561f4eef0
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.github/workflows/ci.yml.gitignore.pylintrcMakefileREADME.mddev-requirements.txtpyproject.tomlrequirements.txtsrc/sapcli-mcp-client.pysrc/sapcli-mcp-server.pysrc/sapclimcp/__main__.pysrc/sapclimcp/argparsertool.pysrc/sapclimcp/cli.pysrc/sapclimcp/config.pysrc/sapclimcp/errors.pysrc/sapclimcp/mcptools.pysrc/sapclimcp/server.pysrc/sapclimcp/toolpatches.pystart-mcp.pytests/e2e/__init__.pytests/e2e/conftest.pytests/e2e/helpers.pytests/e2e/test_00_connectivity.pytests/e2e/test_01_package.pytests/e2e/test_02_program.pytests/e2e/test_03_class.pytests/e2e/test_04_interface.pytests/e2e/test_05_functiongroup.pytests/e2e/test_06_ddl.pytests/e2e/test_07_table.pytests/e2e/test_08_structure.pytests/test_argparsertool.pytests/test_cli_and_server.pytests/test_config.pytests/test_errors.pytests/test_sapcli_mcp_server.pytests/test_toolpatches.py
💤 Files with no reviewable changes (2)
- requirements.txt
- dev-requirements.txt
✅ Files skipped from review due to trivial changes (3)
- tests/e2e/init.py
- .gitignore
- .pylintrc
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/e2e/test_01_package.py
- src/sapcli-mcp-server.py
- src/sapclimcp/main.py
- tests/e2e/test_02_program.py
- tests/e2e/test_04_interface.py
- Makefile
- tests/e2e/test_05_functiongroup.py
- tests/e2e/test_06_ddl.py
- pyproject.toml
- tests/e2e/test_03_class.py
- tests/e2e/test_00_connectivity.py
- src/sapclimcp/errors.py
- tests/test_errors.py
- tests/e2e/test_08_structure.py
- tests/test_argparsertool.py
- tests/e2e/test_07_table.py
- src/sapcli-mcp-client.py
- src/sapclimcp/toolpatches.py
| optional_action = kwargs.get("action") in [ | ||
| "store_true", | ||
| "store_false", | ||
| "count", | ||
| "append", | ||
| "store_const", | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether this repository defines any store_const arguments.
rg -n -C3 "action=[\"']store_const[\"']|action=.*store_const" .Repository: jfilak/mcp-sapcli
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File outline =="
ast-grep outline src/sapclimcp/argparsertool.py --view expanded || true
echo
echo "== Relevant section of src/sapclimcp/argparsertool.py =="
sed -n '130,220p' src/sapclimcp/argparsertool.py
echo
echo "== Search for argparse action usage =="
rg -n -C2 "action\s*=\s*['\"](store_true|store_false|count|append|store_const)['\"]|store_const|store_true|store_false|count|append" src . || trueRepository: jfilak/mcp-sapcli
Length of output: 45045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '40,78p' src/sapclimcp/argparsertool.pyRepository: jfilak/mcp-sapcli
Length of output: 1604
store_const needs explicit schema handling or should be removed from the optional-action list. ArgParserTool._argument_spec_to_json_spec() still falls through to _builtin_to_spec(None) for action="store_const", so registering one will raise Unsupported type. Add a const/default mapping, or drop it from optional_action.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sapclimcp/argparsertool.py` around lines 157 - 163,
`ArgParserTool._argument_spec_to_json_spec()` currently treats
`action="store_const"` as optional but still falls through to
`_builtin_to_spec(None)`, which triggers `Unsupported type`. Update the
`optional_action` handling in `argparsertool.py` so `store_const` is either
removed from that list or explicitly mapped to a JSON spec using
`const`/`default` before calling `_builtin_to_spec`, keeping the behavior
consistent with the other action cases.
| # Work on a copy to avoid mutating the caller's dict | ||
| arguments = dict(arguments) | ||
| system = arguments.pop("system", None) | ||
|
|
||
| # Resolve connection from manager if available | ||
| connection = None | ||
| if self.connection_manager is not None: | ||
| try: | ||
| connection = self.connection_manager.get_connection(system, self.arg_tool.conn_type) | ||
| except ConfigError as ex: | ||
| raise SapcliCommandToolError(str(ex)) from ex | ||
|
|
||
| try: | ||
| cmd_args = self.arg_tool.parse_args(arguments) | ||
| except argparsertool.MissingArgument as ex: | ||
| raise SapcliCommandToolError(str(ex)) | ||
|
|
||
| # pylint: disable-next=comparison-with-callable | ||
| if self.arg_tool.conn_factory == sap.cli.adt_connection_from_args: | ||
| result = self._run_adt(cmd_args) | ||
| # pylint: disable-next=comparison-with-callable | ||
| elif self.arg_tool.conn_factory == sap.cli.gcts_connection_from_args: | ||
| result = self._run_gcts(cmd_args) | ||
| else: | ||
| raise SapcliCommandToolError( | ||
| f"Tool '{self.name}' uses unsupported connection type. " | ||
| "Only ADT and gCTS connections are currently supported." | ||
| raise SapcliCommandToolError(str(ex)) from ex |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse system before removing it from command args.
ConnectionPatch can mark system as required. Popping it before parse_args() makes multi-system/no-default managed calls fail as “missing system” even when the caller supplied it, and also skips schema defaults for single-system configs.
🐛 Suggested change
# Work on a copy to avoid mutating the caller's dict
arguments = dict(arguments)
- system = arguments.pop("system", None)
-
- # Resolve connection from manager if available
- connection = None
- if self.connection_manager is not None:
- try:
- connection = self.connection_manager.get_connection(system, self.arg_tool.conn_type)
- except ConfigError as ex:
- raise SapcliCommandToolError(str(ex)) from ex
try:
cmd_args = self.arg_tool.parse_args(arguments)
except argparsertool.MissingArgument as ex:
raise SapcliCommandToolError(str(ex)) from ex
+
+ system = getattr(cmd_args, "system", None)
+ if hasattr(cmd_args, "system"):
+ delattr(cmd_args, "system")
+
+ # Resolve connection from manager if available
+ connection = None
+ if self.connection_manager is not None:
+ try:
+ connection = self.connection_manager.get_connection(system, self.arg_tool.conn_type)
+ except ConfigError as ex:
+ raise SapcliCommandToolError(str(ex)) from ex📝 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.
| # Work on a copy to avoid mutating the caller's dict | |
| arguments = dict(arguments) | |
| system = arguments.pop("system", None) | |
| # Resolve connection from manager if available | |
| connection = None | |
| if self.connection_manager is not None: | |
| try: | |
| connection = self.connection_manager.get_connection(system, self.arg_tool.conn_type) | |
| except ConfigError as ex: | |
| raise SapcliCommandToolError(str(ex)) from ex | |
| try: | |
| cmd_args = self.arg_tool.parse_args(arguments) | |
| except argparsertool.MissingArgument as ex: | |
| raise SapcliCommandToolError(str(ex)) | |
| # pylint: disable-next=comparison-with-callable | |
| if self.arg_tool.conn_factory == sap.cli.adt_connection_from_args: | |
| result = self._run_adt(cmd_args) | |
| # pylint: disable-next=comparison-with-callable | |
| elif self.arg_tool.conn_factory == sap.cli.gcts_connection_from_args: | |
| result = self._run_gcts(cmd_args) | |
| else: | |
| raise SapcliCommandToolError( | |
| f"Tool '{self.name}' uses unsupported connection type. " | |
| "Only ADT and gCTS connections are currently supported." | |
| raise SapcliCommandToolError(str(ex)) from ex | |
| # Work on a copy to avoid mutating the caller's dict | |
| arguments = dict(arguments) | |
| try: | |
| cmd_args = self.arg_tool.parse_args(arguments) | |
| except argparsertool.MissingArgument as ex: | |
| raise SapcliCommandToolError(str(ex)) from ex | |
| system = getattr(cmd_args, "system", None) | |
| if hasattr(cmd_args, "system"): | |
| delattr(cmd_args, "system") | |
| # Resolve connection from manager if available | |
| connection = None | |
| if self.connection_manager is not None: | |
| try: | |
| connection = self.connection_manager.get_connection(system, self.arg_tool.conn_type) | |
| except ConfigError as ex: | |
| raise SapcliCommandToolError(str(ex)) from ex |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sapclimcp/mcptools.py` around lines 388 - 403, `SapcliMcpTool` is
removing `system` from the input before `arg_tool.parse_args()`, which causes
managed multi-system calls to miss a required field and bypass any schema
defaults. Update the flow in `mcptools.py` so `system` is parsed/validated by
`parse_args()` first, then extracted from the parsed arguments for connection
lookup in `get_connection`, keeping the caller-supplied value available to
`ConnectionPatch` and other argument validation.
1 participant
What this is
First — thanks for sapcli and mcp-sapcli; they're what made this possible. This is an
unsolicited contribution, so please take it in whole, in part, or not at all — the 7
commits split naturally into ~3 reviewable chunks (packaging · core config+server · CI/E2E)
if you'd rather take it incrementally.
This contributes back the "server-managed mode" my fork (
OstryP/mcp-sapcli) has beenrunning daily against four ABAP systems. The driving constraint: it has to work in a
corporate SSO environment, across multiple systems, without the LLM ever seeing
credentials.
In the current upstream model, connection/credential params are part of each tool's
schema, so the caller (the LLM) supplies them. Server-managed mode inverts that: the
server owns connections (named systems in a config file, a default-system selector),
strips the credential/connection parameters out of the generated tool schemas, and
resolves the connection itself per call. Tools expose only business parameters plus an
optional
systemselector.This is more opinionated than a thin CLI wrapper — see "Direction" below; happy to
discuss scope before you take any of it.
Backward compatible. Without
--config, the server runs in the existing (legacy)mode — connection/credential parameters stay in the tool schemas and every call behaves
exactly as it does today. Server-managed mode (credential stripping + named systems)
activates only when a config file is supplied.
What's in it (7 commits)
pyproject.toml(PEP 621,sapcli-mcpentry point, keyring as an opt-in[keyring]extra, pinned sapcli),uv.lock,python -m sapclimcp. Keeps yourexisting flake8 + pylint + mypy lint stack (just extends the
Makefiletargets and[dev]extra to cover the newsapclimcppackage).config.py(named systems,SecretReflazykeyring:/$ENV/literal resolution,CookieSessionInitializervia yourHTTPSessionInitializerprotocol,ConnectionManagercache with TTL/eviction/retry-on-401) and
errors.py(actionable messages + the package exceptionhierarchy). New modules.
ConnectionPatchstrips credentials from tool schemas,SourceDataPatchhandles source payloads,mcptoolsresolves the connectionserver-side. Edits existing
argparsertool.py/mcptools.py/toolpatches.py.server.py(create_mcp_server+ the MCPserver instructions) and
cli.py, the unified entry point (sapcli-mcp/python -m sapclimcp) that parses--config/--stdio/--host/--log-level, selectsthe transport, and provides
credential set/get/deleteover the OS keyring. Adds newmodules; edits the
src/sapcli-mcp-*.pyscripts.install matrix across
{none, keyring}extras, pip-audit. New.sandbox-tested annotations.
280 unit tests pass; flake8 + pylint (10.00/10) + mypy clean.
Bugs fixed along the way
source_datasurfaced a misleading "likely a bug" error → now a dedicatedToolInputError(based onException, notValueError, so a genuineUnicodeEncodeError/ValueErrorstill bubbles to the logger with a stack trace).user+passwordat configload, plus fail-fast (clear error naming the host) when a
$ENV/keyring:ref resolvesto empty at connect time.
side-effect flag.
MCP_SERVER_INSTRUCTIONSdescribed CLIENT as letters+digits → corrected to "3-digit number".point 5); one other suspected issue was checked and verified to be a non-issue.
Discussion points / deliberate non-decisions
Listed so nothing surprises you post-merge; several are genuinely your call.
pyproject.tomladds standard packaging(deps,
sapcli-mcpentry point, editable install) and removesrequirements.txt/dev-requirements.txt. Your flake8 + pylint + mypy stack and its configs areunchanged — the
Makefilelint/checktargets just also cover the newsapclimcppackage, and
[dev]now installs flake8/pylint/mypy somake checkworks afterpip install -e .[dev]. The one user-facing change:pip install -r requirements.txtbecomes
pip install -e .(noted in the README). Happy to keeprequirements.txtalongside
pyproject.tomlif you'd rather not drop it.requires-python = ">=3.12". Follows sapcli's own floor — the pinned sapclirequires
>=3.12, so this isn't an arbitrary bump. (The README rewrite in this PRupdates your "Python => 3.10" line to ">= 3.12" to match.)
sapclipin.pyproject.tomlpinssapcli @ git+...@<sha>for deterministicCI. For a co-released upstream you may prefer a floating ref or release tag.
uv.lockin-tree. Committed for reproducibility; you may prefer to.gitignoreit.
src/sapcli-mcp-client.py) carries literal sandbox passwords. Thispre-existing dev/demo script hardcodes
DEVELOPER+ sandbox-password literals(
ABAPtr2023#00,Welcome1!) — carried over verbatim from your current copy, notintroduced here (my only functional change modernizes its
--localimport; the rest isformatting). Flagging it plainly since this is a credential-security-focused PR: happy to
parameterize them (env-var / placeholder) if you'd prefer them out of the tree, or leave
them as your existing content — your call. The script is otherwise unused by the package.
start-mcp.py. A local convenience launcher; trivial and probably not worthupstreaming — drop if you like.
keyringoptional. Moved to an opt-in[keyring]extra with a soft import, so$ENV/literal-only users don't need it. (Note it still arrives transitively viafastmcp → py-key-value-aio[keyring]; the extra controls only our declared dep.)Known limitations / threat-model notes (server-managed mode)
Surfaced by review; left as documented limitations for you to weigh rather than
pre-emptively solved, since each is a design/scope call on your repo:
--config+ a non-loopback--host, the serveris an unauthenticated proxy to SAP using stored credentials (FastMCP HTTP carries no
auth). Intended deployment is loopback / behind a reverse proxy. Could add a guardrail
that warns/refuses non-loopback
--hosttogether with--config— happy to, if you'dlike it in scope.
config_*tools are still registered but can't run in either mode(they have no
conn_type); the README already flags them "Currently broken". Pre-existing;the clean fix (dispatch them connectionless, or stop registering them) is a separate change.
--experimental+ server-managed = unrestricted ABAP/OSQL execution againststored-credential systems, with no per-tool/per-system allowlist. Today "experimental"
is effectively "trusted operator / sandbox". An allowlist could be added if you want one.
(system, conn_type)and shared across callers.Safe under the single-worker sync execution model; concurrent HTTP callers could in
principle interleave stateful LOCK/UNLOCK (the lock-leak bug class I fixed in my fork in
OstryP/mcp-sapcli#17). Not enforced — documenting in case HTTP concurrency is ever enabled.generic
SAPCliError) leaves a stale connection cached up tocache_ttl_seconds(default 3600s) with no self-heal. Broadening the evict-and-retry trigger is an option.
Deferred follow-up (filed separately)
The bespoke config layer could eventually migrate onto sapcli's native
~/.sapcli/config.yml+auth_plugininstead of a parallel implementation. That's amulti-week refactor with open design questions (connection caching for a long-lived
server vs sapcli's one-shot CLI, cookie-refresh under
auth_plugin, test rewrite), soI've captured the analysis as a separate issue rather than blocking this PR on it:
#1.
A few questions for you
Since this is a sizable, opinionated contribution, a few things I'd rather hear your
call on than assume:
prefer, is (1) packaging/CLI, (2) config + ConnectionManager + tests, (3) CI/E2E.
keep
mcp-sapclia thin wrapper, tell me which pieces (if any) fit and I'll trim.0.1.0+ PyPI work if you want it; your call.