fix(bot-mode): a pending command approval is not a failed DM delivery
terminal_tool's approval gate answers `status: pending_approval` with an EMPTY `error` (#28323) and no `session_id`, so _spawn_delivery's specific branch (`if parsed.get("error")`) was skipped and every unanswered approval fell through to "Delivery to X failed to start: no process id returned" — blaming the spawn for an approval nobody in a non-interactive turn (api_server, `hermes peer dm`, cron) could grant. - _spawn_delivery: the pending shape gets its own message (the runner command needs terminal approval nobody in this turn can grant); a local/peer DM adds "nothing was sent — approve it or add it to command_allowlist and send again". Ownership is never transferred, so the existing finally still reclaims the plaintext DM file. - _try_relay_delivery: the envelope is queued on disk BEFORE the reply waiter spawns and the Desktop drains it independently, so ANY waiter spawn failure is a lost wake-up, not a failed delivery; reporting it as an error made the sender resend and deliver the message twice. The relay path now returns the shape _start_delivery's live-owner branch already uses (status queued + notification_error + "Do NOT resend") instead of inventing a new status value nothing reads. Slimmer redo of #92971 by @jonpol01 (same diagnosis, same relay/local split on `dm_file is None`); the source-text contract test and the `sent_no_reply_wake` status were dropped. Fixes #111716 Co-authored-by: John Paul Soliva <soliva.johnpaul@icloud.com>
This commit is contained in:
@@ -976,3 +976,48 @@ def test_settled_live_wait_unlinks_the_intent_but_a_pending_one_keeps_it(tmp_pat
|
|||||||
assert bot_mode_dm._wait_live_dm(str(tmp_path), "d1", dm_file=dm_file) == 0
|
assert bot_mode_dm._wait_live_dm(str(tmp_path), "d1", dm_file=dm_file) == 0
|
||||||
assert not intent.exists()
|
assert not intent.exists()
|
||||||
assert not dm_file.exists(), "the dm .txt holds the same plaintext as the settled intent"
|
assert not dm_file.exists(), "the dm .txt holds the same plaintext as the settled intent"
|
||||||
|
|
||||||
|
|
||||||
|
def test_pending_approval_spawn_names_the_approval_and_reclaims_the_dm_file(tmp_path, monkeypatch):
|
||||||
|
"""terminal_tool's approval gate answers pending_approval with an EMPTY error and no session_id;
|
||||||
|
a local delivery must say the runner needs approval (nothing was sent), not blame the spawn."""
|
||||||
|
dm_file = tmp_path / "message.txt"
|
||||||
|
dm_file.write_text("secret", encoding="utf-8")
|
||||||
|
import tools.terminal_tool as terminal_tool_module
|
||||||
|
|
||||||
|
pending = terminal_tool_module._error_json("", status="pending_approval", approval_pending=True,
|
||||||
|
command="python3 runner", description="command flagged")
|
||||||
|
monkeypatch.setattr(terminal_tool_module, "terminal_tool", lambda command, **kwargs: pending)
|
||||||
|
|
||||||
|
result = json.loads(bot_mode_dm._spawn_delivery("unused", "@researcher", dm_file=str(dm_file),
|
||||||
|
task_id=None, agent=None))
|
||||||
|
|
||||||
|
assert "approval" in result["error"] and "nothing was sent" in result["error"]
|
||||||
|
assert "no process id" not in result["error"]
|
||||||
|
assert not dm_file.exists()
|
||||||
|
|
||||||
|
|
||||||
|
def test_relay_waiter_that_cannot_start_reports_queued_not_failed(tmp_path, monkeypatch):
|
||||||
|
"""The relay envelope is queued before the reply waiter spawns and the Desktop drains it on its
|
||||||
|
own: a waiter that cannot start is a lost wake-up, not a failed delivery (a hard error makes the
|
||||||
|
sender resend and deliver twice)."""
|
||||||
|
from tools import bot_relay
|
||||||
|
|
||||||
|
root = tmp_path / ".hermes"
|
||||||
|
(root / "profiles" / "default").mkdir(parents=True)
|
||||||
|
bot_relay.write_remote_roster(root, [{"profile": "researcher", "handle": "researcher",
|
||||||
|
"connection_id": "laptop-1", "connection_label": "laptop"}])
|
||||||
|
import tools.terminal_tool as terminal_tool_module
|
||||||
|
|
||||||
|
pending = terminal_tool_module._error_json("", status="pending_approval", approval_pending=True,
|
||||||
|
command="python3 waiter", description="command flagged")
|
||||||
|
monkeypatch.setattr(terminal_tool_module, "terminal_tool", lambda command, **kwargs: pending)
|
||||||
|
|
||||||
|
result = json.loads(bot_mode_dm._try_relay_delivery(root, "researcher", "hello", "default",
|
||||||
|
task_id=None, agent=None))
|
||||||
|
|
||||||
|
assert result["status"] == "queued"
|
||||||
|
assert "error" not in result
|
||||||
|
assert "Do NOT resend" in result["detail"]
|
||||||
|
assert "approval" in result["notification_error"]
|
||||||
|
assert list((bot_relay.relay_root(root) / bot_relay.OUTBOX_DIR).glob("*.json")), "envelope still queued"
|
||||||
|
|||||||
@@ -289,7 +289,19 @@ def _try_relay_delivery(root: Path, raw_target: str, content: str, me: str, *,
|
|||||||
# per the #93091 reason enum).
|
# per the #93091 reason enum).
|
||||||
return json.dumps({"error": str(exc), "reason": exc.reason})
|
return json.dumps({"error": str(exc), "reason": exc.reason})
|
||||||
label = f"@{match['handle']} on {match['connection_label'] or match['connection_id']}"
|
label = f"@{match['handle']} on {match['connection_label'] or match['connection_id']}"
|
||||||
return _spawn_delivery(waiter_command(root, envelope), label, task_id=task_id, agent=agent)
|
raw = _spawn_delivery(waiter_command(root, envelope), label, task_id=task_id, agent=agent)
|
||||||
|
waiter_error = json.loads(raw).get("error")
|
||||||
|
if not waiter_error:
|
||||||
|
return raw
|
||||||
|
# The envelope is already queued and the Desktop drains it on its own, so a waiter that
|
||||||
|
# failed to start loses only the reply wake-up. Reporting a hard failure here makes the
|
||||||
|
# sender resend and deliver the message twice. Same shape as the live-owner branch of
|
||||||
|
# _start_delivery: queued + notification_error.
|
||||||
|
return json.dumps({
|
||||||
|
"status": "queued", "to": label, "notification_error": waiter_error,
|
||||||
|
"detail": (f"Message queued for {label}; the relay delivers it on its own, but the reply "
|
||||||
|
"waiter did not start, so the reply will NOT wake you. Do NOT resend."),
|
||||||
|
})
|
||||||
except Exception:
|
except Exception:
|
||||||
logger.debug("relay delivery attempt failed", exc_info=True)
|
logger.debug("relay delivery attempt failed", exc_info=True)
|
||||||
return None
|
return None
|
||||||
@@ -602,6 +614,12 @@ def _spawn_delivery(command: str, label: str, *, dm_file: Optional[str] = None,
|
|||||||
proc_id = parsed.get("session_id") or ""
|
proc_id = parsed.get("session_id") or ""
|
||||||
if parsed.get("error"):
|
if parsed.get("error"):
|
||||||
return _err(f"Delivery to {label} failed to start: {parsed['error']}")
|
return _err(f"Delivery to {label} failed to start: {parsed['error']}")
|
||||||
|
if parsed.get("status") == "pending_approval":
|
||||||
|
# terminal_tool's approval gate answers with an EMPTY error and no session_id: the runner
|
||||||
|
# never launched because nobody in this turn could approve its command.
|
||||||
|
return _err(f"Delivery to {label} failed to start: its command needs terminal approval that nobody "
|
||||||
|
"in this turn can grant" + (", so nothing was sent. Approve it (or add it to "
|
||||||
|
"command_allowlist) and send again." if dm_file else "."))
|
||||||
if not proc_id:
|
if not proc_id:
|
||||||
return _err(f"Delivery to {label} failed to start: no process id returned")
|
return _err(f"Delivery to {label} failed to start: no process id returned")
|
||||||
# From here the background runner owns the file (removed after the consumer finishes).
|
# From here the background runner owns the file (removed after the consumer finishes).
|
||||||
|
|||||||
Reference in New Issue
Block a user