Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end coverage for the apx_recipe_run tool by provisioning an SSH-accessible target during CI, and improves operational safety by redacting sensitive data and optionally returning an APX debug trace.
Changes:
- Add redaction helpers and per-command debug tracing to APX target preparation/workload execution utilities.
- Extend integration tests to mount SSH credentials into the container and invoke
apx_recipe_run. - Update CI workflow to start an SSH server on the runner and provide credentials to the test; update Dockerfile defaults and harden curl usage.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
mcp-local/utils/apx.py |
Redacts SSH key material/paths from outputs and records a redacted debug trace for APX command execution. |
mcp-local/server.py |
Adds default SSH env paths and conditionally includes APX debug trace in tool responses. |
mcp-local/tests/test_mcp.py |
Generates/mounts SSH materials for container tests and adds an APX recipe-run tool call. |
mcp-local/tests/constants.py |
Introduces an apx_recipe_run MCP request payload used by the integration test. |
mcp-local/Dockerfile |
Hardens remote install curls and sets default SSH_KEY_PATH / KNOWN_HOSTS_PATH env vars. |
.github/workflows/integration-tests.yml |
Provisions an SSH target user + sshd on the runner and exports SSH inputs for tests. |
mcp-local/utils/tests/test.py |
Adds a trivial “Hello world” script (currently not wired into the APX test flow). |
mcp-local/utils/tests/known_hosts |
Empty test fixture file. |
mcp-local/utils/tests/apx.pem |
Empty test fixture file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sudo systemctl restart ssh || sudo service ssh restart || sudo /usr/sbin/sshd | ||
|
|
||
| ssh-keyscan -H 127.0.0.1 > "${KNOWN_HOSTS_PATH}" || true | ||
| ssh-keyscan -H 172.17.0.1 >> "${KNOWN_HOSTS_PATH}" || true | ||
| if [ ! -s "${KNOWN_HOSTS_PATH}" ]; then | ||
| touch "${KNOWN_HOSTS_PATH}" | ||
| fi | ||
| chmod 644 "${KNOWN_HOSTS_PATH}" | ||
|
|
||
| ssh -o BatchMode=yes -o StrictHostKeyChecking=yes -o UserKnownHostsFile="${KNOWN_HOSTS_PATH}" -i "${KEY_PATH}" "${APX_SSH_USER}@127.0.0.1" "sudo -n true && echo ssh_ok" | ||
|
|
There was a problem hiding this comment.
ssh-keyscan is allowed to fail (|| true) and there is no retry/backoff after restarting sshd. If sshd is not ready yet, the known_hosts file can end up empty and the subsequent ssh -o StrictHostKeyChecking=yes ... will fail, causing flaky CI. Add a small retry loop for ssh-keyscan (and/or wait for port 22) and fail early if the file is still empty after retries.
| @@ -0,0 +1 @@ | |||
| print("Hello world") No newline at end of file | |||
There was a problem hiding this comment.
This adds a standalone utils/tests/test.py file, but the integration test workload command currently points elsewhere and the APX workload runs on the SSH target machine (not inside the container). If this script is intended to be used as the APX workload, it either needs to be present on the target machine (or copied there in the workflow) and referenced by that path, otherwise it should be removed to avoid dead test artifacts.
| print("Hello world") | |
| """ | |
| This file is intentionally left non-executable. | |
| The APX/integration workload is defined and invoked elsewhere; this module | |
| should not be treated as a standalone test workload entrypoint. | |
| """ |
| RUN bash -o pipefail -c 'set -eux; \ | ||
| curl -fsSL --retry 5 --retry-delay 2 --retry-all-errors \ | ||
| https://raw.githubusercontent.com/JoeStech/arm-linux-migration-tools/main/scripts/install.sh | bash' |
There was a problem hiding this comment.
The build still downloads and executes a remote install script via curl ... | bash from a moving branch (main). Even with -fsSL and retries, this is a supply-chain risk and can break builds when upstream changes. Prefer pinning to a specific commit/tag (or vendoring the script) and verifying integrity (e.g., checksum/signature) before execution.
| known_hosts_path.write_text( | ||
| "172.17.0.1 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIFakeKnownHostKeyForIntegrationTestsOnly\n", | ||
| encoding="utf-8", |
There was a problem hiding this comment.
When APX_TEST_KNOWN_HOSTS_PATH is not provided, this writes a fake host key entry. That will cause SSH strict host key checking to fail for any real SSH connection, making local runs of the APX integration test fail unexpectedly. Consider skipping the APX recipe-run portion unless APX_TEST_KNOWN_HOSTS_PATH is set (or generate known_hosts via ssh-keyscan against the chosen target).
| known_hosts_path.write_text( | |
| "172.17.0.1 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIFakeKnownHostKeyForIntegrationTestsOnly\n", | |
| encoding="utf-8", | |
| pytest.skip( | |
| "Skipping APX integration recipe run: APX_TEST_KNOWN_HOSTS_PATH " | |
| "must be set to a valid known_hosts file for SSH strict host key checking." |
…t being invoked properly (file path wrong)
No description provided.