Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies Arm Performix (APX) SSH configuration by removing the need to explicitly set SSH_KEY_PATH/KNOWN_HOSTS_PATH in MCP client Docker args, and instead auto-discovering mounted SSH files under /run/keys.
Changes:
- Remove
-e SSH_KEY_PATH=.../-e KNOWN_HOSTS_PATH=...from client configuration examples and update docs accordingly. - Add mount auto-discovery logic in
mcp-local/utils/apx.py(parsing/proc/self/mounts) and wire it intoapx_recipe_run. - Update Performix deployment guidance to align with the new mount-based approach.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| README.md | Updates MCP client configuration examples to drop explicit SSH env vars and document /run/keys auto-discovery. |
| mcp-local/utils/apx.py | Adds logic to discover SSH key/known_hosts mounts under /run/keys and set env vars automatically. |
| mcp-local/server.py | Uses the new auto-discovery function in apx_recipe_run and updates the config error messaging. |
| mcp-local/performix-deployment-scenarios.md | Updates Performix instructions to emphasize mounting SSH files under /run/keys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for line in mounts_path.read_text(encoding="utf-8").splitlines(): | ||
| parts = line.split() | ||
| if len(parts) < 2: | ||
| continue | ||
| target = _decode_mount_field(parts[1]) | ||
| if target == str(run_keys_dir) or target.startswith(f"{run_keys_dir}/"): | ||
| mount_targets.append(target) |
There was a problem hiding this comment.
discover_run_keys_mounts() reads /proc/self/mounts via read_text() without handling OSError/UnicodeDecodeError. If the mounts file exists but is unreadable or contains unexpected encoding, this will raise and can crash apx_recipe_run instead of returning the existing structured error response. Consider wrapping the read/parse loop in a try/except and returning an empty list (or an explicit error indicator) on failure.
| for line in mounts_path.read_text(encoding="utf-8").splitlines(): | |
| parts = line.split() | |
| if len(parts) < 2: | |
| continue | |
| target = _decode_mount_field(parts[1]) | |
| if target == str(run_keys_dir) or target.startswith(f"{run_keys_dir}/"): | |
| mount_targets.append(target) | |
| try: | |
| for line in mounts_path.read_text(encoding="utf-8").splitlines(): | |
| parts = line.split() | |
| if len(parts) < 2: | |
| continue | |
| target = _decode_mount_field(parts[1]) | |
| if target == str(run_keys_dir) or target.startswith(f"{run_keys_dir}/"): | |
| mount_targets.append(target) | |
| except (OSError, UnicodeDecodeError): | |
| return [] |
There was a problem hiding this comment.
@JoeStech This seems like a worthwhile addition
| mount_targets: List[str] = [] | ||
| for line in mounts_path.read_text(encoding="utf-8").splitlines(): | ||
| parts = line.split() | ||
| if len(parts) < 2: | ||
| continue | ||
| target = _decode_mount_field(parts[1]) | ||
| if target == str(run_keys_dir) or target.startswith(f"{run_keys_dir}/"): | ||
| mount_targets.append(target) | ||
|
|
There was a problem hiding this comment.
Auto-discovery currently depends on mount targets under /run/keys. If users mount a directory (e.g. -v ~/.ssh:/run/keys:ro), /proc/self/mounts will typically only contain /run/keys and not per-file mountpoints, so _select_known_hosts_path/_select_ssh_key_path will fail to resolve paths. Either (a) update discovery to also scan the /run/keys directory contents when /run/keys itself is mounted, or (b) clarify in docs that individual files must be bind-mounted.
There was a problem hiding this comment.
I think this is true. I saw that separately and was wondering the same thing. Commented on the exact line below.
| ``` | ||
|
|
||
| **Note**: Replace `/path/to/your/workspace` with the actual path to your project directory that you want the MCP server to access. If you are enabling Arm Performix, also replace the `/path/to/your/ssh/private_key` and `/path/to/your/ssh/known_hosts` paths with your local files. | ||
| **Note**: Replace `/path/to/your/workspace` with the actual path to your project directory that you want the MCP server to access. If you are enabling Arm Performix, also replace the `/path/to/your/ssh/private_key` and `/path/to/your/ssh/known_hosts` paths with your local files. The MCP container auto-discovers files mounted under `/run/keys`, as shown in the configs above. |
There was a problem hiding this comment.
The note implies auto-discovery will always work for anything mounted under /run/keys, but the implementation only sets SSH_KEY_PATH/KNOWN_HOSTS_PATH when it can uniquely identify a known_hosts mount and a single key-like mount. Consider clarifying the constraints (expected filenames/uniqueness) and documenting that users can still set SSH_KEY_PATH and KNOWN_HOSTS_PATH explicitly when auto-discovery is ambiguous.
| **Note**: Replace `/path/to/your/workspace` with the actual path to your project directory that you want the MCP server to access. If you are enabling Arm Performix, also replace the `/path/to/your/ssh/private_key` and `/path/to/your/ssh/known_hosts` paths with your local files. The MCP container auto-discovers files mounted under `/run/keys`, as shown in the configs above. | |
| **Note**: Replace `/path/to/your/workspace` with the actual path to your project directory that you want the MCP server to access. If you are enabling Arm Performix, also replace the `/path/to/your/ssh/private_key` and `/path/to/your/ssh/known_hosts` paths with your local files. The MCP container can auto-discover SSH files mounted under `/run/keys` when it can uniquely identify a `known_hosts` file and a single key-like file, as shown in the configs above (for example, `/run/keys/known_hosts` and `/run/keys/ssh-key.pem`). If you mount multiple candidate key files, use different filenames, or otherwise make auto-discovery ambiguous, set `SSH_KEY_PATH` and `KNOWN_HOSTS_PATH` explicitly in your MCP client configuration. |
| The MCP container will discover these mounts from `/proc/self/mounts` and set the internal `SSH_KEY_PATH` and `KNOWN_HOSTS_PATH` values automatically. | ||
|
|
There was a problem hiding this comment.
This states the container will discover mounts and set SSH_KEY_PATH/KNOWN_HOSTS_PATH automatically, but discovery can fail if /run/keys is mounted as a directory or if multiple candidate mounts exist. Consider softening the wording (e.g., “attempts to auto-discover”) and adding a fallback instruction to set SSH_KEY_PATH/KNOWN_HOSTS_PATH explicitly when discovery is ambiguous.
| The MCP container will discover these mounts from `/proc/self/mounts` and set the internal `SSH_KEY_PATH` and `KNOWN_HOSTS_PATH` values automatically. | |
| The MCP container attempts to discover these mounts from `/proc/self/mounts` and set the internal `SSH_KEY_PATH` and `KNOWN_HOSTS_PATH` values automatically. | |
| If discovery is ambiguous or does not succeed (for example, if `/run/keys` is mounted as a directory or multiple candidate mounts exist), set `SSH_KEY_PATH` and `KNOWN_HOSTS_PATH` explicitly in the MCP server/container configuration. |
| apx_dir = os.environ.get("APX_HOME", "/opt/apx") | ||
| key_path = os.getenv("SSH_KEY_PATH") | ||
| known_hosts_path = os.getenv("KNOWN_HOSTS_PATH") | ||
| ssh_mount_env = resolve_apx_ssh_mount_env() | ||
| key_path = ssh_mount_env["key_path"] | ||
| known_hosts_path = ssh_mount_env["known_hosts_path"] | ||
|
|
||
| if not key_path or not known_hosts_path: | ||
| return { | ||
| "status": "error", | ||
| "recipe": recipe, | ||
| "stage": "config_validation", | ||
| "message": "Missing SSH configuration for APX target access.", | ||
| "suggestion": "Set SSH_KEY_PATH and KNOWN_HOSTS_PATH in the MCP docker run configuration, then retry.", | ||
| "suggestion": "Mount both the SSH private key and known_hosts file into /run/keys, then retry.", | ||
| "details": ( | ||
| "SSH_KEY_PATH and KNOWN_HOSTS_PATH environment variables must be set in the docker run " | ||
| "command in the MCP config file to mount in the container to use APX." | ||
| "APX looks for SSH_KEY_PATH and KNOWN_HOSTS_PATH first, then auto-discovers mounted files " | ||
| f"under /run/keys from /proc/self/mounts. Discovered mounts: {ssh_mount_env['mount_targets']}" | ||
| ), | ||
| } |
There was a problem hiding this comment.
apx_recipe_run now has new configuration behavior (auto-discovering SSH mounts and returning a structured config_validation error with discovered mounts). The integration test suite covers several other tools but does not currently exercise this tool’s config-validation path; adding a test that calls apx_recipe_run without SSH env/mounts and asserts the error shape would help prevent regressions in the new auto-discovery logic.
jp-arm
left a comment
There was a problem hiding this comment.
- Clarifying some key candidate names.
- Question about if they mount a directory and we let that pass through.
| for token in ("ssh", "key", ".pem", "id_rsa", "id_ed25519", "id_ecdsa", "id_dsa") | ||
| ) | ||
| ] | ||
| return key_like[0] if len(key_like) == 1 else None |
There was a problem hiding this comment.
For this line, we end up checking in server.py if ssh path is set. If there is more than one key, then we return None, however in the error message returned to the agent, we don't mention that there can only be one key in the mount targets. And then second to that, I wonder about the candidates above if someone doesn't follow those naming conventions (i.e. renames an rsa key to anything else and the name ends up being ec2, for example).
An option could be to check the beginning of the file contents for the ------BEGIN ... ---- style thing. But that is much slower and maybe not necessary.
| for line in mounts_path.read_text(encoding="utf-8").splitlines(): | ||
| parts = line.split() | ||
| if len(parts) < 2: | ||
| continue | ||
| target = _decode_mount_field(parts[1]) | ||
| if target == str(run_keys_dir) or target.startswith(f"{run_keys_dir}/"): | ||
| mount_targets.append(target) |
There was a problem hiding this comment.
@JoeStech This seems like a worthwhile addition
|
|
||
| def _select_ssh_key_path(mount_targets: List[str], known_hosts_path: Optional[str]) -> Optional[str]: | ||
| candidates = [path for path in mount_targets if path != known_hosts_path] | ||
| if len(candidates) == 1: |
There was a problem hiding this comment.
If this ends up being a directory, then we would pass that through and continue the mcp tool, as mentioned above
| mount_targets: List[str] = [] | ||
| for line in mounts_path.read_text(encoding="utf-8").splitlines(): | ||
| parts = line.split() | ||
| if len(parts) < 2: | ||
| continue | ||
| target = _decode_mount_field(parts[1]) | ||
| if target == str(run_keys_dir) or target.startswith(f"{run_keys_dir}/"): | ||
| mount_targets.append(target) | ||
|
|
There was a problem hiding this comment.
I think this is true. I saw that separately and was wondering the same thing. Commented on the exact line below.
No description provided.