Skip to content

fix: make the remote ready marker work under tcsh login shells - #5519

Merged
ogulcancelik merged 1 commit into
masterfrom
issue/4258-tcsh-machine-add
Oct 26, 2026
Merged

ogulcancelik merged 1 commit into
masterfrom
issue/4258-tcsh-machine-add

Conversation

@ogulcancelik

@ogulcancelik ogulcancelik commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

refs #4258

herdr machine add failed against accounts whose login shell is tcsh: syntax errors, then a hang. The remote ready-marker command put literal newlines inside a single-quoted printf format, which tcsh rejects, so the marker never appeared.

The format now uses \n escapes, which printf expands identically on POSIX shells. A regression test runs the generated command under tcsh -f and skips when tcsh isn't installed.

Verified end to end against a real localhost tcsh account: machine add hung on master and completes with this change.

just check passed on the change, including the Windows lint.

@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

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

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a1574be1-90d7-478b-9332-586e4f4b7a6e
Commits

Reviewing files that changed from the base of the PR and between 5f7efd3 and 0efea50.

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
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a6e0b733-0156-49f4-a039-f993ffd37a82
📥 Commits

Reviewing files that changed from the base of the PR and between 6b52662 and 5f7efd3.

📒 Files selected for processing: 1
  • src/remote/attach.rs

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

Walkthrough

posix_remote_output_command now emits escaped newline sequences in its shell printf format string. A Unix test checks command execution under tcsh, successful output framing, and empty stderr. Bridge-command expectations were updated to match the escaped format.

Suggested reviewers: jjliebig

Priority: Normal

Merge Risk: Minimal · previous review 5f7ef

No actionable merge-blocking risk remains from this change.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title names the main change: the remote ready marker is fixed for tcsh login shells. The raw summary confirms the printf format now uses escaped \n sequences, and a tcsh test was added. The title …
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.
Description check Passed The pull request description directly explains the tcsh login-shell failure, the escaped newline fix, the regression test, and validation results. It matches the changeset.
Full details: Title check

Explanation

The title names the main change: the remote ready marker is fixed for tcsh login shells. The raw summary confirms the printf format now uses escaped \n sequences, and a tcsh test was added. The title is specific and clear, and it matches the objectives.

  • Autofix · 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.

@kangal-bot

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
✅ 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 Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

Fixes the remote ready marker for accounts using tcsh.

  • POSIX remote commands print the ready marker under tcsh.

No actionable issues found. Tests were not run during this review. The full PR diff was reviewed because the previous and current commits have different merge bases with the target commit.

Reviews (2) · Last reviewed commit: "fix: make the remote ready marker work u..." · Reviewed by Greptile

@ogulcancelik
ogulcancelik force-pushed the issue/4258-tcsh-machine-add branch from 5f7efd3 to 0efea50 Compare October 26, 2026 21:32
@ogulcancelik

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

No files to review.

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.

@ogulcancelik
ogulcancelik merged commit 65da135 into master Oct 26, 2026
7 checks passed
@ogulcancelik
ogulcancelik deleted the issue/4258-tcsh-machine-add branch October 26, 2026 22:01
maciejul pushed a commit to maciejul/herdr-supercharged that referenced this pull request Oct 28, 2026
@ShacharPollak11

Copy link
Copy Markdown

hey, waiting for this to be release :)
when can we expect this?

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.

3 participants