From 19bc2d7c9c92e51773dd4a9034f19449b5dafbaa Mon Sep 17 00:00:00 2001 From: "ang-fleet-workers[bot]" <333956806+ang-fleet-workers[bot]@users.noreply.github.com> Date: Sun, 27 Sep 2026 15:09:49 -0700 Subject: [PATCH 1/2] fix(kanban): close C4 board-gate bypasses from FleetReview backfill (t_027d7fe7) 11 confirmed instances fixed with RED-on-base/GREEN regressions: #951 gateway sessionless slash identity, #956 run-scoped runtime cap, #960 goal CLI child fails closed, #999 can't contraction + dashboard claimed-review exit, #1021 no SIGTERM without spawn evidence, #1034 malformed survivor row HOLDs, #1074 session owner profile over env profile (x2), #1081 tool send-back needs a bindable session, #1234 arm only the card's own handoff PR. Verified: 5 affected test files 187 passed; same tests on base source 20 failed (the new/changed regressions only). #985 -> t_becb0042; FP/drops in PR body. --- hermes_cli/goals.py | 11 + hermes_cli/kanban.py | 18 +- hermes_cli/kanban_db.py | 62 +++- hermes_cli/kanban_pr_freshness.py | 14 +- hermes_cli/kanban_survivor.py | 16 +- plugins/kanban/dashboard/plugin_api.py | 28 +- .../hermes_cli/test_kanban_c4_board_gates.py | 297 ++++++++++++++++++ tests/hermes_cli/test_kanban_pr_freshness.py | 22 +- ...test_kanban_reclaim_unprovable_liveness.py | 7 +- .../test_kanban_review_coverage_gate.py | 3 + .../hermes_cli/test_kanban_review_sendback.py | 40 ++- .../test_kanban_second_claim_class.py | 18 +- tools/kanban_tools.py | 21 +- 13 files changed, 523 insertions(+), 34 deletions(-) create mode 100644 tests/hermes_cli/test_kanban_c4_board_gates.py diff --git a/hermes_cli/goals.py b/hermes_cli/goals.py index 15667613c9720..3b6cc7bc53465 100644 --- a/hermes_cli/goals.py +++ b/hermes_cli/goals.py @@ -1386,6 +1386,17 @@ def kanban_handoff_rejection( if worker_run_id is not None: blocked = kb.block_task(conn, task_id, reason=reason, kind="transient", expected_run_id=worker_run_id) return f"{reason}; task {'blocked transient after judge retry' if blocked else 'not blocked (run ownership changed)'}" + # A goal-mode worker handing off through the CLI runs it as a CHILD of + # its terminal tool: it inherits the card's task env but is never the run + # owner, so the resolver above returns None. That is the worker's lineage, + # not an operator -- a judge error must not let its handoff through + # (FleetReview #960). Refuse without blocking (run ownership is unproven). + if ( + worker_run_id is None + and task_id + and (os.environ.get("HERMES_KANBAN_TASK") or "").strip() == task_id + ): + return f"{reason}; handoff refused (caller inherits this card's worker env; judge errors fail closed)" return None diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index a5401e4f7b45f..c1687b4338302 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -1971,16 +1971,24 @@ def _caller_session_id() -> Optional[str]: explicit = (_SLASH_SESSION_ID.get() or "").strip() if explicit: return explicit + in_gateway = os.environ.get("_HERMES_GATEWAY") == "1" try: - from gateway.session_context import resolve_current_session_id - + from gateway.session_context import _SESSION_ID, resolve_current_session_id + + if in_gateway: + # In-process gateway: ONLY a per-turn contextvar bound in this + # context is ours. A plain slash command runs before that bind, + # and the resolver's os.environ fallback is process-global -- + # another chat's session. Sessionless means None, never a + # borrowed identity (FleetReview #951). + bound = _SESSION_ID.get() + return (bound.strip() or None) if isinstance(bound, str) else None resolved = (resolve_current_session_id() or "").strip() if resolved: return resolved - if os.environ.get("_HERMES_GATEWAY") == "1": - return None # in-process: env is another session's, never ours except Exception: - pass + if in_gateway: + return None return (os.environ.get("HERMES_SESSION_ID") or "").strip() or None diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 107d480a5c4dd..59e56a9c71188 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -5347,14 +5347,22 @@ def session_owner_profile(session_id: Optional[str]) -> Optional[str]: def _actor_profiles(actor: MutationActor) -> frozenset[str]: - """Every profile identity the actor legitimately holds: the bound - profile plus the owner profile of each caller session.""" - names = {actor.profile} if actor.profile else set() - for sid in actor.session_ids: - owner = session_owner_profile(sid) - if owner: - names.add(owner) - return frozenset(names) + """Every profile identity the actor legitimately holds. + + The owner profile of each caller session is authoritative. The bound + (env-derived) profile counts only when no caller session resolves to an + owner: a root-home repoint makes it read ``default`` -- an operator + profile -- from inside another profile's session, so keeping both let + that caller pass ``--operator`` or mutate a ``default``-assigned card + (FleetReview #1074). + """ + owners = { + owner for owner in (session_owner_profile(sid) for sid in actor.session_ids) + if owner + } + if owners: + return frozenset(owners) + return frozenset({actor.profile} if actor.profile else ()) def _valid_operator_reason(reason: str) -> bool: @@ -5605,10 +5613,9 @@ def check_home_session( return None if home in actor.session_ids: return None - # Execution lane: the assignee works its card wherever it was born, and a - # dispatched worker always owns the card it was spawned for. - if actor.profile and (row["assignee"] or "") == actor.profile: - return None + # Execution lane: a dispatched worker always owns the card it was spawned + # for. The assignee match lives below, on :func:`_actor_profiles` (session + # owner over env profile), not on the raw env profile (FleetReview #1074). if (os.environ.get("HERMES_KANBAN_TASK") or "").strip() == task_id: return None # ...and the cards it fanned out: item 1 stamps a worker's children with @@ -8378,8 +8385,11 @@ def _prior_worker_still_alive( # An outcome cannot certify exit: operators can write the same outcomes as # worker tools, and a newer synthetic row can hide an older live owner. runs = conn.execute( - "SELECT r.id, r.outcome, r.ended_at, r.started_at, t.max_runtime_seconds " - "FROM task_runs r JOIN tasks t ON t.id = r.task_id " + # The run's OWN runtime cap, snapshotted at claim: the card's current + # cap can be shortened after release while that worker still runs + # (FleetReview #956). A run with no snapshot is probed, never skipped. + "SELECT r.id, r.outcome, r.ended_at, r.started_at, r.max_runtime_seconds " + "FROM task_runs r " "WHERE r.task_id = ? ORDER BY r.id DESC", (task_id,), ).fetchall() @@ -9875,9 +9885,16 @@ def complete_task( # non-milestone PR through fleet-merge.sh. from hermes_cli import kanban_pr_freshness as _fresh try: + # Arm only the card's OWN handoff PR(s), never a PR the prose + # merely mentions (FleetReview #1234 finding). freshness = _fresh.check( still_open, task_id=task_id, allow_arm=not is_milestone_card(conn, task_id), + armable={ + f"{r.repo}#{r.number}" for r in _open_pr.extract_pr_refs( + metadata=metadata, survivor_pr=survivor_pr, + ) + }, ) except _fresh.DraftPrError as draft_err: with write_txn(conn): @@ -12727,8 +12744,8 @@ def _ret(ok: bool, reason: Optional[str] = None): _REVIEW_NA_INABILITY = re.compile( r"\b(?:" r"skip(?:ped|ping|s)?|" - r"(?:could|can)\s*(?:not|n't)|cannot|unable|" - r"(?:did|was|were|does|do)\s*(?:not|n't)\s+(?:run|ran|execute|executed|attempt|attempted|try|tried|finish|finished|complete|completed|get|reach)|" + r"(?:could|can)\s*(?:not|n['\u2019]t)|can['\u2019]t|cannot|unable|" + r"(?:did|was|were|does|do)\s*(?:not|n['\u2019]t)\s+(?:run|ran|execute|executed|attempt|attempted|try|tried|finish|finished|complete|completed|get|reach)|" r"not\s+(?:run|ran|executed|attempted|tried|finished|completed|reached)|" r"ran\s+out|out\s+of\s+time|no\s+time\b|timed?\s*out|" r"fail(?:ed|s)?\s+to\b|errored|crashed|blocked\s+(?:by|on)\b|" @@ -16830,6 +16847,19 @@ def _terminate_reclaimed_worker( if _pid_alive(pid): identity = _owner_identity(int(pid), *owner_window) + if ( + identity == "verified" + and len(owner_window) >= 3 + and owner_window[1] is None + and owner_window[2] is None + ): + # No run-scoped ``spawned`` evidence (legacy/migrated run): only + # the claim lower bound was applied, so ANY process created after + # the claim -- including one that reused the worker's PID -- + # reads "verified". That is unproven, not proven: never SIGTERM + # on it (FleetReview #1021). Liveness callers still treat it as + # alive (fail closed), so this only withholds the signal. + identity = "unverified" info["owner_identity"] = identity if identity == "recycled": # The recorded worker is gone; this PID now belongs to someone diff --git a/hermes_cli/kanban_pr_freshness.py b/hermes_cli/kanban_pr_freshness.py index fac9b3ce9ccfd..ab392fe9670a5 100644 --- a/hermes_cli/kanban_pr_freshness.py +++ b/hermes_cli/kanban_pr_freshness.py @@ -152,9 +152,16 @@ def spawn_arm(repo: str, number: int, sha: str, task_id: str) -> Optional[str]: def check(refs, *, task_id: str, allow_arm: bool, gh: Optional[GhFn] = None, - arm: Optional[ArmFn] = None, behind_max: int = STALE_BEHIND_MAX) -> dict: + arm: Optional[ArmFn] = None, behind_max: int = STALE_BEHIND_MAX, + armable=None) -> dict: """Apply (a)/(b)/(c) to OPEN PR ``refs``. Raises :class:`DraftPrError`. + ``armable`` is the set of ``"o/r#n"`` keys this card actually handed off + (``metadata.pr_url``/``pr``/``pr_urls`` or ``--survivor-pr``). Only those + are ever armed for merge: a PR merely MENTIONED in summary/result prose is + context, not merge authorization. ``complete_task`` always passes it; + ``None`` (direct callers) keeps the legacy every-ref behavior. + Returns a report ``{"prs": {"o/r#n": {...}}}`` for the handoff metadata. All draft checks run before any mutation, so a refused handoff has changed nothing on GitHub. @@ -171,6 +178,7 @@ def check(refs, *, task_id: str, allow_arm: bool, gh: Optional[GhFn] = None, if gh is None: return report arm = arm or spawn_arm + armable_keys = None if armable is None else {str(k).lower() for k in armable} views = [] drafts = [] @@ -208,7 +216,9 @@ def check(refs, *, task_id: str, allow_arm: bool, gh: Optional[GhFn] = None, entry["update_branch"] = "requested" if upd is not None else "failed (fail-open)" # The new head has fresh CI; arming is fleet-merge's job once it is green. continue - if allow_arm and automerge_enabled() and _ci_green(gh, ref.repo, head): + if allow_arm and armable_keys is not None and key.lower() not in armable_keys: + entry["automerge"] = "not armed: PR only mentioned, not this card's handoff PR" + elif allow_arm and automerge_enabled() and _ci_green(gh, ref.repo, head): log = arm(ref.repo, ref.number, head, task_id) entry["automerge"] = f"fleet-merge spawned ({log})" if log else "not spawned" elif allow_arm and automerge_enabled(): diff --git a/hermes_cli/kanban_survivor.py b/hermes_cli/kanban_survivor.py index 152ca433076cc..d723347bdd831 100644 --- a/hermes_cli/kanban_survivor.py +++ b/hermes_cli/kanban_survivor.py @@ -2515,7 +2515,21 @@ def preserve(conn, task_id, metadata=None, *, cleanup=False, workspace=None, survivor_ref=None, survivor_pr=None, survivor_unbound=False, evidence=(), survivor_none=False, survivor_reason=None): """Return a verified survivor or None for non-code work; fail closed on doubt.""" - bases, held, previous = _state(conn, task_id) + try: + bases, held, previous = _state(conn, task_id) + if not isinstance(bases, dict) or not (previous is None or isinstance(previous, dict)): + raise TypeError("survivor row is not a JSON object") + except (ValueError, TypeError) as exc: + # The row is read BEFORE the main try below, so its malformed-record + # backstop cannot see a partially written `bases`/`survivor` value + # (JSONDecodeError) or a non-object one (AttributeError on `.get`): + # the workspace was retained but with no `held_reason` and no + # `workspace_held` event (FleetReview #1034). Same HOLD, same reason. + _log.exception("kanban survivor: unreadable recovery state for %s", task_id) + reason = ("survivor_unavailable: recorded survivor state is malformed " + f"({type(exc).__name__}); repair the row or recover the workspace by hand") + _hold(conn, task_id, reason) + raise _refusal(reason) from exc if cleanup and held: raise SurvivorUnavailable(held) if cleanup and previous and previous.get("kind") == "none": diff --git a/plugins/kanban/dashboard/plugin_api.py b/plugins/kanban/dashboard/plugin_api.py index a48f7b4798764..b9cf10d982222 100644 --- a/plugins/kanban/dashboard/plugin_api.py +++ b/plugins/kanban/dashboard/plugin_api.py @@ -996,6 +996,27 @@ def _review_exit_refused(current_status: Optional[str], new_status: Optional[str ) +def _claimed_review_exit_refused( + conn: sqlite3.Connection, task_id: str, new_status: Optional[str], +) -> bool: + """A ``running`` card whose current run was claimed FROM ``review`` is a + review in progress: moving it to todo/triage/scheduled would close the + reviewer run without ``request_changes`` and hand unreviewed work back + (FleetReview #999). ``ready`` is allowed -- it resumes to ``review``.""" + if new_status is None or new_status in ("ready", "running", "review"): + return False + if new_status in _REVIEW_EXIT_STATUSES: + return False + row = conn.execute( + "SELECT status, current_run_id FROM tasks WHERE id = ?", (task_id,), + ).fetchone() + if row is None or row["status"] != "running" or row["current_run_id"] is None: + return False + return kanban_db._retry_status_for_run( + conn, task_id, row["current_run_id"], + ) == "review" + + @router.patch("/tasks/{task_id}") def update_task(task_id: str, payload: UpdateTaskBody, board: Optional[str] = Query(None)): board = _resolve_board(board) @@ -1004,7 +1025,9 @@ def update_task(task_id: str, payload: UpdateTaskBody, board: Optional[str] = Qu task = kanban_db.get_task(conn, task_id) if task is None: raise HTTPException(status_code=404, detail=f"task {task_id} not found") - if _review_exit_refused(task.status, payload.status): + if _review_exit_refused(task.status, payload.status) or ( + _claimed_review_exit_refused(conn, task_id, payload.status) + ): raise HTTPException(status_code=409, detail=_REVIEW_EXIT_REFUSAL) review_assignee_deferred = ( @@ -1286,6 +1309,9 @@ def _set_status_direct( ).fetchone() if held is None: return False + if _claimed_review_exit_refused(conn, task_id, new_status): + # Refused BEFORE terminating the reviewer worker. + return False released_lock = held["claim_lock"] if held["status"] == "running" and new_status != "running": termination = kanban_db._terminate_reclaimed_worker( diff --git a/tests/hermes_cli/test_kanban_c4_board_gates.py b/tests/hermes_cli/test_kanban_c4_board_gates.py new file mode 100644 index 0000000000000..32e26f883b04e --- /dev/null +++ b/tests/hermes_cli/test_kanban_c4_board_gates.py @@ -0,0 +1,297 @@ +"""FleetReview retro-backfill C4 (authz / ownership / guard bypass), kanban +board gates slice (t_027d7fe7). One regression per confirmed instance; each is +RED on base 5a9d284d49 and GREEN with the fix, with a control that shows the +gate is not passing by wedging everything. + +Instances covered here: #1021 (termination on missing spawn evidence), #1034 +(malformed survivor row read before the HOLD backstop), #1074 (env profile +outranking the caller session's owner profile), #999 (dashboard exit from a +claimed review run), #951 (gateway sessionless slash command borrowing the +process-global session id), #960 (goal-mode CLI child treated as operator). +#999 can't, #956, #1081 and #1234 live beside their existing suites. +""" + +from __future__ import annotations + +import contextvars +import os +import signal +import sqlite3 +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from hermes_cli import kanban_db as kb + + +@pytest.fixture +def board(tmp_path, monkeypatch): + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + for var in ("HERMES_KANBAN_TASK", "HERMES_KANBAN_RUN_ID", "HERMES_KANBAN_DB", + "HERMES_KANBAN_BOARD", "HERMES_DELEGATED_CHILD_CONTEXT", "HERMES_SESSION_ID"): + monkeypatch.delenv(var, raising=False) + kb._INITIALIZED_PATHS.clear() + kb.init_db() + return home + + +# --- #1021: no spawn evidence must never authorize a signal ---------------- + + +def _terminate(monkeypatch, owner_window): + sent = [] + monkeypatch.setattr(kb, "_pid_alive", lambda pid: True) + monkeypatch.setattr(kb.time, "sleep", lambda _s: None) + host = kb._claimer_id().split(":", 1)[0] + info = kb._terminate_reclaimed_worker( + os.getpid(), f"{host}:1", owner_window=owner_window, + signal_fn=lambda pid, sig: sent.append((pid, sig)), + ) + return info, sent + + +def test_missing_spawn_evidence_is_unverified_and_never_signalled(monkeypatch): + """Only the claim lower bound (epoch 0 here) is known: any process created + after the claim -- e.g. one that reused the worker PID -- would read + 'verified' and get SIGTERM. It must be unverified and held instead.""" + info, sent = _terminate(monkeypatch, (0.0, None, None)) + assert sent == [] + assert info["owner_identity"] == "unverified" + assert info["liveness_unprovable"] is True and info["terminated"] is False + + +def test_bounded_spawn_window_still_signals(monkeypatch): + """Control: with a spawned upper bound the recorded worker is verified and + signalled exactly as before.""" + import time as _time + + info, sent = _terminate(monkeypatch, (0.0, _time.time() + 60, None)) + assert info["owner_identity"] == "verified" + assert sent and sent[0] == (os.getpid(), signal.SIGTERM) + + +# --- #1034: malformed survivor row must HOLD, never go silent -------------- + + +@pytest.mark.parametrize("survivor,cleanup", [ + ("{", False), # partially written JSON -> JSONDecodeError in _state() + ("{", True), + ("[1]", True), # valid JSON, not an object -> previous.get AttributeError +]) +def test_malformed_survivor_row_holds_with_event(board, survivor, cleanup): + from hermes_cli import kanban_survivor as ks + + with kb.connect_closing() as conn: + tid = kb.create_task(conn, title="malformed survivor") + conn.execute( + "INSERT INTO task_workspace_survivors(task_id, bases, survivor) " + "VALUES (?, '{}', ?)", (tid, survivor), + ) + conn.commit() + with pytest.raises(ks.SurvivorUnavailable) as exc: + ks.preserve(conn, tid, cleanup=cleanup) + assert "malformed" in str(exc.value) + held = conn.execute( + "SELECT held_reason FROM task_workspace_survivors WHERE task_id = ?", (tid,), + ).fetchone()[0] + assert held and "malformed" in held + kinds = [r[0] for r in conn.execute( + "SELECT kind FROM task_events WHERE task_id = ?", (tid,))] + assert "workspace_held" in kinds + + +# --- #1074: the caller SESSION's profile outranks the env profile ---------- + + +def _session_db(path: Path, sid: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + conn = sqlite3.connect(path) + try: + conn.execute("CREATE TABLE IF NOT EXISTS sessions (id TEXT PRIMARY KEY)") + conn.execute("INSERT INTO sessions(id) VALUES (?)", (sid,)) + conn.commit() + finally: + conn.close() + + +WORKER_SID = "20260927_000001_worker" + + +@pytest.fixture +def worker_session(board, monkeypatch): + """A session that belongs to profile 'daedalus', whose CLI resolves its + env profile as 'default' after repointing the home at the root board.""" + _session_db(board / "profiles" / "daedalus" / "state.db", WORKER_SID) + monkeypatch.setattr(kb, "_caller_session_lineage", lambda sid: ()) + return WORKER_SID + + +def _foreign_card(conn, assignee): + tid = kb.create_task(conn, title="foreign", assignee=assignee, + session_id="20260927_999999_home") + assert kb.block_task(conn, tid, reason="needs input") + return tid + + +def test_root_repoint_default_profile_cannot_mutate_default_card(worker_session): + with kb.connect_closing() as conn: + tid = _foreign_card(conn, "default") + with kb.mutation_actor(session_ids=(worker_session,), profile="default"): + with pytest.raises(kb.ForeignSessionMutationError): + kb.unblock_task(conn, tid) + assert kb.get_task(conn, tid).status == "blocked" + + +def test_root_repoint_default_profile_cannot_use_operator(worker_session): + with kb.connect_closing() as conn: + tid = _foreign_card(conn, "builder") + with kb.mutation_actor(session_ids=(worker_session,), profile="default", + operator="Ace via Apollo: ruled"): + with pytest.raises(kb.ForeignSessionMutationError) as exc: + kb.unblock_task(conn, tid) + assert "daedalus" in str(exc.value) + assert kb.get_task(conn, tid).status == "blocked" + + +def test_session_owner_profile_is_still_the_assignee(worker_session): + """Control: the session's real profile keeps assignee authority.""" + with kb.connect_closing() as conn: + tid = _foreign_card(conn, "daedalus") + with kb.mutation_actor(session_ids=(worker_session,), profile="default"): + assert kb.unblock_task(conn, tid) + + +def test_env_profile_used_when_no_session_owner_found(board, monkeypatch): + """Control: an unknown session (no state.db row) falls back to the env + profile exactly as before.""" + monkeypatch.setattr(kb, "_caller_session_lineage", lambda sid: ()) + with kb.connect_closing() as conn: + tid = _foreign_card(conn, "worker-a") + with kb.mutation_actor(session_ids=("20260927_000002_unknown",), + profile="worker-a"): + assert kb.unblock_task(conn, tid) + + +# --- #999 (dashboard): a claimed review run leaves only via a verdict ----- + + +def _claimed_review(conn): + tid = kb.create_task(conn, title="review me", assignee="builder") + impl = kb.claim_task(conn, tid, claimer="builder:1") + assert impl is not None + assert kb.request_review(conn, tid, summary="ready", reviewer="argus", + expected_run_id=impl.current_run_id, force=True) + assert kb.claim_review_task(conn, tid, claimer="reviewer:1") is not None + assert kb.get_task(conn, tid).status == "running" + return tid + + +@pytest.fixture +def dashboard(board, monkeypatch): + from plugins.kanban.dashboard import plugin_api as api + + calls = [] + + def _killed(pid, lock, **_kw): + calls.append(pid) + return {"prev_pid": pid, "prev_lock": lock, "host_local": True, + "termination_attempted": True, "terminated": True, "sigkill": False} + + monkeypatch.setattr(kb, "_terminate_reclaimed_worker", _killed) + return api, calls + + +@pytest.mark.parametrize("target", ["todo", "triage", "scheduled"]) +def test_dashboard_cannot_park_a_claimed_review_run(dashboard, target): + api, calls = dashboard + with kb.connect_closing() as conn: + tid = _claimed_review(conn) + assert api._set_status_direct(conn, tid, target) is False + task = kb.get_task(conn, tid) + assert task.status == "running" + assert calls == [] # refused before the reviewer worker is touched + + +def test_dashboard_ready_on_claimed_review_resumes_review(dashboard): + """Control: running -> ready still returns the card to review.""" + api, _calls = dashboard + with kb.connect_closing() as conn: + tid = _claimed_review(conn) + assert api._set_status_direct(conn, tid, "ready") is True + assert kb.get_task(conn, tid).status == "review" + + +def test_dashboard_can_still_park_an_implementer_run(dashboard): + """Control: a normal (non-review) running card can still be moved.""" + api, _calls = dashboard + with kb.connect_closing() as conn: + tid = kb.create_task(conn, title="impl", assignee="builder") + assert kb.claim_task(conn, tid, claimer="builder:1") is not None + assert api._set_status_direct(conn, tid, "todo") is True + assert kb.get_task(conn, tid).status == "todo" + + +# --- #951: a sessionless gateway slash command borrows no identity -------- + + +def test_gateway_sessionless_caller_does_not_inherit_env_session(monkeypatch): + from hermes_cli import kanban as kc + + monkeypatch.setenv("_HERMES_GATEWAY", "1") + monkeypatch.setenv("HERMES_SESSION_ID", "20260927_000003_other_chat") + # A fresh context: no per-turn session bound, no explicit slash session. + assert contextvars.Context().run(kc._caller_session_id) is None + + +def test_gateway_bound_session_and_cli_env_still_resolve(monkeypatch): + """Controls: the bound per-turn session wins in the gateway; outside the + gateway the process env is the caller's own.""" + from gateway import session_context as sc + from hermes_cli import kanban as kc + + monkeypatch.setenv("HERMES_SESSION_ID", "20260927_000004_env") + + def bound(): + sc._SESSION_ID.set("20260927_000005_mine") + return kc._caller_session_id() + + monkeypatch.setenv("_HERMES_GATEWAY", "1") + assert contextvars.Context().run(bound) == "20260927_000005_mine" + monkeypatch.delenv("_HERMES_GATEWAY") + assert contextvars.Context().run(kc._caller_session_id) == "20260927_000004_env" + + +# --- #960: a goal-mode worker's CLI child is not an operator --------------- + + +def _raising_judge(**_kw): + raise RuntimeError("judge transport down") + + +def _gate(task_id): + from hermes_cli import goals + + task = SimpleNamespace(goal_mode=True, title="goal card", body="") + return goals.kanban_handoff_rejection( + task, "evidence", conn=None, task_id=task_id, + worker_run_id_for=lambda _tid: None, # CLI child: never the run owner + judge_available=lambda: True, judge=_raising_judge, + ) + + +def test_goal_worker_cli_child_judge_error_fails_closed(monkeypatch): + monkeypatch.setenv("HERMES_KANBAN_TASK", "t_goalcard") + reason = _gate("t_goalcard") + assert reason and "judge error" in reason and "refused" in reason + + +def test_operator_judge_error_still_fails_open(monkeypatch): + """Control: a real operator (no inherited worker env for this card).""" + monkeypatch.delenv("HERMES_KANBAN_TASK", raising=False) + assert _gate("t_goalcard") is None + monkeypatch.setenv("HERMES_KANBAN_TASK", "t_some_other_card") + assert _gate("t_goalcard") is None diff --git a/tests/hermes_cli/test_kanban_pr_freshness.py b/tests/hermes_cli/test_kanban_pr_freshness.py index b12f631a2bac1..69b7362e5cde5 100644 --- a/tests/hermes_cli/test_kanban_pr_freshness.py +++ b/tests/hermes_cli/test_kanban_pr_freshness.py @@ -195,7 +195,25 @@ def test_e2e_green_slice_armed_milestone_not(board, monkeypatch): _use_gh(monkeypatch, FakeGh(), armed) with kb.connect() as conn: tid, run = _claimed(conn) - assert kb.complete_task(conn, tid, summary=f"done {PR_URL}", expected_run_id=run) + assert kb.complete_task(conn, tid, summary=f"done {PR_URL}", + metadata={"pr_url": PR_URL}, expected_run_id=run) mid, mrun = _claimed(conn, title="[milestone] big thing") - assert kb.complete_task(conn, mid, summary=f"done {PR_URL}", expected_run_id=mrun) + assert kb.complete_task(conn, mid, summary=f"done {PR_URL}", + metadata={"pr_url": PR_URL}, expected_run_id=mrun) assert armed == [(REPO, 5, HEAD, tid)] + + +def test_e2e_pr_only_mentioned_in_prose_is_routed_but_never_armed(board, monkeypatch): + """FleetReview #1234: a green fleet PR named only in summary prose (context, + not this card's handoff) routes the card to review but is NOT handed to + fleet-merge.sh. Only metadata.pr_url / --survivor-pr authorize arming.""" + armed = [] + _use_gh(monkeypatch, FakeGh(), armed) + with kb.connect() as conn: + tid, run = _claimed(conn) + assert kb.complete_task(conn, tid, summary=f"background: see {PR_URL}", + expected_run_id=run) + assert _status(conn, tid) == "review" + meta = kb.latest_run(conn, tid).metadata + assert "not armed" in meta["handoff_freshness"]["prs"]["o/r#5"]["automerge"] + assert armed == [] diff --git a/tests/hermes_cli/test_kanban_reclaim_unprovable_liveness.py b/tests/hermes_cli/test_kanban_reclaim_unprovable_liveness.py index 4fb88da09a7be..88426aeda1294 100644 --- a/tests/hermes_cli/test_kanban_reclaim_unprovable_liveness.py +++ b/tests/hermes_cli/test_kanban_reclaim_unprovable_liveness.py @@ -513,7 +513,12 @@ def test_reclaim_requeues_when_termination_actually_succeeded(conn, monkeypatch) """ import signal as _signal - tid, _lock, _run_id = _running_card(conn, worker_pid=4242) + tid, _lock, run_id = _running_card(conn, worker_pid=4242) + # The dispatcher's spawn record: termination signals only a worker whose + # identity is bounded by it (FleetReview #1021 -- a claim lower bound + # alone cannot tell the worker from a process that reused its PID). + kb._append_event(conn, tid, "spawned", {"pid": 4242}, run_id=run_id) + conn.commit() state = {"alive": True} signals: list[int] = [] diff --git a/tests/hermes_cli/test_kanban_review_coverage_gate.py b/tests/hermes_cli/test_kanban_review_coverage_gate.py index 5bd622bf3c58a..98bdca98619d2 100644 --- a/tests/hermes_cli/test_kanban_review_coverage_gate.py +++ b/tests/hermes_cli/test_kanban_review_coverage_gate.py @@ -271,6 +271,9 @@ def test_na_applicability_reason_accepted(review, reason): 'n/a: no mutation tool available here', 'n/a: mutmut missing on host', 'n/a: budget exhausted', 'n/a: c\u200bould n\u200bot run', 'n/a: \uff43ould not run', # fullwidth 'c' folds under NFKC + # FleetReview #999: the "can't" contraction (ASCII and curly apostrophe). + "n/a: can't run mutation tests", 'n/a: can’t run mutation tests', + 'n/a: we couldn’t run it', 'n/a: didn’t run the suite', # Deliberately conservative: an inability WORD anywhere is refused, so an # applicability claim must be phrased without it ("do not differ"). 'n/a: no provider code touched, so vendors cannot differ', diff --git a/tests/hermes_cli/test_kanban_review_sendback.py b/tests/hermes_cli/test_kanban_review_sendback.py index 48b1e8b343625..0398dc4bd84bc 100644 --- a/tests/hermes_cli/test_kanban_review_sendback.py +++ b/tests/hermes_cli/test_kanban_review_sendback.py @@ -308,7 +308,16 @@ def test_cli_parked_send_back_refused_without_bindable_session( assert len(_kinds(conn, tid)) == before -def test_tool_request_changes_sends_back_parked_review(board: Path) -> None: +@pytest.fixture +def chat_session(monkeypatch: pytest.MonkeyPatch) -> str: + """A bindable chat session: the only caller that may open a human-lane + review run from the tool (FleetReview #1081, same rule as the CLI).""" + sid = "20260926_070000_chat" + monkeypatch.setenv("HERMES_SESSION_ID", sid) + return sid + + +def test_tool_request_changes_sends_back_parked_review(board: Path, chat_session) -> None: from tools import kanban_tools as tools with kb.connect() as conn: @@ -325,7 +334,7 @@ def test_tool_request_changes_sends_back_parked_review(board: Path) -> None: def test_tool_send_back_from_gateway_session_attributes_active_profile( - board: Path, monkeypatch: pytest.MonkeyPatch, + board: Path, monkeypatch: pytest.MonkeyPatch, chat_session, ) -> None: """An orchestrator in a gateway session has no HERMES_PROFILE (only dispatched workers do): the review claim and coverage comment name the active profile.""" @@ -345,7 +354,7 @@ def test_tool_send_back_from_gateway_session_attributes_active_profile( _assert_sent_back(conn, tid, "default") -def test_tool_send_back_without_coverage_is_refused(board: Path) -> None: +def test_tool_send_back_without_coverage_is_refused(board: Path, chat_session) -> None: from tools import kanban_tools as tools with kb.connect() as conn: @@ -359,6 +368,31 @@ def test_tool_send_back_without_coverage_is_refused(board: Path) -> None: _assert_untouched(conn, tid, before, runs_before) +@pytest.mark.parametrize("session", [None, "cron_abc123_20260926_070000"]) +def test_tool_parked_send_back_refused_without_bindable_session( + board: Path, monkeypatch: pytest.MonkeyPatch, session, +) -> None: + """FleetReview #1081: the tool path had no session check, so a sessionless + or cron caller could open and close a human-lane review run the CLI + refuses. Now refused identically, before any write.""" + from tools import kanban_tools as tools + + if session is None: + monkeypatch.delenv("HERMES_SESSION_ID", raising=False) + else: + monkeypatch.setenv("HERMES_SESSION_ID", session) + with kb.connect() as conn: + tid = _parked_review(conn) + before, runs_before = _snapshot(conn, tid) + out = json.loads(tools._handle_request_changes({ + "task_id": tid, "reason": "add the boundary test", + "coverage": json.loads(COVERAGE), + })) + assert "human-lane review claim" in out["error"] + with kb.connect() as conn: + _assert_untouched(conn, tid, before, runs_before) + + def test_dashboard_request_changes_route_sends_back_parked_review(board: Path) -> None: pytest.importorskip("fastapi") from fastapi import FastAPI diff --git a/tests/hermes_cli/test_kanban_second_claim_class.py b/tests/hermes_cli/test_kanban_second_claim_class.py index 37192f747c8e5..ebd6e2b25e371 100644 --- a/tests/hermes_cli/test_kanban_second_claim_class.py +++ b/tests/hermes_cli/test_kanban_second_claim_class.py @@ -255,12 +255,26 @@ def test_process_start_window_distinguishes_reused_pid(conn, monkeypatch): def test_expired_bounded_run_does_not_probe_live_pid(conn, monkeypatch): tid, _ = _live_claim(conn, "bounded old run") _external_release(conn, tid) - conn.execute("UPDATE tasks SET max_runtime_seconds=60 WHERE id=?", (tid,)) - conn.execute("UPDATE task_runs SET ended_at=? WHERE task_id=?", (time.time() - 600, tid)) + # The RUN was bounded when it ran (its claim-time snapshot). + conn.execute("UPDATE task_runs SET max_runtime_seconds=60, ended_at=? WHERE task_id=?", + (time.time() - 600, tid)) monkeypatch.setattr(kb, "_pid_alive", lambda _pid: True) assert kb.claim_task(conn, tid) is not None +def test_cap_shortened_after_release_still_probes_live_owner(conn, monkeypatch): + """FleetReview #956: the card's CURRENT cap is not the released run's cap. + An operator shortening max_runtime_seconds after release must not skip the + live-owner probe for a worker that was claimed without that bound.""" + tid, _ = _live_claim(conn, "cap shortened later") + _external_release(conn, tid) + conn.execute("UPDATE task_runs SET ended_at=? WHERE task_id=?", (time.time() - 600, tid)) + conn.execute("UPDATE tasks SET max_runtime_seconds=60 WHERE id=?", (tid,)) + monkeypatch.setattr(kb, "_pid_alive", lambda pid: pid == 424242) + assert kb.claim_task(conn, tid) is None + assert _events(conn, tid, "claim_rejected")[-1]["reason"] == "prior_worker_still_alive" + + def test_real_process_survives_operator_block_without_second_spawn(conn, monkeypatch): monkeypatch.setattr(kb, "_pid_started_in_claim", kb._real_pid_started_in_claim) diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 7a38719a038c3..7bbaf5553135f 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -1156,17 +1156,36 @@ def _handle_request_changes(args: dict, **kw) -> str: try: kb, conn = _connect(board=board) try: + worker_run = _worker_run_id(tid) + parked_session = None + if worker_run is None: + task = kb.get_task(conn, tid) + if task is not None and task.status == "review": + # Same provenance rule as the CLI (``_cmd_request_changes``): + # only a session that could hold a human-lane review claim + # may open one. Sessionless callers, cron jobs and + # delegate children are refused (FleetReview #1081). + from hermes_cli.kanban import _operator_review_session_ref + + parked_session = _operator_review_session_ref() + if parked_session is None: + return tool_error( + f"could not request changes for {tid}: this caller cannot " + f"hold a human-lane review claim (no bindable chat " + f"session; cron jobs and delegate children are refused)" + ) ok, detail = kb.request_changes( conn, tid, reason=reason, - expected_run_id=_worker_run_id(tid), + expected_run_id=worker_run, # A non-worker reviewer (human-lane orchestrator) on a parked # review card opens the review run as itself, atomically. # Gateway sessions carry no worker marker: fall back to the # active profile, never a literal that misattributes the verdict. claimer=_caller_profile() or "reviewer", coverage=coverage, + session_ref=parked_session, ) if not ok: return tool_error( From 66dfd3e2f4da6f48ddce4a807521475a588191e6 Mon Sep 17 00:00:00 2001 From: "ang-fleet-workers[bot]" <333956806+ang-fleet-workers[bot]@users.noreply.github.com> Date: Sun, 27 Sep 2026 15:21:49 -0700 Subject: [PATCH 2/2] fix(kanban): #1021 rule lives in the identity probe, not after the test seam Missing spawned upper bound now makes _real_pid_started_in_claim answer None (unverified) instead of True: termination holds, liveness still fails closed to alive. Moving it out of _terminate_reclaimed_worker keeps the test seam authoritative (CI slice 2: 3 fixtures with stubbed identity). progress_stall fixtures now record the pid via _set_worker_pid (spawned event), as the dispatcher does; the reclaim_unprovable fixture edit is reverted. Verified: progress_stall, core_functionality, c4_board_gates, second_claim, reclaim_unprovable, termination_identity, kanban_db: 339 passed; the 2 failures also fail on unmodified base (worker-env CLI tests). --- hermes_cli/kanban_db.py | 25 ++++++++----------- .../hermes_cli/test_kanban_c4_board_gates.py | 3 +++ .../hermes_cli/test_kanban_progress_stall.py | 6 +++-- ...test_kanban_reclaim_unprovable_liveness.py | 7 +----- 4 files changed, 18 insertions(+), 23 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 59e56a9c71188..db35d009fa550 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -8574,8 +8574,9 @@ def _real_pid_started_in_claim(pid, claimed_at, spawned_at, window ``[claimed_at - 1 s, spawned_at + 2 s]``. ``False``: provably not the recorded worker. ``None``: the needed reading - is unreadable. A missing bound is simply not applied, so missing evidence - never proves a PID recycled. + is unreadable, or the ``spawned`` upper bound is missing (identity + unproven). Missing evidence never proves a PID recycled, and never proves + it is the worker either. """ if start_token is not None: try: @@ -8594,6 +8595,13 @@ def _real_pid_started_in_claim(pid, claimed_at, spawned_at, return False if spawned_at is not None and created > float(spawned_at) + _OWNER_CREATE_LAG_SECONDS: return False + if spawned_at is None: + # Only the claim lower bound is known (no run-scoped ``spawned`` + # evidence: legacy/migrated runs). Any process created after the + # claim -- including one that reused the worker's PID -- fits, so + # this is UNPROVEN, not proven: termination never signals it, while + # every liveness caller still treats it as alive (FleetReview #1021). + return None return True @@ -16847,19 +16855,6 @@ def _terminate_reclaimed_worker( if _pid_alive(pid): identity = _owner_identity(int(pid), *owner_window) - if ( - identity == "verified" - and len(owner_window) >= 3 - and owner_window[1] is None - and owner_window[2] is None - ): - # No run-scoped ``spawned`` evidence (legacy/migrated run): only - # the claim lower bound was applied, so ANY process created after - # the claim -- including one that reused the worker's PID -- - # reads "verified". That is unproven, not proven: never SIGTERM - # on it (FleetReview #1021). Liveness callers still treat it as - # alive (fail closed), so this only withholds the signal. - identity = "unverified" info["owner_identity"] = identity if identity == "recycled": # The recorded worker is gone; this PID now belongs to someone diff --git a/tests/hermes_cli/test_kanban_c4_board_gates.py b/tests/hermes_cli/test_kanban_c4_board_gates.py index 32e26f883b04e..63396334875d9 100644 --- a/tests/hermes_cli/test_kanban_c4_board_gates.py +++ b/tests/hermes_cli/test_kanban_c4_board_gates.py @@ -45,6 +45,9 @@ def board(tmp_path, monkeypatch): def _terminate(monkeypatch, owner_window): sent = [] monkeypatch.setattr(kb, "_pid_alive", lambda pid: True) + # The real identity probe (conftest otherwise lets a stubbed _pid_alive + # vouch for identity): the PID is this live test process. + monkeypatch.setattr(kb, "_pid_started_in_claim", kb._real_pid_started_in_claim) monkeypatch.setattr(kb.time, "sleep", lambda _s: None) host = kb._claimer_id().split(":", 1)[0] info = kb._terminate_reclaimed_worker( diff --git a/tests/hermes_cli/test_kanban_progress_stall.py b/tests/hermes_cli/test_kanban_progress_stall.py index e5cc169c62cdc..b914b2bee0ebe 100644 --- a/tests/hermes_cli/test_kanban_progress_stall.py +++ b/tests/hermes_cli/test_kanban_progress_stall.py @@ -102,7 +102,9 @@ def _in_flight_worker(server, fake_cpu=None): def _running(board, now, pid, *, progress_at, started_ago=1800): tid = kb.create_task(board, title="waiting on capped pool", assignee="argus") task = kb.claim_task(board, tid) - board.execute("UPDATE tasks SET worker_pid=? WHERE id=?", (pid, tid)) + # Record the pid the way the dispatcher does (with its ``spawned`` event): + # termination signals only a worker bounded by that record (#1021). + assert kb._set_worker_pid(board, tid, pid, run_id=task.current_run_id) board.execute("UPDATE task_runs SET started_at=? WHERE id=?", (now - started_ago, task.current_run_id)) board.commit() if progress_at is not None: @@ -150,7 +152,7 @@ def test_run_7914_shape_stalls_at_15_reclaims_at_25_escalates_after_two(board, m again = _in_flight_worker(silent_server, fake_cpu) claimed = kb.claim_task(board, tid) board.execute("UPDATE task_runs SET started_at=? WHERE id=?", (now - 1500, claimed.current_run_id)) - board.execute("UPDATE tasks SET worker_pid=? WHERE id=?", (again.pid, tid)) + assert kb._set_worker_pid(board, tid, again.pid, run_id=claimed.current_run_id) # The faked clock has advanced 600 s past the REAL one, so claim_task # stamped its ``claimed`` event in the future of this worker's real # creation. The owner-identity window (t_0ae83825) would then read the diff --git a/tests/hermes_cli/test_kanban_reclaim_unprovable_liveness.py b/tests/hermes_cli/test_kanban_reclaim_unprovable_liveness.py index 88426aeda1294..4fb88da09a7be 100644 --- a/tests/hermes_cli/test_kanban_reclaim_unprovable_liveness.py +++ b/tests/hermes_cli/test_kanban_reclaim_unprovable_liveness.py @@ -513,12 +513,7 @@ def test_reclaim_requeues_when_termination_actually_succeeded(conn, monkeypatch) """ import signal as _signal - tid, _lock, run_id = _running_card(conn, worker_pid=4242) - # The dispatcher's spawn record: termination signals only a worker whose - # identity is bounded by it (FleetReview #1021 -- a claim lower bound - # alone cannot tell the worker from a process that reused its PID). - kb._append_event(conn, tid, "spawned", {"pid": 4242}, run_id=run_id) - conn.commit() + tid, _lock, _run_id = _running_card(conn, worker_pid=4242) state = {"alive": True} signals: list[int] = []