🐛 fix(kanban): dedup review cards on PR URL in create_task

hermes_cli/kanban_db.py

@@ -103,6 +103,22 @@
 KNOWN_TOOLSET_NAMES = frozenset(name.casefold() for name in get_toolset_names())
 _IS_WINDOWS = sys.platform == "win32"
 
+# The skill that marks a card as a PR review card. The GitHub PR-creation
+# webhook stages a review card carrying this skill; hand-built chains must NOT
+# file one manually (the webhook owns review-card creation). See
+# ``_review_pr_url`` and the PR-URL dedup guard in ``create_task``.
+REVIEW_SKILL = "github-code-review"
+
+# Matches a GitHub pull-request URL and captures owner/repo/number so two
+# spellings of the same PR (trailing path, query string, or surrounding prose)
+# collapse to one canonical identity. The host is matched case-insensitively;
+# the path segments are kept verbatim (GitHub repo names are case-sensitive).
+_PR_URL_RE = re.compile(
+    r"https?://(?:www\.)?github\.com/"
+    r"(?P<owner>[^/\s]+)/(?P<repo>[^/\s]+)/pull/(?P<number>\d+)",
+    re.IGNORECASE,
+)
+
 # A running task's claim is valid for 15 minutes by default; after that the
 # next dispatcher tick reclaims it. Workers that outlive this window should
 # call ``heartbeat_claim(task_id)`` periodically. In practice most kanban
@@ -2059,6 +2075,44 @@ def _canonical_assignee(assignee: Optional[str]) -> Optional[str]:
     return normalize_profile_name(assignee)
 
 
+def _canonical_pr_url(text: Optional[str]) -> Optional[str]:
+    """Return the canonical ``github.com/<owner>/<repo>/pull/<n>`` URL in *text*.
+
+    Normalises host casing/``www.`` and strips any trailing path, query, or
+    fragment so ``.../pull/43``, ``.../pull/43/files`` and ``.../pull/43?w=1``
+    all collapse to one identity. Returns ``None`` when no PR URL is present.
+    """
+    if not text:
+        return None
+    m = _PR_URL_RE.search(text)
+    if not m:
+        return None
+    return (
+        f"https://github.com/{m.group('owner')}/{m.group('repo')}"
+        f"/pull/{m.group('number')}"
+    )
+
+
+def _review_pr_url(
+    skills_list: Optional[Iterable[str]],
+    title: Optional[str],
+    body: Optional[str],
+) -> Optional[str]:
+    """Canonical PR URL iff this card is a review card naming a PR, else None.
+
+    A review card is identified by the ``github-code-review`` skill. The PR URL
+    is read from the title first (the webhook puts it there), then the body.
+    Used to dedup duplicate review cards for the same PR regardless of how the
+    card was filed (webhook auto-card vs. a manually-filed card with a
+    different/absent idempotency key).
+    """
+    if not skills_list:
+        return None
+    if REVIEW_SKILL not in skills_list:
+        return None
+    return _canonical_pr_url(title) or _canonical_pr_url(body)
+
+
 def create_task(
     conn: sqlite3.Connection,
     *,
@@ -2185,6 +2239,35 @@ def create_task(
         if row:
             return row["id"]
 
+    # Review-card PR dedup — a stronger guard than the idempotency key for the
+    # one case it can't cover: a *manually* filed review card for a PR that the
+    # webhook also auto-files. The webhook keys its card on the PR URL, but a
+    # hand-built card carries a different (or no) key, so the key check above
+    # misses it and two review cards strand the pipeline (the phantom looks
+    # "stuck" and its bogus gate blocks the downstream merge card). Here we key
+    # on the PR identity itself: if a non-archived review card already exists
+    # for the same PR, no-op and return it. Archived cards don't block, so a
+    # deliberate re-review (archive the old card, file a new one) still works —
+    # mirroring the idempotency-key semantics above. Candidate set is just the
+    # review cards, so the in-Python canonicalisation stays cheap.
+    pr_url = _review_pr_url(skills_list, title, body)
+    if pr_url:
+        # The ``skills`` column stores a JSON array, so the review skill appears
+        # as a quoted token (e.g. ``["github-code-review"]``). Matching the
+        # quoted form keeps the prefilter from catching a skill that merely
+        # *contains* the name as a substring; the PR-URL compare below is the
+        # authoritative check.
+        rows = conn.execute(
+            "SELECT id, title, body FROM tasks "
+            "WHERE status != 'archived' AND skills LIKE ? "
+            "ORDER BY created_at DESC",
+            (f'%"{REVIEW_SKILL}"%',),
+        ).fetchall()
+        for r in rows:
+            existing = _canonical_pr_url(r["title"]) or _canonical_pr_url(r["body"])
+            if existing == pr_url:
+                return r["id"]
+
     now = int(time.time())
 
     # Resolve workspace_path from board-level default_workdir when the

tests/hermes_cli/test_kanban_core_functionality.py

@@ -89,6 +89,189 @@ def test_no_idempotency_key_never_collides(kanban_home):
         conn.close()
 
 
+# ---------------------------------------------------------------------------
+# Review-card PR dedup (webhook auto-card vs. manually-filed card)
+# ---------------------------------------------------------------------------
+
+PR_URL = "https://github.com/cwest/hermes-agent/pull/43"
+
+
+def test_review_card_dedups_on_pr_url_without_idempotency_key(kanban_home):
+    """A second review card for the same PR no-ops even without a shared key.
+
+    The webhook idempotency-keys its review card on the PR URL, but a
+    *manually* filed card for the same PR carries a different (or no) key,
+    so the key-based dedup misses it. The PR-URL guard catches it anyway.
+    """
+    conn = kb.connect()
+    try:
+        a = kb.create_task(
+            conn,
+            title=f"Review PR #43: {PR_URL}",
+            skills=["github-code-review"],
+            idempotency_key="webhook:" + PR_URL,
+        )
+        # Manual card: same PR, different key, different title wording.
+        b = kb.create_task(
+            conn,
+            title="please review pr 43",
+            body=f"Take a look at {PR_URL}",
+            skills=["github-code-review"],
+        )
+        assert a == b, "second review card for the same PR must dedup to the first"
+    finally:
+        conn.close()
+
+
+def test_review_card_dedup_pr_url_in_body_only(kanban_home):
+    conn = kb.connect()
+    try:
+        a = kb.create_task(
+            conn,
+            title="review",
+            body=f"PR: {PR_URL}",
+            skills=["github-code-review"],
+        )
+        b = kb.create_task(
+            conn,
+            title="review again",
+            body=f"the pr is {PR_URL} thanks",
+            skills=["github-code-review"],
+        )
+        assert a == b
+    finally:
+        conn.close()
+
+
+def test_review_card_dedup_ignored_for_archived(kanban_home):
+    """An archived prior review card does not block a fresh re-review."""
+    conn = kb.connect()
+    try:
+        a = kb.create_task(
+            conn,
+            title="review",
+            body=PR_URL,
+            skills=["github-code-review"],
+        )
+        kb.archive_task(conn, a)
+        b = kb.create_task(
+            conn,
+            title="re-review after changes",
+            body=PR_URL,
+            skills=["github-code-review"],
+        )
+        assert a != b
+    finally:
+        conn.close()
+
+
+def test_review_card_dedup_only_for_review_skill(kanban_home):
+    """A non-review card mentioning the same PR is never deduped."""
+    conn = kb.connect()
+    try:
+        review = kb.create_task(
+            conn,
+            title="review",
+            body=PR_URL,
+            skills=["github-code-review"],
+        )
+        # An implement/merge card can legitimately reference the PR URL.
+        merge = kb.create_task(
+            conn,
+            title="merge",
+            body=f"merge once green: {PR_URL}",
+        )
+        another_review = kb.create_task(
+            conn,
+            title="review 2",
+            body=PR_URL,
+            skills=["github-code-review"],
+        )
+        assert merge != review, "non-review card must not collide with a review card"
+        assert another_review == review, "second review card still dedups"
+    finally:
+        conn.close()
+
+
+def test_review_card_dedup_distinct_prs_do_not_collide(kanban_home):
+    conn = kb.connect()
+    try:
+        a = kb.create_task(
+            conn,
+            title="review 43",
+            body="https://github.com/cwest/hermes-agent/pull/43",
+            skills=["github-code-review"],
+        )
+        b = kb.create_task(
+            conn,
+            title="review 44",
+            body="https://github.com/cwest/hermes-agent/pull/44",
+            skills=["github-code-review"],
+        )
+        assert a != b, "review cards for different PRs must be independent"
+    finally:
+        conn.close()
+
+
+def test_review_card_without_pr_url_not_deduped(kanban_home):
+    """No PR URL → fall back to normal (no PR-based dedup)."""
+    conn = kb.connect()
+    try:
+        a = kb.create_task(
+            conn, title="review something", skills=["github-code-review"]
+        )
+        b = kb.create_task(
+            conn, title="review something else", skills=["github-code-review"]
+        )
+        assert a != b
+    finally:
+        conn.close()
+
+
+def test_review_card_dedup_canonicalises_url_variants(kanban_home):
+    """`.../pull/43`, `.../pull/43/files`, and `www.` host all collapse."""
+    conn = kb.connect()
+    try:
+        a = kb.create_task(
+            conn,
+            title="review",
+            body="https://github.com/cwest/hermes-agent/pull/43",
+            skills=["github-code-review"],
+        )
+        b = kb.create_task(
+            conn,
+            title="review files view",
+            body="https://www.github.com/cwest/hermes-agent/pull/43/files?w=1",
+            skills=["github-code-review"],
+        )
+        assert a == b
+    finally:
+        conn.close()
+
+
+def test_review_card_dedup_substring_skill_not_matched(kanban_home):
+    """A skill that merely contains the review-skill name doesn't trigger dedup."""
+    conn = kb.connect()
+    try:
+        a = kb.create_task(
+            conn,
+            title="not a real review",
+            body=PR_URL,
+            skills=["github-code-review-extra"],
+        )
+        b = kb.create_task(
+            conn,
+            title="real review",
+            body=PR_URL,
+            skills=["github-code-review"],
+        )
+        # The first card isn't a review card (different skill), so the real
+        # review card is created fresh rather than deduping to it.
+        assert a != b
+    finally:
+        conn.close()
+
+
 # ---------------------------------------------------------------------------
 # Spawn-failure circuit breaker
 # ---------------------------------------------------------------------------