From 1c4ea9c99a370906e57a2a2151007292e2bef4f9 Mon Sep 17 00:00:00 2001
From: TomTong <tomtong2015@gmail.com>
Date: Fri, 14 Aug 2026 20:51:41 +0200
Subject: [PATCH] =?UTF-8?q?fix(appearance):=20don't=20let=20the=20boot=20s?=
 =?UTF-8?q?cript=20fabricate=20an=20explicit=20them=E2=80=A6=20(#6808)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(appearance): don't let the boot script fabricate an explicit theme choice

`syncSettings` in static/boot.js is careful to let the server win on a first
visit. Its comment says why:

    // empty (new-browser) state is indistinguishable from a user who chose
    // the defaults.  To avoid blocking server→client sync on first visit we
    // only let localStorage override the server when it carries an explicit
    // user-selectable theme value or a NON-DEFAULT skin.

The inline bootstrap in static/index.html defeats that. It resolves a theme with
`localStorage.getItem('hermes-theme')||'dark'` and then unconditionally writes the
result back:

    localStorage.setItem('hermes-theme',theme);localStorage.setItem('hermes-skin',skin);

On a brand-new browser that stores `hermes-theme=dark` before any request is made.
By the time syncSettings runs, `lsHasExplicitTheme` is true, so:

1. the server's `theme`/`skin` in SETTINGS_DEFAULTS (or settings.json) is ignored, and
2. the reconcile branch below POSTs the fabricated value back to /api/settings,
   overwriting the stored setting it was supposed to be reading.

The net effect is that the server-side appearance default is unreachable: a fresh
browser always lands on dark and then persists it server-side. Deployments that
set a different default in SETTINGS_DEFAULTS never see it applied.

Fix: only rewrite localStorage when there was already appearance state to
normalise. A fresh visit now leaves localStorage untouched, so `lsHasExplicitTheme`
is false and syncSettings takes the server value exactly as its comment intends.

Deliberately keeps the existing normalisation. The write is not pointless — it
canonicalises legacy names (`solarized` → `dark` + `poseidon`). Gating on "was
there any prior state" rather than per-key preserves that: with `hermes-theme=solarized`
and no `hermes-skin`, the derived skin is still written, so the mapping survives
the next load. A per-key guard would have silently dropped it.

Verified by driving the bootstrap in node against a stubbed localStorage. Behaviour
is byte-identical to before for every input that has prior state — legacy
`solarized`, an explicit `light`+`mono`, and a skin-only `slate` all produce the
same painted result and the same localStorage. The only case that changes is the
empty one, which is the bug:

    before   {} → localStorage {hermes-theme: dark, hermes-skin: default}   server locked out
    after    {} → localStorage {}                                            server wins

Painting is unchanged throughout: a fresh visit still renders dark, since that is
still the fallback.

* test(appearance): pin absent vs explicit appearance state in the bootstrap

Adds the regression coverage asked for in review. Nothing previously
distinguished "no appearance stored" from "user chose these values":
tests/test_1003_appearance_autosave.py covers picker autosave and
tests/test_bugfix_sweep.py covers guarded storage access, but neither fails if
the bootstrap starts fabricating a choice again.

Executes the REAL bootstrap, lifted out of static/index.html, in node against a
stubbed localStorage and documentElement — the same harness used to verify the
fix, now committed instead of run by hand. Covers all three states the review
called out, plus the pre-paint assertion:

  - both keys absent   -> no setItem at all, AND the dark class is still applied
                          (the fix changes persistence only, never the paint)
  - either key present -> both values normalised and persisted
  - legacy names       -> solarized/slate/monokai/nord/oled still migrate, and
                          the DERIVED skin is still written

That last group is why the guard is on the pair rather than per key: with
`hermes-theme=solarized` and no `hermes-skin`, the write of the derived skin is
the only thing that persists the mapping, so a per-key guard would silently drop
it on the next load.

Verified as a regression test, not a tautology: against the pre-fix
static/index.html, test_fresh_browser_writes_nothing and
test_guard_is_on_the_pair_not_per_key both FAIL, while the other nine still pass
— the unfixed bootstrap does still paint dark and does still persist
migrations, so only the buggy behaviour differs.

One harness note worth keeping: the driver and the extracted script are passed
as FILES (`node <driver> <script>`). Passing the script via `node -e ... --
<script>` shifts process.argv, so the driver eval'd the JSON scenario instead of
the bootstrap — and since `eval('{}')` is a valid empty block rather than a
syntax error, the empty-store case passed while executing nothing. _run() now
asserts the bootstrap produced some effect.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(appearance): keep `var themes=` intact so the skin tests still find the block

CI caught this; review could not. `tests/test_sienna_skin.py` (x2) and
`tests/test_catppuccin_skin.py` locate the inline bootstrap with

    init_script_idx = INDEX_HTML.find("var themes=")
    end_idx = INDEX_HTML.find("</script>", init_script_idx)
    init_block = INDEX_HTML[init_script_idx:end_idx]

Declaring `_hadAppearance` as the first `var` in that chain turned `var themes=`
into `,themes=`, so `find()` returned -1, `init_block` came out EMPTY, and three
assertions failed against the empty string — including "Default theme must remain
'dark'", which read as though this PR had changed the default. It had not: the
default is untouched, the block simply could not be located any more.

Splitting the declaration (`;var themes=` instead of `,themes=`) restores the
literal. Both are `var` in the same function scope, so behaviour is identical —
the regression module still passes unchanged, including the empty-store case
(no writes, dark class still applied) and all five legacy migrations.

Failed on shards 3 and 4 across 3.11/3.12/3.13 while the same shards passed on
master, which is what pinned it to this change rather than flake.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Hermes Agent <hermes@aip.de>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: nesquena-hermes <nesquena+hermes@gmail.com>
---
 static/index.html                             |   2 +-
 ...pearance_bootstrap_no_fabricated_choice.py | 188 ++++++++++++++++++
 2 files changed, 189 insertions(+), 1 deletion(-)
 create mode 100644 tests/test_6808_appearance_bootstrap_no_fabricated_choice.py

diff --git a/static/index.html b/static/index.html
index 8f268382..1f55169a 100644
--- a/static/index.html
+++ b/static/index.html
@@ -17,7 +17,7 @@
 <meta name="apple-mobile-web-app-status-bar-style" content="black-translucent">
 <meta name="apple-mobile-web-app-title" content="Hermes">
 <link rel="apple-touch-icon" sizes="512x512" href="static/apple-touch-icon.png">
-<script>(function(){try{var themes={light:1,dark:1,system:1},skins={codex:1,terracotta:1,default:1,ares:1,mono:1,graphite:1,github:1,slate:1,poseidon:1,sisyphus:1,charizard:1,sienna:1,catppuccin:1,hepburn:1,nous:1,'geist-contrast':1,neon:1,'neon-soft':1,'neon-paint':1,zeus:1,'verdigris':1},legacy={slate:['dark','slate'],solarized:['dark','poseidon'],monokai:['dark','sisyphus'],nord:['dark','slate'],oled:['dark','default']},t=(localStorage.getItem('hermes-theme')||'dark').toLowerCase(),s=(localStorage.getItem('hermes-skin')||'').toLowerCase(),m=legacy[t],theme=m?m[0]:(themes[t]?t:'dark');var pendingExt=s&&s!=='default'&&!skins[s]&&!legacy[s];var skin=skins[s]?s:(pendingExt?s:(m?m[1]:'default'));localStorage.setItem('hermes-theme',theme);localStorage.setItem('hermes-skin',skin);if(theme==='system')theme=window.matchMedia('(prefers-color-scheme:dark)').matches?'dark':'light';if(theme==='dark')document.documentElement.classList.add('dark');if(skin!=='default')document.documentElement.dataset.skin=skin;}catch(e){document.documentElement.classList.add('dark');}})()</script>
+<script>(function(){try{var _hadAppearance=localStorage.getItem('hermes-theme')!==null||localStorage.getItem('hermes-skin')!==null;var themes={light:1,dark:1,system:1},skins={codex:1,terracotta:1,default:1,ares:1,mono:1,graphite:1,github:1,slate:1,poseidon:1,sisyphus:1,charizard:1,sienna:1,catppuccin:1,hepburn:1,nous:1,'geist-contrast':1,neon:1,'neon-soft':1,'neon-paint':1,zeus:1,'verdigris':1},legacy={slate:['dark','slate'],solarized:['dark','poseidon'],monokai:['dark','sisyphus'],nord:['dark','slate'],oled:['dark','default']},t=(localStorage.getItem('hermes-theme')||'dark').toLowerCase(),s=(localStorage.getItem('hermes-skin')||'').toLowerCase(),m=legacy[t],theme=m?m[0]:(themes[t]?t:'dark');var pendingExt=s&&s!=='default'&&!skins[s]&&!legacy[s];var skin=skins[s]?s:(pendingExt?s:(m?m[1]:'default'));if(_hadAppearance){localStorage.setItem('hermes-theme',theme);localStorage.setItem('hermes-skin',skin);}if(theme==='system')theme=window.matchMedia('(prefers-color-scheme:dark)').matches?'dark':'light';if(theme==='dark')document.documentElement.classList.add('dark');if(skin!=='default')document.documentElement.dataset.skin=skin;}catch(e){document.documentElement.classList.add('dark');}})()</script>
 <script>(function(){try{var fs=localStorage.getItem('hermes-font-size');if(fs&&fs!=='default')document.documentElement.dataset.fontSize=fs;}catch(e){}})()</script>
 <!-- theme-color: surfaces the active app chrome color to native status bars (Safari status bar, PWA, native WKWebView wrappers). Updated dynamically by boot.js when theme/skin changes. The light/dark default values match style.css :root --sidebar / :root.dark --sidebar. -->
 <meta name="theme-color" content="#FAF7F0" media="(prefers-color-scheme: light)">
diff --git a/tests/test_6808_appearance_bootstrap_no_fabricated_choice.py b/tests/test_6808_appearance_bootstrap_no_fabricated_choice.py
new file mode 100644
index 00000000..fd34ef5a
--- /dev/null
+++ b/tests/test_6808_appearance_bootstrap_no_fabricated_choice.py
@@ -0,0 +1,188 @@
+"""Regression checks for #6808: the boot script must not fabricate a theme choice.
+
+`syncSettings` in static/boot.js deliberately lets the server win on a first
+visit, because (its own comment) "empty (new-browser) state is indistinguishable
+from a user who chose the defaults". The inline bootstrap in static/index.html
+used to defeat that: it resolved a theme with
+`localStorage.getItem('hermes-theme')||'dark'` and then wrote the result back
+unconditionally, so a brand-new browser stored `hermes-theme=dark` before any
+request was made. syncSettings then saw an "explicit" value, ignored the
+server's SETTINGS_DEFAULTS, and POSTed the fabricated value back — making the
+server-side appearance default unreachable for deployments that change it.
+
+These cases pin the distinction the fix introduces, which nothing covered
+before: ABSENT appearance state is not the same as EXPLICIT appearance state.
+
+  - both keys absent  -> no setItem at all (and the dark pre-paint is UNCHANGED)
+  - either key present -> both values are normalised and persisted
+  - legacy names       -> migration still happens, exactly as before
+
+The third is the reason the guard is on the PAIR rather than per key. The write
+is not pointless: it canonicalises legacy names (`solarized` -> dark+poseidon).
+With `hermes-theme=solarized` and no `hermes-skin`, a per-key guard would never
+write the derived skin and the mapping would be lost on the next load.
+"""
+import json
+import re
+import shutil
+import subprocess
+import tempfile
+from pathlib import Path
+
+import pytest
+
+
+ROOT = Path(__file__).resolve().parent.parent
+INDEX_HTML = ROOT / "static" / "index.html"
+NODE = shutil.which("node")
+
+pytestmark = pytest.mark.skipif(
+    NODE is None,
+    reason="node is required to execute the appearance bootstrap harness",
+)
+
+
+def _bootstrap_script() -> str:
+    """The inline appearance bootstrap, lifted out of index.html."""
+    html = INDEX_HTML.read_text(encoding="utf-8")
+    for m in re.finditer(r"<script>(.*?)</script>", html, re.S):
+        body = m.group(1)
+        if "hermes-theme" in body and "legacy" in body and "skins" in body:
+            return body
+    raise AssertionError("appearance bootstrap <script> not found in index.html")
+
+
+_DRIVER = r"""
+const fs = require('fs');
+const script = fs.readFileSync(process.argv[2], 'utf8');
+const store = JSON.parse(process.argv[3] || '{}');
+
+const writes = [];
+globalThis.localStorage = {
+  getItem(k) { return Object.prototype.hasOwnProperty.call(store, k) ? store[k] : null; },
+  setItem(k, v) { writes.push([k, String(v)]); store[k] = String(v); },
+  removeItem(k) { delete store[k]; },
+};
+
+const classes = new Set();
+globalThis.document = {
+  documentElement: {
+    classList: { add: (c) => classes.add(c), remove: (c) => classes.delete(c) },
+    dataset: {},
+  },
+  querySelectorAll: () => [],
+};
+globalThis.window = globalThis;
+globalThis.matchMedia = () => ({ matches: false });
+globalThis.window.matchMedia = globalThis.matchMedia;
+
+(0, eval)(script);
+
+process.stdout.write(JSON.stringify({
+  writes,
+  store,
+  classes: [...classes],
+  skin: globalThis.document.documentElement.dataset.skin || null,
+}));
+"""
+
+
+def _run(initial_store: dict) -> dict:
+    """Execute the real bootstrap in node against a stubbed localStorage.
+
+    Driver and script go to FILES and are invoked as `node <driver> <script>`.
+    Passing them via `node -e ... -- <script>` silently shifts process.argv, so
+    the driver eval'd the JSON scenario instead of the bootstrap — and because
+    `eval('{}')` is a valid empty block rather than a syntax error, the
+    empty-store case passed while executing nothing at all.
+    """
+    with tempfile.TemporaryDirectory() as td:
+        driver = Path(td) / "driver.js"
+        script = Path(td) / "bootstrap.js"
+        driver.write_text(_DRIVER, encoding="utf-8")
+        script.write_text(_bootstrap_script(), encoding="utf-8")
+        proc = subprocess.run(
+            [NODE, str(driver), str(script), json.dumps(initial_store)],
+            capture_output=True, text=True, timeout=60, check=False,
+        )
+    assert proc.returncode == 0, f"harness failed: {proc.stderr[:400]}"
+    out = json.loads(proc.stdout)
+    # The harness must have actually run the bootstrap: it always paints.
+    assert out["classes"] or out["skin"] or out["writes"], (
+        "harness produced no effects at all — it probably executed the wrong "
+        "source; check the node argv indices"
+    )
+    return out
+
+
+def test_fresh_browser_writes_nothing():
+    """The bug: an empty store must not become an 'explicit' user choice."""
+    out = _run({})
+    assert out["writes"] == [], (
+        "a fresh visit must not write appearance keys — those writes are what "
+        f"made syncSettings treat the fallback as explicit; got {out['writes']}"
+    )
+    assert out["store"] == {}
+
+
+def test_fresh_browser_still_paints_dark():
+    """The fix changes persistence only, never the pre-paint result."""
+    out = _run({})
+    assert "dark" in out["classes"], (
+        "the dark pre-paint fallback must be unchanged for an empty store"
+    )
+    assert out["skin"] is None
+
+
+@pytest.mark.parametrize(
+    "initial,expected_theme,expected_skin",
+    [
+        ({"hermes-theme": "dark"}, "dark", "default"),
+        ({"hermes-skin": "mono"}, "dark", "mono"),
+        ({"hermes-theme": "light", "hermes-skin": "mono"}, "light", "mono"),
+    ],
+)
+def test_any_prior_state_still_normalises_and_persists(initial, expected_theme, expected_skin):
+    """Either key present is enough to make this an explicit choice."""
+    out = _run(dict(initial))
+    assert out["store"]["hermes-theme"] == expected_theme
+    assert out["store"]["hermes-skin"] == expected_skin
+    assert [k for k, _ in out["writes"]] == ["hermes-theme", "hermes-skin"]
+
+
+@pytest.mark.parametrize(
+    "legacy,theme,skin",
+    [
+        ("solarized", "dark", "poseidon"),
+        ("slate", "dark", "slate"),
+        ("monokai", "dark", "sisyphus"),
+        ("nord", "dark", "slate"),
+        ("oled", "dark", "default"),
+    ],
+)
+def test_legacy_theme_migration_survives(legacy, theme, skin):
+    """A per-key guard would drop the derived skin here. The pair guard must not.
+
+    Only `hermes-theme` is stored, and the skin is DERIVED from the legacy
+    mapping — so the write of `hermes-skin` is the only thing that persists the
+    migration.
+    """
+    out = _run({"hermes-theme": legacy})
+    assert out["store"]["hermes-theme"] == theme
+    assert out["store"]["hermes-skin"] == skin, (
+        f"legacy {legacy!r} must persist its derived skin {skin!r}"
+    )
+
+
+def test_guard_is_on_the_pair_not_per_key():
+    """Source assertion, so the intent survives a future refactor of the block."""
+    script = _bootstrap_script()
+    assert "_hadAppearance" in script
+    assert re.search(
+        r"_hadAppearance\s*=\s*localStorage\.getItem\('hermes-theme'\)\s*!==\s*null\s*\|\|"
+        r"\s*localStorage\.getItem\('hermes-skin'\)\s*!==\s*null",
+        script,
+    ), "the guard must be true when EITHER appearance key is present"
+    assert re.search(r"if\(_hadAppearance\)\{[^}]*setItem\('hermes-theme'", script), (
+        "both appearance writes must sit behind the guard"
+    )