From a64ba08e274975e6a0a4f001241196b4f9e40485 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Sat, 25 Jul 2026 18:26:46 -0400 Subject: [PATCH] fix(webui): address #905 REQUEST_CHANGES on notifications classifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B1: classify_attention_event uses structured flags/category only — never substring-match human-authored title/summary for escalation. B2: make notification ids unique across probe_errors and collisions (include loop index / kind). B3: do not assign probe_errors to fetch_error (avoids false Fetch Warning and double-reporting). Regression tests cover all three blockers. Refs #648 --- tests/test_webui_notifications.py | 189 ++++++++++++++++++++++++++++++ webui/notifications.py | 28 +++-- 2 files changed, 206 insertions(+), 11 deletions(-) diff --git a/tests/test_webui_notifications.py b/tests/test_webui_notifications.py index db89d4c..c928f7a 100644 --- a/tests/test_webui_notifications.py +++ b/tests/test_webui_notifications.py @@ -274,3 +274,192 @@ def test_notifications_app_routes(): res_alias = client.get("/api/notifications") assert res_alias.status_code == 200 assert res_alias.json()["project_id"] == json_data["project_id"] + + +def test_classify_ignores_human_authored_title_and_summary_keywords(): + """B1: keywords in human-authored titles must not escalate routine work (#905).""" + # Routine transition whose title/summary mention critical-boundary words + att_cls, req_human = classify_attention_event( + CATEGORY_WORKFLOW, + "record irrecoverable decision lock provenance", + "PR #999 'record irrecoverable decision lock provenance' is in routine state in-review.", + ) + assert att_cls == ATTENTION_ROUTINE + assert req_human is False + + att_cls, req_human = classify_attention_event( + CATEGORY_WORKFLOW, + "fix unauthorized token path", + "Issue #1 'fix unauthorized token path' state: claimed. hard stop docs only.", + ) + assert att_cls == ATTENTION_ROUTINE + assert req_human is False + + # Structured flags still escalate (machine-driven) + att_cls, req_human = classify_attention_event( + CATEGORY_SYSTEM, + "anything", + "anything with hard stop in text", + is_hard_stop=True, + ) + assert att_cls == ATTENTION_HUMAN_REQUIRED + assert req_human is True + + +def test_notification_ids_are_unique_across_probe_errors_and_collisions(): + """B2: published notification ids must be unique within a snapshot (#905).""" + reg = load_registry() + proj_id = reg.projects[0].id if reg.projects else "gitea-tools" + + mock_queue = QueueSnapshot( + project_id=proj_id, + repo_label="org/repo", + prs=(), + issues=(), + pr_pagination=None, + issue_pagination=None, + ) + mock_leases = LeaseSnapshot( + project_id=proj_id, + repo_label="org/repo", + issue_lock=None, + claim_inventory={}, + reviewer_leases=(), + duplicate_prs=( + CollisionWarning( + kind="duplicate_pr", + message="Multiple open PRs for issue #10", + issue_number=10, + pr_numbers=(10, 11), + ), + CollisionWarning( + kind="duplicate_branch", + message="Another collision without issue", + issue_number=None, + pr_numbers=(12, 13), + ), + CollisionWarning( + kind="duplicate_pr", + message="Second issue collision", + issue_number=10, + pr_numbers=(14, 15), + ), + ), + duplicate_branches=(), + collision_history=(), + fetch_error=None, + ) + mock_version = VersionInfo( + git_sha="abc1234", + git_describe="v1.0.0", + control_plane_schema_version=1, + python_version="3.11", + known=True, + ) + mock_stale = StaleRuntime( + daemon_head="abc1234", + checkout_head="abc1234", + remote_head="abc1234", + stale=False, + determinable=True, + mutation_safe=True, + reasons=(), + ) + mock_health = SystemHealthSnapshot( + status="degraded", + ready=False, + readiness_complete=True, + readiness_reasons=(), + service="webui", + mode="test", + version=mock_version, + started_at="2026-07-25T00:00:00Z", + uptime_seconds=100.0, + timestamp="2026-07-25T00:00:00Z", + deep_probes_requested=True, + dependencies=(), + mcp_namespaces=(), + stale_runtime=mock_stale, + probe_errors=("error alpha", "error beta"), + ) + + snapshot = load_notifications_snapshot( + proj_id, + load_queue=lambda _id: mock_queue, + load_leases=lambda **_kwargs: mock_leases, + load_health=lambda **_kwargs: mock_health, + ) + ids = [item.id for item in snapshot.items] + assert len(ids) == len(set(ids)), f"duplicate notification ids: {ids}" + assert any(i.startswith(f"notif-sys-err-{proj_id}-") for i in ids) + assert any(i.startswith("notif-collision-") for i in ids) + + +def test_probe_errors_do_not_set_fetch_error(): + """B3: probe_errors must not be reported as fetch_error (#905).""" + reg = load_registry() + proj_id = reg.projects[0].id if reg.projects else "gitea-tools" + + mock_queue = QueueSnapshot( + project_id=proj_id, + repo_label="org/repo", + prs=(), + issues=(), + pr_pagination=None, + issue_pagination=None, + fetch_error=None, + ) + mock_leases = LeaseSnapshot( + project_id=proj_id, + repo_label="org/repo", + issue_lock=None, + claim_inventory={}, + reviewer_leases=(), + duplicate_prs=(), + duplicate_branches=(), + collision_history=(), + fetch_error=None, + ) + mock_version = VersionInfo( + git_sha="abc1234", + git_describe="v1.0.0", + control_plane_schema_version=1, + python_version="3.11", + known=True, + ) + mock_stale = StaleRuntime( + daemon_head="abc1234", + checkout_head="abc1234", + remote_head="abc1234", + stale=False, + determinable=True, + mutation_safe=True, + reasons=(), + ) + mock_health = SystemHealthSnapshot( + status="degraded", + ready=False, + readiness_complete=True, + readiness_reasons=(), + service="webui", + mode="test", + version=mock_version, + started_at="2026-07-25T00:00:00Z", + uptime_seconds=100.0, + timestamp="2026-07-25T00:00:00Z", + deep_probes_requested=True, + dependencies=(), + mcp_namespaces=(), + stale_runtime=mock_stale, + probe_errors=("probe blew up",), + ) + + snapshot = load_notifications_snapshot( + proj_id, + load_queue=lambda _id: mock_queue, + load_leases=lambda **_kwargs: mock_leases, + load_health=lambda **_kwargs: mock_health, + ) + assert snapshot.fetch_error is None + # probe errors still appear as items + assert any("probe blew up" in item.summary for item in snapshot.items) diff --git a/webui/notifications.py b/webui/notifications.py index 15753b9..9dd8e90 100644 --- a/webui/notifications.py +++ b/webui/notifications.py @@ -7,13 +7,12 @@ operators receive direct alerts only for human-required escalation boundaries from __future__ import annotations -import os from dataclasses import dataclass, field from datetime import datetime, timezone -from typing import Any, Callable, Sequence +from typing import Any, Callable from webui import console_redaction -from webui.project_registry import find_project, load_registry +from webui.project_registry import load_registry from webui.queue_loader import QueueSnapshot, load_queue_snapshot from webui.lease_loader import LeaseSnapshot, load_lease_snapshot from webui.system_health import SystemHealthSnapshot, load_system_health @@ -158,7 +157,13 @@ def classify_attention_event( 2. Operational queues (blocker, stale lease, unassigned ready work, queue collision) -> ATTENTION_OPERATOR (requires_human=False). 3. Routine state transitions (clean progression, healthy heartbeats) -> ATTENTION_ROUTINE (requires_human=False). + + Classification uses structured flags and category only. Human-authored + ``title`` / ``summary`` text is never substring-matched for escalation + (PR #905 review B1) — callers that need text signals must set flags from + machine-generated status/detail fields before calling this function. """ + del title, summary # kept for API stability; never used for classification if ( is_hard_stop or is_auth_failure @@ -166,9 +171,6 @@ def classify_attention_event( or is_decision_lock or is_validation_failure or category in {CATEGORY_AUTH, CATEGORY_VALIDATION} - or "hard stop" in summary.lower() - or "unauthorized" in summary.lower() - or "irrecoverable" in summary.lower() ): return ATTENTION_HUMAN_REQUIRED, True @@ -234,7 +236,7 @@ def load_notifications_snapshot( now_iso = datetime.now(timezone.utc).isoformat() # 1. System health alerts (highest priority) - for probe_err in getattr(health_snap, "probe_errors", ()): + for err_idx, probe_err in enumerate(getattr(health_snap, "probe_errors", ())): att_cls, req_human = classify_attention_event( CATEGORY_SYSTEM, "System Health Probe Error", @@ -243,7 +245,7 @@ def load_notifications_snapshot( ) items.append( NotificationItem( - id=f"notif-sys-err-{project.id}", + id=f"notif-sys-err-{project.id}-{err_idx}", attention_class=att_cls, category=CATEGORY_SYSTEM, title="System Health Error", @@ -431,16 +433,18 @@ def load_notifications_snapshot( ) ) - for collision in lease_snap.duplicate_prs: + for col_idx, collision in enumerate(lease_snap.duplicate_prs): att_cls, req_human = classify_attention_event( CATEGORY_BLOCKER, f"Duplicate PR Collision ({collision.kind})", collision.message, is_blocker=True, ) + issue_part = collision.issue_number if collision.issue_number is not None else "none" + kind_part = (collision.kind or "unknown").replace(" ", "-") items.append( NotificationItem( - id=f"notif-collision-{collision.issue_number or 0}", + id=f"notif-collision-{kind_part}-{issue_part}-{col_idx}", attention_class=att_cls, category=CATEGORY_BLOCKER, title=f"Collision Alert ({collision.kind})", @@ -459,7 +463,9 @@ def load_notifications_snapshot( operator_count = sum(1 for i in items if i.attention_class == ATTENTION_OPERATOR) routine_count = sum(1 for i in items if i.attention_class == ATTENTION_ROUTINE) - fetch_err = queue_snap.fetch_error or lease_snap.fetch_error or getattr(health_snap, "probe_errors", None) + # Fetch errors are transport/load failures only — not probe results that + # already surface as first-class notification items (PR #905 review B3). + fetch_err = queue_snap.fetch_error or lease_snap.fetch_error if isinstance(fetch_err, (tuple, list)): fetch_err = "; ".join(fetch_err) if fetch_err else None