Skip to content

fix(remote): show SSH authentication output - #3609

Open
akbash-bot wants to merge 1 commit into
herdrdev:masterfrom
akbash-bot:akbash/3606-stream-ssh-auth-output
Open

fix(remote): show SSH authentication output#3609
akbash-bot wants to merge 1 commit into
herdrdev:masterfrom
akbash-bot:akbash/3606-stream-ssh-auth-output

Conversation

@akbash-bot

Copy link
Copy Markdown
Collaborator

Issue

A Tailscale SSH check makes herdr --remote appear to hang because the browser authentication URL is hidden until SSH exits.

Problem

Remote setup collected SSH error output only after each command finished. Authentication notices written while SSH was waiting were therefore unavailable to the user.

How did we fix it?

Herdr now relays SSH setup error output as it arrives. It also retains that output so existing contextual failures and authentication hints still work. SSH commands, connection reuse, and parsed standard output are unchanged.

Verification

The production-binary regression test timed out waiting for the Tailscale notice before the change. It now sees the notice and URL before simulated approval, then confirms setup continues and later failures retain their context. Remote tests and Windows lint pass. The full suite passed 3,089 of 3,091 tests; the uid-0 permission test and an unrelated live-handoff replacement-server test still fail on this runner.

refs #3606

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: cb01aa3f-4e0e-4c49-b325-2d480998feff

📥 Commits

Reviewing files that changed from the base of the PR and between 3150bd9 and 9ef526f.

📒 Files selected for processing (2)
  • src/remote/attach.rs
  • tests/remote_attach.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Remote shell helpers now forward child stderr to the local stderr in real time while retaining the bytes in the returned Output. A shared helper relays stderr, writes optional stdin, waits for the child, and propagates relay errors. A Unix integration test uses a fake SSH executable to verify authentication notices, URLs, later SSH errors, contextual error output, and unsuccessful process status.

Suggested reviewers: ogulcancelik

Merge Risk: ⚪ Minimal · up to 9ef52

Remote SSH authentication messages and URLs are now visible while waiting, while later error context remains available. The covered behavior is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the SSH stderr forwarding change, its purpose, verification, and known unrelated test failures.
Title check ✅ Passed The title clearly and concisely identifies the main change: showing SSH authentication output for remote commands.
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.
  • Fix all pre-merge checks with AI

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.

@kangal-bot

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

@kangal-bot I will review pull request #3609.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Sep 4, 2026

Copy link
Copy Markdown

Greptile Summary

This PR changes remote SSH setup commands to relay stderr while commands are still running and retain the same bytes for contextual errors.

  • Adds a concurrent stderr relay around SSH setup subprocesses.
  • Keeps stdout parsing and captured stderr available through the existing Output contract.
  • Adds a Unix integration test proving authentication instructions appear before approval and later failures preserve context.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code defect established.

The relay drains SSH stderr concurrently, restores it to the returned process output, and the new integration test covers both immediate authentication notices and subsequent contextual failures.

Important Files Changed

Filename Overview
src/remote/attach.rs Introduces concurrent SSH stderr forwarding while preserving captured output for existing parsing and contextual error handling.
tests/remote_attach.rs Adds an end-to-end Unix regression test covering pre-completion authentication output and retained failure context.

Sequence Diagram

sequenceDiagram
    participant H as Herdr
    participant S as SSH process
    participant T as Local stderr
    participant R as Remote host
    H->>S: Start setup command
    S->>R: Execute probe or setup script
    R-->>S: Authentication notice on stderr
    S-->>H: Stream stderr bytes
    H-->>T: Relay bytes immediately
    R-->>S: Command stdout and exit status
    S-->>H: Complete command
    H->>H: Return Output with captured stderr
Loading

Reviews (1): Last reviewed commit: "fix(remote): show SSH authentication out..." | Re-trigger Greptile

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