From 1ca2b5040619f6d44ee8f804590a9bc257fd2e90 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Sat, 25 Jul 2026 01:25:28 -0400 Subject: [PATCH] fix(webui): authority-aware ownership + redacted contamination text (#641) Addresses the two blockers from the PR #898 review at a81db754. B1 - degraded ownership inventory was rendered as affirmative absence. _build_session_rows read the sessions/leases/locks sections without consulting their status, so a session row emitted lease_ids=() and worktree_paths=() whether the session genuinely held nothing or the lease store simply could not be read. The renderer printed both as "none" and "unbound", contradicting the ownership_authority_complete invariant documented on InventorySnapshot. SessionRow now carries lease_authority and worktree_authority. A worktree binding is correlated through lease work numbers, so it is unproven when either the leases or the locks section fails to read -- this covers the narrow variant where locks hold real worktree paths but a degraded leases section leaves work_numbers empty. The renderer emits "unknown (inventory )" with an authority-unproven badge instead of none/unbound, the card names the unreadable sections, and an empty session list from an unreadable sessions section no longer reads as "no sessions recorded". snapshot_to_dict exports ownership_authority_complete, ownership_section_status, and per-row lease_authority / worktree_authority so /api/sessions consumers can distinguish the two cases. B2 - contamination payload strings bypassed redaction. _inspect_contamination copied command_summary, session_id, role and reason_class out of the marker payload with only str(), while every inventory-sourced field on the same page arrives through webui.inventory.scrub(). The write-time redactor stable_branch_push_guard.redact_command is a narrow denylist that leaves absolute $HOME paths, -H 'X-Api-Key: ', --password , and PRIVATE_KEY= intact, and this is the first web surface to render command_summary at all. Adds webui.inventory.scrub_text(), which collapses $HOME and redacts credential-shaped tokens and URL userinfo anywhere inside a string rather than only at its start, and routes the marker payload through it. scrub() and every existing caller are untouched. The command_summary field is kept: it is the #630 evidence naming which daemon was killed. The module docstring claiming absolute paths were already collapsed is corrected. Also: removes the locks_by_session_hint dead loop and its discard (N1), adds the missing trailing newline to webui/runtime_views.py (N4), drops an unused dataclasses.field import, and documents both honesty rules in docs/webui-local-dev.md. Tests: tests/test_webui_sessions_view.py grows from 12 to 26 cases, covering degraded and unavailable ownership sections in both the HTML and JSON paths, the locks-readable/leases-degraded variant, a guard against over-correcting clean inventory into "unknown", the previously untested expired-lease flag, HTML escaping of hostile values in clean and degraded renders, and each secret class the write-time denylist misses. The STATUS_UNAVAILABLE import that was present but unused is now exercised. Validation, from the issue worktree with venv/bin/python (Python 3.14.5, pytest 9.1.1): pytest tests/test_webui_sessions_view.py tests/test_webui_*.py -q -> 512 passed, 376 subtests (was 498 / 372; +14 new tests) pytest tests/test_issue_854_semantic_container_exclusion.py -q -> 13 passed, 8 subtests 13-file runtime/health/inventory/restart set -> 1 failed, 223 passed; the single failure is test_runtime_clarity.py::TestRuntimeClarity:: test_activate_profile_succeeds_when_enabled, the identical test and assertion the reviewer recorded on master at 7af40fb5, so it is baseline-equivalent and not introduced here. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/webui-local-dev.md | 20 ++ tests/test_webui_sessions_view.py | 292 +++++++++++++++++++++++++++++- webui/inventory.py | 65 +++++++ webui/runtime_views.py | 2 +- webui/session_loader.py | 121 +++++++++++-- webui/session_views.py | 97 ++++++++-- 6 files changed, 556 insertions(+), 41 deletions(-) diff --git a/docs/webui-local-dev.md b/docs/webui-local-dev.md index 70bb50b..3e01401 100644 --- a/docs/webui-local-dev.md +++ b/docs/webui-local-dev.md @@ -307,6 +307,26 @@ reconnect/operator restart docs (`docs/mcp-namespace-eof-recovery.md`, document). The page does **not** restart, kill, or take over sessions; manual `pkill` of MCP daemons is contamination, not recovery. +Honesty rules specific to this view: + +* **Ownership columns never assert absence they cannot prove.** When the + `leases` or `locks` section is degraded or unavailable, the Leases and + Worktree-binding cells render `unknown (inventory )` with an + *authority unproven* badge instead of `none` / `unbound`, and a caveat names + the unreadable sections. A worktree binding is correlated through lease work + numbers, so it is unproven when *either* section fails to read. + `/api/sessions` carries the same facts as `ownership_authority_complete`, + `ownership_section_status`, and per-row `lease_authority` / + `worktree_authority`, so a JSON consumer can tell "holds none" from "could + not be read". +* **Contamination text is redacted at the display boundary.** Marker payloads + (`command_summary`, `reason_class`, `session_id`, `role`) are + operator-supplied free text that does not arrive through inventory scrubbing, + so they pass through `webui.inventory.scrub_text`, which collapses `$HOME` and + redacts credential-shaped tokens and URL userinfo *anywhere* in the string. + The write-time redactor is a narrow denylist and is not relied on. The field + itself is kept — it is the `#630` evidence naming which daemon was killed. + ## System-health dashboard (#639) `/system-health` renders the same snapshot the `/api/v1/system/health` API diff --git a/tests/test_webui_sessions_view.py b/tests/test_webui_sessions_view.py index 8bb49fd..4799b77 100644 --- a/tests/test_webui_sessions_view.py +++ b/tests/test_webui_sessions_view.py @@ -3,10 +3,16 @@ Covers clean and stale session rendering, contamination marker surfacing, worktree binding display, sanctioned recovery links (no pkill), nav/live status, and the JSON API export. + +Also pins the two invariants a reviewer found violated at head a81db754: +degraded ownership sections must render as *unknown* rather than as an +affirmative "none"/"unbound", and contamination payload text must be redacted +at the display boundary rather than trusted from the write-time denylist. """ from __future__ import annotations +import os import sys import unittest from pathlib import Path @@ -22,6 +28,7 @@ from webui.inventory import ( AUTHORITY_FILESYSTEM, InventorySection, InventorySnapshot, + STATUS_DEGRADED, STATUS_OK, STATUS_UNAVAILABLE, ) @@ -32,6 +39,7 @@ from webui.session_loader import ( SessionRow, SessionViewSnapshot, _build_session_rows, + _inspect_contamination, load_session_view_snapshot, snapshot_to_dict, ) @@ -77,36 +85,43 @@ def _inventory( locks: tuple[dict, ...] = (), worktrees: tuple[dict, ...] = (), namespaces: tuple[dict, ...] = (), + statuses: dict[str, str] | None = None, ) -> InventorySnapshot: + """Build a snapshot; ``statuses`` degrades named sections (default all ok).""" + status_of = statuses or {} + + def _status(name: str) -> str: + return status_of.get(name, STATUS_OK) + sections = ( InventorySection( name="sessions", authority=AUTHORITY_CONTROL_PLANE_DB, - status=STATUS_OK, + status=_status("sessions"), items=sessions, ), InventorySection( name="leases", authority=AUTHORITY_CONTROL_PLANE_DB, - status=STATUS_OK, + status=_status("leases"), items=leases, ), InventorySection( name="locks", authority=AUTHORITY_FILESYSTEM, - status=STATUS_OK, + status=_status("locks"), items=locks, ), InventorySection( name="worktrees", authority=AUTHORITY_FILESYSTEM, - status=STATUS_OK, + status=_status("worktrees"), items=worktrees, ), InventorySection( name="namespaces", authority=AUTHORITY_FILESYSTEM, - status=STATUS_OK, + status=_status("namespaces"), items=namespaces or ( { @@ -453,5 +468,272 @@ class TestSessionsRoutes(unittest.TestCase): ) +class TestDegradedOwnershipAuthority(unittest.TestCase): + """B1: a section that could not be read must never render as absence.""" + + def _snapshot(self, inventory: InventorySnapshot) -> SessionViewSnapshot: + return SessionViewSnapshot( + runtime=_runtime(), + inventory=inventory, + sessions=_build_session_rows(inventory, contamination=()), + contamination_markers=(), + ) + + def test_unavailable_leases_mark_row_authority_unproven(self): + inventory = _inventory( + sessions=(_clean_session(),), + statuses={"leases": STATUS_UNAVAILABLE}, + ) + row = _build_session_rows(inventory, contamination=())[0] + self.assertEqual(row.lease_ids, ()) + self.assertEqual(row.lease_authority, STATUS_UNAVAILABLE) + # Worktree binding is correlated through lease work numbers, so it + # inherits the unreadable lease section. + self.assertEqual(row.worktree_authority, STATUS_UNAVAILABLE) + self.assertFalse(row.ownership_authority_complete) + + def test_readable_locks_are_not_reported_unbound_when_leases_degrade(self): + # The narrow variant: locks hold a real worktree_path and read cleanly, + # but the lease section that supplies the correlating work number does + # not. The row must say unknown, not "unbound". + inventory = _inventory( + sessions=(_clean_session(),), + locks=( + { + "issue_number": 641, + "worktree_path": "branches/feat-issue-641", + }, + ), + statuses={"leases": STATUS_DEGRADED}, + ) + row = _build_session_rows(inventory, contamination=())[0] + self.assertEqual(row.worktree_paths, ()) + self.assertEqual(row.worktree_authority, STATUS_DEGRADED) + self.assertFalse(row.ownership_authority_complete) + + def test_degraded_render_says_unknown_not_none_or_unbound(self): + inventory = _inventory( + sessions=(_clean_session(),), + statuses={"leases": STATUS_UNAVAILABLE, "locks": STATUS_UNAVAILABLE}, + ) + html = render_sessions_page(self._snapshot(inventory)) + self.assertIn("unknown (inventory unavailable)", html) + self.assertIn("authority unproven", html) + self.assertIn("Ownership authority incomplete", html) + # The affirmative-absence strings must be gone from the row entirely. + self.assertNotIn(">none<", html) + self.assertNotIn(">unbound<", html) + + def test_clean_inventory_still_renders_affirmative_absence(self): + # Guards against over-correcting B1 into "everything is unknown". + inventory = _inventory(sessions=(_clean_session(),)) + html = render_sessions_page(self._snapshot(inventory)) + self.assertIn(">none<", html) + self.assertIn(">unbound<", html) + # The column legend mentions "unknown (inventory …)" as static copy, so + # assert on the per-row marker and the concrete statuses instead. + self.assertNotIn("authority unproven", html) + self.assertNotIn("unknown (inventory unavailable)", html) + self.assertNotIn("unknown (inventory degraded)", html) + self.assertNotIn("Ownership authority incomplete", html) + + def test_json_export_carries_snapshot_and_per_row_authority(self): + inventory = _inventory( + sessions=(_clean_session(),), + statuses={"locks": STATUS_UNAVAILABLE}, + ) + data = snapshot_to_dict(self._snapshot(inventory)) + self.assertFalse(data["ownership_authority_complete"]) + self.assertEqual( + data["ownership_section_status"]["locks"], STATUS_UNAVAILABLE + ) + self.assertEqual(data["ownership_section_status"]["leases"], STATUS_OK) + self.assertIn("unknown, not unowned", data["ownership_note"]) + + row = data["sessions"][0] + self.assertTrue(row["lease_authority_complete"]) + self.assertFalse(row["worktree_authority_complete"]) + self.assertEqual(row["worktree_authority"], STATUS_UNAVAILABLE) + self.assertFalse(row["ownership_authority_complete"]) + self.assertIn("unknown, not unowned", row["ownership_note"]) + + def test_json_export_is_affirmative_when_every_source_reads(self): + inventory = _inventory(sessions=(_clean_session(),)) + data = snapshot_to_dict(self._snapshot(inventory)) + self.assertTrue(data["ownership_authority_complete"]) + self.assertTrue(data["sessions"][0]["ownership_authority_complete"]) + + def test_missing_session_list_is_not_reported_as_no_sessions(self): + inventory = _inventory(statuses={"sessions": STATUS_UNAVAILABLE}) + html = render_sessions_page(self._snapshot(inventory)) + self.assertIn("could not be read", html) + self.assertIn("not evidence that no sessions exist", html) + + def test_expired_lease_flags_row_as_stale(self): + inventory = _inventory( + sessions=(_clean_session(),), + leases=( + { + "lease_id": "lease-expired-1", + "session_id": "prgs-author-111-clean", + "status": "active", + "expired": True, + "work_kind": "issue", + "work_number": 641, + }, + ), + ) + row = _build_session_rows(inventory, contamination=())[0] + self.assertIn("lease-expired", row.stale_flags) + self.assertIn("active-lease-past-expiry", row.stale_flags) + + +class TestContaminationRedaction(unittest.TestCase): + """B2: marker payload text is redacted at the display boundary.""" + + def _marker(self, payload: dict) -> ContaminationMarker: + return _inspect_contamination( + "runtime_recovery_contamination", + remote="prgs", + inspect=lambda **_k: { + "on_disk": True, + "has_payload": True, + "summary": "", + }, + load=lambda **_k: payload, + ) + + def test_home_paths_are_collapsed(self): + home = os.path.expanduser("~") + marker = self._marker( + {"command_summary": f"pkill -f {home}/Development/Gitea-Tools/x.py"} + ) + self.assertNotIn(home, marker.command_summary) + self.assertIn("~/Development/Gitea-Tools/x.py", marker.command_summary) + + def test_secrets_missed_by_the_write_time_denylist_are_redacted(self): + # Each of these was verified in review to survive + # stable_branch_push_guard.redact_command untouched. + cases = ( + ("curl -H 'X-Api-Key: SUPERSECRET123' https://example.invalid", "SUPERSECRET123"), + ("cmd --password hunter2 origin master", "hunter2"), + ("PRIVATE_KEY=abc123 python deploy.py", "abc123"), + ("fetch https://user:pw@example.invalid/x.git", "user:pw"), + ) + for raw, secret in cases: + with self.subTest(raw=raw): + marker = self._marker({"command_summary": raw}) + self.assertNotIn(secret, marker.command_summary) + self.assertIn("[redacted]", marker.command_summary) + + def test_command_summary_is_redacted_not_removed(self): + # It is legitimate #630 evidence: the operator must still see which + # daemon was killed. + marker = self._marker( + { + "command_summary": "pkill -f gitea_mcp_server.py", + "reason_class": "manual_daemon_kill", + "session_id": "prgs-author-111-clean", + "role": "author", + } + ) + self.assertIn("pkill -f gitea_mcp_server.py", marker.command_summary) + self.assertEqual(marker.reason_class, "manual_daemon_kill") + self.assertEqual(marker.session_id, "prgs-author-111-clean") + self.assertEqual(marker.role, "author") + + def test_rendered_page_exposes_no_home_path_from_a_marker(self): + home = os.path.expanduser("~") + marker = self._marker( + { + "command_summary": f"pkill -f {home}/Development/Gitea-Tools/x.py", + "reason_class": "manual_daemon_kill", + } + ) + inventory = _inventory(sessions=(_clean_session(),)) + html = render_sessions_page( + SessionViewSnapshot( + runtime=_runtime(), + inventory=inventory, + sessions=_build_session_rows(inventory, (marker,)), + contamination_markers=(marker,), + ) + ) + self.assertIn("Contamination markers", html) + self.assertNotIn(home, html) + + +class TestSessionsPageEscaping(unittest.TestCase): + """Hostile values from every rendered source stay inert (N2).""" + + HOSTILE = '' + + def test_hostile_session_and_marker_values_are_escaped(self): + session = dict(_clean_session()) + session["session_id"] = f"sid-{self.HOSTILE}" + session["role"] = self.HOSTILE + session["profile"] = self.HOSTILE + session["namespace"] = self.HOSTILE + session["status"] = self.HOSTILE + inventory = _inventory( + sessions=(session,), + leases=( + { + "lease_id": self.HOSTILE, + "session_id": session["session_id"], + "status": "active", + "expired": False, + "work_kind": self.HOSTILE, + "work_number": 641, + }, + ), + locks=( + { + "issue_number": 641, + "worktree_path": self.HOSTILE, + }, + ), + ) + marker = ContaminationMarker( + kind="runtime_recovery_contamination", + on_disk=True, + has_payload=True, + summary=self.HOSTILE, + reason_class=self.HOSTILE, + session_id=session["session_id"], + role=self.HOSTILE, + command_summary=self.HOSTILE, + cleared=False, + ) + html = render_sessions_page( + SessionViewSnapshot( + runtime=_runtime(), + inventory=inventory, + sessions=_build_session_rows(inventory, (marker,)), + contamination_markers=(marker,), + ) + ) + self.assertNotIn("