Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions koan/app/github_command_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -1079,22 +1079,23 @@ def _try_assignment_notification(
log.error("GitHub assign: KOAN_ROOT not set")
return False

from app.missions import list_pending
from app.missions import list_pending, parse_sections
from app.utils import insert_pending_mission

missions_path = Path(koan_root) / "instance" / "missions.md"

# Deduplicate: skip if a mission for the same URL is already pending.
# This prevents duplicate missions when both an assignment notification
# and an @mention comment arrive for the same PR/issue.
# Deduplicate: skip if a mission for the same URL is already pending
# or in progress. The in-progress check prevents re-queuing while a
# review is still running (e.g., a rebase pushes new commits mid-review).
try:
content = missions_path.read_text() if missions_path.exists() else ""
pending = list_pending(content)
sections = parse_sections(content)
active = list_pending(content) + sections.get("in_progress", [])
url_lower = web_url.lower()
for line in pending:
for line in active:
if url_lower in line.lower():
log.debug(
"GitHub assign: mission for %s already pending, skipping",
"GitHub assign: mission for %s already active, skipping",
web_url,
)
mark_notification_read(notif_id)
Expand Down
45 changes: 45 additions & 0 deletions koan/app/github_notification_tracker.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@
"""

import json
import logging
import time
from pathlib import Path

log = logging.getLogger(__name__)


_TRACKER_FILE = ".koan-github-processed.json"
_TRACKER_FILE_THREADS = ".koan-github-processed-threads.json"
Expand Down Expand Up @@ -139,3 +142,45 @@ def _update(data):
locked_json_modify(_threads_path(instance_dir), _update)
except OSError:
pass # Best-effort — don't break notification processing


# ---------------------------------------------------------------------------
# Review cooldown — prevents re-review after bot's own rebase
# ---------------------------------------------------------------------------

_REVIEW_COOLDOWN_SECONDS = 30 * 60 # 30 minutes


def is_review_on_cooldown(instance_dir: str, owner: str, repo: str, pr_number: str) -> bool:
"""Check if a review for this PR was recently queued.

Returns True if a review was queued within the cooldown window.
Prevents the review_requested → review → rebase → new SHA → re-review
feedback loop.
"""
key = f"review_cd:{owner}/{repo}#{pr_number}"
data = _load_threads(instance_dir)
ts = data.get(key)
if ts is None:
return False
return time.time() - ts < _REVIEW_COOLDOWN_SECONDS


def set_review_cooldown(instance_dir: str, owner: str, repo: str, pr_number: str) -> None:
"""Record that a review was just queued for this PR.

# TODO: wire into github_command_handler or loop_manager alongside
# the self-reply detection for belt-and-suspenders re-review prevention.
"""
key = f"review_cd:{owner}/{repo}#{pr_number}"
try:
from app.locked_file import locked_json_modify

def _update(data):
_prune_expired(data)
data[key] = time.time()
_cap_entries(data)

locked_json_modify(_threads_path(instance_dir), _update)
except OSError:
log.warning("Failed to set review cooldown for %s/%s#%s", owner, repo, pr_number)
64 changes: 60 additions & 4 deletions koan/app/review_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
from app.review_schema import validate_review

_ISSUE_URL_RE = re.compile(ISSUE_URL_PATTERN)
_QUOTE_RE = re.compile(r'^>\s*@(\S+):\s*(.+)')


def load_project_learnings(project_name: Optional[str]) -> str:
Expand Down Expand Up @@ -131,6 +132,51 @@ def _filter_threads(
return filtered


def _exclude_replied_issue_comments(
human_comments: List[dict],
bot_comments: list,
) -> List[dict]:
"""Exclude issue comments the bot already replied to.

Issue comments are flat (no ``in_reply_to_id``), so ``_filter_threads``
cannot detect self-replies. Bot replies to issue comments use the
format ``> @user: first_line...``. Match that quote pattern against
human comments to detect prior replies.
"""
replied_quotes: list = []
for bc in bot_comments:
body = bc.get("body", "")
first_line = body.split("\n")[0]
m = _QUOTE_RE.match(first_line)
if not m:
continue
user = m.group(1).lower()
text = m.group(2).strip()
truncated = text.endswith("...")
if truncated:
text = text[:-3].rstrip()
if text:
replied_quotes.append((user, text.lower(), truncated))

if not replied_quotes:
return human_comments

filtered = []
for hc in human_comments:
user = hc.get("user", "").lower()
first_line = hc.get("body", "").split("\n")[0].strip().lower()
already_replied = any(
user == ru and (
first_line[:len(rp)] == rp if was_truncated
else first_line == rp
)
for ru, rp, was_truncated in replied_quotes
)
if not already_replied:
filtered.append(hc)
return filtered


def _fetch_inline_review_comments(
full_repo: str, pr_number: str, bot_username: str = "",
max_thread_depth: int = 0,
Expand Down Expand Up @@ -178,8 +224,14 @@ def _fetch_inline_review_comments(
def _fetch_issue_comments(
full_repo: str, pr_number: str, bot_username: str = "",
) -> List[dict]:
"""Fetch issue-level comments (conversation thread) for a PR."""
results: List[dict] = []
"""Fetch issue-level comments (conversation thread) for a PR.

Collects bot comments separately and uses them to detect prior replies.
Human comments that the bot already replied to (matching quote pattern)
are excluded from the returned list.
"""
human: List[dict] = []
bot_replies: list = []
try:
raw = run_gh(
"api", f"repos/{full_repo}/issues/{pr_number}/comments",
Expand All @@ -191,8 +243,9 @@ def _fetch_issue_comments(
try:
item = json.loads(line)
if _is_bot_user(item, bot_username):
bot_replies.append(item)
continue
results.append({
human.append({
"id": item["id"],
"type": "issue_comment",
"user": item["user"],
Expand All @@ -202,7 +255,10 @@ def _fetch_issue_comments(
continue
except RuntimeError:
pass
return results

if bot_replies and human:
return _exclude_replied_issue_comments(human, bot_replies)
return human


def fetch_repliable_comments(
Expand Down
35 changes: 35 additions & 0 deletions koan/tests/test_github_notification_tracker.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@

from app.github_notification_tracker import (
_MAX_ENTRIES,
_REVIEW_COOLDOWN_SECONDS,
_TTL_SECONDS,
_threads_path,
_tracker_path,
is_comment_tracked,
is_review_on_cooldown,
is_thread_tracked,
set_review_cooldown,
track_comment,
track_thread,
)
Expand Down Expand Up @@ -141,3 +144,35 @@ def test_thread_tracker_independent_from_comment_tracker(self, instance_dir):
track_thread(instance_dir, "thread-Y")
assert not is_comment_tracked(instance_dir, "thread-Y")
assert not is_thread_tracked(instance_dir, "comment-X")


# ---------------------------------------------------------------------------
# Review cooldown (prevents re-review after bot's own rebase)
# ---------------------------------------------------------------------------


class TestReviewCooldown:
def test_not_on_cooldown_initially(self, instance_dir):
assert not is_review_on_cooldown(instance_dir, "owner", "repo", "42")

def test_on_cooldown_after_set(self, instance_dir):
set_review_cooldown(instance_dir, "owner", "repo", "42")
assert is_review_on_cooldown(instance_dir, "owner", "repo", "42")

def test_different_pr_not_on_cooldown(self, instance_dir):
set_review_cooldown(instance_dir, "owner", "repo", "42")
assert not is_review_on_cooldown(instance_dir, "owner", "repo", "99")

def test_cooldown_expires(self, instance_dir):
"""Cooldown expires after the configured window."""
key = "review_cd:owner/repo#42"
expired_ts = time.time() - _REVIEW_COOLDOWN_SECONDS - 1
_threads_path(instance_dir).write_text(json.dumps({key: expired_ts}))
assert not is_review_on_cooldown(instance_dir, "owner", "repo", "42")

def test_cooldown_active_within_window(self, instance_dir):
"""Cooldown active within the configured window."""
key = "review_cd:owner/repo#42"
recent_ts = time.time() - 60 # 1 min ago
_threads_path(instance_dir).write_text(json.dumps({key: recent_ts}))
assert is_review_on_cooldown(instance_dir, "owner", "repo", "42")
84 changes: 84 additions & 0 deletions koan/tests/test_review_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -2324,6 +2324,90 @@ def test_no_bot_username_configured(self):
assert _is_bot_user({"user_type": "User", "user": "koan-bot"}, "") is False


class TestExcludeRepliedIssueComments:
"""Tests for _exclude_replied_issue_comments — flat issue comment dedup."""

def test_excludes_comment_already_replied_to(self):
from app.review_runner import _exclude_replied_issue_comments
human = [
{"id": 1, "user": "alice", "body": "Looks good overall"},
{"id": 2, "user": "bob", "body": "Why this approach?"},
]
bot = [
{"body": "> @alice: Looks good overall\n\nThanks!"},
]
result = _exclude_replied_issue_comments(human, bot)
assert len(result) == 1
assert result[0]["id"] == 2

def test_keeps_unreplied_comments(self):
from app.review_runner import _exclude_replied_issue_comments
human = [
{"id": 1, "user": "alice", "body": "New question"},
]
bot = [
{"body": "> @bob: Something else\n\nReply"},
]
result = _exclude_replied_issue_comments(human, bot)
assert len(result) == 1

def test_handles_truncated_quotes(self):
from app.review_runner import _exclude_replied_issue_comments
long_body = "A" * 200
human = [{"id": 1, "user": "alice", "body": long_body}]
bot = [{"body": f"> @alice: {long_body[:100]}...\n\nReply"}]
result = _exclude_replied_issue_comments(human, bot)
assert len(result) == 0

def test_exact_match_no_false_positive_on_prefix(self):
"""Non-truncated quotes require exact first-line match, not prefix."""
from app.review_runner import _exclude_replied_issue_comments
human = [
{"id": 1, "user": "alice", "body": "Some text is wrong"},
{"id": 2, "user": "alice", "body": "Some text is fine"},
]
bot = [{"body": "> @alice: Some text is fine\n\nAgreed"}]
result = _exclude_replied_issue_comments(human, bot)
assert len(result) == 1
assert result[0]["id"] == 1

def test_no_bot_replies_returns_all(self):
from app.review_runner import _exclude_replied_issue_comments
human = [{"id": 1, "user": "alice", "body": "Question"}]
result = _exclude_replied_issue_comments(human, [])
assert len(result) == 1

def test_summary_comments_not_matched(self):
"""Bot summary comments (<!-- koan-summary -->) don't match as replies."""
from app.review_runner import _exclude_replied_issue_comments
human = [{"id": 1, "user": "alice", "body": "Good work"}]
bot = [{"body": "<!-- koan-summary -->\n## PR Review\n\nLooks fine."}]
result = _exclude_replied_issue_comments(human, bot)
assert len(result) == 1

def test_case_insensitive_user_match(self):
from app.review_runner import _exclude_replied_issue_comments
human = [{"id": 1, "user": "Alice", "body": "Question here"}]
bot = [{"body": "> @alice: Question here\n\nAnswer"}]
result = _exclude_replied_issue_comments(human, bot)
assert len(result) == 0

@patch("app.review_runner.run_gh")
def test_fetch_issue_comments_excludes_replied(self, mock_gh):
"""_fetch_issue_comments uses _exclude_replied_issue_comments."""
from app.review_runner import _fetch_issue_comments

mock_gh.return_value = "\n".join([
json.dumps({"id": 10, "user": "alice", "body": "Nice work", "user_type": "User"}),
json.dumps({"id": 11, "user": "koan-bot", "body": "> @alice: Nice work\n\nThanks!", "user_type": "User"}),
json.dumps({"id": 12, "user": "bob", "body": "New question", "user_type": "User"}),
])

result = _fetch_issue_comments("owner/repo", "1", bot_username="koan-bot")
assert len(result) == 1
assert result[0]["user"] == "bob"


class TestFilterThreads:
"""Tests for _filter_threads — thread-aware self-reply prevention."""

Expand Down
Loading