diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index d12283eef34a..c0314c6c101e 100644 --- a/hermes_cli/kanban_db.py +++ b/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[^/\s]+)/(?P[^/\s]+)/pull/(?P\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///pull/`` 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 diff --git a/tests/hermes_cli/test_kanban_core_functionality.py b/tests/hermes_cli/test_kanban_core_functionality.py index 39e5d3f22d00..b51e63f3dc63 100644 --- a/tests/hermes_cli/test_kanban_core_functionality.py +++ b/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 # ---------------------------------------------------------------------------