diff --git a/README.md b/README.md index d6992f3..af72547 100644 --- a/README.md +++ b/README.md @@ -312,6 +312,23 @@ lory tui Network calls run in background workers, so the UI never blocks on the platform. +Your place in the list is yours: marking, refreshing, and filtering all keep the +row you were on and the code leads you traced for it, rather than throwing you +back to the top. A filter that matches nothing selects nothing — the action keys +say so instead of quietly acting on a finding scrolled off screen. + +`e` takes `$EDITOR` as a command line, so `EDITOR="code -w"` and +`EDITOR="emacsclient -nw"` work; an editor that will not start is reported in +the status bar rather than taking the cockpit down. + +> **`R` needs a server that addresses findings by `ref`.** The platform's +> `retest.request` currently accepts a bare numeric `finding_id`, which it +> resolves in the manual pentest store only. A retest for an engagement or +> incident finding therefore comes back `Finding not found` — the finding is +> fine, the request could not name it. The status bar says as much when it +> happens. Until the tool takes a `ref`, request those retests from the portal. +> `lory mcp tools` shows what your server accepts today. + --- ## Command reference @@ -345,6 +362,10 @@ lory harness checks List available assertions bare id that names findings in more than one store is rejected with the list of refs to choose from, rather than resolved by guessing. +`lory retest` can only address findings the server's own `retest.request` +resolves — see [the caveat above](#the-cockpit). It sends the `ref` where the +tool's schema declares one, and explains the mismatch where it does not. + A typical session: ```bash @@ -566,7 +587,7 @@ A config written for the older session-cookie build still loads: `session_cookie ```bash pip install -e ".[dev]" -pytest # 119 tests, no network required +pytest # 205 tests, no network required ruff check src tests lory harness lint scenarios/ # validate scenarios without calling Lory ``` diff --git a/src/lory_code_security/domain/findings.py b/src/lory_code_security/domain/findings.py index a557d68..fca9824 100644 --- a/src/lory_code_security/domain/findings.py +++ b/src/lory_code_security/domain/findings.py @@ -427,9 +427,33 @@ def request_retest(self, finding: Finding, note: str = "") -> dict[str, Any]: args["note"] = note[:2000] result = self.client.call_tool("retest.request", args) - result.raise_for_error() + try: + result.raise_for_error() + except ToolError as exc: + raise self._explain_retest_failure(exc, finding, args) from exc return result.structured if isinstance(result.structured, dict) else {"raw": result.text} + @staticmethod + def _explain_retest_failure( + exc: ToolError, finding: Finding, args: dict[str, Any] + ) -> ToolError: + """Say *why* a retest bounced when the cause is the id/ref mismatch. + + A server whose ``retest.request`` takes only ``finding_id`` resolves + that integer in one store. Ids repeat across stores, so a retest for + ``engagement-1652`` arrives as ``1652`` and is looked up among manual + pentest findings, which answers "Finding not found" — a message that + reads like the finding was deleted rather than mis-addressed. + """ + if "ref" in args or not finding.ref or "not found" not in str(exc).lower(): + return exc + return ToolError( + f"{exc} — this server's retest.request takes a numeric finding_id " + f"only, so it looked up #{finding.id} in the wrong store. " + f"{finding.key} cannot be retested over MCP until the tool accepts " + f"a ref; request it from the portal instead." + ) + def search_kb(self, query: str, limit: int = 5) -> list[dict[str, Any]]: """Look the finding class up in the vulnerability knowledge base.""" if self.client is None: diff --git a/src/lory_code_security/ui/app.py b/src/lory_code_security/ui/app.py index a80d656..8a56a7a 100644 --- a/src/lory_code_security/ui/app.py +++ b/src/lory_code_security/ui/app.py @@ -46,6 +46,11 @@ ) from lory_code_security.ui import render +#: Shown when an action needs a finding and the table has none highlighted — +#: an empty filter result, or an account with nothing on it. Silence there read +#: as a dead key. +_NO_SELECTION = "no finding selected" + SEVERITY_COLOURS = { "critical": "bold white on red", "high": "bold red", @@ -120,6 +125,11 @@ def __init__(self, cfg: Config, start_cached: bool = False) -> None: self.code_matches_key: str | None = None self.filter_text = "" self.send_code = cfg.send_code_context + #: Set while a table rebuild is putting the cursor back where the user + #: left it. Clearing the table drops the cursor to row 0 and fires + #: RowHighlighted for a row nobody selected; acting on that event moves + #: the selection and discards the trace. See :meth:`apply_filter`. + self._restoring_key: str | None = None # ── layout ────────────────────────────────────────────────────────────── @@ -260,25 +270,41 @@ def show_empty(self, headline: str, detail: str = "") -> None: if detail: body.append(f"\n{detail}", style="dim") self.query_one("#detail", Static).update(body) - self.query_one("#counts", Static).update("0 findings") + self._update_counts(0) self.set_status(headline) def set_findings(self, rows: list[Finding], source: str) -> None: self.all_findings = rows self.apply_filter() - counts = severity_counts(rows) + self.set_status(f"{len(rows)} findings via {source}") + + def _update_counts(self, visible: int) -> None: + """The counts line: severity mix of the account, and how much is shown. + + The severity breakdown always describes the whole account — it is the + shape of the backlog, not of the current filter. The leading count does + track the filter, because "22 findings" above three visible rows reads + as a bug in the filter. + """ + counts = severity_counts(self.all_findings) summary = " ".join( f"{counts[s]} {s[0].upper()}" for s in ("critical", "high", "medium", "low", "info") if counts.get(s) ) - self.query_one("#counts", Static).update(f"{len(rows)} findings {summary}") - self.set_status(f"{len(rows)} findings via {source}") + total = len(self.all_findings) + shown = f"{visible} of {total} findings" if visible != total else f"{total} findings" + self.query_one("#counts", Static).update(f"{shown} {summary}") def apply_filter(self) -> None: rows = filter_findings(self.all_findings, query=self.filter_text) table = self.query_one("#findings-table", DataTable) + # `clear()` resets the cursor to row 0 and re-fires RowHighlighted, so + # the selection has to be captured before the rebuild and put back + # after it. Without this, marking or refreshing from any row but the + # first threw the user back to the top of the list. + keep = self.selected_key table.clear() for finding in rows: state = self.triage.state(finding.key) @@ -297,7 +323,34 @@ def apply_filter(self) -> None: # the moment two stores each held a finding with the same id. key=finding.key, ) - if rows and self.selected_key is None: + + self._update_counts(len(rows)) + + if not rows: + # Nothing is selectable. Leaving `selected_key` pointing at a row + # that is no longer on screen let f/t/m/R act on a finding the user + # could not see — including filing a retest for it. + self.selected_key = None + self.code_matches = [] + self.code_matches_key = None + self._restoring_key = None + self.show_empty( + f"No finding matches {self.filter_text!r}." + if self.filter_text + else "No findings to show.", + "Press esc to clear the filter." if self.filter_text else "", + ) + return + + keys = [f.key for f in rows] + if keep in keys: + index = keys.index(keep) + # Row 0 is where the rebuild already left the cursor; moving to it + # would fire no event, and the guard would never be released. + if index: + self._restoring_key = keep + table.move_cursor(row=index, animate=False) + else: self.selected_key = rows[0].key self.show_detail(rows[0]) @@ -312,7 +365,17 @@ def current(self) -> Finding | None: def _row_highlighted(self, event: DataTable.RowHighlighted) -> None: if event.row_key is None or event.row_key.value is None: return - self.selected_key = str(event.row_key.value) + key = str(event.row_key.value) + + # A rebuild emits a highlight for row 0 before the cursor is put back. + # That is not a move the user made, so ignore everything until the + # restored row arrives. + if self._restoring_key is not None: + if key != self._restoring_key: + return + self._restoring_key = None + + self.selected_key = key # Drop code leads only when the selection genuinely moved. Rebuilding # the table (filter, mark-fixed) re-fires this for the same row, and # discarding the trace there loses work the user just did. @@ -403,6 +466,7 @@ def action_toggle_code_context(self) -> None: def action_trace_code(self) -> None: finding = self.current() if finding is None: + self.set_status(_NO_SELECTION) return self.set_status("searching the working tree…") self.trace_worker(finding) @@ -423,6 +487,7 @@ def trace_worker(self, finding: Finding) -> None: def action_ask_lory(self) -> None: finding = self.current() if finding is None: + self.set_status(_NO_SELECTION) return self.query_one("#lory-pane").add_class("visible") @@ -451,6 +516,7 @@ def action_ask_lory(self) -> None: def action_mark_fixed(self) -> None: finding = self.current() if finding is None: + self.set_status(_NO_SELECTION) return state = "new" if self.triage.state(finding.key) == "fixed" else "fixed" self.triage.set_state(finding.key, state) @@ -459,7 +525,11 @@ def action_mark_fixed(self) -> None: def action_request_retest(self) -> None: finding = self.current() - if finding is None or self.store is None: + if finding is None: + self.set_status(_NO_SELECTION) + return + if self.store is None: + self.set_status("no read path configured — run `lory init`") return self.push_screen( ConfirmScreen( @@ -479,20 +549,41 @@ def retest_worker(self, finding: Finding) -> None: self.call_from_thread(self.set_status, f"retest requested for {finding.key}") def action_open_editor(self) -> None: - """Open the top code lead in $EDITOR, suspending the TUI.""" + """Open the top code lead in $EDITOR, suspending the TUI. + + ``$EDITOR`` is a command line, not a program name: ``code -w`` and + ``emacsclient -nw`` are both common. Running it unsplit looked for a + program literally called ``code -w``, and the resulting + FileNotFoundError propagated out of the action and took the whole + cockpit down with it — losing the session over a typo in an env var. + """ import os + import shlex import subprocess + from textual.app import SuspendNotSupported + if not self.code_matches: self.set_status("no code leads yet — press t to trace") return - editor = os.environ.get("EDITOR", "vi") + editor = os.environ.get("EDITOR", "").strip() or "vi" + try: + command = shlex.split(editor) + except ValueError: # an unbalanced quote in $EDITOR + command = [editor] + if not command: + command = ["vi"] + match = self.code_matches[0] - with self.suspend(): - subprocess.run( - [editor, *_editor_target(editor, match.path, match.line)], check=False - ) + argv = [*command, *_editor_target(command[0], match.path, match.line)] + try: + with self.suspend(): + subprocess.run(argv, check=False) + except SuspendNotSupported: + self.set_status(f"cannot suspend to run {command[0]} on this terminal") + except OSError as exc: + self.set_status(f"could not run $EDITOR ({command[0]}): {exc}") # ── Lory ──────────────────────────────────────────────────────────────── diff --git a/tests/test_findings.py b/tests/test_findings.py index 707f1a6..d8adb3e 100644 --- a/tests/test_findings.py +++ b/tests/test_findings.py @@ -366,3 +366,34 @@ def test_retest_sends_the_ref_only_where_the_schema_takes_it(tmp_path): schemas={"retest.request": ["finding_id", "ref", "note"]}) FindingStore(modern, cache_path=tmp_path / "b.json").request_retest(finding) assert modern.calls[-1] == ("retest.request", {"finding_id": 12, "ref": "engagement-12"}) + + +def test_a_retest_that_cannot_address_the_store_says_so(tmp_path): + """"Finding not found" reads as *deleted*; the real cause is the bare id. + + A server whose retest.request takes only ``finding_id`` resolves that + integer in one store. An engagement finding therefore bounces even though + it is right there in the list. + """ + client = FakeMcp({"retest.request"}, [], is_error=True, error_text="Finding not found") + store = FindingStore(client, cache_path=tmp_path / "c.json") + + with pytest.raises(ToolError) as excinfo: + store.request_retest(Finding(id=1652, ref="engagement-1652", store="engagement")) + + message = str(excinfo.value) + assert "Finding not found" in message # the server's own words survive + assert "wrong store" in message # ...plus why + assert "engagement-1652" in message + + +def test_an_unrelated_retest_failure_is_not_reinterpreted(tmp_path): + """Only the id/ref mismatch gets the extra explanation.""" + client = FakeMcp({"retest.request"}, [], is_error=True, + error_text="retest denied: engagement is closed") + store = FindingStore(client, cache_path=tmp_path / "d.json") + + with pytest.raises(ToolError) as excinfo: + store.request_retest(Finding(id=12, ref="engagement-12", store="engagement")) + + assert "wrong store" not in str(excinfo.value) diff --git a/tests/test_tui.py b/tests/test_tui.py index d6cc5c5..eb29333 100644 --- a/tests/test_tui.py +++ b/tests/test_tui.py @@ -11,7 +11,9 @@ pytest.importorskip("textual") -from textual.widgets import DataTable # noqa: E402 +from contextlib import contextmanager # noqa: E402 + +from textual.widgets import DataTable, Input, Static # noqa: E402 from lory_code_security.core.config import Config # noqa: E402 from lory_code_security.core.errors import TransportError # noqa: E402 @@ -66,6 +68,25 @@ def make_app(monkeypatch, tmp_path, store) -> LoryApp: return LoryApp(cfg) +def _widget_text(app, selector: str) -> str: + """The text a Static is currently showing.""" + return str(app.query_one(selector, Static).visual) + + +def _status(app) -> str: + return _widget_text(app, "#status") + + +def _counts(app) -> str: + return _widget_text(app, "#counts") + + +@contextmanager +def _no_suspend(): + """Stand in for App.suspend(), which a headless driver cannot do.""" + yield + + # ── the crash ─────────────────────────────────────────────────────────────── @@ -272,6 +293,174 @@ async def test_toggling_code_context_updates_the_empty_state(monkeypatch, tmp_pa assert "on" in str(app._lory_intro()) +# ── a rebuild must not move the user off the row they are working on ──────── + + +async def test_marking_a_lower_row_keeps_the_selection(monkeypatch, tmp_path): + """clear() drops the cursor to row 0, which threw the user back to the top. + + The old test only ever marked the first row, where a reset to row 0 is + indistinguishable from no reset at all. + """ + app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS)) + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + await pilot.press("down", "down") + await pilot.pause() + target = app.selected_key + assert target == "incident-31" + + await pilot.press("m") + await pilot.pause() + + assert app.triage.state(target) == "fixed" + assert app.selected_key == target + assert app.query_one("#findings-table", DataTable).cursor_row == 2 + + +async def test_refreshing_keeps_the_selection(monkeypatch, tmp_path): + app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS)) + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + await pilot.press("down") + await pilot.pause() + + await pilot.press("r") + await pilot.pause() + await pilot.pause() + + assert app.selected_key == "engagement-12" + assert app.query_one("#findings-table", DataTable).cursor_row == 1 + + +async def test_a_trace_on_a_lower_row_survives_marking_it(monkeypatch, tmp_path): + """The transient row-0 highlight used to discard the leads on its way past.""" + app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS)) + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + await pilot.press("down") + await pilot.pause() + app.code_matches = [CodeMatch(path=tmp_path / "a.py", line=1, text="x", token="q")] + app.code_matches_key = app.selected_key + + await pilot.press("m") + await pilot.pause() + + assert app.selected_key == "engagement-12" + assert len(app.code_matches) == 1 + + +# ── a filter that matches nothing must not leave a live selection ─────────── + + +async def test_a_filter_matching_nothing_clears_the_selection(monkeypatch, tmp_path): + """f/t/m/R used to act on a finding that was no longer on screen.""" + app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS)) + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + app.query_one("#filter", Input).focus() + await pilot.press("z", "z", "z", "enter") + await pilot.pause() + + assert app.query_one("#findings-table", DataTable).row_count == 0 + assert app.selected_key is None + assert app.current() is None + + +async def test_acting_with_no_selection_says_so(monkeypatch, tmp_path): + """Silence on a keypress reads as a dead key, not as an empty list.""" + store = StubStore(COLLIDING_ROWS) + app = make_app(monkeypatch, tmp_path, store) + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + app.query_one("#filter", Input).focus() + await pilot.press("z", "z", "z", "enter") + await pilot.pause() + + for key in ("f", "t", "m", "R"): + app.set_status("") + await pilot.press(key) + await pilot.pause() + assert "no finding selected" in _status(app), key + + assert store.retests == [] + + +async def test_the_counts_line_tracks_the_filter(monkeypatch, tmp_path): + app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS)) + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + assert _counts(app).startswith("3 findings") + + app.query_one("#filter", Input).focus() + await pilot.press("x", "s", "s", "enter") + await pilot.pause() + + # The severity mix still describes the account; the count tracks the view. + assert _counts(app).startswith("1 of 3 findings") + + +# ── $EDITOR is a command line, not a program name ─────────────────────────── + + +async def test_an_editor_with_arguments_is_split_not_exec_d_whole(monkeypatch, tmp_path): + """`EDITOR="code -w"` looked for a program called "code -w" and crashed.""" + app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS)) + launched: list[list[str]] = [] + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + app.code_matches = [CodeMatch(path=tmp_path / "a.py", line=42, text="x", token="q")] + + monkeypatch.setenv("EDITOR", "vim --clean") + monkeypatch.setattr(app, "suspend", _no_suspend) + monkeypatch.setattr( + "subprocess.run", lambda argv, **kw: launched.append(list(argv)) + ) + await pilot.press("e") + await pilot.pause() + + assert launched == [["vim", "--clean", "+42", str(tmp_path / "a.py")]] + + +async def test_an_editor_that_will_not_start_is_reported_not_raised(monkeypatch, tmp_path): + """An unrunnable $EDITOR must not take the cockpit down with it.""" + app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS)) + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + app.code_matches = [CodeMatch(path=tmp_path / "a.py", line=1, text="x", token="q")] + + monkeypatch.setenv("EDITOR", "definitely-not-installed") + monkeypatch.setattr(app, "suspend", _no_suspend) + await pilot.press("e") + await pilot.pause() + + assert app.is_running + assert "could not run $EDITOR" in _status(app) + + +async def test_a_terminal_that_cannot_suspend_is_reported_not_raised(monkeypatch, tmp_path): + app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS)) + + async with app.run_test(size=(160, 40)) as pilot: + await pilot.pause() + app.code_matches = [CodeMatch(path=tmp_path / "a.py", line=1, text="x", token="q")] + + # run_test() drives a headless driver, which genuinely cannot suspend. + await pilot.press("e") + await pilot.pause() + + assert app.is_running + assert "cannot suspend" in _status(app) + + async def test_marking_one_finding_does_not_mark_its_id_twin(monkeypatch, tmp_path): app = make_app(monkeypatch, tmp_path, StubStore(COLLIDING_ROWS))