fix(cron): keep unowned Bot Chat delivery on its resolved home
Extend deferred dispatch's destination pin to ordinary CLI fallback, so custom-root and active-profile changes cannot redirect a checked target. Refuse a missing destination before launch and name the target on failure. Replace the old env-clearing expectation with two behavioral invariants and retain the native Electron custom-root reproduction. Adapted from the root-boundary fix and diagnosis in #104066. Related #104055, #104066. Co-authored-by: fangliquanflq <fangliquan@qq.com>
This commit is contained in:
@@ -756,18 +756,13 @@ def _deliver_to_bot_chat(job: dict, content: str, profile: str, *, deferred: Opt
|
||||
from agent.delegation_context import delegated_child_subprocess_env
|
||||
from tools.environments.local import strip_launch_profile_env
|
||||
env = strip_launch_profile_env(delegated_child_subprocess_env(os.environ))
|
||||
if deferred is not None:
|
||||
# Admission owns the destination, not the current profile-name resolver.
|
||||
env["HERMES_HOME"] = str(home)
|
||||
if home.parent.name != "profiles":
|
||||
argv += ["-p", "default"] # Ignore a subsequently changed active_profile.
|
||||
elif profile:
|
||||
argv += ["-p", profile]
|
||||
# -p owns profile resolution; this scheduler's HERMES_HOME must not shadow it.
|
||||
env.pop("HERMES_HOME", None)
|
||||
else:
|
||||
# Multiplex workers carry the profile in a ContextVar, not os.environ.
|
||||
env["HERMES_HOME"] = str(source_home)
|
||||
if not home.is_dir():
|
||||
return _fail(f"bot-chat delivery target no longer exists: {home}; do not resend")
|
||||
# Discovery (or deferred admission) owns the destination, not HOME or a
|
||||
# subsequently changed active_profile. Do not resolve the name a second time.
|
||||
env["HERMES_HOME"] = str(home)
|
||||
if home.parent.name != "profiles":
|
||||
argv += ["-p", "default"]
|
||||
|
||||
query_file = None
|
||||
try:
|
||||
@@ -787,7 +782,7 @@ def _deliver_to_bot_chat(job: dict, content: str, profile: str, *, deferred: Opt
|
||||
if result.returncode != 0:
|
||||
tail = (result.stderr or result.stdout or "").strip()[-500:]
|
||||
return _fail(
|
||||
f"bot-chat delivery to profile '{profile_label}' failed (exit {result.returncode})"
|
||||
f"bot-chat delivery to profile '{profile_label}' failed (exit {result.returncode}) at {home}"
|
||||
+ (f": {tail}" if tail else ""))
|
||||
logger.info("Job '%s': delivered to Bot Chat of profile '%s'", job_id, profile_label)
|
||||
return None
|
||||
|
||||
@@ -58,3 +58,33 @@ files. The first follow-up run also exposed a fixture mistake (the changed HOME
|
||||
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.
|
||||
|
||||
## Ordinary custom-root fallback (#104066 / #104055)
|
||||
|
||||
`probe-cron-root.spec.ts` adds the never-deferred sibling: copy it to
|
||||
`apps/desktop/e2e/` and run with the same native fixture (no mock-trigger patch
|
||||
needed for this case). It keeps default's real Desktop Bot Chat lease, submits
|
||||
ordinary cron output to unowned Alpha from a separate Python producer under a
|
||||
custom Hermes root, and holds the real quiet CLI child at loopback inference.
|
||||
The child shim PID must match Alpha's real CLI lease; default's lease is unchanged.
|
||||
After release, Alpha has exactly one input and Desktop renders the output.
|
||||
The same case removes unused Beta and verifies delivery neither recreates Beta nor
|
||||
creates a second `.hermes` root under HOME.
|
||||
|
||||
Both `origin/main`'s scheduler and pre-follow-up `c827ae179d67c` fail with
|
||||
`Profile 'alpha' does not exist` before any recipient turn. Fixed native run:
|
||||
**1 passed (46.3s)**. Two invariant tests exercise the actual CLI startup resolver
|
||||
across named/default/own destinations with a changed active profile, and refusal
|
||||
when the destination is missing initially or disappears during discovery:
|
||||
**5 failed before, 5 passed after**. The old env-clearing test is replaced by these
|
||||
behavior checks rather than retaining the broken expectation.
|
||||
|
||||
Evidence: `/tmp/botmode-cron-root/{before2,origin-main,after}.log`,
|
||||
`after/{owners.json,children.log,rows.json,result.json,missing.json,ordinary-recipient.png}`.
|
||||
The first fixture attempt (`before.log`) used the wrong default row label; the
|
||||
actual Desktop label is Hermes. No production failure is claimed for that attempt.
|
||||
Full cron directory: **1346 passed, 1 skipped across 116 files**; sibling mailbox,
|
||||
DM, gateway consumer and profile tests: **124 passed, 3 skipped across 4 files**.
|
||||
Credit @fangliquanflq's #104066 for the root-boundary diagnosis and anchoring fix;
|
||||
this combined branch reuses its already-resolved destination instead of repeating
|
||||
name resolution. No retry or receipt semantics change.
|
||||
|
||||
105
evals/botmode-dm-delivery/probe-cron-root.spec.ts
Normal file
105
evals/botmode-dm-delivery/probe-cron-root.spec.ts
Normal file
@@ -0,0 +1,105 @@
|
||||
import { execFileSync, spawn } from 'node:child_process'
|
||||
import fs from 'node:fs'
|
||||
import path from 'node:path'
|
||||
import { buildAppEnv, createSandbox, launchDesktop, waitForAppReady, writeEnvFile, writeMockProviderConfig, type MockBackendFixture } from './fixtures'
|
||||
import { MOCK_REPLY, startMockServer } from '../../../tests-js/scripts/mock-server'
|
||||
import { expect, test } from './test'
|
||||
|
||||
const repo = path.resolve(import.meta.dirname, '../../..')
|
||||
const python = path.join(process.env.VIRTUAL_ENV || path.join(repo, '.venv'), 'bin', 'python')
|
||||
const evidence = process.env.BOT_DM_EVIDENCE || '/tmp/botmode-cron-root/native'
|
||||
let fixture: MockBackendFixture
|
||||
let env: Record<string, string>
|
||||
|
||||
test.beforeAll(async () => {
|
||||
fs.mkdirSync(evidence, { recursive: true })
|
||||
const sandbox = createSandbox('cron-root')
|
||||
const mock = await startMockServer({ holdFirstCompletionContaining: 'ORDINARY_CRON_SENTINEL' })
|
||||
for (const name of ['default', 'alpha', 'beta']) {
|
||||
const home = name === 'default' ? sandbox.hermesHome : path.join(sandbox.hermesHome, 'profiles', name)
|
||||
fs.mkdirSync(home, { recursive: true })
|
||||
writeMockProviderConfig(home, mock.url)
|
||||
writeEnvFile(home)
|
||||
fs.writeFileSync(path.join(home, 'SOUL.md'), `# ${name}\nA Bot Mode teammate.\n`)
|
||||
fs.writeFileSync(path.join(home, 'profile.yaml'), `name: ${name}\nui_meta:\n hermes-bots: {}\n`)
|
||||
}
|
||||
const bin = path.join(sandbox.root, 'bin')
|
||||
fs.mkdirSync(bin)
|
||||
fs.writeFileSync(path.join(bin, 'hermes'), `#!/bin/sh\ncd ${repo}\nprintf '%s\\n' "$$ $HERMES_HOME $*" >> ${evidence}/children.log\nexec ${python} -m hermes_cli.main "$@"\n`, { mode: 0o755 })
|
||||
env = buildAppEnv(sandbox, { HOME: sandbox.root, HERMES_DESKTOP_PYTHON: python,
|
||||
HERMES_DESKTOP_HERMES: path.join(bin, 'hermes'), PATH: `${bin}:${process.env.PATH}`,
|
||||
PYTHONPATH: repo, HERMES_SINGLE_QUERY_LINGER_SECONDS: '1' })
|
||||
const { app, page } = await launchDesktop(env)
|
||||
fixture = { app, page, sandbox, mock, mockUrl: mock.url, cleanup: async () => {
|
||||
await app.close().catch(() => undefined)
|
||||
await mock.close()
|
||||
} }
|
||||
fs.writeFileSync(path.join(evidence, 'sandbox.txt'), sandbox.root)
|
||||
await waitForAppReady(fixture, 120_000)
|
||||
})
|
||||
|
||||
test.afterAll(async () => { await fixture?.cleanup() })
|
||||
|
||||
function probe(script: string, extraEnv = {}) {
|
||||
return JSON.parse(execFileSync(python, ['-c', script], { env: { ...env, ...extraEnv }, cwd: repo, encoding: 'utf8', timeout: 30_000 }))
|
||||
}
|
||||
|
||||
async function openBot(name: string) {
|
||||
const page = fixture.page
|
||||
await page.getByRole('button', { name: 'Bots', exact: true }).or(page.getByRole('tab', { name: 'Bots', exact: true })).first().click()
|
||||
const row = page.getByRole('button', { name: new RegExp(`^${name}\\b`, 'i') }).filter({ visible: true }).first()
|
||||
await expect(row).toBeVisible({ timeout: 30_000 })
|
||||
await row.click()
|
||||
const composer = page.locator('[data-slot="composer-root"] [contenteditable="true"]').filter({ visible: true }).first()
|
||||
await expect(composer).toBeVisible({ timeout: 120_000 })
|
||||
return composer
|
||||
}
|
||||
|
||||
test('ordinary cron pins an unowned named target under a custom root', async () => {
|
||||
test.setTimeout(240_000)
|
||||
const page = fixture.page
|
||||
const composer = await openBot('Hermes')
|
||||
await expect(page.getByText('Say something to get started.').filter({ visible: true })).toBeVisible({ timeout: 120_000 })
|
||||
await composer.fill('initialize default Desktop owner')
|
||||
await page.keyboard.press('Enter')
|
||||
await expect(page.getByText(MOCK_REPLY).filter({ visible: true }).first()).toBeVisible({ timeout: 60_000 })
|
||||
const discovery = 'from pathlib import Path; import json,os; from tools.bot_live_delivery import find_canonical_owner; h=Path(os.environ["HERMES_HOME"]); print(json.dumps({"default":find_canonical_owner(h),"alpha":find_canonical_owner(h/"profiles"/"alpha")}))'
|
||||
const before = probe(discovery)
|
||||
expect(before.default.surface).toBe('desktop')
|
||||
expect(before.alpha).toBeNull()
|
||||
const output = fs.openSync(path.join(evidence, 'producer.log'), 'w')
|
||||
const resultPath = path.join(evidence, 'result.json')
|
||||
const script = `import json; from pathlib import Path; from cron.scheduler_delivery import _deliver_to_bot_chat; j={"id":"ordinary-cron","name":"Ordinary cron","execution_id":"never-deferred"}; result=_deliver_to_bot_chat(j,"ORDINARY_CRON_SENTINEL","alpha"); Path(${JSON.stringify(resultPath)}).write_text(json.dumps({"result":result,"job":j}))`
|
||||
const child = spawn(python, ['-c', script], { env, cwd: repo, stdio: ['ignore', output, output] })
|
||||
try {
|
||||
await expect.poll(() => fs.existsSync(resultPath) ? 'exited' : fixture.mock.receivedPrompts.some(p => p.includes('ORDINARY_CRON_SENTINEL')) ? 'held' : 'waiting', { timeout: 90_000 }).not.toBe('waiting')
|
||||
if (fs.existsSync(resultPath)) {
|
||||
console.log('EARLY_RESULT', fs.readFileSync(resultPath, 'utf8'))
|
||||
expect(JSON.parse(fs.readFileSync(resultPath, 'utf8')).result).toBeNull()
|
||||
}
|
||||
await fixture.mock.waitForHeldCompletion()
|
||||
const held = probe(discovery)
|
||||
expect(held.alpha.surface).toBe('cli')
|
||||
const launched = fs.readFileSync(path.join(evidence, 'children.log'), 'utf8').trim().split('\n').at(-1)!
|
||||
expect(Number(launched.split(' ')[0])).toBe(held.alpha.pid)
|
||||
expect(launched).toContain(path.join(fixture.sandbox.hermesHome, 'profiles', 'alpha'))
|
||||
expect(held.default.lease_id).toBe(before.default.lease_id)
|
||||
expect(fs.existsSync(path.join(fixture.sandbox.hermesHome, 'cron', 'bot_chat_pending'))).toBe(false)
|
||||
fs.writeFileSync(path.join(evidence, 'owners.json'), JSON.stringify({ before, held }, null, 2))
|
||||
fixture.mock.releaseHeldStream()
|
||||
await expect.poll(() => child.exitCode, { timeout: 60_000 }).toBe(0)
|
||||
expect(JSON.parse(fs.readFileSync(resultPath, 'utf8')).result).toBeNull()
|
||||
await openBot('alpha')
|
||||
await expect(page.getByText(/ORDINARY_CRON_SENTINEL/).filter({ visible: true }).first()).toBeVisible({ timeout: 60_000 })
|
||||
const rows = probe('import sqlite3,json,os; from pathlib import Path; h=Path(os.environ["HERMES_HOME"]); print(json.dumps({n:sqlite3.connect(h/"state.db" if n=="default" else h/"profiles"/n/"state.db").execute("select session_id,role,content from messages").fetchall() for n in ["default","alpha"]}))')
|
||||
expect(rows.alpha.filter((r: string[]) => r[1] === 'user' && r[2].includes('ORDINARY_CRON_SENTINEL'))).toHaveLength(1)
|
||||
expect(rows.default.filter((r: string[]) => r[2].includes('ORDINARY_CRON_SENTINEL'))).toHaveLength(0)
|
||||
fs.writeFileSync(path.join(evidence, 'rows.json'), JSON.stringify(rows, null, 2))
|
||||
await page.screenshot({ path: path.join(evidence, 'ordinary-recipient.png') })
|
||||
const missing = probe('import json,os; from pathlib import Path; from cron.scheduler_delivery import _deliver_to_bot_chat; h=Path(os.environ["HERMES_HOME"]); p=h/"profiles"/"beta"; p.rename(h/"profiles"/"beta-removed"); result=_deliver_to_bot_chat({"id":"removed","execution_id":"missing"},"MUST_NOT_RUN","beta"); print(json.dumps({"result":result,"recreated":p.exists(),"wrong_root":(Path.home()/".hermes").exists()}))')
|
||||
expect(missing.result).not.toBeNull()
|
||||
expect(missing.recreated).toBe(false)
|
||||
expect(missing.wrong_root).toBe(false)
|
||||
fs.writeFileSync(path.join(evidence, 'missing.json'), JSON.stringify(missing, null, 2))
|
||||
} finally { fixture.mock.releaseHeldStream(); child.kill(); fs.closeSync(output) }
|
||||
})
|
||||
78
tests/cron/test_bot_chat_cli_home.py
Normal file
78
tests/cron/test_bot_chat_cli_home.py
Normal file
@@ -0,0 +1,78 @@
|
||||
"""The unowned CLI lane executes only at the home used for owner discovery."""
|
||||
import json
|
||||
from pathlib import Path
|
||||
import subprocess
|
||||
import sys
|
||||
from unittest.mock import Mock
|
||||
|
||||
import pytest
|
||||
|
||||
from cron import scheduler_delivery as delivery
|
||||
from hermes_constants import reset_hermes_home_override, set_hermes_home_override
|
||||
|
||||
|
||||
@pytest.mark.parametrize("profile", ["beta", "default", ""])
|
||||
def test_cli_keeps_discovered_home_when_launch_selection_changes(tmp_path, monkeypatch, profile):
|
||||
root = tmp_path / "custom"
|
||||
home = root / "profiles" / "beta" if profile == "beta" else root
|
||||
home.mkdir(parents=True)
|
||||
other = root / "profiles" / "other"
|
||||
other.mkdir(parents=True)
|
||||
monkeypatch.setenv("HOME", str(tmp_path))
|
||||
monkeypatch.setenv("HERMES_HOME", str(root))
|
||||
token = set_hermes_home_override(str(root))
|
||||
real_run = subprocess.run
|
||||
seen = []
|
||||
|
||||
def discover(target):
|
||||
assert target == home
|
||||
(root / "active_profile").write_text("other", encoding="utf-8")
|
||||
return None
|
||||
|
||||
def run(argv, **kwargs):
|
||||
# Exercise the actual startup resolver with the production child env/flags.
|
||||
code = ('import json,sys; sys.argv=["hermes"]+json.loads(sys.argv[1]); '
|
||||
'import hermes_cli.main; from hermes_constants import get_hermes_home; '
|
||||
'print(json.dumps(str(get_hermes_home())))')
|
||||
result = real_run([sys.executable, "-c", code, json.dumps(argv[1:])],
|
||||
env=kwargs["env"], capture_output=True, text=True, timeout=30)
|
||||
assert result.returncode == 0, result.stderr
|
||||
seen.append(Path(json.loads(result.stdout.strip().splitlines()[-1])))
|
||||
return subprocess.CompletedProcess(argv, 0, "", "")
|
||||
|
||||
monkeypatch.setattr("tools.bot_live_delivery.find_canonical_live_owner", discover)
|
||||
monkeypatch.setattr(delivery.shutil, "which", lambda _: "/bin/hermes")
|
||||
monkeypatch.setattr(delivery.subprocess, "run", run)
|
||||
try:
|
||||
assert delivery._deliver_to_bot_chat({"id": "job"}, "output", profile) is None
|
||||
assert seen == [home]
|
||||
assert not (tmp_path / ".hermes").exists()
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("removed_during_discovery", [False, True])
|
||||
def test_missing_destination_never_launches_or_recreates(tmp_path, monkeypatch, removed_during_discovery):
|
||||
root = tmp_path / "custom"
|
||||
root.mkdir()
|
||||
home = root / "profiles" / "beta"
|
||||
if removed_during_discovery:
|
||||
home.mkdir(parents=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(root))
|
||||
token = set_hermes_home_override(str(root))
|
||||
run = Mock(return_value=subprocess.CompletedProcess([], 0, "", ""))
|
||||
|
||||
def discover(target):
|
||||
if home.exists():
|
||||
home.rmdir()
|
||||
return None
|
||||
|
||||
monkeypatch.setattr("tools.bot_live_delivery.find_canonical_live_owner", discover)
|
||||
monkeypatch.setattr(delivery.subprocess, "run", run)
|
||||
try:
|
||||
error = delivery._deliver_to_bot_chat({"id": "job"}, "output", "beta")
|
||||
assert error is not None and str(home) in error
|
||||
run.assert_not_called()
|
||||
assert not home.exists()
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
@@ -135,7 +135,7 @@ def test_deliver_runs_canonical_bot_chat_lane():
|
||||
assert err is None
|
||||
argv = calls["argv"]
|
||||
assert argv[0] == "/usr/bin/hermes"
|
||||
assert "-p" not in argv # own profile: subprocess inherits HERMES_HOME
|
||||
assert argv[1:3] == ["-p", "default"] # do not follow active_profile
|
||||
assert "chat" in argv
|
||||
assert "Bot Chat" in argv
|
||||
assert "--create-if-missing" in argv
|
||||
@@ -145,26 +145,6 @@ def test_deliver_runs_canonical_bot_chat_lane():
|
||||
assert not any("the output" in str(a) for a in argv)
|
||||
|
||||
|
||||
def test_deliver_named_profile_uses_p_flag_and_clears_home():
|
||||
calls = {}
|
||||
|
||||
def fake_run(argv, **kwargs):
|
||||
calls["argv"] = argv
|
||||
calls["kwargs"] = kwargs
|
||||
return _completed()
|
||||
|
||||
with mock.patch.object(sched.subprocess, "run", side_effect=fake_run), \
|
||||
mock.patch.object(sched_delivery.shutil, "which", return_value="/usr/bin/hermes"), \
|
||||
mock.patch.dict(sched.os.environ, {"HERMES_HOME": "/tmp/other-profile"}):
|
||||
err = _deliver_to_bot_chat({"id": "j1", "name": "n"}, "out", "research")
|
||||
|
||||
assert err is None
|
||||
argv = calls["argv"]
|
||||
assert argv[1:3] == ["-p", "research"]
|
||||
# -p owns resolution; the scheduler's own HERMES_HOME must not leak in.
|
||||
assert "HERMES_HOME" not in calls["kwargs"]["env"]
|
||||
|
||||
|
||||
def test_deliver_failure_returns_error_string():
|
||||
with mock.patch.object(
|
||||
sched.subprocess, "run", return_value=_completed(returncode=1, stderr="boom")
|
||||
|
||||
@@ -536,7 +536,7 @@ error. A delivery failure does not count toward the job's `failure_streak`
|
||||
- `bot-chat:<profile>` targets another profile **on the same machine**. Names are validated against `hermes profile list` when the job is created; profiles on other gateways or machines can never be targeted, so same-named profiles across machines are unambiguous.
|
||||
- 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/<receipt-id>.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). A deferred request is claimed before launching that lane; interruption or an uncertain subprocess result never causes an automatic resend.
|
||||
- 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/<receipt-id>.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.
|
||||
- **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/<receipt-id>.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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user