Skip to content

Preserve Windows SSH upload failures unless pwsh is missing - #25185

Merged
nwparker merged 17 commits into
mainfrom
fix/windows-ssh-pwsh-fallback-classification
Oct 4, 2026
Merged

nwparker merged 17 commits into
mainfrom
fix/windows-ssh-pwsh-fallback-classification

Conversation

@nwparker

@nwparker nwparker commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​240 $\color{#cf222e}{\Huge{\mathbf{−}}}$​4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​236
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​50 $\color{#cf222e}{\Huge{\mathbf{−}}}$​7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​43

ELI5

A Windows SSH upload that failed could be retried through the older PowerShell path merely because its filename contained 9009 or pwsh.exe. Orca now keeps that failure unless the command result actually establishes that PowerShell 7 is missing.

What Changed

Keep the command exit code and stderr on the existing SSH execution error while preserving its displayed message. The PowerShell fallback reads those fields instead of searching the formatted operation label. Generic timeouts and short-source errors do not change the host's capability verdict.

Accept the missing-command exit code or a complete missing-pwsh diagnostic. The PowerShell NormalView parser checks the command, source line, target, error class and final error identifier; it preserves horizontal separators while reconstructing wrapped physical lines and rejects repeated executable headers. Existing staging, publishing, timeout and transport order remain intact.

Why

The formatted label includes a staging filename, which is not evidence about the execution host. Reusing the existing capability fallback with a narrower error predicate preserves genuine missing-command recovery and propagates write errors. Parsing stable record identifiers avoids depending on localized explanatory prose or a fixed number of lines.

Linked Issue

Related to Windows SSH uploads in #16432 and the inherited verification failure in #25001. This correction does not close the broader upload issue or claim the original CI failure's unrecorded nonce as its sole cause.

Visual Proof

N/A — SSH file-transfer failure handling adds no rendered controls. Native missing-command stderr and complete raw before/after test output are retained.

Testing

  • Genuine Windows 11 / Windows PowerShell 5.1 and cmd.exe missing-pwsh diagnostics captured directly, then used unchanged as regression fixtures. These are native formatting controls, not a live SSH or OpenCode upload.
  • Automated before/after tests reproduce filename false positives, generic errors, mixed write errors, lost wrapped separators and repeated headers. The final separator/header controls fail three tests on unchanged predecessor production and pass after the correction.
  • Current f4e63382 passes 145 tests across six files, all three desktop typechecks, full owned lint and formatting, changed-code checks and the configured repository scan.
  • Missing PowerShell still falls back; timeouts, short sources, other missing commands and ambiguous errors propagate without poisoning the host capability verdict. Staged publishing and cleanup remain covered.

Localized and wrapped fixtures derive from the official formatter/resource sources; they are not claims of executing every locale or display width. The two direct diagnostic children have positive exit waits. The original and verifier outer Windows terminal stops remain unverifiable; their private diagnostic directory was positively observed absent while the verifier was running.

Review

Independent reviews verify the complete frozen source, all three owned files, every other file against named main f199a20c, raw checks and the causal before/after controls. Main through e8310d5a has no changed files in this scope. Fresh public CI will validate the combined merge.

Agent skill upstream boundary

  • Not applicable; no upstream skill-installer material is copied or translated.

Notes

The execution host owns capability results. This correction changes no wire fields, SSH command construction, interpreter selection, deadlines or retry policy. Local and folder workspace callers retain the same transfer path. No new live SSH/OpenCode runtime result is claimed for this commit.

Checklist

  • Focused failure-classification correction and causal controls.
  • User experience, mechanism and reason explained.
  • N/A visual proof explained.
  • Self-reviewed and independently reviewed.
  • Cross-platform, folder workspace and SSH execution boundaries considered.
  • Fresh public CI passes before merge.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 20643ad3-b36a-4769-ae4e-6fc8faabf949
📥 Commits

Reviewing files that changed from the base of the PR and between f4e6338 and c9e4ab0.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3be0f668-83b8-42ea-a3bf-76c41b78a9f1
📥 Commits

Reviewing files that changed from the base of the PR and between f62bd7d and f4e6338.

📒 Files selected for processing (3)
  • src/main/ssh/system-ssh-operation-lifecycle.ts
  • src/main/ssh/system-ssh-windows-upload.test.ts
  • src/main/ssh/system-ssh-windows-write-strategy.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The change adds SystemSshCommandExitError for nonzero or null SSH channel close codes. Windows upload logic now uses this type and specific diagnostics to determine whether PowerShell 7 is unavailable. Tests cover fallback signals, rejected errors, timeouts, and short-source failures, including their effects on capability state and file publishing.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to f4e63

No merge-blocking issue was identified in the Windows SSH upload fallback change. Normal checks can proceed.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f4e63

The change narrows fallback eligibility and prevents filenames from being mistaken for evidence that PowerShell 7 is missing. No introduced security concern was identified. Existing upload ordering and execution privileges are preserved, but interrupted and concurrent behavior has not been validated against a live Windows SSH host in this review.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A classified missing-pwsh result influences later writes sharing the existing execution-host cache key until retry eligibility returns. The key is an SSH configuration alias or a host/port identity with username when provided; the PR does not broaden this state-sharing scope.

Trust Boundaries and Controls

  • observed — The local fallback decision now distinguishes filename-bearing labels from remote execution evidence. A remote host can still supply exit 9009 and cause fallback regardless of stderr, but the base implementation already accepted that status. No new command authority is introduced by the inspected change.

Resilience and Maintainability Implications

  • observed — Timeouts and cancellation remain ordinary errors outside the missing-pwsh predicate, preventing those failures from initiating capability fallback. Cancellation does not confirm remote process termination, and discard remains best effort; these pre-existing limits mean interruption is not a guaranteed remote rollback.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preserve Windows SSH upload failures unless PowerShell is confirmed missing.
Description check ✅ Passed The description covers the required sections, explains the change and rationale, links related issues, gives detailed testing information, and marks visual proof as not applicable. It also notes that …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

  • Narrowed pwsh-absence signal. isPwshUnavailableError now rejects any error that is not a SystemSshCommandExitError and accepts only exit 9009, the exact cmd.exe message, CommandNotFoundException: pwsh.exe, or a complete PowerShell NormalView record — so a staging filename or unrelated stderr can no longer mark PowerShell 7 absent.
  • Diagnostic carried on the error. waitForChannelClose throws a new SystemSshCommandExitError (message text unchanged) exposing exitCode and stderr, letting the fallback classify on raw evidence rather than the formatted operation label.
  • Structural NormalView parser. isPowerShellMissingPwshDiagnostic rejoins wrapped physical lines and checks the source line, 8-tilde underline, CategoryInfo target, and FullyQualifiedErrorId, while rejecting repeated executable headers and mixed write errors.
  • Causal regression coverage. New it.each cases in system-ssh-windows-upload.test.ts reproduce filename false positives, write-denied stderr, mixed errors, wrapped/localized records, timeout and short-source cases.

I verified the 65 tests in the touched test file pass, and confirmed the new cases fail against the predecessor predicate (reverted the classifier to its old message-regex, keeping the new error class) — so they genuinely pin the fix. I also confirmed the message format is byte-identical to before, so no other waitForChannelClose caller or error-consuming path changes behavior. No actionable findings.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@nwparker

nwparker commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Updated this branch with current main in an ordinary merge. The three transfer-fallback files are unchanged; 145 tests in six files, all three typechecks, lint, format, and the changed-code checks pass. Fresh CI will validate this head.

The earlier Linux SSH startup failure remains unexplained. Two separate diagnostic runs passed: the startup case alone, then the original two watcher tests followed by all five parking/startup tests. Both captured the startup command reaching the held shell and surviving reconnect. The observer affects timing, so these passes do not establish a cause or a flakiness fix. No diagnostic observer is included in this PR.

@nwparker
nwparker merged commit 5389925 into main Oct 4, 2026
47 checks passed
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.

1 participant