Skip to content

Modify escapeseq#260

Open
Arjun-Parmani wants to merge 2 commits intoHyperfoil:masterfrom
Arjun-Parmani:Modify_Escapeseq
Open

Modify escapeseq#260
Arjun-Parmani wants to merge 2 commits intoHyperfoil:masterfrom
Arjun-Parmani:Modify_Escapeseq

Conversation

@Arjun-Parmani
Copy link

Modified the escape filter stream code y adding another buffer to store the data and pass it to getBuffered() method before reseting the original buffer.

Copy link
Member

@stalep stalep left a comment

Choose a reason for hiding this comment

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

The storageBuffer is never reset. This means it would grow and cause a memory leak.
The close() method should perhaps flush the barrierBuffer?
There are several tests thats commented out which should be there and there are other tests that's redundant with the inclusion of jansi imo?

flushIndex = writeIndex;//TODO testing if fixes double write
if (barrierBuffer.size() > 0) {
byte[] data = barrierBuffer.toByteArray();
System.out.println("DEBUG: Capturing to Storage buffer: " + new String(data, StandardCharsets.UTF_8));
Copy link
Member

Choose a reason for hiding this comment

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

Should change this to a logger if needed?

Copy link
Author

Choose a reason for hiding this comment

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

Updated to logger

superWrite(barrierBuffer.toByteArray(), 0, barrierBuffer.size());
barrierBuffer.reset();

System.out.println("DEBUG: Capturing to Storage buffer: " + new String(data, StandardCharsets.UTF_8));
Copy link
Member

Choose a reason for hiding this comment

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

Should change this to a logger if needed?

Copy link
Author

Choose a reason for hiding this comment

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

Updated to logger

stream.flush();

String buffered = stream.getBuffered();
System.out.println("Captured: " + buffered);
Copy link
Member

Choose a reason for hiding this comment

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

Should change this to a logger if needed?

Copy link
Author

Choose a reason for hiding this comment

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

Updated to logger

@stalep
Copy link
Member

stalep commented Feb 20, 2026

You seem to have included the commit I've made to fix the CI. I think it's best to keep those pr's separate. Can you please revert that an only have the changes for the issue you try to fix?

@Arjun-Parmani Arjun-Parmani force-pushed the Modify_Escapeseq branch 2 times, most recently from a268dbb to af5038b Compare February 20, 2026 10:31
@stalep
Copy link
Member

stalep commented Feb 20, 2026

There are still behavioral changes I think it's important that you look more into:

getBuffered() - the old getBuffered() returned what's currently pending in the buffer. Your new version accumulates everything ever written into storageBuffer. Look at how getBuffered() is used in
SessionStreams.jsonBuffers() and printBuffers() — think about what those callers expect to see, and whether showing all historical data is useful there.

Removing reset() method: The old class had a public reset() method. Removing public methods is a breaking API change even if no current caller hits it. Think about what reset should mean for your new implementation.

storageBuffer memory management: What happens when a long-running session writes hundreds of KB through this stream? And what does a caller see from getBuffered() right after the 20KB reset triggers — is that useful?

Pre-filtering before jansi: You're manually filtering null bytes, SHIFT_IN/OUT, #ESC, and ESC=/ESC> before passing to jansi. Consider which of these jansi already handles in strip mode and which are genuinely needed. If you need the pre-filtering, add a comment explaining why.

Test coverage: You removed many tests that verified observable behavior (e.g., filter_K_noDigit, filter_all, filter_head, filter_tail, filter_questionMark_1h). The implementation changed, but the expected output for a given input
didn't. Those tests should still pass with jansi — keep them as black-box tests. Only drop tests for removed internals like escapeLength and copyNonNulBytes.

Smaller things are commented out sout() lines which can be removed etc. Good luck! :)

Added new Jansi version and modified the code as per the new version

Refined escape sequence handling by adding a storage buffer which returns the data stored in the orignal buffer stream

Added checks for Buffers to avoid OOM error and removed redundant test cases

Fixed the stoageBuffer issue. Only the latest 20kb is returned.Also added reset() method
@Arjun-Parmani
Copy link
Author

I explored replacing the custom ANSI stripping logic in EscapeFilteredStream with JLine's Jansi library (AnsiOutputStream in strip mode). Here is what I found:

What Jansi handles
Before Jansi, we were manually maintaining a CONTROL_SUFFIX set and an escapeLength() method to identify and strip ANSI sequences byte by byte. Jansi removes all of that and it handles standard ANSI escape sequences, color codes, cursor movement, and erase sequences automatically. It also correctly strips some sequences we were missing, like \u001b[b (cursor-back), which our custom set wasn't catching.

Limitations:

Non-ANSI Filtering: Jansi only looks for ESC sequences. We must still manually loop to filter Null bytes, Shift In, and Shift Out which aren't ANSI codes.

Backspace (\b) — The zshShellTest failed. In zsh, the terminal sends something like e\b to redraw characters on screen. Our custom implementation stripped this correctly, but Jansi just passes it through as-is. I added a manual backspace handler but it only works when e and \b arrive in the same chunk. Because Jansi processes data byte by byte, they often arrive in separate flushes and the fix doesn't hold.

RunTest.ctrlCTail and RunTest.watch_invoke_count - These tests append lines to a file and expect lines[0]="!", lines[1]="foo", lines[2]="bar". With Jansi, '!' was getting pushed to lines[1] instead of lines[0].
The problem was that Jansi was flushing \r\n chunks with no content. LineEmittingStream was emitting an empty string for each one, so by the time '!' arrived, an empty string was already at lines[0].
I fixed this by adding a check in LineEmittingStream to not emit empty strings, , which fixed RunTest. But it broke LineEmittingStreamTest.buffer_flush because that test specifically expects an empty string to be emitted for a lone \n. So fixing one broke the other.

@willr3
Copy link
Collaborator

willr3 commented Feb 27, 2026

I fixed this by adding a check in LineEmittingStream to not emit empty strings

That may appear to be a fix for the observed issue in the test but that would unfortunately be a breaking change for users who rely on qDup to reliably emit each line of output from the remote session. There are use cases where a blank line in the output has meaning to the user. For example, a blank line can be used to separate logical chunks of data and watch is used to count blank lines to monitor the progress of the command.

We need to ensure qDup faithfully exposes the output of the commands used in the session to all parts of the user's script.

@Arjun-Parmani
Copy link
Author

Fixed two test failures — RunTest was emitting empty lines due to Jansi flushing \r\n in separate chunks, and ZshShellTest was failing due to backspace characters arriving in a different chunk than the character they were meant to delete. Also cleaned up the implementation by removing the intermediate barrier buffer, with filtered data now written directly downstream via superWrite.

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