fix(kanban): skip PR/success respawn guards in review lane
Thread lane= into check_respawn_guard. For review-lane dispatch the active_pr and recent_success rules are skipped: a fresh PR URL comment (and often a recent completed run) is the precondition of the canonical review handoff, not a duplicate-work signal. Rate-limit cooldown and the auth-blocker check still apply in every lane. Regression: a review task with a <24h PR comment is spawned by dispatch while a ready-lane task with the same comment stays deferred; a rate_limited latest run still defers the review lane.
This commit is contained in:
parent
af0a418666
commit
a235d1917e
|
|
@ -8770,13 +8770,23 @@ def _clear_failure_counter(conn: sqlite3.Connection, task_id: str) -> None:
|
|||
_clear_spawn_failures = _clear_failure_counter
|
||||
|
||||
|
||||
def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str]:
|
||||
def check_respawn_guard(
|
||||
conn: sqlite3.Connection, task_id: str, *, lane: str = "ready",
|
||||
) -> Optional[str]:
|
||||
"""Return a guard reason if ``task_id`` should NOT be re-spawned, else None.
|
||||
|
||||
Called per ready/review task in ``dispatch_once`` before any claim attempt.
|
||||
Returning a reason defers the spawn this tick; the task stays in its
|
||||
source phase and gets another chance on the next dispatcher tick.
|
||||
|
||||
``lane`` names the dispatch column the task is being spawned from
|
||||
(``"ready"`` or ``"review"``). In the review lane the
|
||||
``recent_success`` and ``active_pr`` rules are skipped: a recent PR
|
||||
URL comment (and often a recent completed run) is the *precondition*
|
||||
of the canonical review handoff — a worker opened a PR and requested
|
||||
review — not a duplicate-work signal. Rate-limit cooldown and the
|
||||
auth-blocker check still apply in every lane.
|
||||
|
||||
Checks in priority order:
|
||||
|
||||
``"rate_limit_cooldown"``
|
||||
|
|
@ -8868,6 +8878,12 @@ def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str]
|
|||
if err and _RESPAWN_BLOCKER_RE.search(err):
|
||||
return "blocker_auth"
|
||||
|
||||
# Review-lane spawns stop here: a recent completed run and a fresh PR
|
||||
# URL comment are the canonical *inputs* to a review handoff (worker
|
||||
# opened a PR, then requested review), not signals of duplicate work.
|
||||
if lane == "review":
|
||||
return None
|
||||
|
||||
# 3. Completed run within guard window — proof of recent success.
|
||||
# Exception: an explicit re-queue AFTER that success (an operator
|
||||
# dragging done→ready, a dependency re-promotion, an unblock, a
|
||||
|
|
@ -9411,7 +9427,7 @@ def _dispatch_once_locked(
|
|||
(row["id"], row["assignee"], current)
|
||||
)
|
||||
continue
|
||||
guard_reason = check_respawn_guard(conn, row["id"])
|
||||
guard_reason = check_respawn_guard(conn, row["id"], lane="review")
|
||||
if guard_reason is not None:
|
||||
result.respawn_guarded.append((row["id"], guard_reason))
|
||||
if not dry_run:
|
||||
|
|
|
|||
|
|
@ -332,6 +332,67 @@ def test_review_dispatch_gate_prevents_phantom_reviewer(
|
|||
assert tid in [s[0] for s in res_on.spawned]
|
||||
|
||||
|
||||
def test_active_pr_guard_skipped_for_review_lane_but_defers_ready_lane(
|
||||
kanban_home: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""B2 regression: a fresh PR-URL comment must not block reviewer spawns.
|
||||
|
||||
A task parked in ``review`` with a PR link younger than 24h is the
|
||||
CANONICAL review handoff (worker opened a PR then requested review) —
|
||||
the review-lane dispatch must still claim/spawn it. The same comment on
|
||||
a ready-lane task is a duplicate-work signal and stays deferred.
|
||||
Rate-limit cooldown still applies in the review lane.
|
||||
"""
|
||||
import hermes_cli.config as cfgmod
|
||||
import hermes_cli.profiles as profmod
|
||||
|
||||
monkeypatch.setattr(profmod, "profile_exists", lambda name: True)
|
||||
monkeypatch.setattr(
|
||||
cfgmod, "load_config",
|
||||
lambda *a, **k: {"kanban": {"review_dispatch": True}},
|
||||
)
|
||||
pr_comment = "Opened https://github.com/example/repo/pull/123 for review."
|
||||
|
||||
with kb.connect() as conn:
|
||||
# Review-lane task with a fresh PR comment.
|
||||
review_id = kb.create_task(conn, title="review me", assignee="reviewer")
|
||||
claimed = kb.claim_task(conn, review_id)
|
||||
assert claimed is not None
|
||||
kb.add_comment(conn, review_id, author="worker", body=pr_comment)
|
||||
assert kb.request_review(
|
||||
conn, review_id, summary="PR ready",
|
||||
expected_run_id=claimed.current_run_id,
|
||||
)
|
||||
# Ready-lane task with the same fresh PR comment.
|
||||
ready_id = kb.create_task(conn, title="already PRed", assignee="worker")
|
||||
kb.add_comment(conn, ready_id, author="worker", body=pr_comment)
|
||||
|
||||
assert kb.check_respawn_guard(conn, ready_id) == "active_pr"
|
||||
assert kb.check_respawn_guard(conn, review_id, lane="review") is None
|
||||
|
||||
res = kb.dispatch_once(conn, dry_run=True)
|
||||
spawned_ids = [s[0] for s in res.spawned]
|
||||
guarded = dict(res.respawn_guarded)
|
||||
assert review_id in spawned_ids
|
||||
assert ready_id not in spawned_ids
|
||||
assert guarded.get(ready_id) == "active_pr"
|
||||
|
||||
# Rate-limit cooldown still defers the review lane.
|
||||
_now = int(__import__("time").time())
|
||||
with kb.write_txn(conn):
|
||||
conn.execute(
|
||||
"INSERT INTO task_runs (task_id, profile, status, outcome, "
|
||||
"started_at, ended_at) VALUES (?, 'reviewer', 'rate_limited', "
|
||||
"'rate_limited', ?, ?)",
|
||||
# ended_at strictly after the review-handoff run so the
|
||||
# "latest run" query deterministically picks this one.
|
||||
(review_id, _now, _now + 5),
|
||||
)
|
||||
assert kb.check_respawn_guard(
|
||||
conn, review_id, lane="review"
|
||||
) == "rate_limit_cooldown"
|
||||
|
||||
|
||||
def test_review_dispatch_preserves_task_skills_and_adds_reviewer_skill(
|
||||
kanban_home: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
|
|
@ -368,7 +429,7 @@ def test_review_dispatch_preserves_task_skills_and_adds_reviewer_skill(
|
|||
monkeypatch.setattr(
|
||||
kb,
|
||||
"check_respawn_guard",
|
||||
lambda _conn, _task_id: "rate_limit_cooldown",
|
||||
lambda _conn, _task_id, **_kw: "rate_limit_cooldown",
|
||||
)
|
||||
guarded = kb.dispatch_once(conn, spawn_fn=spawn)
|
||||
assert guarded.respawn_guarded == [(task_id, "rate_limit_cooldown")]
|
||||
|
|
@ -377,7 +438,7 @@ def test_review_dispatch_preserves_task_skills_and_adds_reviewer_skill(
|
|||
assert guarded_task is not None
|
||||
assert guarded_task.status == "review"
|
||||
|
||||
monkeypatch.setattr(kb, "check_respawn_guard", lambda _conn, _task_id: None)
|
||||
monkeypatch.setattr(kb, "check_respawn_guard", lambda _conn, _task_id, **_kw: None)
|
||||
result = kb.dispatch_once(conn, spawn_fn=spawn)
|
||||
|
||||
assert task_id in [task[0] for task in result.spawned]
|
||||
|
|
|
|||
Loading…
Reference in New Issue