Skip to content

fix: actionable diagnostic for passphrase-protected SSH clones (closes #1976)#2244

Open
danielmeppiel wants to merge 2 commits into
mainfrom
fix/1976-ssh-passphrase-diagnostic
Open

fix: actionable diagnostic for passphrase-protected SSH clones (closes #1976)#2244
danielmeppiel wants to merge 2 commits into
mainfrom
fix/1976-ssh-passphrase-diagnostic

Conversation

@danielmeppiel

Copy link
Copy Markdown
Collaborator

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.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 16, 2026 07:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/stdout content (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

Comment on lines +722 to +733
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>
@danielmeppiel danielmeppiel force-pushed the fix/1976-ssh-passphrase-diagnostic branch from 67601b6 to 485fda3 Compare July 16, 2026 07:27
@danielmeppiel

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_now

Replaces opaque exit-128 on SSH passphrase/publickey failures with actionable ssh-agent and HTTPS guidance, preserving non-interactive safety.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All nine panelists converged on a clean pass with zero open findings. The fix stays inside the error-path-only contract: no credential negotiation, no retry, no raw tty prompt. It detects two distinct SSH failure modes (passphrase-protected key and explicit publickey denial) from captured stderr and emits static, actionable text. The pure-function extraction makes the diagnostic testable in isolation, and the shepherd fold closed the coverage and changelog gaps before terminal review.

Strategically this is a high-leverage reliability fix. SSH passphrase failures are a sharp first-run edge in CI and codespace-style environments. The fix reinforces APM's positioning as the tool that tells users precisely what went wrong and how to fix it, rather than swallowing errors or attempting opaque credential magic.

Aligned with: secure by default: GIT_TERMINAL_PROMPT=0 stays enforced and no passphrase prompt can leak into non-interactive subprocesses; pragmatic as npm: users get concrete remediation steps (ssh-add, deploy key, HTTPS) instead of a raw exit code; multi-harness multi-host: guidance covers local dev, CI, and HTTPS fallback.

Growth signal. This is a support-ticket eliminator: "APM tells you what to do when your SSH key has a passphrase" is worth highlighting in release notes.

Panel summary

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
Loading
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
Loading

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=0 for 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 -- deleted src/apm_cli/deps/bare_cache.py explicit 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] SSH connection with passphrase fails to clone

2 participants