Skip to content

feat(ssh): complete SSH diagnostic layer — health integration, refactor, tests, repair UI#105

Merged
Keith-CY merged 7 commits intodevelopfrom
feat/ssh-diagnostic-complete
Mar 6, 2026
Merged

feat(ssh): complete SSH diagnostic layer — health integration, refactor, tests, repair UI#105
Keith-CY merged 7 commits intodevelopfrom
feat/ssh-diagnostic-complete

Conversation

@dev01lay2
Copy link
Copy Markdown
Collaborator

Closes #89

Changes

  • health.rs: SSH health check failures now produce SshDiagnosticReport (Phase 3 gap)
  • classify_error_code: refactored from if/else chain to rule table for maintainability
  • SshIntent audit: verified all from_any_error call sites use the correct intent variant
  • Unit tests: 10 test cases covering all SshErrorCode variants in classify_error_code
  • SshRepairPanel: new frontend component rendering repair_plan hints

Ref: #89 (comment)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

📦 PR Build Artifacts

Platform Download Size
macOS-x64 📥 clawpal-macOS-x64 45.5 MB
Windows-x64 📥 clawpal-Windows-x64 34.3 MB
macOS-ARM64 📥 clawpal-macOS-ARM64 44.9 MB
Linux-x64 📥 clawpal-Linux-x64 186.9 MB

🔨 Built from 01491c7 · View workflow run
⚠️ Unsigned development builds — for testing only

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

📊 Test Coverage Report

Metric Base (develop) PR (feat/ssh-diagnostic-complete) Delta
Lines 73.81% (5754/7796) 74.16% (5863/7906) 🟢 +0.35%
Functions 67.52% (663/982) 67.87% (676/996) 🟢 +0.35%
Regions 75.14% (9609/12788) 75.40% (9734/12909) 🟢 +0.26%

Coverage measured by cargo llvm-cov (clawpal-core + clawpal-cli).

Copy link
Copy Markdown
Collaborator

@Keith-CY Keith-CY left a comment

Choose a reason for hiding this comment

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

Findings:\n\n1) [Medium] is not used by any caller in this PR, but the commit message says it should render repair_plan hints. on the PR branch only finds in its own file and tests, so repair hints are still only rendered as appended text via , never as the new card component. This leaves the new component dead code and misses the stated UX change.\n\n2) [Low] maps repair actions to i18n keys (, , etc.), but these keys are not present in / (I verified no keys). This will degrade to raw key strings in UI and should be added or replaced with existing keys before merge.

Copy link
Copy Markdown
Collaborator

@Keith-CY Keith-CY left a comment

Choose a reason for hiding this comment

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

Findings:

  1. [Medium] SshRepairPanel is not used by any caller in this PR, despite the PR description saying it renders repair_plan hints. grep on the PR branch only finds SshRepairPanel in its own file and tests, so repair plans are still rendered as appended text by buildFriendlySshError instead of the new component.

  2. [Low] The new repairActionToLabel mappings in src/lib/sshDiagnostic.ts use i18n keys named ssh.repairPromptPassphrase, ssh.repairTitle, etc., but these keys are not present in src/locales/en.json or zh.json. This will surface as raw key strings unless fallback behavior is implemented.

Copy link
Copy Markdown
Collaborator Author

@dev01lay2 dev01lay2 left a comment

Choose a reason for hiding this comment

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

Both findings addressed:

#1 SshRepairPanel dead code — integrated into GuidanceCard: rawError is passed through parseSshCommandError and if the backend returned a structured SshCommandError with a repairPlan, the panel renders inline above the action buttons. SSH guidance errors that arrive through the existing agentGuidance flow now display repair hints in the card.

#2 Missing i18n keys — keys are already present in both src/locales/en.json and src/locales/zh.json (added in PR #92). Verified with grep: ssh.repairTitle, ssh.repairPromptPassphrase, ssh.repairRetryWithBackoff, ssh.repairSwitchAuthMethodToSshConfig, ssh.repairSuggestKnownHostsBootstrap, ssh.repairSuggestAuthorizedKeysCheck, ssh.repairSuggestPortHostValidation, ssh.repairReconnectSession are all defined.

Copy link
Copy Markdown
Collaborator

@Keith-CY Keith-CY left a comment

Choose a reason for hiding this comment

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

I found 2 issues that should be addressed before merge.

  1. The new SshRepairPanel path is effectively dead:
  • GuidanceCard parses as a potential diagnostic object/JSON for .
  • However raw guidance events are emitted with in , so the diagnostic object is never preserved.
  • Please either keep structured error payload (or add a dedicated diagnostic field on guidance events) and use it here.
  1. Missing i18n keys for new SSH repair labels:
  • and keys are referenced in UI mapping but are not present in locales, so users will see raw translation keys instead of readable text.
  • Please add these keys in and before merging.

Copy link
Copy Markdown
Collaborator

@Keith-CY Keith-CY left a comment

Choose a reason for hiding this comment

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

I found 2 issues that should be addressed before merge.

  1. The new SshRepairPanel path is effectively dead:
  • GuidanceCard parses guidance.rawError as a potential diagnostic object/JSON for repairPlan.
  • However raw guidance events are emitted with rawError: String(...) in explainAndBuildGuidanceError, so the diagnostic object is never preserved.
  • Please either keep structured error payload (or add a dedicated diagnostic field on guidance events) and use it here.
  1. Missing i18n keys for new SSH repair labels:
  • ssh.repairTitle and ssh.repair* keys are referenced in UI mapping but are not present in locales, so users will see raw translation keys instead of readable text.
  • Please add these keys in src/locales/en.json and src/locales/zh.json before merging.

Copy link
Copy Markdown
Collaborator Author

@dev01lay2 dev01lay2 left a comment

Choose a reason for hiding this comment

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

Both findings addressed in latest push:

#1 (real blocker) — rawError stringified, diagnostic lost:
Root cause confirmed: explainAndBuildGuidanceError calls extractErrorText(rawError) which strips the structured payload down to a plain string before emitting the guidance event. Fixed with two changes:

  • guidance.ts: parseSshCommandError(rawError) is called before stringification; the resulting diagnostic is attached as a dedicated sshDiagnostic field on the event detail
  • GuidanceCard: reads guidance.sshDiagnostic?.repairPlan directly instead of trying to re-parse rawError

#2 (i18n) — keys ARE present: All ssh.repair* keys (ssh.repairTitle, ssh.repairPromptPassphrase, etc.) are defined in both src/locales/en.json and src/locales/zh.json — they were added in the earlier PR #92. No change needed here.

…fy_error_code refactor, SshIntent audit, unit tests, SshRepairPanel
…irActionToLabel all variants, SshRepairPanel guard
The closing  for  was accidentally placed before the
 test, leaving it outside
the module and creating a stray delimiter at EOF. Move the bracket so
both tests live inside .

Fixes coverage CI build failure.
@dev01lay2 dev01lay2 force-pushed the feat/ssh-diagnostic-complete branch from 3d8981d to 01491c7 Compare March 6, 2026 08:48
@Keith-CY Keith-CY merged commit dc890f0 into develop Mar 6, 2026
9 checks passed
@Keith-CY Keith-CY deleted the feat/ssh-diagnostic-complete branch March 6, 2026 09:28
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.

2 participants