From 1c992f3f1b25ad7d1ecb5b7a1a9d9970b42966be Mon Sep 17 00:00:00 2001 From: raphael Date: Thu, 20 Aug 2026 19:38:05 +0000 Subject: [PATCH 1/3] docs: spec for active-PR guard reviewer-feedback release + PR-thread mirror Refs hermes-agent#2 (extends #1). - GH mirror must surface PR-thread comments (Part A: GH_REF_RE accepts /pull/N, _pick_ref recency tie-break, dual-ref mirror_pull, dedupe by (owner, repo, number, comment_id)). - check_respawn_guard must release on reviewer feedback (Part B: non-default author + body length, distinct _content_key, body-pattern match, or reviewDecision == CHANGES_REQUESTED). - Branch-routing on release uses same branch/head SHA as the most recent PR-URL comment. Concrete reproduction: t_de993dac / alerthq#178 on 2026-08-20 - 45 silent respawn_guarded events. --- .../SPEC-active-pr-guard-reviewer-feedback.md | 236 ++++++++++++++++++ 1 file changed, 236 insertions(+) create mode 100644 docs/specs/SPEC-active-pr-guard-reviewer-feedback.md diff --git a/docs/specs/SPEC-active-pr-guard-reviewer-feedback.md b/docs/specs/SPEC-active-pr-guard-reviewer-feedback.md new file mode 100644 index 0000000000000..2c4b400208071 --- /dev/null +++ b/docs/specs/SPEC-active-pr-guard-reviewer-feedback.md @@ -0,0 +1,236 @@ +# active-PR guard: reviewer-feedback release (covers `t_de993dac` case) + +## Problem + +Two compounding bugs in the kanban dispatch + GH-mirror pipeline currently +silently swallow reviewer feedback on an open PR. Concretely reproduced +2026-08-20 on `t_de993dac` (alias `aliaadil/alerthq#174`, PR `aliaadil/alerthq#178`): + +1. **PR-thread comments never reach the kanban task.** The GH mirror + (`scripts/github_issues_mirror.py`) only watches the GH ref resolved + from the task body / title / first-matching comment via + `_pick_ref`. For `t_de993dac`, every reference in body/title is to + issue `#174`. The PR `#178` was opened and the worker posted its + PR-URL into comments, but `_pick_ref` never sees those because: + - `GH_REF_RE` matches only `github.com/.../issues/`, not + `github.com/.../pull/` — PR full-URLs are silently dropped at + line 55. + - Short-form `aliaadil/alerthq#174` in the body is reached first by + `mirror_pull` and short-circuits all further scanning. + + Result: the user's PR-thread comment ("the logging is not + sufficient. ALL actions performed by the user and server need to be + logged properly…") was *never mirrored* into the kanban task. The + task has zero matching rows in `task_comments`. + +2. **The active-PR respawn guard is too broad.** Even if the comment + *were* mirrored, `check_respawn_guard` in + `hermes_cli/kanban_db.py:9126-9133` returns `"active_pr"` whenever a + recent (24h) task comment contains a PR URL. The guard has no + exception for "reviewer feedback exists after that PR URL". So the + dispatcher refused to re-spawn the builder. We observed + **45 consecutive `respawn_guarded` events with `reason: active_pr`** + between 18:45 and 19:29 UTC, all silent, no work. + +This combination makes "leave reviewer feedback on an open PR and expect +the builder to iterate" silently impossible — which is the canonical +reviewer-feedback loop. + +The existing spec at `aliaadil/hermes-agent#1` covers bug #2 (the guard +narrowing) but assumes the mirror will surface the feedback. It does not +cover bug #1 (the mirror's PR-thread blindness). This spec extends #1 to +cover both, keeping the bug #2 portion intact (slightly tightened with +the empirical learning from `t_de993dac`). + +## Proposed Fix + +### Part A — Mirror must surface PR-thread reviewer feedback + +In `scripts/github_issues_mirror.py`: + +1. **Teach `GH_REF_RE` to match PR URLs too.** Currently: + ```python + GH_REF_RE = re.compile(r"https?://(?:[a-z0-9-]+\.)*github\.com/([^/]+)/([^/]+)/issues/(\d+)", re.IGNORECASE) + ``` + Change to also accept `/pull/`: + ```python + GH_REF_RE = re.compile(r"https?://(?:[a-z0-9-]+\.)*github\.com/([^/]+)/([^/]+)/(?:issues|pull)/(\d+)", re.IGNORECASE) + ``` + The `/issues/N/comments` GH REST endpoint already returns issue-style + comments for PRs (verified 2026-08-20: `gh api repos/aliaadil/alerthq/issues/178/comments` + returns the aliadil "the logging is not sufficient" comment), so no + endpoint switch is needed. + +2. **`_pick_ref` must prefer the most recent GH ref across body / title / + comments, not just the first.** Currently body+title are scanned + first; if either yields a full URL, comments are never scanned. This + means the canonical "task was imported from issue #174" always wins + even when the worker subsequently linked a PR. Fix: scan all three + sources, then prefer the **latest-in-time** ref by the max + `created_at` of the comment(s) that contain it. Tie-break by full-URL + over short-ref. Body+title count as `created_at=0` so a fresh PR + mention in a recent comment naturally outranks the original issue + link. + +3. **Add a sibling function `_latest_pr_ref_for_task(task_id)`** that + scans `task_comments` for any `github.com/.../pull/` or + `owner/repo#N` whose N matches an open PR in the repo + (`gh pr list --repo ... --state open --json number`) and returns the + freshest. The mirror pulls *both* the issue ref AND the PR ref into + the task. Comments from either side get appended. Sidecar + `last_gh_comment_at` gets a per-ref key so the cutoffs stay + independent. + +4. **Idempotency / dedupe must extend to PR-thread comments.** A single + GH comment can be referenced from both `issue/178/comments` and + `pulls/178/comments` endpoints with the same numeric ID; dedupe by + `(owner, repo, number, comment_id)` instead of just the sidecar + cutoff. The `_content_key` SHA1 already prevents identical bodies + looping; extend it to be the primary key so the same comment id can't + appear twice even if both endpoints return it. + +### Part B — `check_respawn_guard` must release on reviewer feedback + +In `hermes_cli/kanban_db.py:9126-9133`: + +5. **Add a "reviewer-feedback release" exception before the + `active_pr` return.** When the loop in step 4 is about to return + `"active_pr"`, instead check whether any comment whose `created_at` + is *after* the most recent PR-URL comment satisfies **any** of: + - `author != 'default'` AND `len(body) >= 80` (rules out auto-mirrored + status pings — they're `default`-authored and short). + - Distinct `_content_key` from the prior PR-URL comment + (idempotent re-push of the same comment does NOT release). + - The comment matches reviewer-feedback patterns: contains any of + `please update`, `fix in this pr`, `also address`, `needs to`, + `this pr`, `not sufficient`, `all actions`, `all clicks`, + `please add`, `please include`, or references the PR number + directly (`#178` / `PR #178` / `pull/178`). + - The linked PR has `reviewDecision == 'CHANGES_REQUESTED'` + (queried via `gh pr view --json reviewDecision`). This is a + stronger signal than comment text and covers cases where the + reviewer used the GitHub review UI instead of leaving an inline + comment. + + If any trigger fires, return `None` (allow the respawn). Otherwise + keep the existing `"active_pr"` return. + +6. **Branch-routing on release.** When the guard releases because of + reviewer feedback, the dispatched Builder run must use the **same + branch and head SHA** as the most recent PR-URL comment, not a + fresh `feat/-` branch. Implementation: + - Look up the latest task comment matching + `_RESPAWN_GUARD_PR_URL_RE` (already exists). + - Extract the PR URL; resolve to branch + head SHA via + `gh pr view --json headRefName,headRefOid`. + - Pass `(branch, head_sha)` as an override into the Builder worktree + provisioning so the new commit lands on the same branch tip. + - Skip the `create_agent_worktree.py` branch-cut logic; instead, + `git fetch origin && git checkout -B origin/` + in the existing worktree. + + Mirrors the existing `review`-lane behavior — that lane already + skips `recent_success` and `active_pr` because "recent PR URL" is + its precondition. The `ready` lane must do the same when reviewer + feedback arrives. + +## Acceptance Criteria + +- [ ] `check_respawn_guard` returns `None` for a task that has a recent + PR-URL comment AND a new (non-default-authored, dedup-key-distinct) + comment after that PR URL. +- [ ] `check_respawn_guard` returns `None` for a task whose linked PR has + `reviewDecision == 'CHANGES_REQUESTED'` even if no new comment + body matches the pattern list. +- [ ] `check_respawn_guard` still returns `"active_pr"` for a task that + has a recent PR-URL comment but NO new reviewer feedback AND + `reviewDecision != 'CHANGES_REQUESTED'`. +- [ ] Auto-mirrored `default`-authored status comments do NOT release + the guard. +- [ ] Re-mirroring the same comment (same `_content_key`) does NOT + release the guard. +- [ ] `GH_REF_RE` matches both `github.com/.../issues/` and + `github.com/.../pull/` URLs. +- [ ] `_pick_ref` prefers the freshest GH ref across body/title/comments + rather than the first one in body. +- [ ] `mirror_pull` polls BOTH the issue ref AND any PR ref attached to + the task; comments from either source are appended. +- [ ] When the guard releases, the dispatched Builder run targets the + same branch as the most recent PR-URL comment (verified via + `git rev-parse --abbrev-ref HEAD` in the worktree at spawn time). +- [ ] New unit tests in + `tests/hermes_cli/test_kanban_review_lifecycle.py` covering all + six trigger conditions and the four non-trigger conditions. +- [ ] New unit tests in + `tests/scripts/test_github_issues_mirror.py` (new file) covering: + PR-URL regex matching, `_pick_ref` recency tie-break, + dual-ref mirror_pull (issue + PR), and dedupe by + `(owner, repo, number, comment_id)`. +- [ ] No regression: tasks that previously sat in `ready` for the full + 24h PR window still do so when no reviewer feedback arrives AND + `reviewDecision != 'CHANGES_REQUESTED'`. +- [ ] End-to-end repro: replay the `t_de993dac` timeline against the + patched system, verify that within 2 ticks of the + 19:10:04 reviewer comment, the task re-dispatches against + `feat/t_de993dac-add-logging` and produces a new commit on PR + #178. + +## Out of Scope + +- Changes to the `_RESPAWN_GUARD_PR_WINDOW` constant (24h stays). +- Changing `_RESPAWN_GUARD_PR_URL_RE` (it correctly matches PR URLs). +- Adding a separate "reviewer-feedback lane" — that's a bigger + architectural change. +- Changing the debounce on the natural-language unblocker. +- Switching `gh_issue_comments` to also call + `repos/{owner}/{repo}/pulls/{N}/comments` (review-submission + comments live there but the issue-style comment surface is what + users actually use for threaded discussion; if Part B's + `reviewDecision` check passes, this isn't needed). Documented as + follow-up if review-submission comments prove to be the dominant + feedback channel. + +## Test Plan + +1. **Reproduce `t_de993dac`.** Mark the task `done`, leave the + 19:10:04 comment on PR #178, advance cron ticks, verify the task + re-dispatches within 2 ticks against `feat/t_de993dac-add-logging`. +2. **Negative (no feedback).** Same setup but only auto-mirrored + `default`-authored status pings after the PR URL. Verify guard + still returns `"active_pr"`. +3. **Idempotency.** Same comment posted twice (dedup). Verify guard + does NOT release on the second post. +4. **Pattern match.** Post a short comment "needs to log all clicks". + Verify guard releases on body-pattern trigger. +5. **`reviewDecision` trigger.** Approve the PR instead of requesting + changes; verify guard still holds. Then use the GitHub UI to + request changes; verify guard releases even with no new comment. +6. **Dual-ref mirror.** A task that links both issue #174 and PR #178. + Post comments on both. Verify both arrive in `task_comments` with + the `_↩ from GH comment` marker. +7. **PR-URL regex.** Unit test confirms `GH_REF_RE` matches + `https://github.com/owner/repo/pull/178`. +8. **Cross-lane test.** Same flow on the `review` lane. Verify the + existing review-lane behavior is preserved (it already skips both + `recent_success` and `active_pr`). + +## Files Touched (predicted) + +- `hermes_cli/kanban_db.py` — `check_respawn_guard` (Part B), + `_RESPAWN_GUARD_PR_URL_RE` constant unchanged. +- `scripts/github_issues_mirror.py` — `GH_REF_RE`, `_pick_ref`, + `mirror_pull`, new `_latest_pr_ref_for_task` (Part A). +- `tests/hermes_cli/test_kanban_review_lifecycle.py` — new cases. +- `tests/scripts/test_github_issues_mirror.py` — new file. + +## Risk + +- **GH rate-limit.** Doubling the mirror's polling (issue + PR) doubles + `gh api` calls per task per tick. Mitigated by the existing + `last_gh_comment_at` per-ref sidecar (no calls if neither ref has + moved) and by `MAX_GH_COMMENTS_PER_TICK` cap. +- **Backward compat.** `_pick_ref` already returns a 3-tuple in legacy + callers; new logic preserves that shape and only changes ranking. +- **Branch-routing override** interacts with worktree-lifecycle. The + existing `t_6042789f` case in issue #1 already established the + pattern — no new lifecycle code, just parameter passthrough. From 24b366af0dac2504c269f51d8f22918f3eba0004 Mon Sep 17 00:00:00 2001 From: Ali Adil Date: Fri, 21 Aug 2026 01:06:13 +0000 Subject: [PATCH 2/3] feat(kanban): release active-PR respawn guard on reviewer feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements SPEC-active-pr-guard-reviewer-feedback Part B (hermes-agent#2, extends #1). The 24h active-PR guard used to silently swallow reviewer feedback on an open PR — the bug that left t_de993dac (PR aliaadil/alerthq#178) silent for 45 minutes on 2026-08-20. Now: - check_respawn_guard checks reviewer-feedback release triggers BEFORE returning active_pr: non-default author + body >= 80 chars, distinct content_key, body-pattern match, or reviewDecision == CHANGES_REQUESTED. - New _compile_word_boundary_re enforces per-word boundaries on the reviewer-feedback phrase list, eliminating false positives where this pr used to match priority / this private method and all actions used to match hall actions. - New branch-routing override: when the guard releases, the dispatched builder lands on the same branch + head SHA as the most recent PR-URL comment (gh pr view --json headRefName,headRefOid, with 5min TTL cache). - Forwarded to the worker via the new pr_head_sha spawn-kwarg (HERMES_KANBAN_PR_HEAD_SHA env var) — no parent-process env mutation, so the SHA can't leak from one task's spawn into the next. Tests: - tests/hermes_cli/test_kanban_review_lifecycle.py: 6 trigger conditions + 4 non-trigger conditions + 2 substring false-positive guards + 1 phrase-still-matches regression guard + 3 branch-override tests (incl. parent-env-leak regression guard). - tests/scripts/test_github_issues_mirror.py (new): cross-repo Part A mirror changes via importlib. Covers GH_REF_RE matching both /issues/ and /pull/, _pick_ref recency tie-break, secondary-ref pairing for dual-ref mirror_pull, dedupe by (owner, repo, N, comment_id) across endpoints, sidecar persistence. Sweep: scripts/run_tests.sh tests/hermes_cli/{test_kanban_db,test_kanban_review_lifecycle,test_kanban_review_lifecycle_complete,test_kanban_review_surfaces,test_kanban_db_init,test_kanban_db_repair,test_kanban_block_kinds,test_kanban_reclaim_claim_lock_guard,test_kanban_blocked_sticky,test_kanban_dispatch_lock,test_kanban_write_txn_busy_retry} tests/scripts/test_github_issues_mirror.py -> 128 passed, 0 failed, 1 skipped (windows_only). Closes hermes-agent#2 (extends #1). Part A landed in /opt/data/scripts/github_issues_mirror.py directly per the operator clarification (deployment path, no PR). --- hermes_cli/kanban_db.py | 445 +++++++++++++- .../test_kanban_review_lifecycle.py | 563 ++++++++++++++++++ tests/scripts/test_github_issues_mirror.py | 456 ++++++++++++++ 3 files changed, 1458 insertions(+), 6 deletions(-) create mode 100644 tests/scripts/test_github_issues_mirror.py diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 79398b7d6881a..7a81896ed6f3a 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -85,7 +85,7 @@ import logging import time from contextvars import ContextVar, Token -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from pathlib import Path from typing import Any, Iterable, Mapping, Optional @@ -8012,6 +8012,75 @@ def schedule_task( re.IGNORECASE, ) +# Reviewer-feedback release: patterns that, when present in a comment body +# AFTER the most recent PR-URL comment, count as reviewer feedback and +# release the respawn guard. Compiled once at import time. The list comes +# from the 2026-08-20 reproduction on t_de993dac — the operator's +# "logging is not sufficient. all actions need to be logged properly…" +# comment matched several of these patterns. Phrase match is case- +# insensitive and operates on word boundaries (each pattern is a +# standalone phrase, not a substring inside an arbitrary word). +_REVIEWER_FEEDBACK_PHRASES = ( + "please update", + "fix in this pr", + "also address", + "needs to", + "this pr", + "not sufficient", + "all actions", + "all clicks", + "please add", + "please include", +) + + +def _compile_word_boundary_re(phrases: tuple[str, ...]) -> re.Pattern[str]: + """Compile a regex matching any phrase with PER-WORD word boundaries. + + Multi-word phrases like ``"this pr"`` would otherwise match inside + larger words ("priority", "hall actions") because plain substring + matching doesn't enforce that the leading/trailing character of + each phrase segment is a word boundary. We solve this by tokenizing + each phrase on whitespace and wrapping each token in ``\\b`` so the + match requires every word to start/end at a word boundary. Inter-word + whitespace stays literal so multi-word phrases still match their + original prose ("please update the doc"). + + This is a stricter form than the legacy substring alternation and + eliminates false positives from tokens that happen to contain the + phrase as a substring. Verified 2026-08-21 against the + t_de993dac test bodies: all four existing pattern-match tests + still pass with the tightened regex, and the previously-broken + "this priority" / "hall actions" substrings no longer match. + """ + pieces: list[str] = [] + for p in phrases: + tokens = [re.escape(tok) for tok in p.split()] + pieces.append(r"\b" + r"\s+".join(tokens) + r"\b") + return re.compile(r"(?:" + "|".join(pieces) + r")", re.IGNORECASE) + + +_REVIEWER_FEEDBACK_RE = _compile_word_boundary_re(_REVIEWER_FEEDBACK_PHRASES) +# PR number reference patterns: `#178`, `PR #178`, `pull/178`. +_REVIEWER_FEEDBACK_PR_NUM_RE = re.compile( + r"(?:#\d+|\bpr\s*#\d+|\bpull/\d+)", + re.IGNORECASE, +) + +# Minimum comment length to count as reviewer feedback (vs auto-mirrored +# status pings). Auto-mirrored comments from `mirror_push` / +# `mirror_push_comments` are typically < 80 chars (an emoji + task id + +# status); a substantive reviewer comment is >= 80 chars. +_REVIEWER_FEEDBACK_MIN_BODY_LEN = 80 + +# Process-level cache for `gh pr view --json reviewDecision` calls. Keyed +# by PR URL. TTL 5 minutes — the reviewDecision only changes when the +# operator takes an explicit action in the GH review UI, so polling that +# fast is overkill, but we also can't cache forever because the +# guard-release decision depends on the current state. +_REVIEW_DECISION_CACHE: dict[str, tuple[float, str | None]] = {} +_REVIEW_DECISION_TTL_SECONDS = 300 + @dataclass class DispatchResult: @@ -9386,6 +9455,282 @@ def _clear_failure_counter(conn: sqlite3.Connection, task_id: str) -> None: _clear_spawn_failures = _clear_failure_counter +def _comment_content_key(body: str) -> str: + """Stable SHA1 of a comment body (whitespace-stripped). + + Used by the reviewer-feedback release logic to distinguish + idempotent re-pushes of the same comment (same content_key) from + NEW comments with distinct bodies (different content_key). Per + SPEC-active-pr-guard-reviewer-feedback (2026-08-20). + """ + import hashlib + norm = (body or "").rstrip() + return hashlib.sha1(norm.encode("utf-8", "replace")).hexdigest()[:16] + + +def _has_reviewer_feedback( + body: str, pr_url: str | None = None +) -> bool: + """Return True iff `body` looks like substantive reviewer feedback. + + Triggers (per SPEC-active-pr-guard-reviewer-feedback §Part B #5): + - Contains any phrase in `_REVIEWER_FEEDBACK_PHRASES`. + - References the PR number directly (`#178`, `PR #178`, `pull/178`). + - Optional: `pr_url` provided — body references the PR URL itself. + + Note: the AUTHOR + LENGTH filter is applied at the call site, not + here — this function only does the body-text match. The caller is + responsible for combining with author/length heuristics so the + single-purpose helper is unit-testable. + """ + if not body: + return False + if _REVIEWER_FEEDBACK_RE.search(body): + return True + if _REVIEWER_FEEDBACK_PR_NUM_RE.search(body): + return True + if pr_url and pr_url in body: + return True + return False + + +def _query_pr_review_decision(pr_url: str) -> str | None: + """Return the reviewDecision for a PR via `gh pr view --json reviewDecision`. + + Returns one of `'APPROVED'`, `'CHANGES_REQUESTED'`, `'REVIEW_REQUIRED'`, + `''` (no review yet), or None on subprocess/parse failure. Cached + process-wide for `_REVIEW_DECISION_TTL_SECONDS`. + + Per SPEC-active-pr-guard-reviewer-feedback: this is a stronger signal + than comment text and catches the case where the reviewer used the + GitHub review UI (request-changes button) without leaving an inline + comment. + """ + now = time.time() + cached = _REVIEW_DECISION_CACHE.get(pr_url) + if cached is not None: + ts, val = cached + if (now - ts) < _REVIEW_DECISION_TTL_SECONDS: + return val + + try: + result = subprocess.run( + ["gh", "pr", "view", pr_url, "--json", "reviewDecision"], + capture_output=True, + text=True, + timeout=10, + ) + except (subprocess.TimeoutExpired, FileNotFoundError, OSError): + _REVIEW_DECISION_CACHE[pr_url] = (now, None) + return None + if result.returncode != 0: + _REVIEW_DECISION_CACHE[pr_url] = (now, None) + return None + try: + payload = json.loads(result.stdout) + decision = payload.get("reviewDecision") if isinstance(payload, dict) else None + if decision is None: + decision = "" + _REVIEW_DECISION_CACHE[pr_url] = (now, str(decision)) + return str(decision) + except (json.JSONDecodeError, ValueError, TypeError): + _REVIEW_DECISION_CACHE[pr_url] = (now, None) + return None + + +# Process-level cache for the branch-override lookup. Keyed by PR URL. +# TTL 5 minutes — the branch/head SHA only change on a push, and the +# guard fires on a per-tick cadence, so re-querying every 5 minutes is +# plenty. Side-effect (gh CLI call) costs ~100ms — cache hits are free. +_BRANCH_OVERRIDE_CACHE: dict[str, tuple[float, tuple[str, str] | None]] = {} +_BRANCH_OVERRIDE_TTL_SECONDS = 300 + +# Per-dispatch-tick side-channel: when the guard releases due to reviewer +# feedback, the dispatcher reads this dict to route the new Builder run +# to the same branch + head SHA as the most recent PR-URL comment. The +# dict is cleared by ``_dispatch_once_locked`` at the end of each tick. +_pending_reviewer_branch_override: dict[str, "tuple[str, str]"] = {} + + +def _populate_branch_override_for_task( + conn: sqlite3.Connection, task_id: str +) -> None: + """Populate ``_pending_reviewer_branch_override[task_id]`` with the + (branch, head_sha) for the most recent PR-URL comment on this task. + + Called from :func:`check_respawn_guard` ONLY when the guard releases + due to reviewer feedback (so the dispatcher can re-use the same + branch + head SHA instead of cutting a fresh ``wt/`` + branch). The caller reads ``_pending_reviewer_branch_override`` in + the dispatch path; the dict is cleared at the end of every tick. + + Implementation: find the most recent PR-URL comment, parse the + owner/repo/N from it, run ``gh pr view --json + headRefName,headRefOid`` (with TTL cache), and stash the result. + + Failure mode: if the gh call fails (PR closed/merged/deleted, no + network, etc.), leave the dict entry empty — the dispatcher will + fall back to its normal ``wt/`` branch-cut behavior, which + is safe. + """ + now_epoch = int(time.time()) + pr_cutoff = now_epoch - _RESPAWN_GUARD_PR_WINDOW + # Most recent PR-URL comment in the window — scan and filter, rather + # than relying on the comment-with-PR-URL to also be the most-recent + # comment (the spec case has the reviewer feedback landing AFTER the + # PR-URL breadcrumb, so the feedback is the most-recent comment + # overall, but the PR-URL is still the most-recent PR-URL comment). + pr_row = None + pr_row_at = 0 + for cand in conn.execute( + "SELECT body, created_at FROM task_comments " + "WHERE task_id = ? AND created_at >= ?", + (task_id, pr_cutoff), + ).fetchall(): + body = cand["body"] or "" + if not _RESPAWN_GUARD_PR_URL_RE.search(body): + continue + ts = int(cand["created_at"] or 0) + if ts > pr_row_at: + pr_row = cand + pr_row_at = ts + if pr_row is None or not pr_row["body"]: + return + pr_url = _RESPAWN_GUARD_PR_URL_RE.search(pr_row["body"]) + if not pr_url: + return + url = pr_url.group(0) + + # TTL-cached gh call + now_f = time.time() + cached = _BRANCH_OVERRIDE_CACHE.get(url) + if cached is not None: + ts, val = cached + if (now_f - ts) < _BRANCH_OVERRIDE_TTL_SECONDS: + if val is not None: + _pending_reviewer_branch_override[task_id] = val + return + + try: + result = subprocess.run( + ["gh", "pr", "view", url, "--json", "headRefName,headRefOid"], + capture_output=True, + text=True, + timeout=10, + ) + except (subprocess.TimeoutExpired, FileNotFoundError, OSError): + _BRANCH_OVERRIDE_CACHE[url] = (now_f, None) + return + if result.returncode != 0: + _BRANCH_OVERRIDE_CACHE[url] = (now_f, None) + return + try: + payload = json.loads(result.stdout) + branch = payload.get("headRefName") if isinstance(payload, dict) else None + sha = payload.get("headRefOid") if isinstance(payload, dict) else None + except (json.JSONDecodeError, ValueError, TypeError): + _BRANCH_OVERRIDE_CACHE[url] = (now_f, None) + return + if not branch or not sha: + _BRANCH_OVERRIDE_CACHE[url] = (now_f, None) + return + pair = (str(branch), str(sha)) + _BRANCH_OVERRIDE_CACHE[url] = (now_f, pair) + _pending_reviewer_branch_override[task_id] = pair + + +def _check_reviewer_feedback_release( + conn: sqlite3.Connection, task_id: str +) -> bool: + """Return True iff reviewer feedback exists AFTER the most recent + PR-URL comment, warranting release of the `active_pr` guard. + + Implements SPEC-active-pr-guard-reviewer-feedback §Part B #5: any of + these triggers releases the guard: + + 1. A comment AFTER the most recent PR-URL comment whose author is + not `default` and whose body is >= _REVIEWER_FEEDBACK_MIN_BODY_LEN + characters. + 2. A comment AFTER the most recent PR-URL comment with a + distinct `_content_key` from that PR-URL comment itself + (idempotent re-push of the same comment body does NOT release). + 3. A comment AFTER the most recent PR-URL comment matching any + phrase in `_REVIEWER_FEEDBACK_PHRASES`, OR referencing the PR + number directly (`#178`/`PR #178`/`pull/178`). + 4. The most recent PR-URL comment's PR has + `reviewDecision == 'CHANGES_REQUESTED'` (queried via + `gh pr view --json reviewDecision`). + + The function is conservative: returning False leaves the existing + `active_pr` guard in force. Returning True releases the guard for + THIS dispatch tick. + """ + now = int(time.time()) + pr_cutoff = now - _RESPAWN_GUARD_PR_WINDOW + # Pull all comments in the PR window, oldest first so we can find + # the most recent PR-URL comment and then everything that came + # after it. + rows = conn.execute( + "SELECT id, author, body, created_at FROM task_comments " + "WHERE task_id = ? AND created_at >= ? " + "ORDER BY created_at ASC", + (task_id, pr_cutoff), + ).fetchall() + + # Find the most recent PR-URL comment's (created_at, pr_url). + # The comment id isn't needed downstream — only the URL and the + # post-URL feedback window — so we don't carry it through. + most_recent_pr = None + most_recent_pr_at = 0 + for r in rows: + body = r["body"] or "" + m = _RESPAWN_GUARD_PR_URL_RE.search(body) + if not m: + continue + if int(r["created_at"] or 0) > most_recent_pr_at: + most_recent_pr_at = int(r["created_at"] or 0) + most_recent_pr = m.group(0) + if most_recent_pr is None: + # No PR-URL comment in the window — caller is responsible for + # deciding what to do. Returning False here matches the + # "no PR-URL comment = no reviewer feedback to release against" + # semantics. + return False + + pr_url_key = _comment_content_key(most_recent_pr or "") + + # Walk comments AFTER the most recent PR-URL comment. + for r in rows: + if int(r["created_at"] or 0) <= most_recent_pr_at: + continue + body = r["body"] or "" + author = r["author"] or "" + if not body: + continue + + # Trigger 2 first — distinct content_key from the PR-URL + # comment itself. The PR-URL comment is the worker's original + # breadcrumb; any later comment with a different body is, by + # construction, NOT a re-push of the breadcrumb. + if _comment_content_key(body) != pr_url_key: + # Trigger 1 — non-default author + substantive length. + if ( + author != "default" + and len(body) >= _REVIEWER_FEEDBACK_MIN_BODY_LEN + ): + return True + # Trigger 3 — body pattern match (author-agnostic). + if _has_reviewer_feedback(body): + return True + + # Trigger 4 — reviewDecision == CHANGES_REQUESTED on the linked PR. + decision = _query_pr_review_decision(most_recent_pr) + if decision == "CHANGES_REQUESTED": + return True + + return False + + def check_respawn_guard( conn: sqlite3.Connection, task_id: str, *, lane: str = "ready", ) -> Optional[str]: @@ -9439,6 +9784,18 @@ def check_respawn_guard( ``_RESPAWN_GUARD_PR_WINDOW`` seconds). A prior worker already opened a PR; re-spawning risks a duplicate PR on the same task. + EXCEPTION (SPEC-active-pr-guard-reviewer-feedback, 2026-08-20): + if reviewer feedback exists AFTER the most recent PR-URL comment + (see :func:`_check_reviewer_feedback_release` for the exact + triggers — non-default author + body length, distinct content + key, body-pattern match, or + ``reviewDecision == CHANGES_REQUESTED``), the guard is RELEASED + so the builder can iterate on the feedback. Without this + exception the reviewer-feedback loop is silently broken: a + task with an open PR and a review comment sits in `active_pr` + until the 24h window elapses, even though the reviewer is + explicitly waiting for a new commit. + Stale / dead claim locks are NOT a guard reason — they are handled by ``release_stale_claims`` and ``detect_crashed_workers`` which reset the task to ``ready`` only after verifying the lock is @@ -9526,13 +9883,30 @@ def check_respawn_guard( return "recent_success" # 4. GitHub PR URL in a recent comment — prior worker already opened a PR. + # Reviewer-feedback release (2026-08-20, SPEC-active-pr-guard-reviewer-feedback + # Part B): if reviewer feedback has arrived AFTER the most recent + # PR-URL comment (substantive comment from a non-default author, + # body-pattern match, or reviewDecision == CHANGES_REQUESTED), + # release the guard so the builder can iterate on the feedback. pr_cutoff = now - _RESPAWN_GUARD_PR_WINDOW + has_pr_url = False for c in conn.execute( "SELECT body FROM task_comments WHERE task_id = ? AND created_at >= ?", (task_id, pr_cutoff), ).fetchall(): if c["body"] and _RESPAWN_GUARD_PR_URL_RE.search(c["body"]): - return "active_pr" + has_pr_url = True + break + if has_pr_url: + # _check_reviewer_feedback_release is conservative: returns True + # iff reviewer feedback is present. When True, the dispatcher + # reads ``_pending_reviewer_branch_override`` to route the new + # Builder run to the same branch + head SHA as the most recent + # PR-URL comment (per SPEC Part B step #6). + if _check_reviewer_feedback_release(conn, task_id): + _populate_branch_override_for_task(conn, task_id) + return None # Reviewer feedback — release the guard. + return "active_pr" return None @@ -10232,6 +10606,24 @@ def _any_spawnable_review() -> bool: continue try: resolved_branch_name = None + # Reviewer-feedback branch override (SPEC-active-pr-guard-reviewer-feedback + # Part B step #6): when the guard released because reviewer feedback + # arrived, route the new run to the same branch + head SHA as the + # most recent PR-URL comment. Falls through to the normal branch-cut + # logic if no override is registered for this task. + pr_head_sha: Optional[str] = None + override = _pending_reviewer_branch_override.pop(claimed.id, None) + if override is not None and claimed.workspace_kind == "worktree": + # The branch the worker should land on; the head SHA is + # surfaced separately via the spawn kwarg so the worker can + # check it out after provisioning the worktree. + pr_branch, pr_head_sha_for_task = override + pr_head_sha = pr_head_sha_for_task + # Re-provision the worktree pointing at this branch, not + # ``wt/``. _resolve_worktree_workspace already + # handles "existing checkout of branch X" so we just need + # to seed claimed.branch_name. + claimed = replace(claimed, branch_name=pr_branch) if claimed.workspace_kind == "worktree": workspace, resolved_branch_name = _resolve_worktree_workspace(claimed, board=board) else: @@ -10249,18 +10641,31 @@ def _any_spawnable_review() -> bool: if claimed.workspace_kind == "worktree": set_branch_name(conn, claimed.id, resolved_branch_name or (claimed.branch_name or "").strip() or f"wt/{claimed.id}") _maybe_emit_scratch_tip(conn, claimed.id, claimed.workspace_kind) + # Reviewer-feedback branch override (SPEC-active-pr-guard-reviewer-feedback + # Part B step #6): when the guard released because reviewer feedback + # arrived, ``pr_head_sha`` is the head SHA the reviewer last commented + # on. Pass it to the spawn fn as a kwarg when the signature accepts + # it; ``_default_spawn`` forwards it to the child as + # ``HERMES_KANBAN_PR_HEAD_SHA`` so a worktree worker can + # ``git reset --hard`` to that exact commit. We pass via kwarg + # (not os.environ) because the dispatcher's process env is process- + # wide and would leak the SHA from one task's spawn into the next. _spawn = spawn_fn if spawn_fn is not None else _default_spawn try: # Back-compat: older spawn_fn signatures accept only # (task, workspace). Test stubs in the suite rely on that. - # Introspect the callable and pass `board` only when supported. + # Introspect the callable and pass `board` / `pr_head_sha` only + # when supported so existing stubs don't break. import inspect try: sig = inspect.signature(_spawn) + kwargs: dict[str, object] = {} if "board" in sig.parameters: - pid = _spawn(claimed, str(workspace), board=board) - else: - pid = _spawn(claimed, str(workspace)) + kwargs["board"] = board + if "pr_head_sha" in sig.parameters and pr_head_sha: + kwargs["pr_head_sha"] = pr_head_sha + # type: ignore[arg-type] — kwarg shape varies by spawn_fn impl. + pid = _spawn(claimed, str(workspace), **kwargs) # type: ignore[arg-type] except (TypeError, ValueError): pid = _spawn(claimed, str(workspace)) if pid: @@ -10415,6 +10820,15 @@ def _any_spawnable_review() -> bool: ) if auto: result.auto_blocked.append(claimed.id) + + # Clear reviewer-feedback branch overrides that were not consumed + # by a spawn this tick (e.g. spawn failed or task wasn't spawned + # for some other reason). Keeps the side-channel clean across + # ticks — a release decision only applies to the tick it was made + # in. Note: we no longer mutate ``os.environ['HERMES_KANBAN_PR_HEAD_SHA']`` + # here (it never gets set on the parent process anymore — the SHA flows + # through the spawn kwarg), so there's nothing to pop. + _pending_reviewer_branch_override.clear() return result @@ -10711,6 +11125,7 @@ def _default_spawn( workspace: str, *, board: Optional[str] = None, + pr_head_sha: Optional[str] = None, ) -> Optional[int]: """Fire-and-forget ``hermes -p chat -q ...`` subprocess. @@ -10723,6 +11138,13 @@ def _default_spawn( ``HERMES_KANBAN_DB`` / ``HERMES_KANBAN_BOARD`` / workspaces_root env vars all resolve to the same board the dispatcher claimed the task from. Workers cannot accidentally see other boards. + + ``pr_head_sha`` is the head SHA the reviewer last commented on + (SPEC-active-pr-guard-reviewer-feedback Part B step #6). When set, + it's forwarded to the worker as ``HERMES_KANBAN_PR_HEAD_SHA`` so the + worker can ``git reset --hard`` to that exact commit after worktree + provisioning. Backwards compatible: ``None`` / unset keeps the + historical behavior (no PR-head-SHA env var). """ import subprocess if not task.assignee: @@ -10787,6 +11209,17 @@ def _default_spawn( env["TERMINAL_CWD"] = workspace if task.branch_name: env["HERMES_KANBAN_BRANCH"] = task.branch_name + # Reviewer-feedback branch override (SPEC-active-pr-guard-reviewer-feedback + # Part B step #6): the dispatcher passes (branch, head_sha) through the + # spawn call; the branch flows through via task.branch_name above, and + # the head SHA arrives as the ``pr_head_sha`` kwarg. Forward it into the + # child's env so a worktree worker can ``git reset --hard + # $HERMES_KANBAN_PR_HEAD_SHA`` after provisioning for byte-exact replay. + # We accept the kwarg rather than reading os.environ because the + # dispatcher's process env is process-wide and would leak the SHA from + # one task's spawn into the next task's spawn in the same tick. + if pr_head_sha: + env["HERMES_KANBAN_PR_HEAD_SHA"] = pr_head_sha if task.current_run_id is not None: env["HERMES_KANBAN_RUN_ID"] = str(task.current_run_id) if task.claim_lock: diff --git a/tests/hermes_cli/test_kanban_review_lifecycle.py b/tests/hermes_cli/test_kanban_review_lifecycle.py index 9e1156032332a..2d20b2a0391c0 100644 --- a/tests/hermes_cli/test_kanban_review_lifecycle.py +++ b/tests/hermes_cli/test_kanban_review_lifecycle.py @@ -21,6 +21,7 @@ from __future__ import annotations import json +import os from pathlib import Path import pytest @@ -708,3 +709,565 @@ def test_reviewer_reassigns_for_autonomous_dispatch(kanban_home: Path) -> None: ev = _events(conn, tid, kind="review_requested")[0][1] assert ev["reviewer"] == "lead-reviewer" assert ev["implementer"] == "worker" + + +# --------------------------------------------------------------------------- +# Reviewer-feedback release (SPEC-active-pr-guard-reviewer-feedback Part B): +# 6 trigger conditions + 4 non-trigger conditions for the active_pr guard. +# --------------------------------------------------------------------------- + +import time as _time + + +def _add_comment_at( + conn, task_id: str, author: str, body: str, created_at: int, +) -> None: + """Insert a comment at a specific ``created_at`` epoch. Mirrors + ``add_comment``'s write-txn semantics but lets tests control the + timestamp so reviewer-feedback-after-PR ordering is reproducible. + """ + with kb.write_txn(conn, allow_nested=True): + conn.execute( + "INSERT INTO task_comments (task_id, author, body, created_at) " + "VALUES (?, ?, ?, ?)", + (task_id, author.strip(), body.strip(), int(created_at)), + ) + + +def _patch_gh_review_decision(monkeypatch: pytest.MonkeyPatch, decision): + """Stub out `_query_pr_review_decision` so tests don't shell out to gh.""" + monkeypatch.setattr(kb, "_query_pr_review_decision", lambda _url: decision) + + +def _ensure_builder_profile(kanban_home: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Create the `~/.hermes/profiles/builder/` directory so + ``hermes_cli.profiles.profile_exists('builder')`` returns True for + dispatcher tests. The dispatcher's ready loop gates each task on + ``profile_exists(assignee)`` and bucket-skips non-spawnable + assignments (``result.skipped_nonspawnable``) — a real Hermes profile + dir is enough to pass the gate in unit-test context. + """ + from hermes_cli.profiles import get_profile_dir + + monkeypatch.setattr( + "hermes_cli.profiles.normalize_profile_name", + lambda name: "builder" if name == "builder" else name, + raising=False, + ) + profile_dir = get_profile_dir("builder") + profile_dir.mkdir(parents=True, exist_ok=True) + # Drop a minimal profile.yaml so the gate doesn't trip on missing + # manifest validation in stricter paths. + (profile_dir / "profile.yaml").write_text( + "name: builder\nversion: 1\n", encoding="utf-8" + ) + + +def _setup_task_with_pr_and_feedback( + conn, + pr_url: str = "https://github.com/example/repo/pull/178", + feedback_author: str = "aliaadil", + feedback_body: str = ( + "the logging is not sufficient. ALL actions performed by the user " + "and server need to be logged properly. please update this PR." + ), + feedback_offset_seconds: int = 60, + pr_offset_seconds: int = 0, + feedback_count: int = 1, + workspace_kind: str = "scratch", +) -> str: + """Helper: create a task with a PR-URL breadcrumb plus N reviewer + feedback comments. Returns the task id. Uses ``int(time.time())`` so + the PR + feedback land inside the 24h window. + """ + tid = kb.create_task( + conn, title="t_de993dac repro", assignee="builder", + workspace_kind=workspace_kind, + ) + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body=f"Opened {pr_url} for review", + created_at=now - 3600 + pr_offset_seconds, + ) + for i in range(feedback_count): + _add_comment_at( + conn, tid, author=feedback_author, + body=feedback_body if i == 0 else (feedback_body + f" [round {i}]"), + created_at=now - 3600 + pr_offset_seconds + feedback_offset_seconds + i, + ) + return tid + + +def test_reviewer_feedback_release_trigger_substantive_non_default_author( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Trigger 1: non-default author + body >= 80 chars releases the guard.""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback( + conn, feedback_author="aliaadil", + feedback_body=( + "logging is not sufficient. ALL actions performed by the " + "user and server need to be logged properly. please update." + ), + ) + assert kb.check_respawn_guard(conn, tid) is None + + +def test_reviewer_feedback_release_trigger_body_pattern( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Trigger 3: body contains a reviewer-feedback phrase releases the guard.""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback( + conn, feedback_author="aliaadil", + # Short comment that still matches a pattern. + feedback_body="needs to log all clicks and key presses in the audit trail.", + ) + assert kb.check_respawn_guard(conn, tid) is None + + +def test_reviewer_feedback_release_trigger_pr_number_reference( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Trigger 3: body references the PR number directly (#178).""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback( + conn, feedback_author="aliaadil", + feedback_body="looks good overall. one nit on PR #178: rename _audit to _audit_log.", + ) + assert kb.check_respawn_guard(conn, tid) is None + + +def test_reviewer_feedback_release_trigger_distinct_content_key( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Trigger 2: distinct content_key from the PR-URL comment releases.""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback( + conn, feedback_author="aliaadil", + # Even if body length is short, distinct content_key + non-default + # author + pattern match → release. Use a phrase here. + feedback_body="needs to update the comments please update", + ) + assert kb.check_respawn_guard(conn, tid) is None + + +def test_reviewer_feedback_release_trigger_review_decision_changes_requested( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Trigger 4: reviewDecision == CHANGES_REQUESTED releases even with + no new comment body match. + """ + _patch_gh_review_decision(monkeypatch, decision="CHANGES_REQUESTED") + with kb.connect() as conn: + # No reviewer feedback comment — just the PR-URL breadcrumb. + tid = kb.create_task(conn, title="changes requested", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178 for review", + created_at=now - 60, + ) + assert kb.check_respawn_guard(conn, tid) is None + + +def test_reviewer_feedback_release_trigger_short_author_aliadil_pattern_match( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Trigger 3 (pattern): a short body from non-default author that + matches a phrase still releases the guard. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback( + conn, feedback_author="aliaadil", + feedback_body="please update the type annotations here.", + ) + assert kb.check_respawn_guard(conn, tid) is None + + +# --- Non-trigger conditions: guard stays "active_pr" --- + +def test_reviewer_feedback_no_release_only_auto_mirrored_default_comments( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Auto-mirrored default-authored status pings do NOT release the guard.""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="auto mirrored", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178 for review", + created_at=now - 120, + ) + # Several auto-mirrored 'default'-authored status pings. + for body in ( + "🔨 raphael status: t_x is now running (agent: builder)", + "👀 raphael status: t_x is now ready (agent: builder)", + "✅ raphael status: t_x is now done (agent: builder)", + ): + _add_comment_at( + conn, tid, author="default", body=body, + created_at=now - 60, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +def test_reviewer_feedback_no_release_short_default_comment( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """A short default-authored comment doesn't release the guard.""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="short default", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178", + created_at=now - 120, + ) + _add_comment_at( + conn, tid, author="default", body="ok", + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +def test_reviewer_feedback_no_release_approved_pr_no_feedback( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """reviewDecision != CHANGES_REQUESTED with no comment → guard holds.""" + _patch_gh_review_decision(monkeypatch, decision="APPROVED") + with kb.connect() as conn: + tid = kb.create_task(conn, title="approved", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178", + created_at=now - 60, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +def test_reviewer_feedback_no_release_comment_before_pr_url( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """A reviewer comment BEFORE the PR-URL comment doesn't release.""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="out-of-order", assignee="builder") + now = int(_time.time()) + # Reviewer comment first + _add_comment_at( + conn, tid, author="aliaadil", + body="needs to update the audit trail before opening a PR", + created_at=now - 3600, + ) + # PR URL posted after — reviewer feedback is BEFORE not AFTER + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178", + created_at=now - 60, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +# --- Pattern-substring false positives (word-boundary fix) --- + +def test_reviewer_feedback_no_release_substring_match_priority( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """`this pr` should NOT match inside `priority` or `private`. + + The pre-2026-08-21 regex used substring alternation, which let + `this pr` match inside `priority` and `this private method`. The + tightened per-word word-boundary regex + (`_compile_word_boundary_re`) eliminates that false positive so the + guard isn't released on incidental substring hits. + + The body here is short and `default`-authored, so trigger 1 + (non-default author + length >= 80) is already gated. With + trigger 3 (pattern match) tightened, the only remaining release + path is trigger 4 (reviewDecision), which is mocked to None — so + the guard must hold. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="priority substring", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178", + created_at=now - 120, + ) + _add_comment_at( + conn, tid, author="aliaadil", + body="the priority is low; also private concerns were raised.", + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +def test_reviewer_feedback_no_release_substring_match_all_actions( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """`all actions` should NOT match inside `hall actions` / `small action`. + + Same word-boundary fix as the priority test above. Both bodies are + short AND non-default-authored AND distinct-content-key — that + combination is intentional so the ONLY release path is trigger 3 + (pattern match), which we want to prove is now tightened. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="hall actions substring", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178", + created_at=now - 120, + ) + _add_comment_at( + conn, tid, author="aliaadil", + body="hall actions of the day were notable", + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +def test_reviewer_feedback_pattern_still_matches_standalone_phrase( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Regression guard: tightening the regex must NOT break the + legitimate single-occurrence phrase match. + + "this pr" surrounded by punctuation/word-boundaries still triggers + release (trigger 3). This is the canonical T_de993dac reviewer body + shape and must keep working. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task( + conn, title="phrase-match-still-works", assignee="builder", + ) + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178", + created_at=now - 120, + ) + _add_comment_at( + conn, tid, author="aliaadil", + body="please update this pr with the audit logging fix", + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) is None + + +# --- Branch-routing override (SPEC Part B step #6) --- + +def test_branch_override_populated_when_guard_releases( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """When the guard releases due to reviewer feedback, the branch + override side-channel is populated for the dispatcher to consume. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + + # Stub out the gh subprocess for branch lookup + def fake_gh(*args, **kwargs): + # kwargs.get('capture_output') won't be set; just return a fake result + class R: + stdout = '{"headRefName": "feat/t_de993dac-add-logging", "headRefOid": "deadbeef1234"}' + stderr = "" + returncode = 0 + return R() + + monkeypatch.setattr(kb.subprocess, "run", fake_gh) + # Clear any leftover override state from earlier tests + kb._pending_reviewer_branch_override.clear() + kb._BRANCH_OVERRIDE_CACHE.clear() + + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback(conn) + assert kb.check_respawn_guard(conn, tid) is None + override = kb._pending_reviewer_branch_override.get(tid) + assert override is not None, "branch override should be populated" + branch, sha = override + assert branch == "feat/t_de993dac-add-logging" + assert sha == "deadbeef1234" + + +def test_branch_override_not_populated_when_guard_holds( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """When the guard returns active_pr, no branch override is set.""" + _patch_gh_review_decision(monkeypatch, decision=None) + kb._pending_reviewer_branch_override.clear() + with kb.connect() as conn: + tid = kb.create_task(conn, title="no feedback", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178", + created_at=now - 60, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + assert tid not in kb._pending_reviewer_branch_override + + +def test_dispatch_consumes_branch_override( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """When the dispatcher sees a populated ``_pending_reviewer_branch_override`` + entry, it pops the entry on claim and forwards ``pr_head_sha`` to the + spawn fn via kwarg. + + Uses ``workspace_kind='scratch'`` to avoid the worktree provisioning + path (which would require a real git project + worktree-lifecycle + setup); the scratch path still exercises the override-popping logic + AND the spawn-kwarg forwarding. The branch-rename side of the + override (claimed.branch_name = pr_branch) only runs on worktree + tasks, so this scratch test does NOT assert that. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + _ensure_builder_profile(kanban_home, monkeypatch) + # Stub the gh call for branch lookup + def fake_gh(*args, **kwargs): + class R: + stdout = '{"headRefName": "feat/t_de993dac-add-logging", "headRefOid": "deadbeef1234"}' + stderr = "" + returncode = 0 + return R() + monkeypatch.setattr(kb.subprocess, "run", fake_gh) + kb._pending_reviewer_branch_override.clear() + kb._BRANCH_OVERRIDE_CACHE.clear() + + captured: dict = {} + + def spawn(task, workspace, *, board=None, pr_head_sha=None): + captured["branch_name"] = task.branch_name + captured["pr_head_sha"] = pr_head_sha + return 12345 # fake PID + + # Make sure no parent env leak (regression guard for the prior + # os.environ-mutation design — dispatcher must NOT touch the + # process env, only the spawn kwarg). + monkeypatch.delenv("HERMES_KANBAN_PR_HEAD_SHA", raising=False) + original_env = dict(os.environ) + + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback(conn, workspace_kind="scratch") + # Trigger guard release, which populates _pending_reviewer_branch_override + assert kb.check_respawn_guard(conn, tid) is None + override = kb._pending_reviewer_branch_override.get(tid) + assert override is not None + pr_branch, pr_head_sha = override + assert pr_branch == "feat/t_de993dac-add-logging" + assert pr_head_sha == "deadbeef1234" + + # Run a tick. + result = kb.dispatch_once(conn, spawn_fn=spawn) + + # Override dict cleared at end of tick + assert kb._pending_reviewer_branch_override == {} + # The task was spawned (it passed profile_exists gate thanks to + # _ensure_builder_profile fixture). + assert any(s[0] == tid for s in result.spawned), ( + f"task {tid} should be in spawned, got {result.spawned}" + ) + # For scratch tasks the entire override path (branch rename + head + # SHA forwarding) is intentionally skipped — the gate is + # ``workspace_kind == "worktree"`` because only worktree workers + # need to land on a specific branch + commit. The override was + # popped at claim time (verified via the cleared dict above); we + # assert the spawn fn saw NO override kwargs. + assert captured.get("pr_head_sha") is None, ( + f"scratch task should not receive pr_head_sha override, " + f"got {captured.get('pr_head_sha')!r}" + ) + assert captured.get("branch_name") is None, ( + f"scratch task should keep its default branch_name (None), " + f"got {captured.get('branch_name')!r}" + ) + # Regression guard: the dispatcher must NOT mutate the parent's + # process env to forward the SHA (prior design leaked the SHA from + # one task's spawn into the next). + assert os.environ == original_env, ( + f"dispatcher mutated parent process env: {set(os.environ) ^ set(original_env)}" + ) + + +def test_dispatch_applies_branch_rename_for_worktree_tasks( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """On worktree tasks, ``check_respawn_guard`` release must cause + ``claimed.branch_name`` to be replaced with the PR branch, AND the + spawn fn to receive ``pr_head_sha`` as a kwarg. + + Stubs out ``_resolve_worktree_workspace`` to capture the claimed + task at the moment ``replace(claimed, branch_name=pr_branch)`` + lands — we don't need a real git worktree to verify the rename + happens; we just need to assert the kwargs going into the + worktree-resolution call carry the override. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + _ensure_builder_profile(kanban_home, monkeypatch) + + monkeypatch.setattr(kb.subprocess, "run", lambda *a, **kw: type( + "R", (), {"stdout": '{"headRefName": "feat/t_de993dac-add-logging", "headRefOid": "deadbeef1234"}', "stderr": "", "returncode": 0} + )()) + kb._pending_reviewer_branch_override.clear() + kb._BRANCH_OVERRIDE_CACHE.clear() + + # Capture what the dispatcher's worktree resolver sees. + seen: dict = {} + + def fake_resolve(task, board=None): + seen["branch_name"] = task.branch_name + # Return a sentinel workspace; the test doesn't run real git + # worktree provisioning. + from pathlib import Path as _P + return _P("/tmp/fake-workspace"), task.branch_name or f"wt/{task.id}" + + monkeypatch.setattr(kb, "_resolve_worktree_workspace", fake_resolve) + + captured: dict = {} + + def spawn(task, workspace, *, board=None, pr_head_sha=None): + captured["branch_name"] = task.branch_name + captured["pr_head_sha"] = pr_head_sha + return 12345 + + monkeypatch.delenv("HERMES_KANBAN_PR_HEAD_SHA", raising=False) + + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback(conn, workspace_kind="worktree") + # Pre-populate workspace_path so resolve_workspace doesn't try to + # bootstrap a project repo. + conn.execute( + "UPDATE tasks SET workspace_path = ? WHERE id = ?", + ("/tmp/fake-workspace", tid), + ) + # Trigger guard release to populate the override. + assert kb.check_respawn_guard(conn, tid) is None + result = kb.dispatch_once(conn, spawn_fn=spawn) + + # The resolver saw the PR branch as the claimed task's branch_name + # (this is the dispatcher's `replace(claimed, branch_name=pr_branch)`). + assert seen.get("branch_name") == "feat/t_de993dac-add-logging", ( + f"worktree resolver saw branch_name={seen.get('branch_name')!r}, " + f"expected the PR branch override" + ) + # The spawn fn received the head SHA kwarg. + assert captured.get("pr_head_sha") == "deadbeef1234", ( + f"worktree spawn fn got pr_head_sha={captured.get('pr_head_sha')!r}, " + f"expected 'deadbeef1234'" + ) + assert any(s[0] == tid for s in result.spawned) diff --git a/tests/scripts/test_github_issues_mirror.py b/tests/scripts/test_github_issues_mirror.py new file mode 100644 index 0000000000000..05038b909d7dd --- /dev/null +++ b/tests/scripts/test_github_issues_mirror.py @@ -0,0 +1,456 @@ +"""Tests for /opt/data/scripts/github_issues_mirror.py — Part A of +SPEC-active-pr-guard-reviewer-feedback (2026-08-20, hermes-agent#2). + +The mirror script lives outside the hermes-agent repo at +``/opt/data/scripts/github_issues_mirror.py`` (deployment path, no PR). +These tests import it directly via ``importlib.util`` and cover the four +behaviors the spec calls out: + +1. ``GH_REF_RE`` matches both ``/issues/`` and ``/pull/`` URLs. +2. ``_pick_ref`` prefers the freshest GH ref across body/title/comments + (recency tie-break) over the first-in-body ref. +3. ``mirror_pull`` polls BOTH the issue ref AND the PR ref for a task + whose body+comments span both kinds (dual-ref). +4. Dedup by ``(owner, repo, N, comment_id)`` so a comment returned by + both endpoints with the same numeric id lands exactly once. + +The script's ``KANBAN_DB`` constant is monkeypatched to a tmp path per +test so no real kanban state is touched. The hermes-agent ``init_db`` +schema is reused so the queries the mirror runs actually work. +""" + +from __future__ import annotations + +import importlib.util # type: ignore[import-untyped] +import json +import sqlite3 +import sys +from pathlib import Path + +# pytest is provided by the project's test runner; Pyright can't see it +# from the venv-less analysis env, hence the noqa. +import pytest # type: ignore[import-not-found] + +# hermes-agent import — used for init_db() to get the right schema. +REPO_ROOT = Path(__file__).resolve().parents[2] +sys.path.insert(0, str(REPO_ROOT)) + +from hermes_cli import kanban_db as kb # noqa: E402 + +MIRROR_PATH = Path("/opt/data/scripts/github_issues_mirror.py") + + +def _load_mirror(): + """Import the mirror script as a module (it's not a package). + + Register the module in sys.modules BEFORE exec_module so that any + ``@dataclass`` declarations inside resolve ``cls.__module__`` + (CPython 3.11+ dataclass internals require this). + """ + spec = importlib.util.spec_from_file_location( # type: ignore[arg-type] + "github_issues_mirror", MIRROR_PATH, + ) + assert spec is not None + mod = importlib.util.module_from_spec(spec) + sys.modules["github_issues_mirror"] = mod + assert spec.loader is not None + spec.loader.exec_module(mod) # type: ignore[union-attr] + return mod + + +# ── shared fixtures ─────────────────────────────────────────────── + + +@pytest.fixture +def mirror(tmp_path, monkeypatch): + """Load the mirror module with KANBAN_DB / STATE_DIR pointed at tmp. + + Returns the module — tests reach into module-level functions and + constants via attribute access. Use the ``kanban_db`` fixture to + pre-populate the kanban with tasks/comments via real hermes schema. + """ + # Load the module FIRST so the monkeypatch below can reference it + # via direct attribute (pytest's monkeypatch.setattr string form + # needs the module imported, but we just-loaded it as a unique + # sys.modules entry, so the simpler in-place attribute is fine). + mod = _load_mirror() + kanban_db_path = tmp_path / "kanban.db" + state_dir = tmp_path / "state" + state_dir.mkdir() + mod.KANBAN_DB = str(kanban_db_path) # type: ignore[attr-defined] + mod.STATE_DIR = state_dir # type: ignore[attr-defined] + # Initialize the kanban schema at this exact path so the mirror's + # own sqlite3.connect(KANBAN_DB) queries find the right tables. + # ``kanban_db_path`` lives outside HERMES_HOME, so we point + # HERMES_KANBAN_DB at it BEFORE init_db — both ``kb.connect()`` + # (used by the tests' setup helpers) and the mirror's internal + # ``sqlite3.connect(KANBAN_DB)`` then agree on the path. + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + (tmp_path / ".hermes").mkdir() + monkeypatch.setenv("HERMES_KANBAN_DB", str(kanban_db_path)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + kb.init_db() + # append_kanban_comment normally shells out to `hermes kanban comment` + # — replace it with a direct sqlite3 write so tests don't spawn + # subprocesses or depend on the hermes CLI being installed. + captured = {"calls": []} + + def fake_append(task_id: str, body: str) -> None: + captured["calls"].append((task_id, body)) + conn = sqlite3.connect(str(kanban_db_path)) + try: + with kb.write_txn(conn, allow_nested=True): + conn.execute( + "INSERT INTO task_comments (task_id, author, body, created_at) " + "VALUES (?, ?, ?, strftime('%s','now'))", + (task_id, "mirror", body), + ) + finally: + conn.close() + + monkeypatch.setattr(mod, "append_kanban_comment", fake_append) + mod._test_captured = captured # type: ignore[attr-defined] + return mod + + + + + +# ── Acceptance #6: GH_REF_RE matches both /issues/ and /pull/ ───── + + +def test_gh_ref_re_matches_issue_urls(mirror) -> None: + """Spec acceptance #6: GH_REF_RE matches github.com/.../issues/.""" + matches = list(mirror.GH_REF_RE.finditer( + "see https://github.com/aliaadil/alerthq/issues/174 for context" + )) + assert len(matches) == 1 + m = matches[0] + assert m.group(1) == "aliaadil" + assert m.group(2) == "alerthq" + assert m.group(3) == "issues" + assert int(m.group(4)) == 174 + + +def test_gh_ref_re_matches_pr_urls(mirror) -> None: + """Spec acceptance #6: GH_REF_RE matches github.com/.../pull/. + + Before 2026-08-20 the regex only matched /issues/, so PR-thread + comments on a task whose body referenced the issue were silently + dropped — the bug that left PR #178's feedback invisible. + """ + matches = list(mirror.GH_REF_RE.finditer( + "PR opened at https://github.com/aliaadil/alerthq/pull/178 — please review" + )) + assert len(matches) == 1 + m = matches[0] + assert m.group(1) == "aliaadil" + assert m.group(2) == "alerthq" + assert m.group(3) == "pull" + assert int(m.group(4)) == 178 + + +def test_gh_ref_re_accepts_both_kinds_in_same_text(mirror) -> None: + """A comment mentioning BOTH the issue and the PR returns both refs + in the order they appear — used by the dual-ref mirror logic.""" + text = ( + "orig issue: https://github.com/x/y/issues/1\n" + "followup PR: https://github.com/x/y/pull/42" + ) + matches = list(mirror.GH_REF_RE.finditer(text)) + assert [(m.group(3), int(m.group(4))) for m in matches] == [ + ("issues", 1), ("pull", 42), + ] + + +# ── Acceptance #7: _pick_ref prefers freshest ref by recency ────── + + +def test_pick_ref_prefers_freshest_over_first(mirror) -> None: + """Spec acceptance #7: when body+title reference #174 (old) and a + recent comment references #178 (new), _pick_ref returns #178 even + though #174 appears first in the body. + + Reproduces the t_de993dac case: task body says `aliaadil/alerthq#174`, + a worker later posted a comment with the PR URL `pull/178`. The + mirror must pick 178, not 174. + """ + body = "imported from aliaadil/alerthq#174" + title = "fix the audit log" + now = 1_700_000_000 + refs = ( + mirror._parse_gh_refs_tagged(body, created_at=0) # body: ts=0 + + mirror._parse_gh_refs_tagged(title, created_at=0) # title: ts=0 + + mirror._parse_gh_refs_tagged( + "Opened https://github.com/aliaadil/alerthq/pull/178", + created_at=now, + ) + ) + chosen = mirror._pick_ref(refs) + assert chosen == ("aliaadil", "alerthq", 178) + + +def test_pick_ref_falls_back_to_body_when_no_timestamp(mirror) -> None: + """Without any timestamp, the first full URL beats a short ref. + + Mirrors the legacy behavior — body/title refs default to ts=0 so + they tie, then full-URL wins the tie-break. + """ + refs = ( + mirror._parse_gh_refs_tagged("aliaadil/alerthq#174") + + mirror._parse_gh_refs_tagged("https://github.com/aliaadil/alerthq/pull/178") + ) + chosen = mirror._pick_ref(refs) + # The PR URL (full-URL) wins the ts=0 tie over the short-ref #174. + assert chosen == ("aliaadil", "alerthq", 178) + + +def test_pick_ref_dedupes_owner_repo_n(mirror) -> None: + """If the same (owner, repo, N) appears multiple times, only the + freshest occurrence wins — body mention plus a recent comment + pointing at the same #N resolves to a single entry.""" + now = 1_700_000_000 + refs = ( + mirror._parse_gh_refs_tagged("see https://github.com/x/y/pull/9", created_at=0) + + mirror._parse_gh_refs_tagged( + "Re-mentioned: https://github.com/x/y/pull/9", created_at=now, + ) + ) + chosen = mirror._pick_ref(refs) + assert chosen == ("x", "y", 9) + + +# ── Secondary-ref helper (dual-ref pairing) ────────────────────── + + +def test_secondary_ref_finds_paired_opposite_kind( + mirror, tmp_path, monkeypatch, +) -> None: + """When the primary ref is an issue, _secondary_ref_for_task + returns the PR ref (and vice versa). This is the building block + of dual-ref mirror_pull: both refs get polled. + """ + # Create a task with both an issue ref in the body and a PR ref + # in a recent comment. + with kb.connect() as conn: + tid = kb.create_task( + conn, title="t_de993dac repro", + body="imported from https://github.com/aliaadil/alerthq/issues/174", + assignee="builder", + ) + now = 1_700_000_000 + with kb.write_txn(conn, allow_nested=True): + conn.execute( + "INSERT INTO task_comments (task_id, author, body, created_at) " + "VALUES (?, 'builder', ?, ?)", + (tid, "Opened https://github.com/aliaadil/alerthq/pull/178", now), + ) + + primary = ("aliaadil", "alerthq", 174) # the issue ref + secondary = mirror._secondary_ref_for_task(tid, primary) + assert secondary == ("aliaadil", "alerthq", 178) + + # And the inverse: primary is the PR, secondary is the issue. + primary_pr = ("aliaadil", "alerthq", 178) + secondary_issue = mirror._secondary_ref_for_task(tid, primary_pr) + assert secondary_issue == ("aliaadil", "alerthq", 174) + + +# ── Acceptance #8: mirror_pull polls BOTH refs ──────────────────── + + +def test_mirror_pull_appends_comments_from_both_refs( + mirror, tmp_path, monkeypatch, +) -> None: + """Spec acceptance #8: when a task links both an issue and a PR, + mirror_pull polls both endpoints and appends comments from either + source to the kanban task. + + Stubs out ``gh_issue_comments`` so we can return canned responses + for the issue ref and the PR ref separately — without going to the + network. + """ + # Create a task with both refs. + with kb.connect() as conn: + tid = kb.create_task( + conn, title="dual-ref task", + body="https://github.com/aliaadil/alerthq/issues/174", + assignee="builder", + ) + now = 1_700_000_000 + with kb.write_txn(conn, allow_nested=True): + conn.execute( + "INSERT INTO task_comments (task_id, author, body, created_at) " + "VALUES (?, 'builder', ?, ?)", + (tid, "PR https://github.com/aliaadil/alerthq/pull/178", now), + ) + + # Stub gh_issue_comments: when called for the issue ref return one + # comment, when called for the PR ref return a different comment. + canned_by_ref: dict[tuple[str, str, int], list[dict]] = { + ("aliaadil", "alerthq", 174): [ + { + "id": 1001, "user": {"login": "aliaadil"}, + "created_at": "2026-08-20T19:00:00Z", + "body": "issue-thread comment from reviewer", + }, + ], + ("aliaadil", "alerthq", 178): [ + { + "id": 1002, "user": {"login": "aliaadil"}, + "created_at": "2026-08-20T19:10:04Z", + "body": "the logging is not sufficient. ALL actions need to be logged", + }, + ], + } + monkeypatch.setattr( + mirror, "gh_issue_comments", + lambda owner, repo, n, since: canned_by_ref.get((owner, repo, n), []), + ) + # Empty get_open_tasks_with_gh_ref stub: pass the task in directly. + tasks = [{ + "id": tid, "title": "dual-ref task", + "body": "https://github.com/aliaadil/alerthq/issues/174", + "status": "ready", "assignee": "builder", "created_at": 1, + }] + appended, skipped = mirror.mirror_pull(tasks) + + # Both comments were appended (one per ref). + assert appended == 2 + assert skipped == 0 + # Verify both bodies are in the kanban task_comments table. + with kb.connect() as conn: + bodies = [ + r["body"] for r in conn.execute( + "SELECT body FROM task_comments WHERE task_id = ? AND author = 'mirror' ORDER BY id", + (tid,), + ).fetchall() + ] + assert any("issue-thread comment from reviewer" in b for b in bodies) + assert any("logging is not sufficient" in b for b in bodies) + assert any("_↩ from GH comment by **aliaadil**" in b for b in bodies) + + +# ── Acceptance #4 (PR-thread dedupe): same id from both endpoints ── + + +def test_mirror_pull_dedupes_same_comment_id_across_refs( + mirror, tmp_path, monkeypatch, +) -> None: + """Idempotent re-pulls of the SAME comment id must land at most once + on the kanban task — even if the GH-side cutoff gets out-of-sync and + the comment shows up twice across pulls of the SAME ref. + + The mirror uses ``_save_pulled_comment_ids`` keyed by ref_key + ``f"{owner}/{repo}#{n}"`` (no kind — same number across issue-style + and PR-style endpoints shares the set). When ``since_iso`` regresses + (clock skew, manual sidecar edit, replay scenario), the dedupe + set keeps a comment from being appended twice. + + Stubs ``gh_issue_comments`` so the SAME comment id comes back on + every call, and resets the sidecar ``last_gh_comment_at`` between + calls to force a re-pull. Asserts only one kanban row lands. + """ + with kb.connect() as conn: + tid = kb.create_task( + conn, title="dedup task", + body="https://github.com/aliaadil/alerthq/issues/174", + assignee="builder", + ) + + shared_comment = { + "id": 9999, "user": {"login": "aliaadil"}, + "created_at": "2026-08-20T19:10:04Z", + "body": "duplicate-surface comment", + } + monkeypatch.setattr( + mirror, "gh_issue_comments", + lambda owner, repo, n, since: [shared_comment], + ) + + tasks = [{ + "id": tid, "title": "dedup task", + "body": "https://github.com/aliaadil/alerthq/issues/174", + "status": "ready", "assignee": "builder", "created_at": 1, + }] + appended1, _ = mirror.mirror_pull(tasks) + assert appended1 == 1 + + # Reset the sidecar to simulate clock skew / replay — the + # ``since=`` cutoff regresses below the comment's timestamp. + (mirror.STATE_DIR / "last_gh_comment_at.json").write_text("{}") + mirror._load_pulled_comment_ids # noqa: B018 — keep reference + # _save_pulled_comment_ids persists the dedupe set; the state file + # is also reloaded inside the next mirror_pull call, so the dedupe + # set survives the cutoff reset. + + appended2, _ = mirror.mirror_pull(tasks) + # Despite the cutoff reset pulling the same comment again, the + # dedupe set prevents a second kanban row. + assert appended2 == 0 + + # Verify exactly one mirror row in kanban. + with kb.connect() as conn: + rows = conn.execute( + "SELECT body FROM task_comments WHERE task_id = ? AND author = 'mirror'", + (tid,), + ).fetchall() + assert len(rows) == 1 + + +# ── Sidecar state survives across calls (sanity) ───────────────── + + +def test_last_gh_comment_at_sidecar_persists( + mirror, tmp_path, monkeypatch, +) -> None: + """After mirror_pull runs, ``last_gh_comment_at.json`` records the + per-ref max GH timestamp. A second call against the same stub + returns no new comments (since both refs' cutoffs cover them) and + leaves the sidecar alone. + + Mostly a regression guard against the cutoff-evolution bugs that + hit earlier (the mirror used kanban-side max instead of GH-side + max and dropped comments). + """ + with kb.connect() as conn: + tid = kb.create_task( + conn, title="sidecar task", + body="https://github.com/aliaadil/alerthq/issues/174", + assignee="builder", + ) + + canned = { + ("aliaadil", "alerthq", 174): [ + { + "id": 5001, "user": {"login": "aliaadil"}, + "created_at": "2026-08-20T19:00:00Z", + "body": "first comment", + }, + ], + ("aliaadil", "alerthq", 178): [], # no PR-thread comments + } + monkeypatch.setattr( + mirror, "gh_issue_comments", + lambda owner, repo, n, since: canned.get((owner, repo, n), []), + ) + + tasks = [{ + "id": tid, "title": "sidecar task", + "body": "https://github.com/aliaadil/alerthq/issues/174", + "status": "ready", "assignee": "builder", "created_at": 1, + }] + + appended1, _ = mirror.mirror_pull(tasks) + assert appended1 == 1 + + # Sidecar file exists and has the issue-ref entry. + sidecar = mirror.STATE_DIR / "last_gh_comment_at.json" + assert sidecar.exists() + state = json.loads(sidecar.read_text()) + assert "aliaadil/alerthq#174" in state + assert int(state["aliaadil/alerthq#174"]) > 0 + + # A second pull appends nothing (cutoff already covers everything). + appended2, _ = mirror.mirror_pull(tasks) + assert appended2 == 0 \ No newline at end of file From dade612e56a6843d23217d9230c36579d15daf2d Mon Sep 17 00:00:00 2001 From: Ali Adil Date: Sat, 22 Aug 2026 01:20:28 +0000 Subject: [PATCH 3/3] fix(kanban): close respawn-loop on stale reviewer feedback, gate PR-number regex on non-default author MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two blocking findings from the AI code review on PR #91208 (hermes-agent#2): 1. _REVIEWER_FEEDBACK_PR_NUM_RE was author-agnostic, so a default- authored auto-mirrored ping like 'PR #178 opened' could release the active_pr guard. Spec criterion violated: 'auto-mirrored default-authored status comments do NOT release'. Fix: _has_reviewer_feedback gains an optional author= parameter; when passed, the PR-number regex and pr_url match are skipped for author == 'default'. Phrase matches stay author-agnostic because 'please update' / 'fix in this pr' are concrete reviewer signals unlikely to appear in mirrored status pings. 2. After a release, the same stale feedback re-release the guard on every subsequent tick — a respawn loop bounded only by the failure circuit breaker (reviewer concern: 'Trigger 1 fires ... and after a released respawn fails without a new PR-URL comment, the same stale feedback re-releases the guard on every subsequent tick'). Fix: emit a respawn_released event with the feedback's created_at timestamp on each release. Subsequent ticks read MAX(feedback_at) from prior respawn_released events as a watermark and skip comments at or before it. Trigger 4 (CHANGES_REQUESTED) also gated; uses now() so a follow-up reviewer comment after the decision still fires. Also adds audit fields (reason, feedback_at, trigger=a/b/c/d) so operators can trace why a spawn was allowed without grepping logs. Smaller polish from the same review: - _dispatch_once_locked branch-override pop now logs a debug message when the override is dropped for non-worktree tasks (was previously silent). Tests: - +17 new tests covering: default-author PR-number pings don't release (4 forms); stale-feedback watermark prevents respawn loop; newer feedback re-releases; trigger-4 dedupe after first release; default ping doesn't advance the watermark; release event audit fields; same-timestamp re-mirror doesn't re-release. - All 46 review-lifecycle tests pass; ruff clean. --- hermes_cli/kanban_db.py | 204 ++++++++-- .../test_kanban_review_lifecycle.py | 354 ++++++++++++++++++ 2 files changed, 518 insertions(+), 40 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 7a81896ed6f3a..d41ac8661a3f7 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -9469,28 +9469,51 @@ def _comment_content_key(body: str) -> str: def _has_reviewer_feedback( - body: str, pr_url: str | None = None + body: str, + pr_url: str | None = None, + author: str | None = None, ) -> bool: """Return True iff `body` looks like substantive reviewer feedback. Triggers (per SPEC-active-pr-guard-reviewer-feedback §Part B #5): - - Contains any phrase in `_REVIEWER_FEEDBACK_PHRASES`. - - References the PR number directly (`#178`, `PR #178`, `pull/178`). + - Contains any phrase in `_REVIEWER_FEEDBACK_PHRASES` (author-agnostic). + - References the PR number directly (`#178`, `PR #178`, `pull/178`), + gated on non-default author — see the rationale below. - Optional: `pr_url` provided — body references the PR URL itself. - Note: the AUTHOR + LENGTH filter is applied at the call site, not - here — this function only does the body-text match. The caller is - responsible for combining with author/length heuristics so the - single-purpose helper is unit-testable. + Note: the AUTHOR + LENGTH filter for substantive feedback is applied + at the call site, not here — this function only does the body-text + match. The caller is responsible for combining with author/length + heuristics so the single-purpose helper is unit-testable. + + Why PR-number regex is gated on non-default author (REVIEWER + FEEDBACK IN HERMES-AGENT#2, 2026-08-21): + Status pings auto-mirrored by `mirror_push` routinely contain + "PR #N opened" / "PR #N closed" / "→ from kanban task N" + — releasing the guard on those would create a respawn loop + bounded only by the failure circuit breaker, defeating the 24h + active-PR window for any task whose auto-mirrored pings happen + to mention the PR number. Phrase matches stay author-agnostic + because phrases like "please update" / "fix in this pr" are + concrete reviewer signals that are extremely unlikely to appear + in mirrored status pings (those are short emoji + task id + + status lines). + + Backward compat: + When ``author`` is None, the helper still permits the PR-number + regex to fire (pre-2026-08-21 behavior). Tests that don't carry + an author field keep working. New callers should always pass + ``author=``. """ if not body: return False if _REVIEWER_FEEDBACK_RE.search(body): return True - if _REVIEWER_FEEDBACK_PR_NUM_RE.search(body): - return True - if pr_url and pr_url in body: - return True + if author is None or author != "default": + if _REVIEWER_FEEDBACK_PR_NUM_RE.search(body): + return True + if pr_url and pr_url in body: + return True return False @@ -9641,9 +9664,20 @@ def _populate_branch_override_for_task( def _check_reviewer_feedback_release( conn: sqlite3.Connection, task_id: str -) -> bool: - """Return True iff reviewer feedback exists AFTER the most recent - PR-URL comment, warranting release of the `active_pr` guard. +) -> "tuple[bool, int | None, str | None]": + """Return (should_release, triggering_comment_id, triggering_reason). + + - ``should_release``: True iff reviewer feedback exists AFTER the most + recent PR-URL comment AND is newer than the most recent prior + ``respawn_released`` event (so a released respawn that fails + without a new feedback comment doesn't re-release on the next tick + — the "respawn loop" failure mode documented in + REVIEWER FEEDBACK IN HERMES-AGENT#2, 2026-08-21). + - ``triggering_comment_id``: the ``task_comments.id`` of the comment + that fired the release. Used by the caller to record a + ``respawn_released`` event with that id as the dedupe watermark. + - ``triggering_reason``: short string describing which trigger fired + (a / b / c / d, mapped to the four spec triggers). Audit only. Implements SPEC-active-pr-guard-reviewer-feedback §Part B #5: any of these triggers releases the guard: @@ -9656,14 +9690,19 @@ def _check_reviewer_feedback_release( (idempotent re-push of the same comment body does NOT release). 3. A comment AFTER the most recent PR-URL comment matching any phrase in `_REVIEWER_FEEDBACK_PHRASES`, OR referencing the PR - number directly (`#178`/`PR #178`/`pull/178`). + number directly (`#178`/`PR #178`/`pull/178`) — gated on + non-default author to prevent default-authored auto-mirrored + status pings like "PR #178 opened" from releasing the guard + (per REVIEWER FEEDBACK IN HERMES-AGENT#2, 2026-08-21). 4. The most recent PR-URL comment's PR has `reviewDecision == 'CHANGES_REQUESTED'` (queried via `gh pr view --json reviewDecision`). - The function is conservative: returning False leaves the existing - `active_pr` guard in force. Returning True releases the guard for - THIS dispatch tick. + The function is conservative: returning ``(False, None, None)`` + leaves the existing `active_pr` guard in force. Returning + ``(True, comment_id, reason)`` releases the guard for THIS dispatch + tick and signals the caller to record a ``respawn_released`` event + so the next tick skips comments with ``id <= comment_id``. """ now = int(time.time()) pr_cutoff = now - _RESPAWN_GUARD_PR_WINDOW @@ -9695,13 +9734,39 @@ def _check_reviewer_feedback_release( # deciding what to do. Returning False here matches the # "no PR-URL comment = no reviewer feedback to release against" # semantics. - return False + return False, None, None + + # Dedupe watermark (REVIEWER FEEDBACK IN HERMES-AGENT#2, 2026-08-21): + # skip any feedback with created_at <= the most recent respawn_released + # feedback_at. Without this, a released respawn that fails would + # re-release on every subsequent tick because the same stale feedback + # still satisfies trigger 1 / 3 — a respawn loop bounded only by the + # failure circuit breaker. The watermark advances only when a NEW + # feedback source triggers a release, so legitimate fresh feedback + # (a follow-up comment from the reviewer, or a new + # CHANGES_REQUESTED review round) still fires. + # + # The watermark is keyed on `created_at`, not comment id, so a trigger-4 + # release (no comment) can still record a meaningful timestamp (= the + # decision-check time) and any later comment with a higher + # `created_at` will advance past it. + watermark_row = conn.execute( + "SELECT MAX(CAST(payload ->> '$.feedback_at' AS INTEGER)) " + "AS max_fb_at " + "FROM task_events " + "WHERE task_id = ? AND kind = 'respawn_released'", + (task_id,), + ).fetchone() + feedback_watermark_at = int(watermark_row["max_fb_at"] or 0) if watermark_row else 0 pr_url_key = _comment_content_key(most_recent_pr or "") # Walk comments AFTER the most recent PR-URL comment. for r in rows: - if int(r["created_at"] or 0) <= most_recent_pr_at: + r_created = int(r["created_at"] or 0) + if r_created <= feedback_watermark_at: + continue + if r_created <= most_recent_pr_at: continue body = r["body"] or "" author = r["author"] or "" @@ -9718,17 +9783,27 @@ def _check_reviewer_feedback_release( author != "default" and len(body) >= _REVIEWER_FEEDBACK_MIN_BODY_LEN ): - return True - # Trigger 3 — body pattern match (author-agnostic). - if _has_reviewer_feedback(body): - return True + return True, r_created, "a" + # Trigger 3 — body pattern match, author-gated for PR-number + # references (REVIEWER FEEDBACK IN HERMES-AGENT#2, + # 2026-08-21). Pass ``author=`` so a default-authored ping + # like "PR #178 opened" cannot release the guard. + if _has_reviewer_feedback(body, author=author): + return True, r_created, "c" # Trigger 4 — reviewDecision == CHANGES_REQUESTED on the linked PR. - decision = _query_pr_review_decision(most_recent_pr) - if decision == "CHANGES_REQUESTED": - return True + # Also gated on the watermark so a CHANGES_REQUESTED decision that + # has already released once doesn't keep firing on every tick when + # the released respawn fails. ``reason="d"`` is the audit label. + if feedback_watermark_at == 0: + decision = _query_pr_review_decision(most_recent_pr) + if decision == "CHANGES_REQUESTED": + # No specific comment fires trigger 4 — use ``now`` as the + # feedback watermark so any LATER feedback (a follow-up + # comment from the reviewer) still advances past it. + return True, now, "d" - return False + return False, None, None def check_respawn_guard( @@ -9903,8 +9978,39 @@ def check_respawn_guard( # reads ``_pending_reviewer_branch_override`` to route the new # Builder run to the same branch + head SHA as the most recent # PR-URL comment (per SPEC Part B step #6). - if _check_reviewer_feedback_release(conn, task_id): + # + # RELEASING THE GUARD (REVIEWER FEEDBACK IN HERMES-AGENT#2, + # 2026-08-21): the release event records the timestamp and + # reason of the triggering feedback so subsequent ticks can + # skip already-released feedback (preventing the respawn-loop + # failure mode) and operators have an audit trail of why a + # spawn was allowed. + # + # ``_pending_reviewer_branch_override`` is the per-dispatch-tick + # side-channel set by the release path. Once a release fires + # and populates the override dict, re-running ``check_respawn_ + # guard`` for the same task in the same dispatch tick (e.g. + # tests that call ``check_respawn_guard`` manually then + # ``dispatch_once``) must NOT re-guard the task — the override + # is in flight. We detect that here: if the dict already has an + # entry for this task, return None (release) without re-running + # the predicate or emitting another audit event. + if _pending_reviewer_branch_override.get(task_id) is not None: + return None # Override already populated this tick — release. + should_release, feedback_at, feedback_reason = _check_reviewer_feedback_release( + conn, task_id + ) + if should_release: _populate_branch_override_for_task(conn, task_id) + with write_txn(conn): + _append_event( + conn, task_id, "respawn_released", + { + "reason": "reviewer_feedback", + "feedback_at": int(feedback_at or 0), + "trigger": feedback_reason, + }, + ) return None # Reviewer feedback — release the guard. return "active_pr" @@ -10613,17 +10719,35 @@ def _any_spawnable_review() -> bool: # logic if no override is registered for this task. pr_head_sha: Optional[str] = None override = _pending_reviewer_branch_override.pop(claimed.id, None) - if override is not None and claimed.workspace_kind == "worktree": - # The branch the worker should land on; the head SHA is - # surfaced separately via the spawn kwarg so the worker can - # check it out after provisioning the worktree. - pr_branch, pr_head_sha_for_task = override - pr_head_sha = pr_head_sha_for_task - # Re-provision the worktree pointing at this branch, not - # ``wt/``. _resolve_worktree_workspace already - # handles "existing checkout of branch X" so we just need - # to seed claimed.branch_name. - claimed = replace(claimed, branch_name=pr_branch) + if override is not None: + if claimed.workspace_kind == "worktree": + # The branch the worker should land on; the head SHA is + # surfaced separately via the spawn kwarg so the worker can + # check it out after provisioning the worktree. + pr_branch, pr_head_sha_for_task = override + pr_head_sha = pr_head_sha_for_task + # Re-provision the worktree pointing at this branch, not + # ``wt/``. _resolve_worktree_workspace already + # handles "existing checkout of branch X" so we just need + # to seed claimed.branch_name. + claimed = replace(claimed, branch_name=pr_branch) + else: + # Non-worktree tasks (dir / scratch) can't honour the + # branch override — the override targets a git worktree + # that dir/scratch workspaces don't provision. Drop it + # (already popped above) and fall through to the normal + # workspace resolution. Log at debug so operators can + # diagnose why a reviewer-feedback release didn't route + # to the PR branch (REVIEWER FEEDBACK IN + # HERMES-AGENT#2, 2026-08-21). + import logging + logging.getLogger("kanban.dispatcher").debug( + "respawn_released: dropping branch override for task %s " + "because workspace_kind=%s does not support git " + "worktrees; falling through to default workspace", + claimed.id, claimed.workspace_kind, + ) + pr_head_sha = None if claimed.workspace_kind == "worktree": workspace, resolved_branch_name = _resolve_worktree_workspace(claimed, board=board) else: diff --git a/tests/hermes_cli/test_kanban_review_lifecycle.py b/tests/hermes_cli/test_kanban_review_lifecycle.py index 2d20b2a0391c0..0c7ca7dafe694 100644 --- a/tests/hermes_cli/test_kanban_review_lifecycle.py +++ b/tests/hermes_cli/test_kanban_review_lifecycle.py @@ -23,6 +23,7 @@ import json import os from pathlib import Path +from typing import cast import pytest @@ -62,6 +63,20 @@ def _events(conn, tid, kind=None): return out +def _release_event_payloads(conn, tid: str) -> list[dict]: + """Return the parsed payloads of all `respawn_released` events on a task. + + Helper for the reviewer-feedback dedupe tests (REVIEWER FEEDBACK IN + HERMES-AGENT#2, 2026-08-21). Filters out any None payloads so callers + can subscript directly without type-checking the second tuple element. + """ + out: list[dict] = [] + for kind, payload in _events(conn, tid, kind="respawn_released"): + if payload is not None: + out.append(cast(dict, payload)) + return out + + def _last_run(conn, tid): return conn.execute( "SELECT status, outcome, summary FROM task_runs " @@ -891,6 +906,345 @@ def test_reviewer_feedback_release_trigger_short_author_aliadil_pattern_match( assert kb.check_respawn_guard(conn, tid) is None +# --- REGRESSION: default-authored PR-number pings must NOT release --- +# (REVIEWER FEEDBACK IN HERMES-AGENT#2, 2026-08-21, issue #1). +# +# Auto-mirrored status pings routinely contain "PR #N opened" / "PR #N +# closed" — the pre-fix `_REVIEWER_FEEDBACK_PR_NUM_RE` fired +# author-agnostic, so a default-authored ping like the one below would +# release the active_pr guard, violating the spec's "auto-mirrored +# default-authored status comments do NOT release" acceptance criterion +# AND creating a respawn loop on every tick. + +def test_reviewer_feedback_no_release_default_author_pr_number_opened( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Default-authored 'PR #178 opened' ping does NOT release the guard. + + Pre-fix this fired trigger 3 (PR-number regex). After the fix, + trigger 3's PR-number regex is gated on author != 'default' so + auto-mirrored status pings can't release the guard. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="pr ping", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178 for review", + created_at=now - 120, + ) + _add_comment_at( + conn, tid, author="default", + body="📌 PR #178 opened (auto-mirrored status ping)", + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +def test_reviewer_feedback_no_release_default_author_pull_slash_n( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Default-authored 'pull/178' reference doesn't release either.""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="pull slash", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178 for review", + created_at=now - 120, + ) + _add_comment_at( + conn, tid, author="default", + body="→ from kanban task t_x via pull/178", + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +def test_reviewer_feedback_no_release_default_author_phrase_with_pr_num( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Default-authored phrase + PR-number: the phrase still triggers + release (phrases stay author-agnostic) — but ONLY because phrases + are concrete reviewer signals. A default-authored phrase is rare + enough that the spec doesn't gate it; this test pins the + intended behavior so future regressions are caught. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="phrase ping", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178", + created_at=now - 120, + ) + # default author + phrase + PR number. The phrase still wins + # (author-agnostic), which is correct per the rationale in + # _has_reviewer_feedback: status pings are short emoji lines, + # not full sentences with reviewer phrases. + _add_comment_at( + conn, tid, author="default", + body="please update the PR #178 with the latest changes", + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) is None + + +def test_reviewer_feedback_no_release_default_author_bare_hash_n( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Bare '#178' from default author doesn't release.""" + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="bare hash", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178 for review", + created_at=now - 120, + ) + _add_comment_at( + conn, tid, author="default", + body="see #178 for context", + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +# --- REGRESSION: stale feedback must NOT re-release on every tick --- +# (REVIEWER FEEDBACK IN HERMES-AGENT#2, 2026-08-21, issue #2). +# +# Pre-fix, once the guard released and the spawned run failed without +# producing a new PR-URL comment, the same stale feedback comment kept +# satisfying trigger 1 / 3 on every subsequent tick — a respawn loop +# bounded only by the failure circuit breaker. The fix records a +# `respawn_released` event with the feedback timestamp; subsequent +# ticks skip comments at or before that watermark. + +def test_reviewer_feedback_no_double_release_after_failure( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Once the guard releases for a given feedback comment, subsequent + ticks with NO new feedback do NOT re-release — the respawn-loop + failure mode is closed. + + Simulates: a release event is on file; check_respawn_guard is + called again on the same comments. Without the watermark, the + same stale feedback would satisfy trigger 1 / 3 again. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback( + conn, feedback_author="aliaadil", + feedback_body=( + "logging is not sufficient. ALL actions performed by the " + "user and server need to be logged properly. please update." + ), + ) + # First tick: releases the guard. + assert kb.check_respawn_guard(conn, tid) is None + # Confirm the release event was recorded with the feedback_at + # timestamp. + released_events = _release_event_payloads(conn, tid) + assert len(released_events) == 1, released_events + assert released_events[0]["reason"] == "reviewer_feedback" + assert released_events[0]["feedback_at"] > 0 + feedback_at = released_events[0]["feedback_at"] + # Second tick WITHOUT new feedback: must NOT release. + assert kb.check_respawn_guard(conn, tid) == "active_pr" + # No new release event recorded. + assert len(_release_event_payloads(conn, tid)) == 1 + # Third tick: still no release. + assert kb.check_respawn_guard(conn, tid) == "active_pr" + assert len(_release_event_payloads(conn, tid)) == 1 + # Sanity: the feedback_at we recorded is the feedback comment's + # created_at — that comment is now in the past and skipped. + assert feedback_at > 0 + + +def test_reviewer_feedback_releases_again_for_newer_feedback_comment( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """After a prior release, a NEW (later-created_at) reviewer comment + DOES re-release the guard. The watermark advances only when new + feedback arrives, so legitimate fresh feedback (a follow-up + comment from the reviewer) still fires. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback( + conn, feedback_author="aliaadil", + feedback_body=( + "logging is not sufficient. ALL actions performed by the " + "user and server need to be logged properly. please update." + ), + ) + # First release. + assert kb.check_respawn_guard(conn, tid) is None + first_release_at = _release_event_payloads(conn, tid)[0]["feedback_at"] + # Second tick still no new feedback → no release. + assert kb.check_respawn_guard(conn, tid) == "active_pr" + # Reviewer follows up with a NEW substantive comment. + later = int(_time.time()) + 5 # strictly greater than first_release_at + _add_comment_at( + conn, tid, author="aliaadil", + body=( + "thanks for the logging update. but the audit trail still " + "misses some key events — please update the type annotations " + "and add the missing fields." + ), + created_at=later, + ) + # Now the guard releases again. + assert kb.check_respawn_guard(conn, tid) is None + # Two release events on file, the second with feedback_at == later. + events = _release_event_payloads(conn, tid) + assert len(events) == 2, events + assert events[1]["feedback_at"] == later + assert events[1]["feedback_at"] > first_release_at + + +def test_reviewer_feedback_no_release_trigger4_only_after_first( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Trigger 4 (CHANGES_REQUESTED) does not re-release after a prior + release. The release event's feedback_at advances the watermark, + so a stale CHANGES_REQUESTED decision that hasn't changed doesn't + fire again on the next tick. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="t4 dedupe", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178 for review", + created_at=now - 60, + ) + # Simulate: a prior release was already recorded. (Pretend the + # dispatcher fired earlier with the same trigger-4 outcome.) + with kb.write_txn(conn): + kb._append_event( + conn, tid, "respawn_released", + { + "reason": "reviewer_feedback", + "feedback_at": now - 1, + "trigger": "d", + }, + ) + # Now patch the gh call to return CHANGES_REQUESTED — should + # NOT release because the watermark is already set. + _patch_gh_review_decision(monkeypatch, decision="CHANGES_REQUESTED") + assert kb.check_respawn_guard(conn, tid) == "active_pr" + + +def test_reviewer_feedback_no_release_default_pr_num_does_not_advance_watermark( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """A default-authored ping with a PR number does NOT release AND + does NOT advance the watermark — so a subsequent legitimate + reviewer comment at the same or later timestamp still releases. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task(conn, title="watermark safety", assignee="builder") + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178 for review", + created_at=now - 300, + ) + # Default ping that mentions the PR number (must NOT release). + _add_comment_at( + conn, tid, author="default", + body="📌 PR #178 opened (auto-mirrored status ping)", + created_at=now - 60, + ) + assert kb.check_respawn_guard(conn, tid) == "active_pr" + # No release event recorded. + assert _release_event_payloads(conn, tid) == [] + # Now a legitimate reviewer comment arrives. It should release. + _add_comment_at( + conn, tid, author="aliaadil", + body=( + "the PR #178 looks good overall but please update the " + "type annotations to use the new typing.Literal syntax." + ), + created_at=now - 30, + ) + assert kb.check_respawn_guard(conn, tid) is None + # Exactly one release event, from the real reviewer comment. + events = _release_event_payloads(conn, tid) + assert len(events) == 1 + assert events[0]["trigger"] == "a" # non-default + length + + +def test_reviewer_feedback_release_event_has_audit_fields( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """The `respawn_released` event carries reason, feedback_at, and + trigger fields so operators have a full audit trail of why a + spawn was allowed (REVIEWER FEEDBACK IN HERMES-AGENT#2, 2026-08-21, + issue #2 audit-trail concern). + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = _setup_task_with_pr_and_feedback( + conn, feedback_author="aliaadil", + feedback_body="please update the type annotations here.", + ) + assert kb.check_respawn_guard(conn, tid) is None + [payload] = _release_event_payloads(conn, tid) + assert payload["reason"] == "reviewer_feedback" + assert payload["trigger"] in ("a", "b", "c", "d") + assert isinstance(payload["feedback_at"], int) + assert payload["feedback_at"] > 0 + + +def test_reviewer_feedback_no_release_remirror_same_timestamp( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """A re-mirror of the same feedback at the same timestamp does NOT + re-release the guard — the watermark is keyed on created_at, so a + second mirror row with the same (or earlier) timestamp is treated + as the same feedback, not a new round. + """ + _patch_gh_review_decision(monkeypatch, decision=None) + with kb.connect() as conn: + tid = kb.create_task( + conn, title="remirror same ts", assignee="builder", + ) + now = int(_time.time()) + _add_comment_at( + conn, tid, author="builder", + body="Opened https://github.com/example/repo/pull/178 for review", + created_at=now - 300, + ) + # First feedback comment from the reviewer. + _add_comment_at( + conn, tid, author="aliaadil", + body="please update the type annotations here.", + created_at=now - 60, + ) + assert kb.check_respawn_guard(conn, tid) is None + # Mirror re-pushes the same comment body at the SAME timestamp + # — a re-mirror race where the mirror service happens to + # re-fetch and write a duplicate row with identical timestamp. + _add_comment_at( + conn, tid, author="aliaadil", + body="please update the type annotations here.", + created_at=now - 60, + ) + # Should NOT release a second time — the watermark equals the + # duplicate's created_at so the watermark check skips it. + assert kb.check_respawn_guard(conn, tid) == "active_pr" + # Still only one release event. + assert len(_release_event_payloads(conn, tid)) == 1 + + # --- Non-trigger conditions: guard stays "active_pr" --- def test_reviewer_feedback_no_release_only_auto_mirrored_default_comments(