diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 12f06f30ff..290ae6e44f 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -4373,7 +4373,7 @@ async function applyUpdates(opts: { stopSafeBlockers?: boolean } = {}) { // immediately, so child.pid is NOT the script's pid — the script // claims the update marker itself with its own $PID as its first // action, and a relaunched Desktop parks on that. - const wrapped = wrapHandoffForDetachedConsole(scriptHandoff, [ + const wrappedArgs = [ '-InstallRoot', updateRoot, '-Branch', @@ -4382,7 +4382,14 @@ async function applyUpdates(opts: { stopSafeBlockers?: boolean } = {}) { String(process.pid), '-RelaunchExe', process.execPath - ]) + ] + // Same remote-ownership rule as the posix hand-off (#117529): a + // remote-served Desktop must not let the update (re)start a local + // messaging gateway that competes with the remote host's polling. + if (globalRemoteActive()) { + wrappedArgs.push('-NoGateway') + } + const wrapped = wrapHandoffForDetachedConsole(scriptHandoff, wrappedArgs) child = spawnUpdaterProcess(wrapped.command, wrapped.args, { cwd: HERMES_HOME, @@ -4766,6 +4773,13 @@ async function applyUpdatesPosixHandoff(opts: any) { } const args = [...handoff.args, '--install-root', updateRoot, '--branch', branch, '--desktop-pid', String(process.pid)] + // A remote-served Desktop owns no local messaging gateway: `hermes update + // --gateway` would (re)start one here anyway, and with the same channel + // credentials as the remote host it becomes a competing long-poll consumer + // (#117529). Keep --gateway for the local-ownership default. + if (globalRemoteActive()) { + args.push('--no-gateway') + } const updateStartedAt = Math.floor(Date.now() / 1000) // Relaunch target: the running .app bundle on mac (script swaps the diff --git a/scripts/desktop-update/posix.sh b/scripts/desktop-update/posix.sh index 7f73d69beb..b343cc1787 100755 --- a/scripts/desktop-update/posix.sh +++ b/scripts/desktop-update/posix.sh @@ -38,6 +38,7 @@ set -u ORIGINAL_ARGS=("$@") INSTALL_ROOT="" BRANCH="main" DESKTOP_PID=0 RELAUNCH_TARGET="" RELAUNCH_CWD="" SANDBOX_FALLBACK=0 RELAUNCH_ARGS=() +NO_GATEWAY=0 NO_UI=0 NO_MARKER_CLEANUP=0 SELF_TEST_UI=0 SELF_TEST_GATE=0 SELF_TEST_MARKER=0 SELF_TEST_TCC_HEAL=0 HANDOFF_DAEMONIZED=0 @@ -49,6 +50,7 @@ while [ $# -gt 0 ]; do --relaunch-target) RELAUNCH_TARGET="$2"; shift 2 ;; --relaunch-cwd) RELAUNCH_CWD="$2"; shift 2 ;; --sandbox-fallback) SANDBOX_FALLBACK=1; shift ;; + --no-gateway) NO_GATEWAY=1; shift ;; --no-ui) NO_UI=1; shift ;; --no-marker-cleanup) NO_MARKER_CLEANUP=1; shift ;; --self-test-ui) SELF_TEST_UI=1; shift ;; @@ -771,9 +773,19 @@ if "${UPDATE_INVOKE[@]}" update --help 2>/dev/null | grep -q -- '--keep-stash'; else log "installed hermes predates --keep-stash; running without it" fi -log "running: ${UPDATE_INVOKE[*]} update --yes --gateway $KEEP_STASH --branch $BRANCH" +# --gateway restarts the local messaging gateway after the update. The +# Desktop omits it (--no-gateway) when it is served by a remote gateway +# (#117529): restarting a local one there is never wanted, and with the same +# channel credentials as the remote host it becomes a competing long-poll +# consumer (e.g. Telegram rejects one of the two getUpdates callers). +GATEWAY_FLAG="--gateway" +if [ "$NO_GATEWAY" -eq 1 ]; then + GATEWAY_FLAG="" + log "update requested without --gateway (remote-served Desktop)" +fi +log "running: ${UPDATE_INVOKE[*]} update --yes $GATEWAY_FLAG $KEEP_STASH --branch $BRANCH" publish_stage "Updating code and dependencies" -OUT="$("${UPDATE_INVOKE[@]}" update --yes --gateway $KEEP_STASH --branch "$BRANCH" 2>&1)"; CODE=$? +OUT="$("${UPDATE_INVOKE[@]}" update --yes $GATEWAY_FLAG $KEEP_STASH --branch "$BRANCH" 2>&1)"; CODE=$? printf '%s\n' "$OUT" >> "$LOG" 2>/dev/null log "hermes update exit code: $CODE" @@ -795,7 +807,7 @@ if [ "$CODE" -ne 0 ] && [ "$CODE" -ne 2 ]; then fi log "retrying once (freshly pulled fix loads on the second run)" publish_stage "Retrying update" - OUT="$("${UPDATE_INVOKE[@]}" update --yes --gateway $KEEP_STASH --branch "$BRANCH" 2>&1)"; CODE=$? + OUT="$("${UPDATE_INVOKE[@]}" update --yes $GATEWAY_FLAG $KEEP_STASH --branch "$BRANCH" 2>&1)"; CODE=$? printf '%s\n' "$OUT" >> "$LOG" 2>/dev/null log "retry exit code: $CODE" fi diff --git a/scripts/desktop-update/windows.ps1 b/scripts/desktop-update/windows.ps1 index 60aa605018..17bb6b3235 100644 --- a/scripts/desktop-update/windows.ps1 +++ b/scripts/desktop-update/windows.ps1 @@ -48,6 +48,7 @@ param( [string]$RelaunchExe = "", [switch]$NoUi, [switch]$NoMarkerCleanup, + [switch]$NoGateway, [switch]$SelfTestUi, [switch]$SelfTestPipeDrain, [switch]$SelfTestMarker, @@ -1597,7 +1598,18 @@ try { Write-HandoffLog $finalMsg exit $finalCode } - $updateArgs = @("-m", "hermes_cli.main", "update", "--yes", "--gateway", "--force", "--branch", $Branch) + # --gateway restarts the local messaging gateway after the update. The + # Desktop passes -NoGateway when it is served by a remote gateway + # (#117529): restarting a local one there is never wanted, and with the + # same channel credentials as the remote host it becomes a competing + # long-poll consumer (e.g. Telegram rejects one of the two getUpdates + # callers). + $gatewayArg = @("--gateway") + if ($NoGateway) { + $gatewayArg = @() + Write-HandoffLog "update requested without --gateway (remote-served Desktop)" + } + $updateArgs = @("-m", "hermes_cli.main", "update", "--yes") + $gatewayArg + @("--force", "--branch", $Branch) # --keep-stash: never re-apply local source edits after the update (they # stay parked in git stash). Probe --help first: the flag ships with newer # backends and an unknown flag would abort argparse with exit 2, which diff --git a/tests/scripts/desktop_update/test_desktop_update_gateway_flag.py b/tests/scripts/desktop_update/test_desktop_update_gateway_flag.py new file mode 100644 index 0000000000..825a41adc6 --- /dev/null +++ b/tests/scripts/desktop_update/test_desktop_update_gateway_flag.py @@ -0,0 +1,104 @@ +"""The hand-off's `--gateway` flag, exercised against the real posix script. + +`hermes update --gateway` (re)starts the local messaging gateway after the +update. A Desktop served by a remote gateway (#117529) must not ask for that: +the restarted local gateway shares the remote host's channel credentials and +becomes a competing long-poll consumer — Telegram answers the conflict by +rejecting one of the two `getUpdates` callers, taking the production bot +offline. The Desktop therefore passes `--no-gateway` whenever its active +connection is remote-shaped; these tests pin both sides of that contract +against the real `posix.sh`, so neither the default (local ownership) nor the +opt-out can silently regress. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import time +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent.parent +SHIM_DIR = REPO_ROOT / "scripts" / "desktop-update" + +requires_posix_handoff = pytest.mark.skipif( + not (os.path.exists("/bin/bash") and os.path.exists("/usr/bin/python3")), + reason="posix.sh detaches through /bin/bash and /usr/bin/python3", +) + +# Stands in for `hermes`: answers the `update --help` probe (so --keep-stash +# is kept), and appends every non-help invocation's argv as one JSON line so +# the tests can inspect exactly what the update was invoked with. +FAKE_HERMES = """#!/bin/bash +case "$*" in *--help*) echo "--keep-stash"; exit 0 ;; esac +printf '%s\\n' "$*" >> "$HERMES_TEST_ARGV" +exit 0 +""" + + +def _run_handoff(tmp_path: Path, extra_args: list[str]) -> list[str]: + """Run the real hand-off end to end; return the argv of each hermes call.""" + install_root = tmp_path / "hermes-agent" + (install_root / "venv" / "bin").mkdir(parents=True) + hermes = install_root / "venv" / "bin" / "hermes" + hermes.write_text(FAKE_HERMES) + hermes.chmod(0o755) + + argv_log = tmp_path / "argv.jsonl" + env = {**os.environ, "TMPDIR": str(tmp_path), "HERMES_TEST_ARGV": str(argv_log)} + subprocess.run( + ["/bin/bash", str(SHIM_DIR / "posix.sh"), "--install-root", str(install_root), "--no-ui", *extra_args], + env=env, + timeout=60, + check=True, + ) + + result = tmp_path / ".hermes-update-result.json" + deadline = time.monotonic() + 45 + while time.monotonic() < deadline and not result.exists(): + time.sleep(0.1) + assert result.exists(), "hand-off never wrote its result file" + + return argv_log.read_text().splitlines() + + +@requires_posix_handoff +def test_default_handoff_asks_for_the_local_gateway(tmp_path): + """A locally-served Desktop owns its gateway: the update must restart it.""" + calls = _run_handoff(tmp_path, []) + + update_calls = [c for c in calls if " update " in f" {c} "] + assert update_calls, "hand-off never ran hermes update" + assert "--gateway" in update_calls[0].split() + + +@requires_posix_handoff +def test_no_gateway_flag_omits_gateway_from_update(tmp_path): + """A remote-served Desktop (#117529): no local gateway may be (re)started. + + The competing long-poll consumer is silent channel outage risk, and the + flag must survive the hand-off's own retry path — every update invocation + is checked, not just the first. + """ + calls = _run_handoff(tmp_path, ["--no-gateway"]) + + update_calls = [c for c in calls if " update " in f" {c} "] + assert update_calls, "hand-off never ran hermes update" + for call in update_calls: + argv = call.split() + assert "--gateway" not in argv, f"--gateway reappeared in update argv: {call}" + assert "--keep-stash" in argv, "--no-gateway must not disturb --keep-stash" + assert "--yes" in argv + + +@requires_posix_handoff +def test_no_gateway_flag_leaves_the_result_clean(tmp_path): + """The opt-out only drops --gateway; the hand-off still completes OK.""" + calls = _run_handoff(tmp_path, ["--no-gateway"]) + + assert calls, "hand-off never invoked hermes" + result = json.loads((tmp_path / ".hermes-update-result.json").read_text()) + assert result.get("status") != "error", result diff --git a/tests/scripts/desktop_update/test_desktop_update_windows_gateway_flag.py b/tests/scripts/desktop_update/test_desktop_update_windows_gateway_flag.py new file mode 100644 index 0000000000..62467333e9 --- /dev/null +++ b/tests/scripts/desktop_update/test_desktop_update_windows_gateway_flag.py @@ -0,0 +1,62 @@ +"""The Windows hand-off's `--gateway` flag, source-level (Linux CI cannot run PowerShell). + +`hermes update --gateway` (re)starts the local messaging gateway after the +update. A Desktop served by a remote gateway (#117529) must not ask for that: +the restarted local gateway shares the remote host's channel credentials and +becomes a competing long-poll consumer — Telegram answers the conflict by +rejecting one of the two `getUpdates` callers, taking the production bot +offline. The Desktop passes `-NoGateway` on that path; these tests pin the +script side of the contract the same source-level way +`test_desktop_update_windows_python_handoff.py` guards its invocation rule. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent.parent +WINDOWS_PS1 = REPO_ROOT / "scripts" / "desktop-update" / "windows.ps1" + + +def _handoff_source() -> str: + """The script with its ``-SelfTest*`` fixture blocks removed (same strip as + test_desktop_update_windows_python_handoff.py), normalized to LF.""" + source = WINDOWS_PS1.read_text(encoding="utf-8").replace("\r\n", "\n") + return re.sub( + r"\n(?P *)if \(\$SelfTest\w+\) \{.*?\n(?P=indent)\}\n", + "\n", + source, + ) + + +def test_no_gateway_switch_is_declared() -> None: + assert re.search(r"^\s{4}\[switch\]\$NoGateway,\s*$", _handoff_source(), re.M), ( + "scripts/desktop-update/windows.ps1 must declare [switch]$NoGateway so " + "a remote-served Desktop (#117529) can opt out of the local gateway " + "restart." + ) + + +def test_gateway_flag_is_conditional_not_inline() -> None: + """`--gateway` may only reach the update argv through $gatewayArg. + + An inline literal would mean someone reintroduced an unconditional local + gateway restart — the exact regression (#117529) this guards against. + """ + source = _handoff_source() + + gateway_literals = [line for line in source.splitlines() if '"--gateway"' in line] + assert len(gateway_literals) == 1, ( + "Expected exactly one \"--gateway\" literal in windows.ps1 (the " + f"$gatewayArg default); found: {gateway_literals}" + ) + assert "$gatewayArg = @(\"--gateway\")" in gateway_literals[0] + + assert re.search(r"if \(\$NoGateway\)\s*\{\s*\n\s*\$gatewayArg = @\(\)", source), ( + "-NoGateway must empty $gatewayArg before the update argv is assembled." + ) + assert "$gatewayArg + @(\"--force\"" in source, ( + "The update argv must be assembled from $gatewayArg so -NoGateway " + "actually removes --gateway from the invocation." + )