diff --git a/cron/bot_chat_delivery.py b/cron/bot_chat_delivery.py index 458a6d3957..5d38f96f24 100644 --- a/cron/bot_chat_delivery.py +++ b/cron/bot_chat_delivery.py @@ -92,7 +92,12 @@ def _drain(root: Path) -> None: atomic_json_write(path, record, fsync_dir=True, mode=0o600) job = record["job"] job.pop("_bot_chat_delivery_receipts", None) - error = _deliver_to_bot_chat(job, record["content"], record["profile"], deferred=record) + try: + error = _deliver_to_bot_chat(job, record["content"], record["profile"], deferred=record) + except Exception as exc: + # The claim survives uncertainty; one failed attempt must not stop peers. + error = f"{type(exc).__name__}: {exc}" + logger.exception("Deferred Bot Chat delivery %s failed", record["id"]) receipt = job.get("_bot_chat_delivery_receipts", {}).get( f"bot-chat:{record['profile'] or '(own)'}") status = "transferred" if receipt else "ambiguous" if error else "settled" diff --git a/evals/botmode-dm-delivery/README.md b/evals/botmode-dm-delivery/README.md index 1f2679a8df..7d4b9bbdb8 100644 --- a/evals/botmode-dm-delivery/README.md +++ b/evals/botmode-dm-delivery/README.md @@ -59,6 +59,23 @@ was not created, so `--in ~` correctly refused); that failed receipt is retained in `native-green.log`, and both source legs were rerun with an existing HOME. Prior `/tmp/botmode-dm-recovery*` evidence remains untouched. +## Per-record delivery exception isolation + +Set `BOT_DM_EXCEPTION=1` and select `-g "cron output"` for the controlled native +exception probe. `exception-tick.py` raises `PermissionError` at the actual +post-discovery target `Path.is_dir()` boundary, not at the delivery helper. +The same exception was first reproduced with real directory traversal permission +loss on Python 3.11/Linux; the retained test uses a portable controlled fault. +Before the guard, native tick exits 1, leaving the head claimed and sibling queued. +Afterward, the head is ambiguous, the sibling settles and renders once, and a +second real tick replays neither. Logs: `/tmp/botmode-dm-exception-{red,green}.log`; +receipts and screenshot: `/tmp/botmode-dm-exception/{red,green}/`. + +The review's repeated-head starvation claim is not reachable: the claim commits +before delivery and later scans skip every non-queued record. One failed tick is +real; recurring replay of that same head is not. Indefinite queued/payload retention +is intentional, with no TTL or automatic ambiguous retry introduced here. + ## Ordinary custom-root fallback (#104066 / #104055) `probe-cron-root.spec.ts` adds the never-deferred sibling: copy it to diff --git a/evals/botmode-dm-delivery/exception-tick.py b/evals/botmode-dm-delivery/exception-tick.py new file mode 100644 index 0000000000..c0482dac9d --- /dev/null +++ b/evals/botmode-dm-delivery/exception-tick.py @@ -0,0 +1,41 @@ +"""Controlled filesystem fault; run only against the native disposable sandbox.""" +import json +import os +from pathlib import Path + +from cron import scheduler_delivery as delivery +from cron.bot_chat_delivery import _root +from cron.scheduler import tick + +root = _root() +original_which = delivery.shutil.which +original_is_dir = Path.is_dir +armed = False +raised = 0 + + +def resolve_cli(*args, **kwargs): + global armed + armed = raised == 0 + return original_which(*args, **kwargs) + + +def is_dir(path): + global armed, raised + if armed and path == Path(os.environ["HERMES_HOME"]) / "profiles" / "beta": + armed = False + raised += 1 + raise PermissionError("controlled target traversal denied after discovery") + return original_is_dir(path) + + +delivery.shutil.which = resolve_cli +Path.is_dir = is_dir +try: + tick(verbose=False) + tick(verbose=False) +finally: + Path.is_dir = original_is_dir + delivery.shutil.which = original_which + records = [json.loads(p.read_text(encoding="utf-8")) for p in root.glob("*.json") if p.name != "broken.json"] + Path(os.environ["BOT_DM_EXCEPTION_RECEIPT"]).write_text(json.dumps({"raised": raised, "records": records}, indent=2), encoding="utf-8") diff --git a/evals/botmode-dm-delivery/probe-dm-delivery.spec.ts b/evals/botmode-dm-delivery/probe-dm-delivery.spec.ts index 7ce033821d..6b57e5a027 100644 --- a/evals/botmode-dm-delivery/probe-dm-delivery.spec.ts +++ b/evals/botmode-dm-delivery/probe-dm-delivery.spec.ts @@ -49,15 +49,26 @@ test('cron output waits for a CLI-only owner and arrives after owner release', a try { await fixture.mock.waitForHeldCompletion() const script = 'import json; from cron.scheduler_delivery import _deliver_to_bot_chat; j={"id":"cli-residual","name":"CLI residual","execution_id":"fixed-execution"}; result=_deliver_to_bot_chat(j,"CLI_OWNER_CRON_SENTINEL","beta"); print(json.dumps({"result":result,"job":j}))' - const result = JSON.parse(execFileSync(python, ['-c', script], { env: cronEnv, cwd: repo, encoding: 'utf8', timeout: 30_000 })) + const setup = process.env.BOT_DM_EXCEPTION ? 'from pathlib import Path; from cron.bot_chat_delivery import defer; from hermes_constants import get_hermes_home; defer("e"*64,{"id":"exception-head"},"EXCEPTION_MUST_NOT_RUN","beta",get_hermes_home()/"profiles"/"beta"); ' : '' + const result = JSON.parse(execFileSync(python, ['-c', setup + script], { env: cronEnv, cwd: repo, encoding: 'utf8', timeout: 30_000 })) console.log('CLI_OWNER_CRON_ADMISSION', JSON.stringify(result)) fs.writeFileSync(path.join(evidence, 'cli-owner-admission.json'), JSON.stringify(result, null, 2)) if (process.env.BOT_DM_CORRUPT) fs.writeFileSync(path.join(fixture.sandbox.hermesHome, 'cron', 'bot_chat_pending', 'broken.json'), '{') fixture.mock.releaseHeldStream() await expect.poll(() => child.exitCode, { timeout: 60_000 }).toBe(0) fs.mkdirSync(path.join(fixture.sandbox.root, 'changed-launch-home'), { recursive: true }) - const ticker = spawn(python, ['-c', 'import time; from cron.scheduler import tick; from cron.bot_chat_delivery import _running; tick(verbose=False);\nwhile _running: time.sleep(0.1)'], { env: { ...cronEnv, HOME: path.join(fixture.sandbox.root, 'changed-launch-home') }, cwd: repo, stdio: ['ignore', output, output] }) - await expect.poll(() => ticker.exitCode, { timeout: 90_000 }).toBe(0) + const tickArgs = process.env.BOT_DM_EXCEPTION ? [path.join(repo, 'evals/botmode-dm-delivery/exception-tick.py')] : ['-c', 'import time; from cron.scheduler import tick; from cron.bot_chat_delivery import _running; tick(verbose=False);\nwhile _running: time.sleep(0.1)'] + const receiptPath = path.join(evidence, 'exception-receipts.json') + const ticker = spawn(python, tickArgs, { env: { ...cronEnv, HOME: path.join(fixture.sandbox.root, 'changed-launch-home'), BOT_DM_EXCEPTION_RECEIPT: receiptPath }, cwd: repo, stdio: ['ignore', output, output] }) + await expect.poll(() => ticker.exitCode, { timeout: 90_000 }).not.toBeNull() + expect(ticker.exitCode).toBe(0) + if (process.env.BOT_DM_EXCEPTION) { + const receipts = JSON.parse(fs.readFileSync(receiptPath, 'utf8')) + expect(receipts.raised).toBe(1) + expect(receipts.records.find((r: { id: string }) => r.id === 'e'.repeat(64)).status).toBe('ambiguous') + expect(receipts.records.find((r: { id: string }) => r.id !== 'e'.repeat(64)).status).toBe('settled') + expect(dbMessages('beta').filter(([, text]) => text.includes('EXCEPTION_MUST_NOT_RUN'))).toHaveLength(0) + } await openBot('beta') expect(dbMessages('beta').filter(([role, text]) => role === 'user' && text.includes('CLI_OWNER_CRON_SENTINEL'))).toHaveLength(1) await expect(fixture.page.getByText(/CLI_OWNER_CRON_SENTINEL/).filter({ visible: true }).first()).toBeVisible({ timeout: 45_000 }) diff --git a/tests/cron/test_bot_chat_pending.py b/tests/cron/test_bot_chat_pending.py index ad811451fa..4cf3d751a0 100644 --- a/tests/cron/test_bot_chat_pending.py +++ b/tests/cron/test_bot_chat_pending.py @@ -1,5 +1,6 @@ """Only never-started cron delivery may wait for a CLI owner's release.""" import subprocess +from pathlib import Path from unittest.mock import Mock import pytest @@ -40,6 +41,47 @@ def test_cli_owner_deferral_and_attempt_fence(tmp_path, monkeypatch, error): db.close() +def test_delivery_exception_retains_attempt_and_continues_siblings(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + blocked_parent = tmp_path / "blocked" + blocked_home = blocked_parent / "recipient" + blocked_home.mkdir(parents=True) + for home in (blocked_home, tmp_path): + db = SessionDB(db_path=home / "state.db") + db.create_session(session_id="chat", source="cli") + db.set_session_title("chat", "Bot Chat") + db.close() + queue.defer("b" * 64, {"id": "bad"}, "bad output", "", blocked_home) + queue.defer("a" * 64, {"id": "good"}, "good output", "", tmp_path) + calls = [] + original_is_dir = Path.is_dir + armed = False + + def resolve_cli(_): + nonlocal armed + armed = True + return "/bin/hermes" + + def is_dir(self): + if armed and self == blocked_home: + raise PermissionError("target traversal denied after discovery") + return original_is_dir(self) + + def run(*args, **kwargs): + calls.append(kwargs["env"]["HERMES_HOME"]) + return subprocess.CompletedProcess([], 0, "", "") + + monkeypatch.setattr(delivery.shutil, "which", resolve_cli) + monkeypatch.setattr(delivery.subprocess, "run", run) + monkeypatch.setattr(Path, "is_dir", is_dir) + queue.drain() + assert queue.read_pending("b" * 64)["status"] == "ambiguous" + assert "PermissionError" in queue.read_pending("b" * 64)["error"] + assert queue.read_pending("a" * 64)["status"] == "settled" + queue.drain() + assert calls == [str(tmp_path)] + + def test_pending_queue_uses_admission_order_and_keeps_claims(tmp_path, monkeypatch): monkeypatch.setenv("HERMES_HOME", str(tmp_path)) job = {"id": "job"} diff --git a/website/docs/user-guide/features/cron.md b/website/docs/user-guide/features/cron.md index b96d26016a..c9f691deb8 100644 --- a/website/docs/user-guide/features/cron.md +++ b/website/docs/user-guide/features/cron.md @@ -537,6 +537,7 @@ error. A delivery failure does not count toward the job's `failure_streak` - Each delivery costs the target bot one full agent turn — mind the schedule frequency. - Composes with other targets (`bot-chat,telegram`) but is never included in `all`. - If the canonical chat is open in a mailbox-capable Desktop/TUI backend, delivery is **durably queued immediately**, whether the bot is idle or busy. Only that live owner runs the incoming turn; cron does not start a competing CLI writer. If a CLI-only or older unsupported owner holds the chat, cron retains the never-started output under the sending profile's `cron/bot_chat_pending/.json`. Later scheduler ticks deliver after that owner releases the chat, in admission order. Deferred work retains its admitted destination home and receipt ID even if the scheduler's launch root changes; a missing/renamed destination is not recreated or resolved to another profile. A `transferred` pending record points to the live-owner receipt, not a failed turn. Malformed JSON records are retained and logged without blocking other queued outputs. With no owner, the existing `hermes chat -c "Bot Chat" --create-if-missing` lane remains available (normal session ownership checks still apply). That child uses the exact destination home already checked by cron, including custom roots; inherited `HOME` or a changed active profile cannot redirect it. A missing destination directory is refused before launch, not recreated. A deferred request is claimed before launching that lane; interruption or an uncertain subprocess result never causes an automatic resend. +- Never-started outputs have no TTL: if an unsupported owner never releases, they remain queued rather than being silently dropped. Receipts retain their payloads indefinitely. An unexpected delivery exception is logged and retained as `ambiguous`, without stopping sibling deliveries in that drain; claimed/ambiguous attempts are never automatically replayed. - **Queued is not completed.** Cron records receipt IDs and `queued`/`claimed` statuses in `last_delivery_queued`, with delivery outcome `queued` (neither delivered nor failed). A successful job shows `delivery_queued`; genuine errors on other targets still take precedence as delivery failures. The bot may complete later. The durable receipt in the target profile's `runtime/bot_live_delivery/.json` is authoritative; cron's historical status is not automatically refreshed. - Rechecking the same execution inspects its existing receipt, even if the owner has disappeared. It never falls back to another writer after acceptance. `failed`, `cancelled`, or `ambiguous` receipts are not automatically replayed; inspect the chat and receipt before intentionally starting new work. Each new cron execution has a distinct delivery ID.