442: opskit go picker — aligned columns, no timeout

bin/opskit-go.py

@@ -180,6 +180,50 @@ def _git_info(path):
         return "unknown", "unknown"
 
 
+PICKER_NAME_W = 28
+PICKER_BRANCH_W = 18
+PICKER_STATUS_W = 8
+
+
+def _pad(value, width):
+    """Left-justify to a fixed width, truncating long values with an ellipsis."""
+    if len(value) > width:
+        value = value[: width - 1] + "…"
+    return f"{value:<{width}}"
+
+
+def _picker_header():
+    return (
+        f"{'NAME':<{PICKER_NAME_W}} {'BRANCH':<{PICKER_BRANCH_W}} "
+        f"{'STATUS':<{PICKER_STATUS_W}} RESUME"
+    )
+
+
+def _picker_row(proj, git_lookup):
+    """One fixed-width display row: NAME | BRANCH | STATUS | RESUME."""
+    name = proj.get("rel") or proj.get("name", "unknown")
+    path = proj.get("path", "")
+    branch, status = git_lookup(path) if path else ("unknown", "unknown")
+    resume = "[resume]" if proj.get("has_resume") else ""
+    return (
+        f"{_pad(name, PICKER_NAME_W)} {_pad(branch, PICKER_BRANCH_W)} "
+        f"{_pad(status, PICKER_STATUS_W)} {resume}"
+    ).rstrip()
+
+
+def build_picker(projects, git_lookup=_git_info):
+    """Return (header, lines) for the fzf picker.
+
+    Each line is ``"<path>\\t<display>"``: field 1 is the selection value
+    (returned via fzf ``--accept-nth=1``), field 2 is the aligned display.
+    """
+    lines = [
+        f"{proj.get('path', '')}\t{_picker_row(proj, git_lookup)}"
+        for proj in projects
+    ]
+    return _picker_header(), lines
+
+
 def display_project_info(proj):
     """Display project info for the confirmation screen."""
     lines = []
@@ -256,39 +300,43 @@ def launch_project(project_path):
 
 
 def run_fzf_selection(projects):
-    """Run fzf to let the user select a project. Returns selected path or None."""
-    # Build fzf input: tab-separated name and path, sorted by last-used
-    lines = []
-    for proj in projects:
-        name = proj.get("rel") or proj.get("name", "unknown")
-        path = proj.get("path", "")
-        # Use path as the selection value
-        lines.append(f"{name}\t{path}")
+    """Run fzf to let the user select a project. Returns selected path or None.
 
-    input_data = "\n".join(lines) + "\n"
+    No timeout: this is an interactive picker, not a batch call. Esc/Ctrl+C
+    cancels (nonzero fzf exit) and returns None.
+    """
+    header, lines = build_picker(projects)
+    input_data = "\n".join(lines) + "\n" if lines else ""
 
     try:
         fzf_proc = subprocess.run(
-            ["fzf", "--height=40%", "--reverse", "--prompt=opskit> "],
+            [
+                "fzf",
+                "--height=40%",
+                "--reverse",
+                "--prompt=opskit> ",
+                "--delimiter=\t",
+                "--with-nth=2",   # display the aligned columns (field 2)
+                "--accept-nth=1",  # return only the path (field 1)
+                f"--header={header}",
+            ],
             input=input_data,
-            capture_output=True, text=True, timeout=60,
+            capture_output=True,
+            text=True,
         )
-        if fzf_proc.returncode != 0:
-            return None  # User cancelled
-
-        selected = fzf_proc.stdout.strip()
-        if not selected:
-            return None
+    except (FileNotFoundError, OSError):
+        print("Error: fzf not found", file=sys.stderr)
+        sys.exit(1)
 
-        # Extract path from tab-separated line
-        parts = selected.split("\t")
-        if len(parts) >= 2:
-            return parts[1]
-        return selected
+    if fzf_proc.returncode != 0:
+        return None  # User cancelled (Esc/Ctrl+C)
 
-    except (subprocess.TimeoutExpired, FileNotFoundError, OSError):
-        print("Error: fzf not found or timed out", file=sys.stderr)
-        sys.exit(1)
+    selected = fzf_proc.stdout.strip()
+    if not selected:
+        return None
+    # --accept-nth=1 yields the path; split anyway so we still work if a
+    # build ignores --accept-nth and returns the whole line.
+    return selected.split("\t")[0]
 
 
 def main():

plans/442-opskit-go-picker-columns.md

@@ -0,0 +1,57 @@
+# PLAN: `opskit go` picker — column layout and no timeout
+
+Closes #442.
+
+## Goal
+
+The fzf picker in `opskit go` must be readable and must not self-terminate.
+Two defects:
+
+1. **No columns.** `run_fzf_selection` fed fzf `f"{rel}\t{path}"` with no header and
+   no fixed widths, so fzf expanded the tab to a tab stop and the long absolute path
+   dominated each line — no column boundaries.
+2. **Times out.** fzf was invoked with `timeout=60`; leaving the picker open longer
+   killed it, printed "fzf not found or timed out", and exited 1.
+
+## Design
+
+Pure, unit-testable helpers plus a rewritten `run_fzf_selection`:
+
+- `_picker_header()` → header using the same fixed column widths:
+  `NAME | BRANCH | STATUS | RESUME`.
+- `_picker_row(proj, git_lookup)` → one display row, space-padded to fixed widths,
+  long values truncated with `…`; branch/status from `git_lookup(path)`.
+- `build_picker(projects, git_lookup=_git_info)` → `(header, lines)`; each line is
+  `f"{path}\t{display}"` — field 1 is the selection value, field 2 the display.
+- `run_fzf_selection(projects)`:
+  - fzf args add `--delimiter=\t --with-nth=2 --accept-nth=1 --header=<header>`
+    (display field 2; return field 1 = path; show a header). Keeps
+    `--height=40% --reverse --prompt=opskit> `.
+  - **no `timeout`** — interactive picker; Esc/Ctrl+C cancels (nonzero rc → `None`).
+  - return `stdout.split("\t")[0]`, so it still works on a build that ignores
+    `--accept-nth`.
+- Branch/dirty are computed lazily via `_git_info` for the displayed rows only.
+
+## Notes from implementation
+
+- fzf `--with-nth` scopes the fuzzy search to the **displayed** columns, so the
+  hidden path is not searchable — the visible `NAME` (the `rel` selector) is. Fine.
+- `--accept-nth` needs fzf ≥ 0.48 (0.67 here); the defensive split covers older.
+
+## Critique
+
+- **Security:** unchanged input surface (paths from cache/config); argv list, no shell;
+  fixed widths, no format injection.
+- **Stability:** fzf missing → clear error; empty input handled by the caller; no
+  timeout means an interactive wait can never be cut short.
+- **Maintainability:** formatting isolated in pure helpers → tested without git/fzf.
+- **Effectiveness/Value:** fixes both complaints; no scan-cost regression.
+- **Flaw → resolution:** truncating a long name could hide a distinguishing suffix.
+  Resolution: `rel` is already the unique-ish selector and we now search it; accepted.
+
+## Tests
+
+`tests/test_tui_picker.py`: header names the columns; header/row column offsets align;
+`[resume]` shown only when present; long names truncate to the column width; each line's
+field 1 is the path; `run_fzf_selection` passes **no** `timeout` and passes the column
+options; cancel and empty selection return `None`. Full suite: `make test`.

tests/test_tui_picker.py

@@ -0,0 +1,126 @@
+"""Tests for the bin/opskit-go.py fzf picker layout (issue #442).
+
+Loads the real script as a module so tests exercise the shipped code.
+"""
+
+import importlib.util
+import sys
+from pathlib import Path
+
+_ROOT = Path(__file__).parent.parent
+sys.path.insert(0, str(_ROOT))
+
+
+def _load_go_module():
+    spec = importlib.util.spec_from_file_location(
+        "opskit_go_picker", _ROOT / "bin" / "opskit-go.py"
+    )
+    module = importlib.util.module_from_spec(spec)
+    spec.loader.exec_module(module)
+    return module
+
+
+_go = _load_go_module()
+
+
+class _Proc:
+    def __init__(self, returncode, stdout=""):
+        self.returncode = returncode
+        self.stdout = stdout
+
+
+def _fake_run(returncode, stdout, captured):
+    def fake(*args, **kwargs):
+        captured["argv"] = args[0]
+        captured["kwargs"] = kwargs
+        return _Proc(returncode, stdout)
+    return fake
+
+
+PROJECTS = [
+    {"name": "Demo", "path": "/home/user/Projects/Demo", "rel": "Demo"},
+    {"name": "app", "path": "/home/user/Projects/group/app", "rel": "group/app"},
+]
+
+
+class TestPickerHeader:
+    def test_header_names_the_columns(self):
+        header = _go._picker_header()
+        for col in ("NAME", "BRANCH", "STATUS", "RESUME"):
+            assert col in header
+
+
+class TestPickerRow:
+    def test_columns_align_with_header(self):
+        header = _go._picker_header()
+        row = _go._picker_row(
+            {"name": "Demo", "path": "/p/Demo", "rel": "Demo", "has_resume": True},
+            git_lookup=lambda _p: ("main", "dirty"),
+        )
+        assert header.index("BRANCH") == row.index("main")
+        assert header.index("STATUS") == row.index("dirty")
+        assert header.index("RESUME") == row.index("[resume]")
+
+    def test_clean_project_has_no_resume_flag(self):
+        row = _go._picker_row(
+            {"name": "Demo", "path": "/p/Demo", "rel": "Demo"},
+            git_lookup=lambda _p: ("main", "clean"),
+        )
+        assert "clean" in row
+        assert "[resume]" not in row
+
+    def test_long_name_is_truncated_to_column_width(self):
+        row = _go._picker_row(
+            {"name": "x", "path": "/p/x", "rel": "a" * 60},
+            git_lookup=lambda _p: ("main", "clean"),
+        )
+        name_col = row[: _go.PICKER_NAME_W]
+        assert len(name_col) == _go.PICKER_NAME_W
+        assert "…" in name_col
+
+
+class TestBuildPicker:
+    def test_line_field_one_is_the_path(self):
+        _header, lines = _go.build_picker(PROJECTS, git_lookup=lambda _p: ("main", "clean"))
+        assert lines[0].split("\t")[0] == "/home/user/Projects/Demo"
+        assert lines[1].split("\t")[0] == "/home/user/Projects/group/app"
+
+    def test_one_line_per_project(self):
+        _header, lines = _go.build_picker(PROJECTS, git_lookup=lambda _p: ("main", "clean"))
+        assert len(lines) == len(PROJECTS)
+
+    def test_git_lookup_is_used_for_branch_and_status(self):
+        _header, lines = _go.build_picker(PROJECTS, git_lookup=lambda _p: ("feature/x", "dirty"))
+        assert "feature/x" in lines[0]
+        assert "dirty" in lines[0]
+
+
+class TestRunFzfSelection:
+    def test_no_timeout_and_column_options(self, monkeypatch):
+        captured = {}
+        monkeypatch.setattr(_go, "_git_info", lambda _p: ("main", "clean"))
+        monkeypatch.setattr(_go.subprocess, "run", _fake_run(0, PROJECTS[0]["path"] + "\n", captured))
+
+        selected = _go.run_fzf_selection(PROJECTS)
+
+        assert "timeout" not in captured["kwargs"]
+        argv = captured["argv"]
+        assert "--delimiter=\t" in argv
+        assert "--with-nth=2" in argv
+        assert "--accept-nth=1" in argv
+        assert any(a.startswith("--header=") for a in argv)
+        assert selected == PROJECTS[0]["path"]
+
+    def test_cancel_returns_none(self, monkeypatch):
+        captured = {}
+        monkeypatch.setattr(_go, "_git_info", lambda _p: ("main", "clean"))
+        monkeypatch.setattr(_go.subprocess, "run", _fake_run(130, "", captured))
+
+        assert _go.run_fzf_selection(PROJECTS) is None
+
+    def test_empty_selection_returns_none(self, monkeypatch):
+        captured = {}
+        monkeypatch.setattr(_go, "_git_info", lambda _p: ("main", "clean"))
+        monkeypatch.setattr(_go.subprocess, "run", _fake_run(0, "\n", captured))
+
+        assert _go.run_fzf_selection(PROJECTS) is None