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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions PATCHES.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,4 @@ default; do **not** retire such a row on a PR-merge signal. See the #44338 row.
| fork PR (TBD — upstream PR not yet opened) | Fix the kanban crash-detector reaping a re-claimed worker mid-init. `detect_crashed_workers` grants a freshly-spawned worker a launch-window grace (default 30s, `HERMES_KANBAN_CRASH_GRACE_SECONDS`) so its PID can become visible on `/proc` before liveness is checked — but it measured the grace from `tasks.started_at`, which is pinned to the task's first-ever start (`COALESCE` on every claim) and never refreshed on re-claim. A card re-claimed for its next lane (implement → review) therefore inherited a stale `started_at`, the grace had long since expired, and the new worker was reaped as `crashed (pid not alive)` before it finished initializing (plugin load); with `failure_limit=2` the card then landed in `blocked`/`gave_up` without the work ever being attempted. The fix measures the grace from the active `task_runs` row via `COALESCE(r.started_at, t.started_at)` joined on `current_run_id` — the **exact per-attempt pattern `enforce_max_runtime` already uses** (see `test_max_runtime_uses_current_run_start_after_retry`), so retries/re-claims get a fresh window; falls back to `tasks.started_at` when no run row is present (preserves first-claim behavior). Single-file change in `hermes_cli/kanban_db.py` + regression test `test_detect_crashed_workers_grace_uses_current_run_start_after_reclaim`. Proven live: under the fix the auto-dispatcher spawned a worker on a card with a 3600s-stale `started_at` and it reached its agent loop and completed (run 14, 12s) where pre-fix runs crashed mid-init. **Retire trigger:** open the upstream PR (phase 2), then auto-retire when it merges in a tagged release ≥ base; until then carry as upstream-pending. | upstream-pending | v2026.6.19 |
| fork-local (no upstream PR) | Homestead-specific expansion of the `skills/github/github-code-review` skill for the kanban PR-review pipeline: the automated/non-interactive (webhook) review context, post-once idempotency guards (head-SHA dedup, blocked-timeout-is-unknown-not-failed, write-payload-to-file-before-POST), the humanizer/de-claude gate, and four reference files (`responding-to-and-resolving-review-threads` — the AUTHOR-resolves-threads loop incl. the Casey-2026-06-24 author-vs-reviewer decision; `webhook-triggered-reviews`; `consolidating-duplicate-reviews`; `editing-a-posted-review`) plus `scripts/commentable_lines.py`. This is the durable git home for material that previously lived ONLY in the `~/.hermes` deploy (silent drift). Deliberate divergence from upstream's generic review skill — homestead/kanban-specific, never sent upstream. **Retire trigger:** never auto-retires (permanent-local); remove only if the homestead PR-review pipeline is retired. | permanent-local | v2026.6.19 |
| fork PR (TBD — no upstream PR; fork-internal review-lane semantics) | Fix the kanban crash-detector dropping a crashed **reviewer** back into the build lane. A card moved to `review` (a worker opened a PR and parked it) is claimed by `claim_review_task`, which CAS-transitions `review → running` and records a `claimed` event carrying `source_status: "review"` — so while the reviewer works the row status is `running`, indistinguishable from a build run. PR #16 only handled a dead worker on a card parked in a NON-`running` lane; it did NOT cover a reviewer that dies while actively `running`. When `detect_crashed_workers` reaped such a crash it ran the running-crash UPDATE `SET status='ready'`, losing the review lane: (i) the normal `ready` dispatch then re-ran the IMPLEMENTER instead of respawning the reviewer (PR-under-review silently falls back into the build lane), and (ii) `check_respawn_guard` recomputed `is_review=False`, re-tripping the `recent_success`/`active_pr` guards (the original build run is a recent `completed` run + left a PR-URL comment) and deferring respawn for the full window. The fix reads the durable `source_status: "review"` signal off the crashed run's `claimed` event (scoped to `current_run_id`; no new schema column) and, on a genuine crash, restores the card to `review` instead of `ready` — claim still cleared, CAS still guarded on `status='running'`, crash event / run outcome / failure counter / circuit breaker bookkeeping unchanged. To keep a flaky reviewer from looping forever in `review`, the crash-path breaker-trip UPDATE in `_record_task_failure` widens its WHERE-IN from `('ready','running')` to `('ready','running','review')`, so a repeatedly-crashing reviewer still trips to `blocked` via the normal failure-count path. Rate-limited (cooldown defer) and protocol-violation (immediate trip) sub-cases unchanged — only the lane on a genuine crash changes. Single-file change in `hermes_cli/kanban_db.py` + 4 regression tests in `tests/hermes_cli/test_kanban_db.py` (crashed reviewer → `review`; crashed build run → `ready` unchanged; repeatedly-crashing reviewer → `blocked`; restored `review` card is free of the `recent_success`/`active_pr` guards). Fork-internal review-lane semantics (the `claim_review_task` source_status signal is fork machinery) — not surfaced to NousResearch. **Retire trigger:** never auto-retires on an upstream PR-merge signal (no upstream PR); remove only if the fork's review-lane dispatch is retired or upstream adopts equivalent review-lane-aware crash recovery. | permanent-local | v2026.6.19 |
| fork PR (TBD — no upstream PR; fork-internal review-lane semantics) | Auto-route a reviewer's `review-changes-requested` block back to the original author from the housekeeping tick, closing the reviewer→author hop the GitHub `pull_request_review` webhook cannot close when reviewer and author share one GitHub identity. The reviewer (lamport) runs as the `cwest` identity that OWNS the team PRs, so GitHub rejects his `REQUEST_CHANGES` with HTTP 422 and he falls back to a `COMMENT` event — which is NOT `changes_requested`, so the webhook router never bounces and the card sits `blocked` until a human hand-routes it. The fix is board-internal and lives entirely in the dispatcher, NOT the reviewer (the reviewer's terminal action stays a clean `kanban_block`, preserving the lane-corruption-safe design): `auto_route_review_bounce` scans `blocked` cards on each `dispatch_once` tick (before `recompute_ready`), and for any card whose most-recent sticky `blocked` event carries the `review-changes-requested` reason prefix, reassigns it to the original author (resolved from the `assigned` event history — the `from` of the move whose `to` is the current reviewer — never a literal profile name) and unblocks it via the existing `unblock_task`, which clears the `active_pr`/`recent_success` respawn guards exactly like a manual block→unblock cutoff, plus a `dispatcher`-authored `[audit]` comment naming the PR and verdict gist. Idempotent (the route flips the card off `blocked`, so a later tick won't re-fire; two ticks → one route, one comment). The `awaiting-casey-signoff` PASS/acceptance block is excluded by the prefix match (must stay `blocked`+casey), as are non-review and circuit-breaker (`gave_up`) blocks. Two `check_respawn_guard` carve-outs support the route: the `recent_success` guard now honors a trailing `unblocked` event (the bounced build run is the work being reworked — it must not veto respawn), and the dup-PR scan excludes the dispatcher's own same-second audit comment. Toggle `kanban.auto_route_review_bounce` (default ON) gates the whole path; wired through `dispatch_once`, the `hermes kanban dispatch` CLI, and the gateway dispatcher watcher. Changes in `hermes_cli/kanban_db.py`, `hermes_cli/kanban.py`, `gateway/kanban_watchers.py` + 8 regression tests in `tests/hermes_cli/test_kanban_auto_route_review_bounce.py` (core route; route via real `dispatch_once`; acceptance block does NOT route; idempotency; routed card dispatchable; non-review block; circuit-breaker block; toggle off). Upstream has no equivalent (no `review-changes-requested`/`auto_route`/`bounce` concept in upstream `hermes_cli/*.py`) — cwest-team review-loop tooling. **Retire trigger:** never auto-retires (permanent-local); remove only if the fork's review-lane dispatch is retired. | permanent-local | v2026.6.19 |
11 changes: 11 additions & 0 deletions gateway/kanban_watchers.py
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,16 @@ async def _kanban_dispatcher_watcher(self) -> None:
max_in_progress_per_profile,
)

# Read kanban.auto_route_review_bounce — when a reviewer terminates a
# review with a clean review-changes-requested block, the housekeeping
# tick routes that card back to the original author (closes the
# reviewer→author hop the GitHub webhook can't when reviewer and author
# share one identity). Defaults ON; set false to keep bounce blocks
# parked for a human to route.
auto_route_review_bounce_enabled = bool(
kanban_cfg.get("auto_route_review_bounce", True)
)

# Initial delay so the gateway finishes wiring adapters before the
# dispatcher spawns workers (those workers may hit gateway notify
# subscriptions etc.). Matches the notifier watcher's delay.
Expand Down Expand Up @@ -927,6 +937,7 @@ def _tick_once_for_board(slug: str) -> "Optional[object]":
stale_timeout_seconds=stale_timeout_seconds,
default_assignee=default_assignee,
max_in_progress_per_profile=max_in_progress_per_profile,
auto_route_review_bounce_enabled=auto_route_review_bounce_enabled,
)
except sqlite3.DatabaseError as exc:
if _is_corrupt_board_db_error(exc):
Expand Down
6 changes: 6 additions & 0 deletions hermes_cli/kanban.py
Original file line number Diff line number Diff line change
Expand Up @@ -2126,6 +2126,9 @@ def _coerce_positive_int(value):
_kanban_cfg.get("max_in_progress_per_profile")
)
max_in_progress = _coerce_positive_int(_kanban_cfg.get("max_in_progress"))
auto_route_review_bounce_enabled = bool(
_kanban_cfg.get("auto_route_review_bounce", True)
)
# CLI --max overrides config kanban.max_spawn when both are present;
# CLI is the more explicit signal so it wins.
cli_max = getattr(args, "max", None)
Expand All @@ -2136,6 +2139,7 @@ def _coerce_positive_int(value):
default_assignee = None
max_in_progress_per_profile = None
max_in_progress = None
auto_route_review_bounce_enabled = True
max_spawn = getattr(args, "max", None)
with kb.connect_closing() as conn:
res = kb.dispatch_once(
Expand All @@ -2146,6 +2150,7 @@ def _coerce_positive_int(value):
failure_limit=getattr(args, "failure_limit", kb.DEFAULT_SPAWN_FAILURE_LIMIT),
default_assignee=default_assignee,
max_in_progress_per_profile=max_in_progress_per_profile,
auto_route_review_bounce_enabled=auto_route_review_bounce_enabled,
)
if getattr(args, "json", False):
print(json.dumps({
Expand All @@ -2155,6 +2160,7 @@ def _coerce_positive_int(value):
"stale": res.stale,
"auto_blocked": res.auto_blocked,
"promoted": res.promoted,
"routed_review_bounce": res.routed_review_bounce,
"spawned": [
{"task_id": tid, "assignee": who, "workspace": ws}
for (tid, who, ws) in res.spawned
Expand Down
Loading
Loading