Skip to content

fix(desktop): show the Hermes icon in the update hand-off window - #103632

Open
chelsealong wants to merge 1 commit into
NousResearch:mainfrom
chelsealong:fix/desktop-update-shim-favicon
Open

chelsealong wants to merge 1 commit into
NousResearch:mainfrom
chelsealong:fix/desktop-update-shim-favicon

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #103629. The macOS/Linux desktop update hand-off shows a "chromeless
default-browser app window" (scripts/desktop-update/posix.sh spawning
chrome/chromium --app=http://127.0.0.1:<port>/, serving
scripts/desktop-update/ui.html over scripts/desktop-update/serve-ui.py)
while hermes update runs. That window never served a favicon, so instead
of the Hermes mark, the taskbar/window switcher showed a generic default
icon — exactly what the reporter saw on Ubuntu 26.04.1. Verified via
grep -r icon scripts/desktop-update/: zero hits before this change.

Chromium's --app= mode uses the page's favicon as the window's taskbar
icon (that's the whole point of "site-specific browser" app windows), so
serving one from the shim is enough to fix this — no Electron BrowserWindow
is even involved in this particular window (that's a separate, unrelated
in-app "Updating Hermes…" overlay that already looks fine).

Changes Made

  • scripts/desktop-update/serve-ui.py — accepts an optional 4th argv (icon
    file path); if given and readable, serves it at GET /favicon.ico as
    image/x-icon. Missing/unreadable icon degrades to a plain 404, matching
    every other optional input this shim already tolerates.
  • scripts/desktop-update/posix.shstart_ui() resolves the repo's
    existing web/public/favicon.ico (already used as the web app's own
    favicon) and passes it through to serve-ui.py; falls back to no icon if
    the file isn't there (old/partial checkouts).
  • scripts/desktop-update/ui.html — adds <link rel="icon" href="/favicon.ico">.
  • tests/test_desktop_update_shim_progress.py — extends the existing real
    (no-mock) serve-ui.py subprocess fixture with an icon-path variant, and
    adds two new tests: the icon is served correctly when a path is given, and
    the route cleanly 404s (not a crash) when it isn't.

Windows (scripts/desktop-update/windows.ps1) has its own in-process HTTP
listener and WinForms fallback that also don't set an icon today, but I
deliberately left that out of this PR: I can't execute or verify the
Windows-only WinForms path from this environment, and this repo's own test
suite treats windows.ps1 execution as windows_only / CI-lane-only for
that reason (see tests/test_desktop_update_windows_pipe_drain.py). Happy
to follow up there if maintainers want it, ideally reviewed/tested by
someone on Windows.

How to Test

source .venv/bin/activate   # venv with `pip install -e ".[all,dev]"`
scripts/run_tests.sh tests/test_desktop_update_shim_progress.py -v

Output:

Discovered 1 test files (~7 tests) under ['tests/test_desktop_update_shim_progress.py']; running with -j 8
[100.0% |     7/~7 | ✓7 | ✗0] ✓ tests/test_desktop_update_shim_progress.py (7✓, 4.5s)

=== Summary: 1 files, 7 tests passed, 0 failed (100% complete) in 4.5s (8 workers) ===

Proved the new test actually exercises the fix: with the three source files
reverted (git stash of posix.sh/serve-ui.py/ui.html, keeping only the
test change) the same run gives 1 failed, 6 passed — the new
test_favicon_served_when_icon_path_given fails with HTTPError: 404,
while the new negative test (test_favicon_missing_when_no_icon_path_given)
and all pre-existing tests still pass, confirming the new test targets
exactly the added behavior. Restoring the fix returns it to 7 passed, 0 failed.

Also ran, both clean:

ruff check scripts/desktop-update/serve-ui.py tests/test_desktop_update_shim_progress.py
ruff format --check scripts/desktop-update/serve-ui.py tests/test_desktop_update_shim_progress.py
shellcheck scripts/desktop-update/posix.sh   # no new findings vs. pre-existing SC2317/SC2034 info/warning noise
python3 scripts/check-windows-footguns.py --diff HEAD   # "No Windows footguns found"

Platforms Tested

Linux (Ubuntu, matching the report) — ran the real serve-ui.py subprocess
end-to-end via the test suite above. Did not have access to macOS or Windows
hardware; the POSIX code path (posix.sh) is shared between macOS and Linux
and touches nothing platform-specific.

Checklist

  • I've read the Contributing Guide
  • Commit message follows Conventional Commits
  • Searched existing PRs for a duplicate — none found referencing [Feature]: Add Hermes app icon to updater window header #103629 or this shim's favicon handling
  • PR contains only changes related to this fix
  • Ran scripts/run_tests.sh tests/test_desktop_update_shim_progress.py — all pass
  • Added tests for the change (and proved they fail without it)
  • Tested on Ubuntu (matches reporter's platform)

AI assistance disclosure

This PR was prepared with the help of an AI coding agent (investigation,
implementation, and test verification), reviewed by me before submission.

The macOS/Linux update shim (posix.sh -> serve-ui.py -> ui.html, a
chromeless browser --app window) never served a favicon, so the update
window showed the browser's generic default icon instead of Hermes's,
making it unidentifiable at a glance in the taskbar/window switcher.

serve-ui.py now takes an optional icon path and serves it at
/favicon.ico; posix.sh passes the repo's existing web/public/favicon.ico
when present; ui.html declares the icon link. Missing/unreadable icon
degrades to no favicon, matching the shim's existing fail-soft posture.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) area/install-update Installer, updater, packaging, wheels, doctor labels Sep 5, 2026
andrexibiza added a commit to andrexibiza/hermes-agent that referenced this pull request Sep 5, 2026
A delayed browser could miss the 900ms terminal event and spin forever after the updater exited. Retain terminal delivery until the page acknowledges it, bound unavailable-client teardown and failed requests, and preserve a truthful final display.

Fixes NousResearch#103747. Builds on OutThisLife and Teknium detached handoff work in NousResearch#83634 and the NousResearch#75895 quiet-window design. Continues Axl Ibiza Windows update investigation (NousResearch#60233, NousResearch#94107, NousResearch#100763), including source/review contributions carried by merged NousResearch#93353 and NousResearch#85170. Existing NousResearch#102373, NousResearch#103140, NousResearch#95719, NousResearch#97299 and NousResearch#103632 retain their separate scopes.
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

PR 103632 — fix(desktop): show the Hermes icon in the update hand-off window

Summary: Threads the app favicon.ico through posix.sh into the loopback serve-ui.py shim as a third route, with graceful 404 when the asset is missing. Tiny, safe change with tests for both present and absent icon. Non-blocking notes only.

Non-blocking findings:

  1. scripts/desktop-update/serve-ui.py:68 — exact-match routing means /favicon.ico?v=2 (cache-busted requests some browsers emit) falls to 404. The shipped <link> href is bare so this is benign today; a split("?")[0]-style match would future-proof it.
  2. Platform coverage question: only posix.sh passes the icon. If the Windows updater (*.ps1/*.bat hand-off, if one exists) shows the same chromeless window, it still gets the generic icon — please confirm whether a Windows-side equivalent is needed or out of scope.
  3. scripts/desktop-update/serve-ui.py:53-59 — the icon is read once at startup; a replaced asset mid-update would serve stale bytes. Irrelevant for a minutes-long update flow; noting only for completeness.
  4. scripts/desktop-update/posix.sh:11[ -f "$icon" ] || icon="" correctly degrades when the asset is missing (old checkouts, pruned web/). Good defensive default, and the 404 test pins it.

No security concerns (self.path is only exact-matched, icon_path comes from argv — no request-controlled file access). Good to go.

@chelsealong

Copy link
Copy Markdown
Contributor Author

Re non-blocking finding #2 (Windows coverage): already addressed in the PR description — windows.ps1 has its own in-process HTTP listener/WinForms fallback and is intentionally out of scope here, since I can't execute or verify that path from this environment and this repo's test suite treats windows.ps1 as windows_only/CI-lane-only (see tests/test_desktop_update_windows_pipe_drain.py). Happy to follow up in a separate PR if a maintainer wants it. Items 1, 3, 4 are noted as non-blocking, no action taken.

teknium1 pushed a commit that referenced this pull request Sep 6, 2026
A delayed browser could miss the 900ms terminal event and spin forever after the updater exited. Retain terminal delivery until the page acknowledges it, bound unavailable-client teardown and failed requests, and preserve a truthful final display.

Fixes #103747. Builds on OutThisLife and Teknium detached handoff work in #83634 and the #75895 quiet-window design. Continues Axl Ibiza Windows update investigation (#60233, #94107, #100763), including source/review contributions carried by merged #93353 and #85170. Existing #102373, #103140, #95719, #97299 and #103632 retain their separate scopes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Add Hermes app icon to updater window header

3 participants