Repository navigation
Fix mode selector cycles mid-prompt #193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| #!/usr/bin/env python3 | ||
| """ | ||
| Capture the rendered footer (bottom screen lines) of the Claude Code TUI while | ||
| cycling edit modes with Shift+Tab, both when idle and while Claude is working. | ||
|
|
||
| Answers: which line is the bottom-most non-blank one in each mode, and does the | ||
| mode indicator always live on it? ScreenContentDetector.detectEditMode() only | ||
| inspects that single line, so anything rendered below the mode indicator makes | ||
| the mode undetectable. | ||
|
|
||
| Cross-platform: uses pywinpty on Windows, the stdlib pty elsewhere, and pyte to | ||
| render the byte stream into a screen grid (the same view JediTerm gives the plugin). | ||
|
|
||
| pip install pyte pywinpty # pywinpty only on Windows | ||
| python test_edit_mode_footer.py [--work] | ||
| """ | ||
|
|
||
| import os | ||
| import shutil | ||
| import sys | ||
| import tempfile | ||
| import threading | ||
| import time | ||
|
|
||
| import pyte | ||
|
|
||
| COLS, ROWS = 120, 40 | ||
| SHIFT_TAB = '\x1b[Z' | ||
|
|
||
|
|
||
| class Term: | ||
| def __init__(self, argv, cwd): | ||
| env = dict(os.environ) | ||
| env.pop('CLAUDECODE', None) | ||
| env.pop('CLAUDE_CODE_ENTRYPOINT', None) | ||
| env['TERM'] = 'xterm-256color' | ||
| self.screen = pyte.Screen(COLS, ROWS) | ||
| self.stream = pyte.Stream(self.screen) | ||
| self.lock = threading.Lock() | ||
| if os.name == 'nt': | ||
| from winpty import PtyProcess | ||
| self.proc = PtyProcess.spawn(argv, cwd=cwd, env=env, dimensions=(ROWS, COLS)) | ||
| self._read = lambda: self.proc.read(4096) | ||
| self._write = self.proc.write | ||
| else: | ||
| import pty | ||
| import subprocess | ||
| master, slave = pty.openpty() | ||
| import fcntl, struct, termios | ||
| fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack('HHHH', ROWS, COLS, 0, 0)) | ||
| self.proc = subprocess.Popen(argv, cwd=cwd, env=env, stdin=slave, | ||
| stdout=slave, stderr=slave, close_fds=True) | ||
| self._read = lambda: os.read(master, 4096).decode('utf-8', 'replace') | ||
| self._write = lambda s: os.write(master, s.encode()) | ||
| threading.Thread(target=self._pump, daemon=True).start() | ||
|
|
||
| def _pump(self): | ||
| while True: | ||
| try: | ||
| data = self._read() | ||
| except Exception: | ||
| return | ||
| if not data: | ||
| time.sleep(0.01) | ||
| continue | ||
| with self.lock: | ||
| self.stream.feed(data) | ||
|
|
||
| def write(self, s): | ||
| self._write(s) | ||
|
|
||
| def lines(self): | ||
| with self.lock: | ||
| return [l.rstrip() for l in self.screen.display] | ||
|
|
||
| def bottom(self, n=4): | ||
| nb = [l for l in self.lines() if l.strip()] | ||
| return nb[-n:] | ||
|
|
||
| def wait_for(self, text, timeout=30): | ||
| end = time.time() + timeout | ||
| while time.time() < end: | ||
| if any(text in l for l in self.lines()): | ||
| return True | ||
| time.sleep(0.1) | ||
| return False | ||
|
|
||
| def close(self): | ||
| try: | ||
| if os.name == 'nt': | ||
| self.proc.terminate(force=True) | ||
| else: | ||
| self.proc.kill() | ||
| except Exception: | ||
| pass | ||
|
|
||
|
|
||
| def dump(term, label): | ||
| print(f'--- {label}') | ||
| for l in term.bottom(): | ||
| print(' |' + l) | ||
|
|
||
|
|
||
| def main(): | ||
| work = '--work' in sys.argv | ||
| claude = shutil.which('claude') | ||
| cwd = tempfile.mkdtemp(prefix='cc-mode-footer-') | ||
| term = Term([claude], cwd) | ||
| try: | ||
| if term.wait_for('Yes, I trust', 15): | ||
| time.sleep(1) | ||
| term.write('\x1b[B') # default is "No, exit" — move to "Yes" | ||
| time.sleep(0.5) | ||
| term.write('\r') | ||
| if not term.wait_for('shift+tab', 30) and not term.wait_for('for shortcuts', 5): | ||
| dump(term, 'startup (no footer found)') | ||
| time.sleep(2) | ||
| dump(term, 'idle initial') | ||
| for i in range(5): | ||
| term.write(SHIFT_TAB) | ||
| time.sleep(1.2) | ||
| dump(term, f'idle after shift+tab #{i + 1}') | ||
|
|
||
| if work: | ||
| term.write('Count slowly from 1 to 40, one number per line, no tools.') | ||
| time.sleep(0.5) | ||
| term.write('\r') | ||
| time.sleep(2.5) | ||
| dump(term, 'working initial') | ||
| for i in range(5): | ||
| term.write(SHIFT_TAB) | ||
| time.sleep(1.2) | ||
| dump(term, f'working after shift+tab #{i + 1}') | ||
| term.wait_for('40', 60) | ||
| time.sleep(3) | ||
| dump(term, 'idle after response') | ||
| finally: | ||
| term.close() | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| main() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -977,6 +977,45 @@ void trySendShiftTabsUntilMode_returnsfalseWhenModeNeverReached() throws Excepti | |
| assertEquals(5, (int) tabsSent.get(), "5 tabs must have been sent"); | ||
| } | ||
|
|
||
| /** | ||
| * Regression: selecting a second mode while a Shift+Tab switch is still running | ||
| * started a second worker thread. Both sent Shift+Tabs concurrently, each steering | ||
| * toward its own target, so the mode kept cycling and landed on an arbitrary mode. | ||
| * A new request must instead be picked up by the single running worker. | ||
| */ | ||
| @Test | ||
| void onEditModeComboChanged_secondRequestDuringSwitch_isHandledBySameWorker() throws Exception { | ||
| // Simulated CC cycle (2.1.x order): DEFAULT → ACCEPT_EDITS → PLAN → AUTO → DEFAULT | ||
| EditMode[] cycle = {EditMode.DEFAULT, EditMode.ACCEPT_EDITS, EditMode.PLAN, EditMode.AUTO}; | ||
| java.util.concurrent.atomic.AtomicInteger ccMode = new java.util.concurrent.atomic.AtomicInteger(0); | ||
| java.util.Set<Thread> senders = java.util.concurrent.ConcurrentHashMap.newKeySet(); | ||
| ClaudeSessionModel m = new ClaudeSessionModel(); | ||
| m.setEditMode(EditMode.DEFAULT); | ||
| ClaudeSessionController c = new ClaudeSessionController(m, Collections::emptyList) { | ||
| @Override boolean isConnected() { return true; } | ||
| @Override Optional<EditMode> detectCurrentMode() { return Optional.of(cycle[ccMode.get()]); } | ||
| @Override void sendOneShiftTab() { | ||
| senders.add(Thread.currentThread()); | ||
| ccMode.updateAndGet(i -> (i + 1) % cycle.length); | ||
| } | ||
| }; | ||
| setModeSwitchPollMs(c, 1); | ||
|
|
||
| c.onEditModeComboChanged(EditMode.AUTO); | ||
| c.onEditModeComboChanged(EditMode.PLAN); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test can be flaky: it assumes the second request arrives while worker 1 is still running, which only holds because the fake poll interval is 1 ms and the request follows immediately. On a loaded CI machine worker 1 may finish first, then a second thread is started and Make it deterministic by blocking the first Shift+Tab until the second request has been issued, e.g.: java.util.concurrent.CountDownLatch secondIssued = new java.util.concurrent.CountDownLatch(1);
// in the overridden sendOneShiftTab():
senders.add(Thread.currentThread());
try { secondIssued.await(2, java.util.concurrent.TimeUnit.SECONDS); } catch (InterruptedException e) { Thread.currentThread().interrupt(); }
ccMode.updateAndGet(i -> (i + 1) % cycle.length);
// in the test body:
c.onEditModeComboChanged(EditMode.AUTO);
c.onEditModeComboChanged(EditMode.PLAN);
secondIssued.countDown(); |
||
|
|
||
| java.lang.reflect.Field inProgress = ClaudeSessionController.class.getDeclaredField("modeSwitchInProgress"); | ||
| inProgress.setAccessible(true); | ||
| long deadline = System.currentTimeMillis() + 5_000; | ||
| while (inProgress.getBoolean(c) && System.currentTimeMillis() < deadline) { | ||
| Thread.sleep(5); | ||
| } | ||
|
|
||
| assertFalse(inProgress.getBoolean(c), "switch must finish"); | ||
| assertEquals(EditMode.PLAN, cycle[ccMode.get()], "must end on the most recently requested mode"); | ||
| assertEquals(1, senders.size(), "Shift+Tabs must be sent by a single worker thread"); | ||
| } | ||
|
|
||
| private static void setModeSwitchPollMs(ClaudeSessionController c, int ms) throws Exception { | ||
| java.lang.reflect.Field f = ClaudeSessionController.class.getDeclaredField("modeSwitchPollMs"); | ||
| f.setAccessible(true); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix, the single-worker retargeting is the right design.
One robustness issue: the old
finally { modeSwitchInProgress = false; }was removed, and thiscatchonly handlesIOException | InterruptedException. IfsendOneShiftTab()throws an unchecked exception (e.g. an NPE becauseconnectorbecamenullwhen the session was stopped mid-switch),editModeSwitchTargetstays non-null forever. After that everysendShiftTabsUntilModecall just "retargets" and returns (no worker ever runs again), andmodeSwitchInProgressstaystrue, so the screen→model mode sync in the poll loop is disabled for the rest of the session.Catching
RuntimeExceptionhere reuses the existing cleanup block. (I'd avoid moving the cleanup intofinally: on a normal exit it could clobber the target of a new worker started right after the loop ended.)