Skip to content

fix: decode SSH subprocess output tolerantly - #37265

Closed
honor2030 wants to merge 1 commit into
NousResearch:mainfrom
honor2030:fix/ssh-subprocess-tolerant-decoding
Closed

fix: decode SSH subprocess output tolerantly#37265
honor2030 wants to merge 1 commit into
NousResearch:mainfrom
honor2030:fix/ssh-subprocess-tolerant-decoding

Conversation

@honor2030

@honor2030 honor2030 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

⚠️ Superseded — please do not merge

This PR is obsolete and is being closed. The subprocess text-decoding bug class, including the SSH call sites changed here, landed on main through #70875.

Why this PR is being closed

Where the maintained change lives

Closing this PR unmerged. The branch is left intact for reference.

@teknium1 teknium1 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.

Thanks for the focused fix. The reported initialization failure is still present on current main: tools/environments/ssh.py:149 (and six sibling direct calls) uses text=True with captured output but no tolerant decoding.

Problems

  • tests/tools/test_ssh_environment_decoding.py:31 depends on the old one-line call layout. Current main formats every relevant call across multiple lines, such as tools/environments/ssh.py:104-110 and 149-155, so this scan would produce no snippets after salvage even if the implementation is correct.
  • The direct subprocess.run scope leaves SSHEnvironment._run_bash (tools/environments/ssh.py:353) delegating to _popen_bash, whose Popen(..., text=True) at tools/environments/base.py:146-152 still has no encoding/error parameters. Please explicitly decide whether that execution path belongs in this bug class.

Suggested changes

  • Apply the seven kwargs changes to the current multiline calls and replace the source-layout assertion with behavior-oriented subprocess tests.
  • If execution output is in scope, cover _popen_bash; otherwise narrow the stated guarantee to direct SSH/scp setup and sync calls.

Automated hermes-sweeper review.

def test_ssh_subprocess_text_captures_all_use_tolerant_decoding():
source = ssh_env.Path(ssh_env.__file__).read_text(encoding="utf-8")
snippets = [
line for line in source.splitlines()

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.

This only recognizes one-line subprocess.run(..., capture_output=True, text=True) calls. Current main formats all seven relevant SSH calls across multiple lines, so this test would find no snippets after salvage even when the decoding kwargs are present. Please use a behavior-oriented test or a layout-independent inspection.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
@honor2030

Copy link
Copy Markdown
Contributor Author

Closing as superseded by merged #70875. That PR covers the SSH subprocess decoding sites and adds repository-wide enforcement. This branch remains intact for reference.

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

Labels

backend/ssh SSH remote execution P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants