From 81b0117c37008317d8012cd9d4b4ab0e4a2192e6 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN Date: Thu, 20 Aug 2026 11:37:17 -0400 Subject: [PATCH] Fix: every TUI palette command taking a path was broken on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last two Windows failures were not POSIX-assuming tests. The tests were right and the application was wrong. `parse_palette_command` split its input with `shlex.split`, which defaults to POSIX mode where a backslash is an ESCAPE character. On Windows that quietly destroys every path a user types: C:\Users\me\Temp\exports -> C:UsersmeTempexports So doc-export, export-field, db-export, db-import, parent-seed-backup and totp-export all wrote to a mangled relative path — or nowhere — and then reported success. A user asking the TUI to export their vault on Windows got a success message and no file. The two failing tests were reporting exactly that, from the one platform that runs them. Both palettes now share `split_palette_args`, which uses non-POSIX splitting on Windows and strips the quotes that mode leaves on a quoted token, so paths with spaces still work. v3 imports it from v2 rather than keeping its own copy, since two parsers that must agree will eventually not. The `windows` argument is an explicit parameter rather than a bare os.name check, so both branches are tested from either platform — including a test that pins what POSIX mode does to a Windows path, so anyone "simplifying" this back to shlex.split sees precisely what they are restoring. That parameter is the actual fix for the class of bug: the reason this survived is that the Windows branch could only ever run on Windows. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN --- TODO.md | 14 +++++++-- src/seedpass/tui_v2/app.py | 30 +++++++++++++++++- src/seedpass/tui_v3/app.py | 7 +++-- src/tests/test_tui_v2_helpers.py | 53 ++++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 5 deletions(-) diff --git a/TODO.md b/TODO.md index b97d0a3c..284e74ad 100644 --- a/TODO.md +++ b/TODO.md @@ -253,8 +253,18 @@ everything still open, in the order it should be tackled. deliberately with the sync round-trip re-validated against a real relay rather than folded into unrelated work. (Found 2026-08-20 on the first CI run of this branch.) - - [ ] **Two TUI tests still fail on Windows, and only Windows ever runs - them.** Surfaced 2026-08-20 once the Tests workflow could report a + - [x] **Two TUI tests still fail on Windows, and only Windows ever runs + them.** RESOLVED — and they were right. The tests were not + POSIX-assuming; the APP was. `parse_palette_command` used + `shlex.split`, which defaults to POSIX mode where a backslash is + an escape character, so on Windows + `C:\Users\me\exports` became `C:Usersmeexports` and every + palette command taking a path — doc-export, export-field, + db-export, db-import, parent-seed-backup, totp-export — wrote to a + mangled relative path or nowhere, while telling the user it had + worked. Both palettes now share `split_palette_args`, which takes + an explicit `windows` flag so BOTH branches are tested from either + platform. Original wording kept below for the record. Surfaced 2026-08-20 once the Tests workflow could report a Windows failure at all (see below). Four failed; two were literal `/tmp/...` comparisons against paths the app echoes platform-normalized, and those are fixed by deriving the expected diff --git a/src/seedpass/tui_v2/app.py b/src/seedpass/tui_v2/app.py index d160e1a8..8a11c127 100644 --- a/src/seedpass/tui_v2/app.py +++ b/src/seedpass/tui_v2/app.py @@ -2,6 +2,7 @@ from __future__ import annotations import importlib.util import json +import os import shlex import time from pathlib import Path @@ -30,7 +31,7 @@ def parse_palette_command(command: str) -> tuple[str, list[str]]: if not raw: raise ValueError("Palette: command required") try: - parts = shlex.split(raw) + parts = split_palette_args(raw) except ValueError as exc: raise ValueError(f"Palette parse error: {exc}") from exc if not parts: @@ -38,6 +39,33 @@ def parse_palette_command(command: str) -> tuple[str, list[str]]: return parts[0].lower(), parts[1:] +def split_palette_args(raw: str, *, windows: bool | None = None) -> list[str]: + """Split a palette command line into tokens, preserving Windows paths. + + `shlex.split` defaults to POSIX mode, where a backslash is an ESCAPE + character. On Windows that silently destroys every path a user types: + `C:\\Users\\me\\exports` comes back as `C:Usersmeexports`, so + doc-export, export-field, db-export, db-import and parent-seed-backup all + wrote to a mangled relative path — or nowhere — while reporting success. + Found by the Windows CI job once it could run these tests at all. + + Non-POSIX mode keeps backslashes but also keeps the surrounding quotes on + a quoted token, so those are stripped back off. `windows` is a parameter + rather than a bare `os.name` check so both branches are testable from + either platform. + """ + on_windows = os.name == "nt" if windows is None else windows + if not on_windows: + return shlex.split(raw) + parts = shlex.split(raw, posix=False) + unquoted: list[str] = [] + for part in parts: + if len(part) >= 2 and part[0] == part[-1] and part[0] in "\"'": + part = part[1:-1] + unquoted.append(part) + return unquoted + + def pagination_window( total_rows: int, page_size: int, page_index: int ) -> tuple[int, int, int, int]: diff --git a/src/seedpass/tui_v3/app.py b/src/seedpass/tui_v3/app.py index 65c5adcc..0e13a18a 100644 --- a/src/seedpass/tui_v3/app.py +++ b/src/seedpass/tui_v3/app.py @@ -48,10 +48,13 @@ class CommandProcessor: self.app = app def execute(self, raw: str) -> None: - import shlex + # Shared with the v2 palette so the two cannot drift: plain + # shlex.split eats backslashes, which destroys every Windows path a + # user types into a command that takes one. + from seedpass.tui_v2.app import split_palette_args try: - parts = shlex.split(raw) + parts = split_palette_args(raw) except Exception as e: self.app.notify(f"Parse error: {e}", severity="error") return diff --git a/src/tests/test_tui_v2_helpers.py b/src/tests/test_tui_v2_helpers.py index 5538c143..f9efc143 100644 --- a/src/tests/test_tui_v2_helpers.py +++ b/src/tests/test_tui_v2_helpers.py @@ -6,6 +6,7 @@ from seedpass.tui_v2.app import ( pagination_window, parse_palette_command, render_qr_ascii, + split_palette_args, truncate_entry_for_display, ) @@ -79,3 +80,55 @@ def test_pagination_window_large_vault_sizes(rows: int) -> None: assert page == last_page assert total_pages >= 1 assert 0 <= start <= end <= rows + + +class TestSplitPaletteArgs: + """Both platform branches, tested from either platform. + + `windows` is an explicit parameter rather than a bare `os.name` check + precisely so these can run everywhere. Without that, the Windows branch + would only ever be exercised on Windows — which is how the bug below + survived: shlex.split defaults to POSIX mode, where backslash is an ESCAPE + character, so every Windows path a user typed came back with its + separators eaten and doc-export, export-field, db-export, db-import and + parent-seed-backup wrote nowhere while reporting success. + """ + + def test_windows_paths_keep_their_separators(self) -> None: + assert split_palette_args( + r"doc-export C:\Users\me\Temp\exports", windows=True + ) == ["doc-export", r"C:\Users\me\Temp\exports"] + + def test_posix_mode_would_have_eaten_them(self) -> None: + # The bug, pinned. If someone "simplifies" the helper back to a plain + # shlex.split, this is what Windows users get. + assert split_palette_args( + r"doc-export C:\Users\me\Temp\exports", windows=False + ) == ["doc-export", "C:UsersmeTempexports"] + + def test_quoted_windows_paths_survive_with_quotes_removed(self) -> None: + # Non-POSIX shlex keeps the quotes on the token; they have to come off + # or the path gains a literal quote character. + assert split_palette_args( + 'db-export "C:\\Program Files\\out.enc"', windows=True + ) == ["db-export", r"C:\Program Files\out.enc"] + + def test_posix_behaviour_is_unchanged(self) -> None: + assert split_palette_args("doc-export /tmp/exports", windows=False) == [ + "doc-export", + "/tmp/exports", + ] + assert split_palette_args('db-export "/tmp/with space.enc"', windows=False) == [ + "db-export", + "/tmp/with space.enc", + ] + + def test_quoting_still_groups_on_windows(self) -> None: + assert split_palette_args( + 'link-add 42 related_to "note text"', windows=True + ) == [ + "link-add", + "42", + "related_to", + "note text", + ]