Skip to content

Fix/strip only cr in session#201

Merged
carlosejimenez merged 5 commits intomainfrom
fix/strip-only-cr-in-session
Jun 13, 2025
Merged

Fix/strip only cr in session#201
carlosejimenez merged 5 commits intomainfrom
fix/strip-only-cr-in-session

Conversation

@carlosejimenez
Copy link
Member

  • This PR updates the command input and output processing
  • Remove windows-style "crlf" line endings from pexpect outputs
  • Remove extra endline from echo in local.BashSession._run_normal's fallback UNIQUE_STRING based output parsing
  • Updates tests related to execution / testing output to be even stricter

This update makes outputs more reliable, consistent, and true to standard unix shells' expected output. For instance, outputs of echo commands will include a training newline, which was being stripped before.

@klieret klieret requested a review from Copilot June 12, 2025 02:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances newline handling by stripping Windows-style CRLFs, preserving intended newlines in session outputs, and updating tests to expect exact trailing newlines.

  • Strip \r\n to \n in control-character removal
  • Remove .strip() calls to keep command output newlines
  • Change fallback marker to use echo -n and tighten test assertions

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/test_execution.py Updated test assertions to require exact trailing newlines
src/swerex/runtime/local.py Added CRLF normalization, removed stripping of output, tweaked echo
Comments suppressed due to low confidence (3)

src/swerex/runtime/local.py:95

  • [nitpick] Consider moving the ANSI regex compilation to a module-level constant to avoid recompiling it on every call to _strip_control_chars.
ansi_escape = re.compile(r"\x1B[@-_][0-?]*[ -/]*[@-~]")

tests/test_execution.py:65

  • [nitpick] The test name test_execute_command_with_echon is misleading; consider renaming to test_execute_command_with_echo_n for clarity.
async def test_execute_command_with_echon(remote_runtime: RemoteRuntime):

tests/test_execution.py:55

  • Add a test that simulates Windows-style CRLF output (e.g., via printf 'line1\r\nline2\r\n') to verify the new CRLF normalization logic.
async def test_execute_command_with_empty_string_in_session(runtime_with_default_session: RemoteRuntime):

carlosejimenez and others added 2 commits June 12, 2025 16:37
@carlosejimenez carlosejimenez merged commit 25b1697 into main Jun 13, 2025
5 checks passed
@carlosejimenez carlosejimenez deleted the fix/strip-only-cr-in-session branch June 13, 2025 05:06
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