From 67d5835eb89b9b864b338778978c915588af23c5 Mon Sep 17 00:00:00 2001 From: Chaim Date: Tue, 7 Jul 2026 15:05:51 +0000 Subject: [PATCH] =?UTF-8?q?fix(agents):=20=D7=A0=D7=99=D7=AA=D7=95=D7=91?= =?UTF-8?q?=20=D7=94=D7=A2=D7=A8=D7=AA-=D7=99=D7=95"=D7=A8=20=D7=9C=D7=90?= =?UTF-8?q?=20=D7=9C-issue=20=D7=A1=D7=92=D7=95=D7=A8=20(cancelled)=20?= =?UTF-8?q?=E2=80=94=20=D7=AA=D7=9E=D7=99=D7=93=20=D7=9C-CEO=20=D7=94?= =?UTF-8?q?=D7=97=D7=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit בעיה חוזרת: הקלדת הוראה בתיבת "כתוב הוראה לסוכנים..." נבלעה בשקט. השרת בחר את היעד ב-`active[-1]` תוך סינון `done` בלבד — לא `cancelled`. כשה-child האחרון בוטל (למשל 1027-04-26: "חישוב טיעונים" cancelled שנוצר אחרי ה-issue הראשי), ההערה נותבה אליו; Paperclip מדלג על wakeup ל-issue מבוטל → ההוראה לא הגיעה לאיש. תיקון (G1 — נרמול במקור, לא תיקון-בקריאה): - `pick_default_comment_target` חדש ב-paperclip_client — מעדיף את ה-issue הראשי החי של ה-CEO (top-level+open), נופל ל-open כלשהו, ואז top-level, ואף פעם לא מעדיף child-סגור. `get_case_issues` מחזיר כעת `parent_id`. - app.py מנתב דרכו; מחזיר `issue_status` כדי שה-UI יוכל לסמן יעד-סגור. - נחשף דרך ה-Port (G12) כ-`pc_pick_default_comment_target`. - 6 בדיקות ל-picker (כולל תרחיש 1027-04-26 המדויק). Invariants: G1 (נרמול-במקור), G2 (אין מסלול-ניתוב מקביל), G12 (מגע-Paperclip דרך ה-Port). Co-Authored-By: Claude Opus 4.8 (1M context) --- web/agent_platform_port.py | 2 + web/app.py | 12 +++-- web/paperclip_client.py | 41 ++++++++++++++++- web/tests/test_comment_target.py | 78 ++++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 web/tests/test_comment_target.py diff --git a/web/agent_platform_port.py b/web/agent_platform_port.py index 71cf907..2aba28b 100644 --- a/web/agent_platform_port.py +++ b/web/agent_platform_port.py @@ -47,6 +47,7 @@ from web.paperclip_client import ( get_agents_for_case as pc_get_agents_for_case, get_agents_for_company as pc_get_agents, get_case_issues as pc_get_case_issues, + pick_default_comment_target as pc_pick_default_comment_target, get_issue_comments as pc_get_issue_comments, get_issue_interactions as pc_get_issue_interactions, get_project_url, @@ -158,6 +159,7 @@ __all__ = [ # issues / workflow "pc_create_workflow_issue", "pc_get_case_issues", + "pc_pick_default_comment_target", # agents / wakeups "pc_get_agents_for_case", "pc_get_agents", diff --git a/web/app.py b/web/app.py index bcfbfc4..f841e0c 100644 --- a/web/app.py +++ b/web/app.py @@ -69,6 +69,7 @@ from web.agent_platform_port import ( pc_get_agents, pc_get_agents_for_case, pc_get_case_issues, + pc_pick_default_comment_target, pc_get_issue_comments, pc_get_issue_interactions, pc_get_run_events, @@ -4609,7 +4610,8 @@ class AgentCommentRequest(BaseModel): async def api_post_agent_comment(case_number: str, req: AgentCommentRequest): """Post a comment on a Paperclip issue linked to a case. - If issue_id is omitted, the most recent non-done issue is used. + If ``issue_id`` is omitted, routing prefers the live CEO main issue and + never a closed (done/cancelled) issue — see ``_pick_default_comment_target``. """ issues = await pc_get_case_issues(case_number) if not issues: @@ -4620,14 +4622,14 @@ async def api_post_agent_comment(case_number: str, req: AgentCommentRequest): if not target: raise HTTPException(404, f"Issue {req.issue_id} לא שייך לתיק {case_number}") else: - # Pick the most recent non-done issue, or the last one - active = [i for i in issues if i["status"] != "done"] - target = active[-1] if active else issues[-1] + target = pc_pick_default_comment_target(issues) result = await pc_post_comment(target["id"], target["company_id"], req.body) - # Find the identifier for the response + # Echo the resolved target so the UI can show where it landed and flag it + # when the chair explicitly targeted a closed issue (whose wakeup is skipped). result["issue_identifier"] = target.get("identifier", "") + result["issue_status"] = target.get("status", "") return result diff --git a/web/paperclip_client.py b/web/paperclip_client.py index 7a4f5f3..29a7e8b 100644 --- a/web/paperclip_client.py +++ b/web/paperclip_client.py @@ -484,7 +484,8 @@ async def get_case_issues(case_number: str) -> list[dict]: """SELECT DISTINCT ON (i.id) i.id, i.title, i.status, i.identifier, i.priority, i.assignee_agent_id, a.name AS assignee_name, - i.started_at, i.completed_at, i.created_at, i.company_id + i.started_at, i.completed_at, i.created_at, i.company_id, + i.parent_id FROM issues i LEFT JOIN agents a ON i.assignee_agent_id = a.id LEFT JOIN plugin_state ps ON ps.scope_id = i.id::text @@ -510,6 +511,7 @@ async def get_case_issues(case_number: str) -> list[dict]: "completed_at": r["completed_at"].isoformat() if r["completed_at"] else None, "created_at": r["created_at"].isoformat() if r["created_at"] else None, "company_id": str(r["company_id"]), + "parent_id": str(r["parent_id"]) if r["parent_id"] else None, } for r in sorted_rows ] @@ -517,6 +519,43 @@ async def get_case_issues(case_number: str) -> list[dict]: await conn.close() +# A comment posted to a closed issue never wakes an agent — Paperclip skips the +# wakeup for done/cancelled issues, so the chair's instruction is silently +# swallowed. The default-target picker MUST therefore avoid these statuses. +CLOSED_ISSUE_STATUSES = frozenset({"done", "cancelled"}) + + +def pick_default_comment_target(issues: list[dict]) -> dict: + """Choose which issue a chair comment routes to when none is specified. + + ``issues`` is ordered oldest→newest (see :func:`get_case_issues`). Comment + routing is meant to reach the CEO, whose live "התחל תהליך ניסוח" issue is + top-level (``parent_id is None``) and open. Preference order: + 1. newest OPEN top-level issue — the live CEO main issue; + 2. newest OPEN issue of any depth — a live sub-issue if no main is open; + 3. newest top-level issue even if closed — better than a closed child; + 4. newest issue overall — last resort. + Excluding closed statuses is the fix for the recurring bug where the newest + non-done issue was a *cancelled* child, so the wakeup was skipped and the + instruction vanished. ``issues`` must be non-empty. + """ + def is_open(i: dict) -> bool: + return i["status"] not in CLOSED_ISSUE_STATUSES + + def is_top(i: dict) -> bool: + return i.get("parent_id") is None + + for pred in ( + lambda i: is_open(i) and is_top(i), + is_open, + is_top, + ): + matches = [i for i in issues if pred(i)] + if matches: + return matches[-1] + return issues[-1] + + async def get_issue_comments(issue_ids: list[str]) -> list[dict]: """Get all comments on a list of Paperclip issues, with agent metadata.""" if not issue_ids: diff --git a/web/tests/test_comment_target.py b/web/tests/test_comment_target.py new file mode 100644 index 0000000..f5efb20 --- /dev/null +++ b/web/tests/test_comment_target.py @@ -0,0 +1,78 @@ +"""Tests for pick_default_comment_target — chair-comment routing. + +Regression guard for the recurring bug where a chair instruction typed into the +"כתוב הוראה לסוכנים..." box was routed to a *cancelled* child issue, so +Paperclip skipped the wakeup and the instruction was silently swallowed. + +The picker must never default to a closed (done/cancelled) issue and must prefer +the live top-level CEO issue. Pure function — no DB, no FastAPI. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +import pytest + +os.environ.setdefault("PAPERCLIP_DB_URL", "postgres://x:x@127.0.0.1:54329/paperclip") +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) # web/ + +pc = pytest.importorskip("paperclip_client", reason="web deps unavailable") + + +def _iss(id_, status, parent_id=None): + return {"id": id_, "status": status, "parent_id": parent_id, "identifier": id_} + + +def test_prefers_open_top_level_over_newer_cancelled_child(): + """The exact 1027-04-26 scenario: newest non-done issue is a cancelled child; + routing must still land on the live top-level CEO issue.""" + issues = [ + _iss("main-old", "done"), # oldest top-level, closed + _iss("main-live", "in_review"), # live CEO main issue + _iss("child-cancelled", "cancelled", "main-live"), # newest non-done → the trap + _iss("child-done", "done", "main-live"), + ] + assert pc.pick_default_comment_target(issues)["id"] == "main-live" + + +def test_never_returns_cancelled_when_an_open_issue_exists(): + issues = [ + _iss("a", "done"), + _iss("b", "in_progress", "a"), + _iss("c", "cancelled", "a"), + ] + assert pc.pick_default_comment_target(issues)["status"] != "cancelled" + + +def test_falls_back_to_open_child_when_no_open_top_level(): + issues = [ + _iss("main", "done"), + _iss("child-live", "in_progress", "main"), + ] + assert pc.pick_default_comment_target(issues)["id"] == "child-live" + + +def test_prefers_top_level_when_everything_closed(): + """No open issue anywhere — pick the newest top-level (better than a child).""" + issues = [ + _iss("main-1", "done"), + _iss("main-2", "cancelled"), + _iss("child", "done", "main-2"), + ] + assert pc.pick_default_comment_target(issues)["id"] == "main-2" + + +def test_single_issue(): + issues = [_iss("only", "cancelled")] + assert pc.pick_default_comment_target(issues)["id"] == "only" + + +def test_picks_newest_open_top_level(): + issues = [ + _iss("main-1", "in_review"), + _iss("main-2", "in_review"), + ] + assert pc.pick_default_comment_target(issues)["id"] == "main-2"