fix: actionable diagnostic for passphrase-protected SSH clones (closes #1976)#2244
fix: actionable diagnostic for passphrase-protected SSH clones (closes #1976)#2244danielmeppiel wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves dependency clone failure diagnostics when git clone fails in APM’s intentionally non-interactive SSH execution path (notably for passphrase-protected SSH keys), by detecting common passphrase/publickey markers in captured git stderr and appending actionable guidance (e.g., use ssh-agent / ssh-add, CI deploy keys, or token-backed HTTPS).
Changes:
- Add SSH passphrase/publickey failure detection based on captured git error text and append an actionable diagnostic to the clone failure message.
- Include captured
stderr/stdoutcontent (when available) in the “Last error” text used for clone failure reporting. - Add a regression unit test covering the new passphrase-protected SSH diagnostic behavior.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/deps/test_bare_cache_clone_failure.py | Adds a regression test asserting the new actionable diagnostic is present for passphrase-related SSH clone failures. |
| src/apm_cli/deps/bare_cache.py | Adds helpers to extract git failure text, detect passphrase/publickey SSH failures, and append an actionable diagnostic; updates error rendering to include captured stderr/stdout. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| def _git_failure_text(error: Exception) -> str: | ||
| """Return command error text including captured stderr/stdout when present.""" | ||
| parts = [str(error)] | ||
| for attr in ("stderr", "stdout"): | ||
| stream = getattr(error, attr, None) | ||
| if not stream: | ||
| continue | ||
| if isinstance(stream, bytes): | ||
| parts.append(stream.decode("utf-8", errors="replace")) | ||
| else: | ||
| parts.append(str(stream)) | ||
| return " ".join(part.strip() for part in parts if part and part.strip()) |
Adds explicit publickey-only and non-SSH negative coverage for the clone failure diagnostic, and records the user-facing fix in the changelog. Addresses panel follow-ups from test coverage and growth. apm-spec-waiver: Diagnostic-only failure text; no OpenAPM protocol behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
67601b6 to
485fda3
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Clean pure-helper extraction; missing branch coverage was folded before terminal. |
| CLI Logging Expert | 0 | 0 | 0 | Diagnostic is actionable, ASCII, and preserves the non-interactive contract. |
| DevX UX Expert | 0 | 0 | 0 | User gets concrete local and CI remedies without APM pretending it can unlock SSH keys. |
| Supply Chain Security Expert | 0 | 0 | 0 | No credential flow changed; raw stderr/stdout remains sanitized and the diagnostic is static text. |
| OSS Growth Hacker | 0 | 0 | 0 | Release-note visibility for this support-saving fix was folded into CHANGELOG. |
| Auth Expert | 0 | 0 | 0 | Auth semantics stay delegated to git/ssh-agent; no token precedence or prompt behavior changed. |
| Doc Writer | 0 | 0 | 0 | No docs drift; the error text is self-contained and the changelog now records it. |
| Test Coverage Expert | 0 | 0 | 0 | Passphrase marker, explicit SSH publickey-only, and non-SSH negative paths are covered after the fold. |
| Performance Expert | 0 | 0 | 0 | Error-path-only string scanning adds no hot-path cost. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
direction TB
class build_clone_failure_message {
<<Pure Function>>
+build_clone_failure_message(...) str
}
class _git_failure_text {
<<Pure Function>>
+_git_failure_text(error) str
}
class _is_ssh_passphrase_or_auth_failure {
<<Pure Function>>
+_is_ssh_passphrase_or_auth_failure(text, dep_ref) bool
}
class _ssh_passphrase_diagnostic {
<<Pure Function>>
+_ssh_passphrase_diagnostic(error, dep_ref) str
}
class DependencyReference {
<<ValueObject>>
+repo_url str
+host str
+explicit_scheme str
}
build_clone_failure_message ..> _ssh_passphrase_diagnostic : calls
_ssh_passphrase_diagnostic ..> _git_failure_text : calls
_ssh_passphrase_diagnostic ..> _is_ssh_passphrase_or_auth_failure : calls
_is_ssh_passphrase_or_auth_failure ..> DependencyReference : reads explicit_scheme
class _git_failure_text:::touched
class _is_ssh_passphrase_or_auth_failure:::touched
class _ssh_passphrase_diagnostic:::touched
class build_clone_failure_message:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[Clone attempt fails] --> B[build_clone_failure_message]
B --> C[_ssh_passphrase_diagnostic]
C --> D[_git_failure_text: str plus stderr/stdout]
D --> E{Passphrase marker?}
E -- yes --> H[Append ssh-agent guidance]
E -- no --> F{explicit ssh and publickey denied?}
F -- yes --> H
F -- no --> I[No SSH-specific diagnostic]
H --> J[Sanitized last error appended]
I --> J
Recommendation
Merge at will. Zero open findings, exact-head test coverage with mutation-break verification, local lint and architecture gates green, GitHub CI green, and all three strategic reservations explicitly satisfied.
Reservations carried from strategic-alignment
The parent orchestrator aligned this issue WITH the following reservations. Each was weighed by the panel this run:
- Fix must not regress non-interactive/CI usage:
GIT_TERMINAL_PROMPT=0for headless runs is correct; the bug is the missing actionable failure surface, not the suppression -- addressed by leaving clone prompting behavior unchanged and adding only post-failure diagnostics. - Preferred direction is detect ssh-agent availability and surface a human-readable diagnostic on passphrase/publickey failure, not raw tty passphrase prompts inside subprocess calls -- addressed by static guidance for
ssh-add, CI deploy keys, and token-backed HTTPS without any prompt path. - P6 Reliability over magic: no silent retry or opaque credential negotiation; the user must know exactly why the clone failed and what to do -- addressed by deterministic stderr marker detection and explicit remediation text.
Folded in this run
- (panel) Add explicit SSH publickey-only and non-SSH negative coverage for the clone failure diagnostic -- resolved in
485fda31d6e0694bf6b6b2ec3cfabc657495da49. - (panel) Add CHANGELOG entry for the user-facing SSH diagnostic improvement -- resolved in
485fda31d6e0694bf6b6b2ec3cfabc657495da49.
Regression-trap evidence (mutation-break gate)
tests/unit/deps/test_bare_cache_clone_failure.py::test_clone_failure_message_explains_explicit_ssh_publickey_failure-- deletedsrc/apm_cli/deps/bare_cache.pyexplicit SSH publickey denial branch; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/, uv run --extra dev ruff format --check src/ tests/, pylint R0801, and bash scripts/lint-auth-signals.sh passed locally on 485fda31d6e0694bf6b6b2ec3cfabc657495da49.
CI
gh pr checks 2244 --repo microsoft/apm --watch observed all checks passing on 485fda31d6e0694bf6b6b2ec3cfabc657495da49 after 1 CI recovery iteration (Spec conformance waiver commit-message rationale for diagnostic-only failure text).
Mergeability status
Captured from gh pr view 2244 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2244 | 485fda3 |
ship_now | 1 | 2 | 0 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
1 outer iteration; 2 Copilot rounds. Final panel verdict: ship_now.
Ready for maintainer review.
Full per-persona findings
All active panelists returned no open findings after the shepherd fold. Earlier signals were folded into the branch: publickey-only/negative-path tests and the changelog entry.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
SSH clones with passphrase-protected keys fail in APM's intentionally non-interactive git subprocess path, but the captured stderr was collapsed into an opaque exit-128 message. This change detects SSH passphrase/publickey failures from captured git stderr and appends an actionable diagnostic telling users to load the key into ssh-agent with ssh-add, use an ssh-agent-backed deploy key in CI, or switch to token-backed HTTPS without enabling raw passphrase prompts. Tested with uv run --extra dev pytest tests/unit/deps/test_bare_cache_clone_failure.py tests/unit/deps/test_bare_cache_sparse.py tests/unit/deps/test_shared_clone_cache.py -q; uv run --extra dev pytest tests/ -k 'git or ssh or clone or auth' -q; uv run --extra dev ruff check src/ tests/ --quiet; uv run --extra dev ruff format --check src/ tests/ --quiet. Closes #1976.