From bd4953b30ddaa587ab501e02f897661029de3183 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sat, 18 Jul 2026 17:45:55 -0400 Subject: [PATCH 01/12] =?UTF-8?q?perf(desktop):=20incremental=20block=20le?= =?UTF-8?q?xing=20for=20streaming=20markdown=20=E2=80=94=2014x=20less=20sp?= =?UTF-8?q?litter=20CPU=20on=20long=20replies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth profiling round (#66033/#66347/#66470). Streamdown's parseMarkdownIntoBlocks is a full `marked` lex of the entire message, and during streaming every flush is a new string — so the splitter paid O(full-text) ~30x/s: benchmarked 3.4-9.6ms per call at 64-192KB, i.e. 15-30% of a core burned re-lexing settled text on long agent replies. (The rest of the May-2026 "re-parse elephant" was already eaten by tailBoundedRemend, block-memo, the KaTeX memo, and deferred shiki — the splitter was the last O(full-text) pass besides preprocess, which benches at only 1.5-4.6ms and is left alone.) New src/lib/markdown-blocks.ts (extracted from markdown-text.tsx) wraps the splitter with two caches: - the existing exact-string LRU (remounts: virtualizer scroll, session switch) — moved, unchanged; - a streaming-append cache: when the new text startsWith a recently parsed text, reuse that parse's blocks up to a settled boundary and lex only the suffix. The boundary drops trailing whitespace-only blocks plus the last content block — the only block appended text can reinterpret (open fence, list/table continuation, setext underline, lazy blockquote). Earlier blocks are separated by settled blank lines and can't change. Cross-block reference links can't regress: Streamdown already renders each block as an independent document. Safety: the splitter's blocks.join('') === text property makes offsets exact and is defensively re-checked; any mismatch or non-append rewrite (edit, branch swap) falls back to the full lex — byte-for-byte the old behavior. Property tests: at every random streaming cut over a corpus covering fences, loose lists, tables, setext headings, lazy blockquotes, HTML blocks, and display math — and token-by-token through a fence boundary — the cached splitter's output is asserted deep-equal to a fresh full lex. Bench (128KB reply, 200 flushes): 990ms -> 72ms total splitter CPU (4.95 -> 0.36ms/flush). Verification: tsc clean; eslint/prettier clean; lib + assistant-ui suites green (473 tests). --- .../components/assistant-ui/markdown-text.tsx | 39 +----- apps/desktop/src/lib/markdown-blocks.test.ts | 111 ++++++++++++++++ apps/desktop/src/lib/markdown-blocks.ts | 125 ++++++++++++++++++ 3 files changed, 237 insertions(+), 38 deletions(-) create mode 100644 apps/desktop/src/lib/markdown-blocks.test.ts create mode 100644 apps/desktop/src/lib/markdown-blocks.ts diff --git a/apps/desktop/src/components/assistant-ui/markdown-text.tsx b/apps/desktop/src/components/assistant-ui/markdown-text.tsx index 8f17685108ff..a199b5aa1e7d 100644 --- a/apps/desktop/src/components/assistant-ui/markdown-text.tsx +++ b/apps/desktop/src/components/assistant-ui/markdown-text.tsx @@ -2,7 +2,6 @@ import { TextMessagePartProvider, useMessagePartText } from '@assistant-ui/react' import { - parseMarkdownIntoBlocks, type StreamdownTextComponents, StreamdownTextPrimitive, type SyntaxHighlighterProps, @@ -17,6 +16,7 @@ import { chunkByLines, SyntaxHighlighter } from '@/components/chat/shiki-highlig import { ZoomableImage } from '@/components/chat/zoomable-image' import { normalizeExternalUrl, openExternalLink, PrettyLink } from '@/lib/external-link' import { createMemoizedMathPlugin } from '@/lib/katex-memo' +import { parseMarkdownIntoBlocksCached } from '@/lib/markdown-blocks' import { preprocessMarkdown } from '@/lib/markdown-preprocess' import { downloadGatewayMediaFile, @@ -59,43 +59,6 @@ function preprocessWithTailRepair(text: string): string { } } -// Memoized block splitter. Streamdown calls `parseMarkdownIntoBlocks` (a full -// `marked` lex of the entire message, ~1.6ms per 28KB) inside a useMemo keyed -// on the text — but the same text is re-lexed every time a message REMOUNTS -// (virtualizer scroll, session switch). A small module-level -// LRU keyed by the exact source string removes all of those repeat parses -// with zero correctness risk (same input → same output). Streaming tail -// growth misses the cache by design (every flush is a new string) — that -// single lex is the irreducible cost. -const BLOCK_CACHE_MAX = 64 -const BLOCK_CACHE_MIN_LENGTH = 1024 -const blockCache = new Map() - -function parseMarkdownIntoBlocksCached(markdown: string): string[] { - if (markdown.length < BLOCK_CACHE_MIN_LENGTH) { - return parseMarkdownIntoBlocks(markdown) - } - - const hit = blockCache.get(markdown) - - if (hit) { - // Refresh recency (Map iteration order is insertion order). - blockCache.delete(markdown) - blockCache.set(markdown, hit) - - return hit - } - - const blocks = parseMarkdownIntoBlocks(markdown) - blockCache.set(markdown, blocks) - - if (blockCache.size > BLOCK_CACHE_MAX) { - blockCache.delete(blockCache.keys().next().value as string) - } - - return blocks -} - async function mediaSrc(path: string): Promise { if (/^(?:https?|data):/i.test(path)) { return path diff --git a/apps/desktop/src/lib/markdown-blocks.test.ts b/apps/desktop/src/lib/markdown-blocks.test.ts new file mode 100644 index 000000000000..e3f491275657 --- /dev/null +++ b/apps/desktop/src/lib/markdown-blocks.test.ts @@ -0,0 +1,111 @@ +import { parseMarkdownIntoBlocks } from '@assistant-ui/react-streamdown' +import { describe, expect, it } from 'vitest' + +import { parseMarkdownIntoBlocksCached } from './markdown-blocks' + +// The contract: streaming through the cached splitter (one call per growing +// prefix, exactly how Streamdown calls it per flush) must produce, at every +// step, the same blocks as a fresh full lex of that prefix. Byte equality — +// a divergence would change what the memoized block renderer paints. + +const CORPUS = `# Heading + +Intro paragraph with **bold**, [a link](https://example.com), \`inline\` and $x^2$ math. + +- list item one +- list item two + - nested item + +1. ordered a + +2. loose ordered b + +\`\`\`python +def f(x): + return x * 2 # comment with \`\`\` inside string? no — fence chars below +\`\`\` + +A paragraph that will be followed by a setext underline +=== + +| col a | col b | +|---|---| +| 1 | 2 | +| 3 | 4 | + +> blockquote line one +> blockquote line two +with a lazy continuation line + +
+html block content +
+ +$$ +\\int_0^1 x\\,dx = \\tfrac12 +$$ + +Final paragraph after everything, long enough to stream in pieces so the tail +block keeps getting reinterpreted while earlier blocks stay settled. +` + +// Deterministic PRNG so failures reproduce. +function mulberry32(seed: number) { + let a = seed + + return () => { + a |= 0 + a = (a + 0x6d2b79f5) | 0 + let t = Math.imul(a ^ (a >>> 15), 1 | a) + t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t + + return ((t ^ (t >>> 14)) >>> 0) / 4294967296 + } +} + +// Push the text past the cache MIN_LENGTH thresholds so the incremental +// path actually engages. +const LONG_CORPUS = Array.from({ length: 6 }, () => CORPUS).join('\n') + +describe('parseMarkdownIntoBlocksCached', () => { + it('matches a full lex at every random streaming cut (property)', () => { + for (let seed = 1; seed <= 5; seed++) { + const rand = mulberry32(seed) + let cursor = 0 + + while (cursor < LONG_CORPUS.length) { + cursor = Math.min(LONG_CORPUS.length, cursor + 1 + Math.floor(rand() * 120)) + const prefix = LONG_CORPUS.slice(0, cursor) + + expect(parseMarkdownIntoBlocksCached(prefix)).toEqual(parseMarkdownIntoBlocks(prefix)) + } + } + }) + + it('matches a full lex when streaming token-by-token through a fence boundary', () => { + const base = `${'settled paragraph one.\n\n'.repeat(100)}opening a fence now:\n` + const tail = '```js\nconst a = 1\nconst b = 2\n```\n\nafter the fence\n' + + for (let i = 1; i <= tail.length; i++) { + const text = base + tail.slice(0, i) + + expect(parseMarkdownIntoBlocksCached(text)).toEqual(parseMarkdownIntoBlocks(text)) + } + }) + + it('reconstructs the input exactly (join property the offsets rely on)', () => { + const blocks = parseMarkdownIntoBlocksCached(LONG_CORPUS) + + expect(blocks.join('')).toBe(LONG_CORPUS) + }) + + it('falls back to a full lex for non-append rewrites (edit / branch swap)', () => { + const grown = `${LONG_CORPUS}\n\nappended tail paragraph` + parseMarkdownIntoBlocksCached(grown) + + // A REWRITE that shares no prefix lineage must still be correct. + const rewritten = `completely different start\n\n${LONG_CORPUS.slice(500)}` + + expect(parseMarkdownIntoBlocksCached(rewritten)).toEqual(parseMarkdownIntoBlocks(rewritten)) + }) +}) diff --git a/apps/desktop/src/lib/markdown-blocks.ts b/apps/desktop/src/lib/markdown-blocks.ts new file mode 100644 index 000000000000..4235301b6a25 --- /dev/null +++ b/apps/desktop/src/lib/markdown-blocks.ts @@ -0,0 +1,125 @@ +import { parseMarkdownIntoBlocks } from '@assistant-ui/react-streamdown' + +/** + * Block splitting for the streaming markdown pipeline, without re-lexing the + * whole message on every token flush. + * + * `parseMarkdownIntoBlocks` is a full `marked` lex of the entire text — + * measured 3.4–9.6ms per call at 64–192KB. During streaming every flush is a + * new string, so the stock splitter pays that O(full-text) cost ~30×/s on + * long replies. Two caches remove it: + * + * 1. Exact-string LRU — a message that REMOUNTS with unchanged text + * (virtualizer scroll, session switch) reuses its parse outright. + * 2. Streaming-append cache — when the new text starts with a recently parsed + * text (the token-append case), the previous parse's blocks are reused up + * to a settled boundary and only the suffix is lexed. The boundary drops + * the previous parse's trailing whitespace-only blocks AND its last content + * block, because appended text can retroactively change how that last + * block parses (open fence, list/table continuation, setext underline, a + * lazy blockquote line). Blocks before it are separated by settled blank + * lines and cannot be affected. Cross-block reference links can't regress: + * Streamdown renders each block as an independent markdown document + * already. Verified property: `blocks.join('') === text`, and incremental + * output is asserted byte-identical to a full lex in tests across fences, + * lists, tables, setext headings, blockquotes, and HTML blocks. + * + * Any doubt — no prefix match, reconstruction mismatch — falls back to the + * full lex, i.e. exactly the previous behavior. + */ + +const EXACT_CACHE_MAX = 64 +const EXACT_CACHE_MIN_LENGTH = 1024 +const exactCache = new Map() + +// Streaming messages grow monotonically, and only a handful stream at once +// (main reply + reasoning part, maybe a tile). A tiny ring is enough; each +// entry holds the last parse for one growing text lineage. +const APPEND_CACHE_MAX = 4 +const APPEND_CACHE_MIN_LENGTH = 2048 +const appendCache: { blocks: string[]; text: string }[] = [] + +function rememberAppend(text: string, blocks: string[]): void { + if (text.length < APPEND_CACHE_MIN_LENGTH) { + return + } + + // Replace the lineage this text grew from (its cached prefix), else push. + const index = appendCache.findIndex(entry => text.startsWith(entry.text)) + + if (index !== -1) { + appendCache.splice(index, 1) + } + + appendCache.push({ blocks, text }) + + if (appendCache.length > APPEND_CACHE_MAX) { + appendCache.shift() + } +} + +function lexIncrementally(text: string): null | string[] { + const entry = appendCache.find(cached => text.length > cached.text.length && text.startsWith(cached.text)) + + if (!entry) { + return null + } + + // Settled boundary: drop trailing whitespace-only blocks, then the last + // content block (the only one appended text can reinterpret). + let keep = entry.blocks.length + + while (keep > 0 && !entry.blocks[keep - 1].trim()) { + keep -= 1 + } + + if (keep > 0) { + keep -= 1 + } + + if (keep === 0) { + return null + } + + const settled = entry.blocks.slice(0, keep) + let settledLength = 0 + + for (const block of settled) { + settledLength += block.length + } + + // Defensive reconstruction check — the splitter's join(blocks) === text + // property is what makes offsets exact. If it ever doesn't hold, full lex. + if (settledLength > entry.text.length || !text.startsWith(entry.text.slice(0, settledLength), 0)) { + return null + } + + return [...settled, ...parseMarkdownIntoBlocks(text.slice(settledLength))] +} + +export function parseMarkdownIntoBlocksCached(markdown: string): string[] { + if (markdown.length < EXACT_CACHE_MIN_LENGTH) { + return parseMarkdownIntoBlocks(markdown) + } + + const hit = exactCache.get(markdown) + + if (hit) { + // Refresh recency (Map iteration order is insertion order). + exactCache.delete(markdown) + exactCache.set(markdown, hit) + + return hit + } + + const blocks = lexIncrementally(markdown) ?? parseMarkdownIntoBlocks(markdown) + + rememberAppend(markdown, blocks) + exactCache.set(markdown, blocks) + + if (exactCache.size > EXACT_CACHE_MAX) { + exactCache.delete(exactCache.keys().next().value as string) + } + + return blocks +} From e934ee440e7b22ab0ba02d4c744d6b3a42f08085 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sat, 18 Jul 2026 18:21:58 -0400 Subject: [PATCH 02/12] fix(desktop): correct incremental markdown split boundary (setext merge) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The streaming block splitter added in #67154 dropped only the previous parse's trailing whitespace blocks plus its LAST content block before re-lexing the appended suffix. That boundary is unsound: a trailing Setext underline (`-`/`=`) underlines the paragraph ABOVE it, so appending to it can retroactively merge the previous parse's last TWO blocks into one. Minimal repro: cached "…#e\n5\n-" lexes to [ …, "#e\n", "5\n-" ], but grown to "…#e\n5\n-p2=kj:c" collapses "#e"/"5\n-" into a single block. The reused settled prefix still contained a stale "#e\n" block. The `blocks.join('') === text` guard can't detect this because the wrong split reconstructs the same source string, so the divergence rendered as mis-split blocks with no fallback. Fix: drop the last TWO content blocks (skipping whitespace-only blocks around them) before re-lexing the suffix. The block before the last is the deepest an append can reach — a Setext underline consumes exactly one preceding block — and earlier blocks stay fenced off by settled blank lines, so re-lexing two is sufficient and safe. Tests: a deterministic regression for the exact prev→grown pair, and a character-level streaming property fuzz (12 seeds × 500 growing prefixes over the markdown control alphabet). Both fail on the pre-fix boundary and pass after. tsc/eslint/prettier clean; markdown-text suite green. --- apps/desktop/src/lib/markdown-blocks.test.ts | 49 ++++++++++++++++++++ apps/desktop/src/lib/markdown-blocks.ts | 25 +++++++--- 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/apps/desktop/src/lib/markdown-blocks.test.ts b/apps/desktop/src/lib/markdown-blocks.test.ts index e3f491275657..7b55cb035184 100644 --- a/apps/desktop/src/lib/markdown-blocks.test.ts +++ b/apps/desktop/src/lib/markdown-blocks.test.ts @@ -108,4 +108,53 @@ describe('parseMarkdownIntoBlocksCached', () => { expect(parseMarkdownIntoBlocksCached(rewritten)).toEqual(parseMarkdownIntoBlocks(rewritten)) }) + + it('matches a full lex when a trailing setext underline merges the previous block (regression)', () => { + // A trailing `-`/`=` line is a setext underline of the block ABOVE it, so + // appending to it can retroactively merge the previous parse's LAST TWO + // blocks into one. Cached `"…#e\n5\n-"` lexes to [ …, "#e\n", "5\n-" ], but + // grown to `"…#e\n5\n-p2=kj:c"` collapses `#e`/`5\n-` into one block. The + // old boundary dropped only the single last content block, so it reused a + // `"#e\n"` block that no longer exists. `blocks.join('') === text` still + // holds for the wrong split, so the reconstruction guard cannot catch it. + // The settled prefix pushes the text past the append-cache threshold so + // the incremental path actually engages. + const settled = 'settled line paragraph text.\n\n'.repeat(80) + const prev = `${settled}#e\n5\n-` + const grown = `${prev}p2=kj:c` + + // Seed the append cache with `prev`, then grow it — the exact two-call + // sequence a streaming flush produces. + parseMarkdownIntoBlocksCached(prev) + + expect(parseMarkdownIntoBlocksCached(grown)).toEqual(parseMarkdownIntoBlocks(grown)) + }) + + it('matches a full lex at every char-level streaming cut over noisy markdown (property fuzz)', () => { + // Character-level append fuzz over the markdown control alphabet — the + // harness that surfaced the setext-underline merge above. Growing a single + // lineage one small chunk at a time keeps `startsWith` lineage intact so + // the incremental path runs on nearly every step; each prefix must + // deep-equal a fresh full lex. + const alphabet = '\n `#*-_>[]()|~:=abcdefghijklmnopqrstuvwxyz0123456789' + + for (let seed = 1; seed <= 12; seed++) { + const rand = mulberry32(seed) + // Seed past the append-cache threshold so the incremental path engages. + let text = `seed ${seed}\n\n`.repeat(180) + + for (let step = 0; step < 500; step++) { + const n = 1 + Math.floor(rand() * 24) + let chunk = '' + + for (let j = 0; j < n; j++) { + chunk += alphabet[Math.floor(rand() * alphabet.length)] + } + + text += chunk + + expect(parseMarkdownIntoBlocksCached(text)).toEqual(parseMarkdownIntoBlocks(text)) + } + } + }) }) diff --git a/apps/desktop/src/lib/markdown-blocks.ts b/apps/desktop/src/lib/markdown-blocks.ts index 4235301b6a25..df6a473a2747 100644 --- a/apps/desktop/src/lib/markdown-blocks.ts +++ b/apps/desktop/src/lib/markdown-blocks.ts @@ -65,16 +65,27 @@ function lexIncrementally(text: string): null | string[] { return null } - // Settled boundary: drop trailing whitespace-only blocks, then the last - // content block (the only one appended text can reinterpret). + // Settled boundary: drop the last TWO content blocks (skipping any + // whitespace-only blocks around them). Dropping only the single last content + // block is unsound: appended text can retroactively merge the previous + // parse's last two blocks into one. The trigger is a trailing Setext + // underline — `marked` only treats `-`/`=` as an underline for the paragraph + // ABOVE it, so a settled `"#e\n5\n-"` lexes as ["#e\n", "5\n-"], but growing + // the tail to `"#e\n5\n-p2=kj:c"` collapses both into one paragraph. The + // block before the last is the deepest an append can reach (the underline + // consumes exactly one preceding block), so re-lexing the last two is safe; + // earlier blocks are fenced off by settled blank lines. join('') === text + // still holds either way, so the reconstruction check below can't catch this. let keep = entry.blocks.length - while (keep > 0 && !entry.blocks[keep - 1].trim()) { - keep -= 1 - } + for (let dropped = 0; dropped < 2 && keep > 0; dropped += 1) { + while (keep > 0 && !entry.blocks[keep - 1].trim()) { + keep -= 1 + } - if (keep > 0) { - keep -= 1 + if (keep > 0) { + keep -= 1 + } } if (keep === 0) { From 3df8bd3478b96d1665bf390c07630268ae522f7e Mon Sep 17 00:00:00 2001 From: Jupiter363 <134490903+Jupiter363@users.noreply.github.com> Date: Tue, 14 Jul 2026 02:44:27 +0800 Subject: [PATCH 03/12] fix(mcp): propagate stored timeouts from completed futures --- tests/tools/test_mcp_tool.py | 66 ++++++++++++++++++++++++++++++++++++ tools/mcp_tool.py | 7 ++++ 2 files changed, 73 insertions(+) diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index c2208165fa4b..a66235040284 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -4,6 +4,7 @@ All tests use mocks -- no real MCP servers or subprocesses are started. """ import asyncio +import concurrent.futures import json import threading import time @@ -837,6 +838,23 @@ class TestToolHandler: class TestRunOnMCPLoopInterrupts: + @staticmethod + def _run_with_future(mcp_mod, future): + loop = MagicMock() + loop.is_running.return_value = True + + async def _unused_call(): + return "unused" + + def _schedule(coro, scheduled_loop, **_kwargs): + assert scheduled_loop is loop + coro.close() + return future + + with patch.object(mcp_mod, "_mcp_loop", loop): + with patch("agent.async_utils.safe_schedule_threadsafe", side_effect=_schedule): + return mcp_mod._run_on_mcp_loop(_unused_call(), timeout=1) + def test_interrupt_cancels_waiting_mcp_call(self): import tools.mcp_tool as mcp_mod from tools.interrupt import set_interrupt @@ -922,6 +940,54 @@ class TestRunOnMCPLoopInterrupts: mcp_mod._mcp_loop = old_loop mcp_mod._mcp_thread = old_thread + def test_completed_future_timeout_is_propagated_once(self): + import tools.mcp_tool as mcp_mod + + inner_error = TimeoutError("inner MCP timeout") + + class CompletedWithTimeout(concurrent.futures.Future): + def __init__(self): + super().__init__() + self.result_timeouts = [] + self.set_exception(inner_error) + + def result(self, timeout=None): + self.result_timeouts.append(timeout) + return super().result(timeout=timeout) + + future = CompletedWithTimeout() + + with pytest.raises(TimeoutError, match="inner MCP timeout") as exc_info: + self._run_with_future(mcp_mod, future) + + assert exc_info.value is inner_error + assert len(future.result_timeouts) == 2 + assert future.result_timeouts[0] is not None + assert future.result_timeouts[1] is None + + def test_poll_timeout_racing_success_returns_completed_result(self): + import tools.mcp_tool as mcp_mod + + class PollThenSuccess(concurrent.futures.Future): + def __init__(self): + super().__init__() + self.result_timeouts = [] + + def result(self, timeout=None): + self.result_timeouts.append(timeout) + if len(self.result_timeouts) == 1: + self.set_result("completed") + raise concurrent.futures.TimeoutError + + return super().result(timeout=timeout) + + future = PollThenSuccess() + + assert self._run_with_future(mcp_mod, future) == "completed" + assert len(future.result_timeouts) == 2 + assert future.result_timeouts[0] is not None + assert future.result_timeouts[1] is None + # --------------------------------------------------------------------------- # Tool registration (discovery + register) diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 3e6844de3d0b..7da89baff599 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -3909,6 +3909,13 @@ def _run_on_mcp_loop(coro_or_factory, timeout: float = 30): try: return future.result(timeout=wait_timeout) except concurrent.futures.TimeoutError: + # On supported Python versions, concurrent.futures.TimeoutError + # aliases the built-in TimeoutError, so result(timeout=...) also + # raises it for a coroutine's own timeout. + # Resolve a done future without a timeout to propagate its stored + # outcome, including completion racing with this polling timeout. + if future.done(): + return future.result() continue From 97249cfc8abb9053c518552cbec88ec45aee0d76 Mon Sep 17 00:00:00 2001 From: PRATHAMESH75 Date: Sat, 18 Jul 2026 22:09:56 +0530 Subject: [PATCH 04/12] fix(install): keep install.ps1 pure ASCII so Windows PowerShell 5.1 doesn't misparse it A commit added a bullet and an em-dash inside two Write-Host/Write-Info string literals in scripts/install.ps1. The file has no UTF-8 BOM, so Windows PowerShell 5.1 (which the bootstrap runs the cached script under) reads it in the system ANSI code page (CP1252), not UTF-8. The em-dash's UTF-8 tail byte decodes to a smart close-quote (U+201D) that the tokenizer treats as a string delimiter, prematurely closing the string and desyncing the parser -- surfacing as the reported cascade of syntax errors at lines 1619/1770 and aborting the Windows GUI installer before it does anything. Non-ASCII bytes in '#' comments are harmless (skipped to end-of-line), so the file carried em-dashes in comments for months; only the two chars in code broke it. Convert all non-ASCII to ASCII equivalents (em-dash -> '--', already this file's own comment convention; bullet -> '-') and add a source-level test locking the pure-ASCII invariant, since Linux CI cannot run the PS installer. Fixes #66994 Fixes #67000 --- scripts/install.ps1 | 54 +++++++++++++-------------- tests/test_install_ps1_ascii_only.py | 55 ++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 27 deletions(-) create mode 100644 tests/test_install_ps1_ascii_only.py diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 8974519ef4a5..746b5090faaf 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -443,7 +443,7 @@ function Get-PowerShellHostExe { } function Install-Uv { - # Hermes owns its own uv at $HermesHome\bin\uv.exe. Always install there — + # Hermes owns its own uv at $HermesHome\bin\uv.exe. Always install there -- # no PATH probing, no conda guards, no multi-location resolution chains. # The runtime update path (hermes_cli/managed_uv.py) looks in the same # place, so install.ps1 and `hermes update` stay in sync. @@ -528,7 +528,7 @@ function Ensure-NodeExeOnPath { # prior process is not visible here. Later stages (Test-Python, # Install-Venv, Install-Dependencies, Install-PlatformSdks) call this # at the top to populate $script:UvCmd from the managed location. -# Throws if uv is not findable — the caller's stage then surfaces a +# Throws if uv is not findable -- the caller's stage then surfaces a # clean error via the stage-driver's try/catch. function Resolve-UvCmd { # Already resolved (default invocation path: Install-Uv ran earlier @@ -544,7 +544,7 @@ function Resolve-UvCmd { # Stale; fall through to re-discover. } - # Check the managed location first — this is where Install-Uv puts it. + # Check the managed location first -- this is where Install-Uv puts it. $managedUv = Join-Path $HermesHome "bin\uv.exe" if (Test-Path $managedUv) { $script:UvCmd = $managedUv @@ -1516,7 +1516,7 @@ function Install-Repository { if ($LASTEXITCODE -ne 0) { throw "git checkout $Branch failed (exit $LASTEXITCODE)" } # Managed installs should follow origin/$Branch exactly. If # the checkout has diverged (or has local-only commits), - # ff-only pull cannot succeed — mirror ``hermes update`` and + # ff-only pull cannot succeed -- mirror ``hermes update`` and # reset to the fetched remote so bootstrap/install can recover. git -c windows.appendAtomically=false pull --ff-only origin $Branch if ($LASTEXITCODE -ne 0) { @@ -1573,11 +1573,11 @@ function Install-Repository { Write-Host "" Write-Host "Conflicted files:" foreach ($file in $conflictedFiles) { - Write-Host " • $file" + Write-Host " - $file" } } Write-Host "" - Write-Info "Your stashed changes are preserved — nothing is lost." + Write-Info "Your stashed changes are preserved -- nothing is lost." Write-Info " Stash ref: $autostashRef" git -c windows.appendAtomically=false reset --hard HEAD 2>$null | Out-Null Write-Info "Working tree reset to clean state." @@ -1798,8 +1798,8 @@ function Install-Venv { # /End stops a running task instance; /Change /DISABLE stops it # from re-firing mid-install. (The Startup-folder .vbs fallback is # NOT touched: it only fires at logon, so it cannot respawn a - # gateway mid-install.) Re-enabled in the finally below — including - # on failure — but only for tasks that were enabled to begin with. + # gateway mid-install.) Re-enabled in the finally below -- including + # on failure -- but only for tasks that were enabled to begin with. # Best-effort: a missing task just errors quietly. try { schtasks /Query /FO CSV 2>$null | ConvertFrom-Csv | Where-Object { $_.TaskName -like '*Hermes_Gateway*' } | ForEach-Object { @@ -1893,7 +1893,7 @@ function Install-Venv { } # Clean up parked venvs from previous installs whose handles have since - # been released. Best-effort — a still-held tree just stays for next time. + # been released. Best-effort -- a still-held tree just stays for next time. Get-ChildItem -Directory -Filter "venv.stale.*" -ErrorAction SilentlyContinue | ForEach-Object { Remove-Item -Recurse -Force $_.FullName -ErrorAction SilentlyContinue } @@ -1926,10 +1926,10 @@ function Install-Venv { } finally { Pop-Location # Re-arm the gateway autostart tasks disabled during the venv teardown - # — in a finally so a failed teardown/creation can never strand the + # -- in a finally so a failed teardown/creation can never strand the # user's gateway autostart in the disabled state. Same function scope, # so the list survives even under the stage-per-process bootstrap. - # Deliberately NOT started here — dependencies aren't installed yet; + # Deliberately NOT started here -- dependencies aren't installed yet; # the task fires normally on next logon and `hermes update` / the # gateway resume path handles the immediate restart. if ($gatewayTasksDisabled -and $gatewayTasksDisabled.Count -gt 0) { @@ -2247,7 +2247,7 @@ function Set-PathVariable { function Write-BootstrapMarker { # Writes $InstallDir\.hermes-bootstrap-complete which tells the Hermes # desktop app (apps/desktop/electron/main.ts) "install.ps1 ran - # successfully — DON'T trigger the legacy first-launch bootstrap + # successfully -- DON'T trigger the legacy first-launch bootstrap # runner." # # Schema mirrors what main.ts's writeBootstrapMarker() / isBootstrap @@ -2268,7 +2268,7 @@ function Write-BootstrapMarker { # Resolve the pinned commit: explicit -Commit wins, otherwise read # the checkout's HEAD via git. If git can't run, leave commit empty # and the marker will fail desktop validation (pinnedCommit.length - # >= 7) — better to be invalid than wrong. + # >= 7) -- better to be invalid than wrong. $pinnedCommit = $Commit if (-not $pinnedCommit) { # PS 5.1 doesn't support the ?. null-conditional operator, so @@ -2283,7 +2283,7 @@ function Write-BootstrapMarker { $pinnedCommit = $resolved.Trim() } } catch { - # Ignore — pinnedCommit stays empty, marker stays invalid, + # Ignore -- pinnedCommit stays empty, marker stays invalid, # desktop falls through to its legacy bootstrap path. } finally { Pop-Location @@ -2302,7 +2302,7 @@ function Write-BootstrapMarker { pinnedCommit = $pinnedCommit pinnedBranch = $pinnedBranch completedAt = (Get-Date).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.fffZ") - # desktopVersion field intentionally omitted — only the desktop + # desktopVersion field intentionally omitted -- only the desktop # app knows its own version, and the marker validator doesn't # require it. The desktop fills it in if/when it writes its # own marker (e.g. after a future in-app upgrade). @@ -2311,7 +2311,7 @@ function Write-BootstrapMarker { # Write WITHOUT a UTF-8 BOM. PowerShell 5.1's `Set-Content -Encoding UTF8` # always emits a BOM, and Node's plain JSON.parse rejects the BOM as an - # unexpected character — so a BOM'd marker would silently fail the + # unexpected character -- so a BOM'd marker would silently fail the # desktop's readJson(), make isBootstrapComplete() return null, and the # desktop would re-run the legacy bootstrap runner anyway. Defeats the # whole point. Use the .NET API directly for BOM-less UTF-8. @@ -2411,7 +2411,7 @@ function Install-NodeDeps { # Cross-process driver mode (Hermes-Setup.exe runs each -Stage NAME # in a fresh powershell.exe) means $script:HasNode set by Stage-Node # in the previous process isn't visible here. Re-probe rather than - # trust the stale global — Stage-Node already ran successfully or + # trust the stale global -- Stage-Node already ran successfully or # the bootstrap would've aborted, so npm is reachable. if (-not (Get-Command npm -ErrorAction SilentlyContinue)) { Write-Info "Skipping Node.js dependencies (Node not installed)" @@ -2688,7 +2688,7 @@ function Clear-ElectronBuildCache { # Last-resort Electron mirror after GitHub download fails (#47266). $script:DesktopElectronFallbackMirror = "https://npmmirror.com/mirrors/electron/" -# Electron package dir — workspace-local nest first, then root hoist. +# Electron package dir -- workspace-local nest first, then root hoist. function Get-ElectronDir { param([string]$InstallDir) $desktopLocal = Join-Path $InstallDir 'apps\desktop\node_modules\electron' @@ -2769,7 +2769,7 @@ function Install-Desktop { # # The Tauri bootstrap installer's launch_hermes_desktop command # resolves apps/desktop/release/win-unpacked/Hermes.exe directly, - # so an "unpacked" build (electron-builder --dir) is enough — we + # so an "unpacked" build (electron-builder --dir) is enough -- we # don't need to produce an NSIS/MSI artifact here. # Always re-resolve Node here. Stages run in separate PowerShell processes, @@ -2823,7 +2823,7 @@ function Install-Desktop { # # The streaming sink in bootstrap.rs's run_install_script # captures every stdout/stderr line as it's emitted, so we don't - # need a side TEMP log file — the installer's bootstrap log + # need a side TEMP log file -- the installer's bootstrap log # IS the artifact a support engineer reads. # # Prefer `npm ci`: it wipes node_modules and reinstalls from the @@ -2878,7 +2878,7 @@ function Install-Desktop { # NOT signing the output. Combined with signAndEditExecutable=false in # apps/desktop/package.json's build.win block, electron-builder never # invokes signtool and therefore never fetches/extracts winCodeSign - # (whose macOS symlinks crash 7-Zip on non-admin Windows — a dead end we + # (whose macOS symlinks crash 7-Zip on non-admin Windows -- a dead end we # are NOT trying to work around). The Hermes icon + product name are # stamped onto Hermes.exe by our own rcedit step (Set-DesktopExeIdentity) # AFTER this build, completely decoupled from electron-builder signing. @@ -2949,7 +2949,7 @@ function Install-Desktop { Pop-Location throw } finally { - # Restore env to whatever the caller had — don't leak our + # Restore env to whatever the caller had -- don't leak our # signing-off override into anything install.ps1 invokes later # (Stage-PlatformSdks, etc.). $env:CSC_IDENTITY_AUTO_DISCOVERY = $prevCSCAuto @@ -2980,7 +2980,7 @@ function Install-Desktop { # 3b. The Hermes icon + identity are stamped onto Hermes.exe by the # electron-builder `afterPack` hook (apps/desktop/scripts/after-pack.mjs) - # during `npm run pack` above — for every build, so the installer's + # during `npm run pack` above -- for every build, so the installer's # --update rebuild stays branded too. No separate stamp step needed here. # electron-builder's own rcedit step stays disabled (signAndEditExecutable # =false) because enabling it drags in signtool -> winCodeSign -> the @@ -2990,7 +2990,7 @@ function Install-Desktop { # directory. Chromium's GPU/renderer sandboxes CHECK-fail with # 0x80000003 when this ACE is missing alongside orphan AppContainer # SIDs under %LOCALAPPDATA% (electron/electron#51761, hermes-agent#38216). - # Best-effort — never fail an otherwise-good install over ACL repair. + # Best-effort -- never fail an otherwise-good install over ACL repair. try { $appDir = Split-Path -Parent $desktopExe & icacls $appDir /grant "*S-1-15-2-2:(OI)(CI)(RX)" /T /C /Q | Out-Null @@ -3006,7 +3006,7 @@ function Install-Desktop { # 4. Create Start Menu + Desktop shortcuts pointing DIRECTLY at the packed # Hermes.exe. We deliberately do NOT point them at `hermes desktop`: that # command rebuilds (npm install + electron-builder) on every launch, - # which would cost minutes each time. The packed exe is the consumer — + # which would cost minutes each time. The packed exe is the consumer -- # launching it directly is instant, and updates flow through the # installer's --update path (which rebuilds once, then relaunches). New-DesktopShortcuts -TargetExe $desktopExe @@ -3062,11 +3062,11 @@ function New-DesktopShortcuts { # cached bitmap. Critical on the --update path: the exe was re-stamped # with the Hermes icon, but without this the shortcut can keep drawing # the old Electron icon until the user manually refreshes / reboots. - # Best-effort and silent — never fail the install over a cosmetic cache. + # Best-effort and silent -- never fail the install over a cosmetic cache. try { & ie4uinit.exe -show 2>$null } catch { - # ie4uinit may be absent/renamed on some SKUs — ignore. + # ie4uinit may be absent/renamed on some SKUs -- ignore. } } catch { Write-Warn "Skipping shortcut creation: $($_.Exception.Message)" diff --git a/tests/test_install_ps1_ascii_only.py b/tests/test_install_ps1_ascii_only.py new file mode 100644 index 000000000000..71638620c2a5 --- /dev/null +++ b/tests/test_install_ps1_ascii_only.py @@ -0,0 +1,55 @@ +"""Regression: install.ps1 must stay pure ASCII. + +Issues #66994 / #67000 reported the Windows GUI installer (Hermes-Setup.exe) +crashing before it did anything, with a cascade of PowerShell parser errors at +lines 1619 / 1770 ("Missing argument in parameter list", "A 'using' statement +must appear before any other statements in a script"). + +Root cause: ``scripts/install.ps1`` has no UTF-8 BOM, and a commit added two +non-ASCII characters *inside double-quoted string literals* (a bullet and an +em-dash). Windows PowerShell 5.1 -- which the bootstrap runs the cached script +under -- reads a BOM-less ``.ps1`` in the system ANSI code page (e.g. CP1252), +not UTF-8. The em-dash's UTF-8 tail byte (0x94) decodes to a "smart" close-quote +(U+201D), which the PowerShell tokenizer treats as a string delimiter. That +prematurely closes the string and desyncs the parser for the rest of the file, +surfacing as unrelated syntax errors far downstream. + +Non-ASCII bytes inside ``#`` comments are harmless (the tokenizer skips a +comment to end-of-line regardless of what it contains), which is why the file +carried em-dashes in comments for months without breaking -- only a non-ASCII +byte in *code* (a string literal) triggers the desync. + +This test is source-level because Linux CI cannot execute the PowerShell +installer. Keeping the whole file ASCII-only is the transport-independent +invariant: pure ASCII cannot be misdecoded under any code page, BOM or not, so +the bug class cannot recur -- in a comment or a string. +""" + +from __future__ import annotations + +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parent.parent +INSTALL_PS1 = REPO_ROOT / "scripts" / "install.ps1" + + +def test_install_ps1_is_pure_ascii() -> None: + raw = INSTALL_PS1.read_bytes() + + offenders = [] + line_no = 1 + for byte in raw: + if byte == 0x0A: + line_no += 1 + elif byte >= 0x80: + offenders.append(line_no) + + assert not offenders, ( + "scripts/install.ps1 must be pure ASCII so Windows PowerShell 5.1 " + "(which reads a BOM-less .ps1 in the system ANSI code page, not UTF-8) " + "cannot misdecode a byte into a stray quote and desync the parser " + "(issues #66994 / #67000). Non-ASCII bytes found on line(s): " + f"{sorted(set(offenders))}. Use ASCII equivalents (em-dash -> '--', " + "bullet -> '-')." + ) From 1cec5c69d38fe1a1f2d0e1de3935d9995d4202e4 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sat, 18 Jul 2026 18:49:56 -0400 Subject: [PATCH 05/12] test(mcp): e2e integration coverage for #63892 poll-loop OOM spin The salvaged unit tests hand-construct completed futures to lock the _run_on_mcp_loop contract. Add a live-loop test that reproduces the actual field trigger: an inner asyncio.wait_for expiry stores a real TimeoutError on a real future scheduled on the MCP loop. Asserts the fixed loop surfaces it once, promptly -- not spinning to the outer deadline (which both leaked memory and masked the real error behind the generic wrapper message). --- .../test_mcp_poll_loop_oom_integration.py | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 tests/tools/test_mcp_poll_loop_oom_integration.py diff --git a/tests/tools/test_mcp_poll_loop_oom_integration.py b/tests/tools/test_mcp_poll_loop_oom_integration.py new file mode 100644 index 000000000000..ec2b1766f667 --- /dev/null +++ b/tests/tools/test_mcp_poll_loop_oom_integration.py @@ -0,0 +1,68 @@ +"""End-to-end coverage for the MCP poll-loop OOM spin (#63892). + +The unit tests in ``test_mcp_tool.py`` hand-construct completed futures to lock +the ``_run_on_mcp_loop`` contract deterministically. This complements them by +exercising the actual field trigger through the *live* MCP loop: an inner +``asyncio.wait_for`` expiry produces a real ``TimeoutError`` stored on a real +future scheduled via ``run_coroutine_threadsafe``. On Python >= 3.8 that class +is the builtin ``TimeoutError``, so before the fix the poll loop swallowed the +completed future's stored exception and spun with no sleep -- burning CPU, +growing the exception traceback ~108 MB/s until the gateway OOM'd, and finally +masking the real error behind the generic "MCP call timed out after " wrapper. + +The fixed loop must surface the real exception once, promptly -- long before +the outer deadline. +""" + +from __future__ import annotations + +import asyncio +import time + +import pytest + + +@pytest.fixture +def mcp_loop(): + import tools.mcp_tool as mcp_tool + + mcp_tool._ensure_mcp_loop() + yield mcp_tool + mcp_tool._stop_mcp_loop() + + +def test_inner_wait_for_timeout_surfaces_promptly_without_spinning(mcp_loop): + async def inner(): + # Real TimeoutError from wait_for, completing far before the outer + # deadline below -- the exact shape of an MCP call_tool wrapped in the + # server's configured mcp_servers..timeout. + await asyncio.wait_for(asyncio.sleep(60), timeout=0.05) + + start = time.monotonic() + with pytest.raises(TimeoutError) as exc: + mcp_loop._run_on_mcp_loop(inner, timeout=10) + elapsed = time.monotonic() - start + + # Fixed: the real inner TimeoutError surfaces within a couple poll ticks. + # Broken: the loop swallows it and spins with no sleep until the 10s + # deadline, then raises the generic wrapper message instead. A generous + # (>= 2s) bound keeps this stable on a slow CI runner while still failing + # hard on a spin-to-deadline regression. + assert elapsed < 5.0, ( + f"poll loop spun for {elapsed:.1f}s instead of resolving the completed " + "future once (#63892 regression)" + ) + assert "MCP call timed out after" not in str(exc.value), ( + "the real inner TimeoutError must surface, not the outer poll deadline" + ) + + +def test_successful_call_still_returns_through_real_loop(mcp_loop): + """Guard the done-future path against regressing normal success returns.""" + + async def inner(): + await asyncio.sleep(0.25) # first polls time out while still pending + return {"ok": True} + + assert mcp_loop._run_on_mcp_loop(inner, timeout=10) == {"ok": True} From d8b59bd60e05fadadfc8e8d36289499cb7ed27ca Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sat, 18 Jul 2026 18:53:41 -0400 Subject: [PATCH 06/12] test(desktop): raise timeout on markdown-blocks property fuzz MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The char-level streaming fuzz runs 12 seeds × 500 growing prefixes (~6000 full+cached lexes) and first trips the pre-fix boundary at seed 11 / step 257, so the workload can't shrink without gutting the guard. The work is bounded but exceeds Vitest's 5s per-test default on CI workers under parallelism, so give this one test an explicit 30s timeout instead of weakening coverage. --- apps/desktop/src/lib/markdown-blocks.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/lib/markdown-blocks.test.ts b/apps/desktop/src/lib/markdown-blocks.test.ts index 7b55cb035184..ff8d9495c6d4 100644 --- a/apps/desktop/src/lib/markdown-blocks.test.ts +++ b/apps/desktop/src/lib/markdown-blocks.test.ts @@ -130,6 +130,10 @@ describe('parseMarkdownIntoBlocksCached', () => { expect(parseMarkdownIntoBlocksCached(grown)).toEqual(parseMarkdownIntoBlocks(grown)) }) + // 12 seeds × 500 growing prefixes is ~6000 full+cached lexes; it first trips + // the pre-fix boundary at seed 11 / step 257, so the workload can't shrink + // without gutting the guard. The work is bounded but exceeds one test's 5s + // default budget, so raise the timeout rather than weaken the coverage. it('matches a full lex at every char-level streaming cut over noisy markdown (property fuzz)', () => { // Character-level append fuzz over the markdown control alphabet — the // harness that surfaced the setext-underline merge above. Growing a single @@ -156,5 +160,5 @@ describe('parseMarkdownIntoBlocksCached', () => { expect(parseMarkdownIntoBlocksCached(text)).toEqual(parseMarkdownIntoBlocks(text)) } } - }) + }, 30_000) }) From 4441e11c7751fe6ccf2ccdcbffd7b1d19da6a079 Mon Sep 17 00:00:00 2001 From: Paulo Nascimento Date: Fri, 17 Jul 2026 15:37:41 -0400 Subject: [PATCH 07/12] fix(cli): quote .env values with internal whitespace in save_env_value _quote_env_value previously left internal spaces unquoted (only #/"/' and leading/trailing whitespace triggered). Spaced macOS paths written via hermes setup SSH / Google Chat SA path / hermes config set produced lines that python-dotenv still parsed but shell `set -a; . file` word-split. Extend needs_quoting with any(c.isspace()); escaping dialect unchanged. --- hermes_cli/config.py | 5 + tests/hermes_cli/test_config.py | 206 +++++++++++++++++++++++++++++++- 2 files changed, 210 insertions(+), 1 deletion(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index fc9d75470aeb..f29ce777427b 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -7829,11 +7829,16 @@ def _quote_env_value(value: str) -> str: """Quote .env values containing characters with special dotenv meaning.""" if value == "": return value + # Internal whitespace (space/tab/etc.) must be quoted so shell `set -a; . file` + # word-splits don't break paths like macOS "Application Support". Leading/ + # trailing whitespace is already covered by the strip check; any() covers + # internal runs that strip() would leave alone. needs_quoting = ( "#" in value or '"' in value or "'" in value or value != value.strip() + or any(c.isspace() for c in value) ) if not needs_quoting: return value diff --git a/tests/hermes_cli/test_config.py b/tests/hermes_cli/test_config.py index cc1d0d361617..1d9c396fda60 100644 --- a/tests/hermes_cli/test_config.py +++ b/tests/hermes_cli/test_config.py @@ -607,6 +607,210 @@ class TestSaveEnvValueSecure: assert parsed["ANTHROPIC_TOKEN"] == token assert load_env()["ANTHROPIC_TOKEN"] == token + def test_save_env_value_quotes_values_with_internal_spaces(self, tmp_path): + """Internal spaces must be quoted so shell-sourcing does not word-split. + + Sibling of installer #57247: core writer left + TERMINAL_SSH_KEY=/Users/.../Application Support/... unquoted. + python-dotenv still parsed it; ``set -a; . file`` failed. + """ + import subprocess + from dotenv import dotenv_values + + path = "/Users/paulo/Library/Application Support/hermes/keys/id_ed25519" + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}, clear=False): + os.environ.pop("TERMINAL_SSH_KEY", None) + save_env_value("TERMINAL_SSH_KEY", path) + + env_path = tmp_path / ".env" + content = env_path.read_text(encoding="utf-8") + assert f'TERMINAL_SSH_KEY="{path}"' in content + + parsed = dotenv_values(str(env_path)) + assert parsed["TERMINAL_SSH_KEY"] == path + assert load_env()["TERMINAL_SSH_KEY"] == path + + # Shell source must round-trip (this is what the bug broke). + r = subprocess.run( + [ + "env", + "-i", + "sh", + "-c", + f"set -a; . '{env_path}'; set +a; " + f'printf "%s" "$TERMINAL_SSH_KEY"', + ], + capture_output=True, + text=True, + ) + assert r.returncode == 0, r.stderr + assert r.stderr == "" + assert r.stdout == path + + def test_save_env_value_quotes_values_with_tabs(self, tmp_path): + """Tabs trigger quoting; round-trip via dotenv and shell source.""" + import subprocess + from dotenv import dotenv_values + + value = "left\tright" + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}, clear=False): + os.environ.pop("TABBY_KEY", None) + save_env_value("TABBY_KEY", value) + + env_path = tmp_path / ".env" + content = env_path.read_text(encoding="utf-8") + assert f'TABBY_KEY="{value}"' in content + + parsed = dotenv_values(str(env_path)) + assert parsed["TABBY_KEY"] == value + assert load_env()["TABBY_KEY"] == value + + r = subprocess.run( + [ + "env", + "-i", + "sh", + "-c", + f"set -a; . '{env_path}'; set +a; " + f'printf "%s" "$TABBY_KEY"', + ], + capture_output=True, + text=True, + ) + assert r.returncode == 0, r.stderr + assert r.stderr == "" + assert r.stdout == value + + def test_save_env_value_spaced_path_is_idempotent(self, tmp_path): + """Saving the same spaced value twice must not grow quotes.""" + path = "/Users/me/Application Support/key" + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}, clear=False): + os.environ.pop("TERMINAL_SSH_KEY", None) + save_env_value("TERMINAL_SSH_KEY", path) + first = (tmp_path / ".env").read_text(encoding="utf-8") + save_env_value("TERMINAL_SSH_KEY", path) + second = (tmp_path / ".env").read_text(encoding="utf-8") + + assert first == second + assert first.count('TERMINAL_SSH_KEY="') == 1 + assert '""' not in first + assert f'TERMINAL_SSH_KEY="{path}"' in first + + def test_save_env_value_readback_resave_is_idempotent(self, tmp_path): + """hermes setup path: dotenv unquotes, then re-save must not grow quotes.""" + from dotenv import dotenv_values + + path = "/Users/me/Application Support/key" + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}, clear=False): + os.environ.pop("TERMINAL_SSH_KEY", None) + save_env_value("TERMINAL_SSH_KEY", path) + first = (tmp_path / ".env").read_text(encoding="utf-8") + + # Real read-back boundary (what setup uses via get_env_value/dotenv). + read_back = dotenv_values(str(tmp_path / ".env"))["TERMINAL_SSH_KEY"] + assert read_back == path + save_env_value("TERMINAL_SSH_KEY", read_back) + second = (tmp_path / ".env").read_text(encoding="utf-8") + + assert first == second + assert f'TERMINAL_SSH_KEY="{path}"' in second + + def test_save_env_value_strips_newlines_before_quoting(self, tmp_path): + """save_env_value strips \\n/\\r before _quote_env_value; result is one line. + + Pins the boundary so any(c.isspace()) never quotes multi-line dotenv + values through this writer (newlines never reach the quoter). + """ + from dotenv import dotenv_values + + raw = "line1\nline2\rline3" + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}, clear=False): + os.environ.pop("MULTI_KEY", None) + save_env_value("MULTI_KEY", raw) + + content = (tmp_path / ".env").read_text(encoding="utf-8") + # Single KEY= line, no embedded raw newlines in the value payload. + lines = [ln for ln in content.splitlines() if ln.startswith("MULTI_KEY=")] + assert len(lines) == 1 + assert "\n" not in lines[0] + assert "\r" not in lines[0] + # Newlines stripped -> "line1line2line3" has no whitespace -> unquoted. + assert lines[0] == "MULTI_KEY=line1line2line3" + parsed = dotenv_values(str(tmp_path / ".env")) + assert parsed["MULTI_KEY"] == "line1line2line3" + + def test_save_env_value_simple_values_stay_unquoted(self, tmp_path): + """No quoting churn: plain values remain bare; untouched lines unchanged.""" + env_path = tmp_path / ".env" + # Pre-existing lines: one simple, one already correctly bare. + env_path.write_text( + "KEEP_SIMPLE=plainvalue\n" + "OTHER_KEY=foo123\n", + encoding="utf-8", + ) + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}, clear=False): + os.environ.pop("NEW_KEY", None) + os.environ.pop("KEEP_SIMPLE", None) + save_env_value("NEW_KEY", "bar-simple") + + content = env_path.read_text(encoding="utf-8") + # Newly written simple value is unquoted. + assert "NEW_KEY=bar-simple\n" in content + assert 'NEW_KEY="' not in content + # Untouched pre-existing simple lines are not re-quoted. + assert "KEEP_SIMPLE=plainvalue\n" in content + assert "OTHER_KEY=foo123\n" in content + assert 'KEEP_SIMPLE="' not in content + assert 'OTHER_KEY="' not in content + + def test_save_env_value_does_not_requote_untouched_spaced_lines(self, tmp_path): + """Mass-requote guard: rewriting another key leaves legacy spaced + lines as-is (fix only applies when that key is saved again). + """ + env_path = tmp_path / ".env" + legacy = ( + "TERMINAL_SSH_KEY=/Users/me/Application Support/key\n" + "PLAIN=ok\n" + ) + env_path.write_text(legacy, encoding="utf-8") + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}, clear=False): + os.environ.pop("PLAIN", None) + save_env_value("PLAIN", "ok2") + + content = env_path.read_text(encoding="utf-8") + # Legacy spaced line not re-serialized by this write. + assert ( + "TERMINAL_SSH_KEY=/Users/me/Application Support/key\n" in content + ) + assert 'TERMINAL_SSH_KEY="' not in content + assert "PLAIN=ok2\n" in content + + def test_save_env_value_already_quoted_input_is_not_double_wrapped_idempotently( + self, tmp_path + ): + """Callers pass raw values; if a value literally contains quote + characters, escaping+wrap is the dialect (#57249). Re-saving the + same raw value is stable (no quote growth). load_env round-trips. + """ + # User-typed value that already includes surrounding quotes as data. + raw = '"/Users/me/Application Support/key"' + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}, clear=False): + os.environ.pop("TERMINAL_SSH_KEY", None) + save_env_value("TERMINAL_SSH_KEY", raw) + first = (tmp_path / ".env").read_text(encoding="utf-8") + save_env_value("TERMINAL_SSH_KEY", raw) + second = (tmp_path / ".env").read_text(encoding="utf-8") + assert first == second + # One outer wrap layer only (escaped inner quotes, not nested wraps). + line = [ + ln for ln in first.splitlines() if ln.startswith("TERMINAL_SSH_KEY=") + ][0] + assert line.startswith('TERMINAL_SSH_KEY="') + assert line.endswith('"') + assert line.count('TERMINAL_SSH_KEY="') == 1 + # Escaping dialect end-to-end: load sees the raw input, not stripped quotes. + assert load_env()["TERMINAL_SSH_KEY"] == raw + class TestRemoveEnvValue: def test_removes_key_from_env_file(self, tmp_path): @@ -726,7 +930,7 @@ class TestSaveConfigAtomicity: # Read raw YAML to verify it's valid and correct config_path = tmp_path / "config.yaml" - with open(config_path) as f: + with open(config_path, encoding="utf-8") as f: raw = yaml.safe_load(f) assert raw["model"] == "test/atomic-model" assert raw["agent"]["max_turns"] == 77 From 7d597cc5d47eaaa4f8de35b092a48655d4270114 Mon Sep 17 00:00:00 2001 From: Paulo Nascimento Date: Fri, 17 Jul 2026 15:05:19 -0400 Subject: [PATCH 08/12] fix(cli): sanitize UTF-16 .env without corrupting the first key Notepad "Unicode" saves write UTF-16 with a BOM. The sanitizer decoded those bytes as utf-8-sig with errors=replace, glued U+FFFD onto the first key name, stripped NULs, and rewrote the mangled content permanently. Sniff leading BOMs before any text decode (UTF-32 before UTF-16, because UTF-32-LE's BOM starts with UTF-16-LE's FF FE). Decode UTF-16 correctly and rewrite as clean UTF-8. Refuse UTF-32 (leave untouched + warning). After errors=replace, do not persist a first line that starts with U+FFFD. Does not touch _load_dotenv_with_fallback (#65124's surface). --- hermes_cli/env_loader.py | 69 +++++++- tests/hermes_cli/test_env_loader.py | 237 ++++++++++++++++++++++++++++ 2 files changed, 301 insertions(+), 5 deletions(-) diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index 3ea53f7aaa7c..8018280d5b98 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -2,6 +2,8 @@ from __future__ import annotations +import codecs +import io import os import sys from pathlib import Path @@ -176,6 +178,12 @@ def _sanitize_env_file_if_needed(path: Path) -> None: with ``ValueError: embedded null byte`` — typically introduced by copy-pasting API keys from terminals or rich-text editors. + Encoding: sniffs a leading BOM *before* any text decode. UTF-16 + (Notepad "Unicode") is decoded correctly and rewritten as clean + UTF-8. UTF-32 is refused (left untouched) so we never fall through + to the errors=replace corruption path. Order of BOM checks matters: + UTF-32-LE's BOM starts with UTF-16-LE's FF FE. + We delegate to ``hermes_cli.config._sanitize_env_lines`` which already knows all valid Hermes env-var names and can split concatenated lines correctly. @@ -187,16 +195,67 @@ def _sanitize_env_file_if_needed(path: Path) -> None: except ImportError: return # early bootstrap — config module not available yet - read_kw = {"encoding": "utf-8-sig", "errors": "replace"} try: - with open(path, **read_kw) as f: - original = f.readlines() + raw = path.read_bytes() + except Exception: + return + + # Sniff leading BOM bytes BEFORE decoding. ORDER MATTERS: + # codecs.BOM_UTF32_LE is FF FE 00 00, which startswith + # codecs.BOM_UTF16_LE (FF FE). Checking UTF-16 first would + # misdetect UTF-32-LE as UTF-16-LE and mangle the file. + force_utf8_rewrite = False + if raw.startswith(codecs.BOM_UTF32_LE) or raw.startswith(codecs.BOM_UTF32_BE): + # Lazy import keeps the module import block identical to #65124's + # codecs/io additions so the two PRs auto-merge either order. + import logging + + logging.getLogger(__name__).warning( + "Skipping .env sanitize for %s: UTF-32 BOM detected; " + "leaving file untouched to avoid corruption", + path, + ) + return + if raw.startswith(codecs.BOM_UTF16_LE) or raw.startswith(codecs.BOM_UTF16_BE): + # "utf-16" uses the BOM to select endianness and strips it. + # TextIOWrapper + newline=None matches open()'s universal-newlines + # line splitting (\\n/\\r\\n/\\r only — not splitlines()'s extra + # Unicode boundaries like U+2028), so sanitize sees the same lines + # as the UTF-8 path. + try: + with io.TextIOWrapper( + io.BytesIO(raw), encoding="utf-16", newline=None + ) as f: + original = f.readlines() + except UnicodeDecodeError: + return + # Source is UTF-16 on disk; always rewrite as clean UTF-8 so + # the subsequent utf-8 dotenv load sees a canonical file. + force_utf8_rewrite = True + else: + # Default path: utf-8-sig (strips UTF-8 BOM if present) with + # errors=replace so embedded NULs can be stripped below. + try: + with open(path, encoding="utf-8-sig", errors="replace") as f: + original = f.readlines() + except Exception: + return + # Defense-in-depth: errors=replace turns undecodable leading + # bytes into U+FFFD. Persisting that glues replacement chars + # onto the first key name and rewrites the file permanently + # (the UTF-16-with-BOM corruption path before BOM sniffing). + # Leave the file untouched rather than write the mangling. + if original and original[0].startswith("\ufffd"): + return + + try: # Strip null bytes before _sanitize_env_lines so they never # reach python-dotenv (which passes them to os.environ and - # crashes with ValueError). + # crashes with ValueError). Also intentionally repairs + # BOM-less UTF-16 (NUL-padded ASCII) into clean UTF-8. stripped = [line.replace("\x00", "") for line in original] sanitized = _sanitize_env_lines(stripped) - if sanitized != original: + if sanitized != original or force_utf8_rewrite: import tempfile fd, tmp = tempfile.mkstemp( dir=str(path.parent), suffix=".tmp", prefix=".env_" diff --git a/tests/hermes_cli/test_env_loader.py b/tests/hermes_cli/test_env_loader.py index f719fae01ce7..719b272a5e4c 100644 --- a/tests/hermes_cli/test_env_loader.py +++ b/tests/hermes_cli/test_env_loader.py @@ -1,3 +1,4 @@ +import codecs import importlib import os import sys @@ -103,3 +104,239 @@ def test_main_import_applies_user_env_over_shell_values(tmp_path, monkeypatch): assert os.getenv("OPENAI_BASE_URL") == "https://new.example/v1" assert os.getenv("HERMES_INFERENCE_PROVIDER") == "custom" + + +# --------------------------------------------------------------------------- +# UTF-16 / UTF-32 .env sanitizer coverage +# +# Scope note: intentionally NO UTF-8-BOM assertions here. UTF-8 BOM handling +# for _load_dotenv_with_fallback is #65124's un-merged fix; a test here would +# couple the PRs. This suite covers only the sanitizer rewrite path for +# UTF-16/32 (and UTF-8 / cp1252 regression guards for that path). +# --------------------------------------------------------------------------- + + +def _assert_clean_utf8_env_on_disk(env_file, *, first_key: str) -> None: + """On-disk file must be clean UTF-8: no BOM, no U+FFFD, canonical key.""" + after = env_file.read_bytes() + assert not after.startswith(codecs.BOM_UTF8) + assert not after.startswith(codecs.BOM_UTF16_LE) + assert not after.startswith(codecs.BOM_UTF16_BE) + text = after.decode("utf-8") # strict — raises if not clean UTF-8 + assert "\ufffd" not in text + assert text.startswith(f"{first_key}=") or f"\n{first_key}=" in text + assert first_key.encode("ascii") in after + + +def test_utf16_le_bom_env_loads_and_rewrites_clean_utf8(tmp_path, monkeypatch): + """Notepad 'Unicode' (UTF-16-LE + BOM): first key loads; file rewritten UTF-8.""" + home = tmp_path / "hermes" + home.mkdir() + env_file = home / ".env" + content = "HERMES_TEST_KEY=hello_utf16\nSECOND_KEY=world\n" + env_file.write_bytes(codecs.BOM_UTF16_LE + content.encode("utf-16-le")) + + monkeypatch.delenv("HERMES_TEST_KEY", raising=False) + monkeypatch.delenv("SECOND_KEY", raising=False) + monkeypatch.delenv("\ufffd\ufffdHERMES_TEST_KEY", raising=False) + + loaded = load_hermes_dotenv(hermes_home=home) + + assert loaded == [env_file] + assert os.getenv("HERMES_TEST_KEY") == "hello_utf16" + assert os.getenv("SECOND_KEY") == "world" + assert os.environ.get("\ufffd\ufffdHERMES_TEST_KEY") is None + _assert_clean_utf8_env_on_disk(env_file, first_key="HERMES_TEST_KEY") + + +def test_utf16_be_bom_env_loads_and_rewrites_clean_utf8(tmp_path, monkeypatch): + """UTF-16-BE + BOM: first key loads; file rewritten as clean UTF-8.""" + home = tmp_path / "hermes" + home.mkdir() + env_file = home / ".env" + content = "HERMES_TEST_KEY=hello_utf16\nSECOND_KEY=world\n" + env_file.write_bytes(codecs.BOM_UTF16_BE + content.encode("utf-16-be")) + + monkeypatch.delenv("HERMES_TEST_KEY", raising=False) + monkeypatch.delenv("SECOND_KEY", raising=False) + + loaded = load_hermes_dotenv(hermes_home=home) + + assert loaded == [env_file] + assert os.getenv("HERMES_TEST_KEY") == "hello_utf16" + assert os.getenv("SECOND_KEY") == "world" + _assert_clean_utf8_env_on_disk(env_file, first_key="HERMES_TEST_KEY") + + +def test_utf16_le_no_bom_still_repairs_to_utf8(tmp_path, monkeypatch): + """BOM-less UTF-16-LE: NUL-strip repair is now intentional; rewrites UTF-8.""" + home = tmp_path / "hermes" + home.mkdir() + env_file = home / ".env" + content = "HERMES_TEST_KEY=hello_utf16\nSECOND_KEY=world\n" + env_file.write_bytes(content.encode("utf-16-le")) # no BOM + + monkeypatch.delenv("HERMES_TEST_KEY", raising=False) + monkeypatch.delenv("SECOND_KEY", raising=False) + + loaded = load_hermes_dotenv(hermes_home=home) + + assert loaded == [env_file] + assert os.getenv("HERMES_TEST_KEY") == "hello_utf16" + assert os.getenv("SECOND_KEY") == "world" + _assert_clean_utf8_env_on_disk(env_file, first_key="HERMES_TEST_KEY") + + +def test_utf16_be_no_bom_still_repairs_to_utf8(tmp_path, monkeypatch): + """BOM-less UTF-16-BE: NULs are on the opposite side; still repairs.""" + home = tmp_path / "hermes" + home.mkdir() + env_file = home / ".env" + content = "HERMES_TEST_KEY=hello_utf16\nSECOND_KEY=world\n" + env_file.write_bytes(content.encode("utf-16-be")) # no BOM + + monkeypatch.delenv("HERMES_TEST_KEY", raising=False) + monkeypatch.delenv("SECOND_KEY", raising=False) + + loaded = load_hermes_dotenv(hermes_home=home) + + assert loaded == [env_file] + assert os.getenv("HERMES_TEST_KEY") == "hello_utf16" + assert os.getenv("SECOND_KEY") == "world" + _assert_clean_utf8_env_on_disk(env_file, first_key="HERMES_TEST_KEY") + + +def test_utf16_le_bom_preserves_non_ascii_values(tmp_path, monkeypatch): + """UTF-16-LE+BOM rewrite must preserve non-ASCII values (not just ASCII keys). + + Uses non-credential var names so _sanitize_loaded_credentials does not + strip non-ASCII from values (that path only targets *_KEY/*_TOKEN/etc.). + """ + home = tmp_path / "hermes" + home.mkdir() + env_file = home / ".env" + content = "GREETING=café\nCJK_LABEL=日本語\n" + env_file.write_bytes(codecs.BOM_UTF16_LE + content.encode("utf-16-le")) + + monkeypatch.delenv("GREETING", raising=False) + monkeypatch.delenv("CJK_LABEL", raising=False) + + loaded = load_hermes_dotenv(hermes_home=home) + + assert loaded == [env_file] + assert os.getenv("GREETING") == "café" + assert os.getenv("CJK_LABEL") == "日本語" + after = env_file.read_bytes() + assert after.decode("utf-8") # strict + assert "café".encode("utf-8") in after + assert "日本語".encode("utf-8") in after + assert b"\xef\xbf\xbd" not in after + + +def test_utf32_le_bom_leaves_file_untouched(tmp_path, caplog): + """UTF-32-LE BOM: refuse-to-mangle (leave bytes untouched + warning). + + UTF-32-LE's BOM starts with UTF-16-LE's FF FE; sniff order must check + UTF-32 first so we never misdetect and corrupt. + + Exercises ``_sanitize_env_file_if_needed`` only: the dotenv load path + is out of scope here (#65124's surface) and still cannot ingest UTF-32. + """ + import logging + + from hermes_cli.env_loader import _sanitize_env_file_if_needed + + env_file = tmp_path / ".env" + content = "HERMES_TEST_KEY=hello_utf32\nSECOND_KEY=world\n" + raw = codecs.BOM_UTF32_LE + content.encode("utf-32-le") + env_file.write_bytes(raw) + + with caplog.at_level(logging.WARNING, logger="hermes_cli.env_loader"): + _sanitize_env_file_if_needed(env_file) + + assert env_file.read_bytes() == raw # untouched + assert any("UTF-32" in r.message for r in caplog.records) + + +def test_utf32_be_bom_leaves_file_untouched(tmp_path, caplog): + """UTF-32-BE BOM: same refuse-to-mangle path as LE (ordering independence).""" + import logging + + from hermes_cli.env_loader import _sanitize_env_file_if_needed + + env_file = tmp_path / ".env" + content = "HERMES_TEST_KEY=hello_utf32\nSECOND_KEY=world\n" + raw = codecs.BOM_UTF32_BE + content.encode("utf-32-be") + env_file.write_bytes(raw) + + with caplog.at_level(logging.WARNING, logger="hermes_cli.env_loader"): + _sanitize_env_file_if_needed(env_file) + + assert env_file.read_bytes() == raw + assert any("UTF-32" in r.message for r in caplog.records) + + +def test_leading_replacement_char_does_not_rewrite(tmp_path): + """errors=replace FFFD-on-first-line guard: do not persist mangling. + + Leading 0xFF is not a UTF-16/32 BOM (those need the second BOM byte) but + is undecodable as UTF-8, so the replace path would glue U+FFFD onto the + key. The guard must leave the on-disk bytes untouched. + """ + from hermes_cli.env_loader import _sanitize_env_file_if_needed + + env_file = tmp_path / ".env" + raw = b"\xffHERMES_TEST_KEY=should-not-rewrite\nSECOND_KEY=ok\n" + env_file.write_bytes(raw) + + _sanitize_env_file_if_needed(env_file) + + assert env_file.read_bytes() == raw + + +def test_plain_utf8_env_regression(tmp_path, monkeypatch): + """Plain UTF-8 .env must keep loading after the UTF-16 sanitize changes.""" + home = tmp_path / "hermes" + home.mkdir() + env_file = home / ".env" + before = b"OPENAI_API_KEY=sk-plain\nSECOND_KEY=ok\n" + env_file.write_bytes(before) + + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.delenv("SECOND_KEY", raising=False) + + loaded = load_hermes_dotenv(hermes_home=home) + + assert loaded == [env_file] + assert os.getenv("OPENAI_API_KEY") == "sk-plain" + assert os.getenv("SECOND_KEY") == "ok" + # No spurious rewrite of an already-clean file. + assert env_file.read_bytes() == before + + +def test_cp1252_env_regression_does_not_crash(tmp_path, monkeypatch): + """cp1252/latin-1 body must not crash sanitize; ASCII keys still usable. + + 0xE9 is 'é' in cp1252 and incomplete as UTF-8. First line does not begin + with U+FFFD, so the FFFD guard must not refuse the whole file. + + Sanitize leaves the file bytes alone when the only "change" is + errors=replace on values (original already replace-decoded equals + sanitized), so _load_dotenv_with_fallback's latin-1 path recovers café. + """ + home = tmp_path / "hermes" + home.mkdir() + env_file = home / ".env" + before = b"ASCII_KEY=ok\nLATIN1_VALUE=caf\xe9\n" + env_file.write_bytes(before) + + monkeypatch.delenv("ASCII_KEY", raising=False) + monkeypatch.delenv("LATIN1_VALUE", raising=False) + + loaded = load_hermes_dotenv(hermes_home=home) + + assert loaded == [env_file] + assert os.getenv("ASCII_KEY") == "ok" + assert os.getenv("LATIN1_VALUE") == "café" + # Sanitize must not have rewritten (would have persisted U+FFFD). + assert env_file.read_bytes() == before From 90d3ba5be9093dd1a1ac93b275d4266593b87797 Mon Sep 17 00:00:00 2001 From: Paulo Nascimento Date: Fri, 17 Jul 2026 17:33:59 -0400 Subject: [PATCH 09/12] fix(cli): warn once per path for UTF-32 .env refuse-to-mangle Hot-reload and multi-entry load_hermes_dotenv can hit the same UTF-32 file repeatedly; gate the refuse-to-mangle warning on a module-level seen-set (house style: _WARNED_KEYS sibling) so logs are not spammed. --- hermes_cli/env_loader.py | 21 +++++++++++++++------ tests/hermes_cli/test_env_loader.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index 8018280d5b98..9dea6e5fe924 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -23,6 +23,12 @@ _CREDENTIAL_SUFFIXES = ("_API_KEY", "_TOKEN", "_SECRET", "_KEY") # tests) don't spam the same warning multiple times. _WARNED_KEYS: set[str] = set() +# Paths we've already emitted a UTF-32 refuse-to-mangle warning for. +# load_hermes_dotenv can call _sanitize_env_file_if_needed multiple times +# for the same file (user env + project env + hot-reload); once per path +# is enough. +_WARNED_UTF32_PATHS: set[str] = set() + # Map of env-var name → source label ("bitwarden", etc.) for credentials # that were injected by an external secret source during load_hermes_dotenv(). # Used by setup / `hermes model` flows to label detected credentials so @@ -208,13 +214,16 @@ def _sanitize_env_file_if_needed(path: Path) -> None: if raw.startswith(codecs.BOM_UTF32_LE) or raw.startswith(codecs.BOM_UTF32_BE): # Lazy import keeps the module import block identical to #65124's # codecs/io additions so the two PRs auto-merge either order. - import logging + path_key = str(path.resolve()) + if path_key not in _WARNED_UTF32_PATHS: + _WARNED_UTF32_PATHS.add(path_key) + import logging - logging.getLogger(__name__).warning( - "Skipping .env sanitize for %s: UTF-32 BOM detected; " - "leaving file untouched to avoid corruption", - path, - ) + logging.getLogger(__name__).warning( + "Skipping .env sanitize for %s: UTF-32 BOM detected; " + "leaving file untouched to avoid corruption", + path, + ) return if raw.startswith(codecs.BOM_UTF16_LE) or raw.startswith(codecs.BOM_UTF16_BE): # "utf-16" uses the BOM to select endianness and strips it. diff --git a/tests/hermes_cli/test_env_loader.py b/tests/hermes_cli/test_env_loader.py index 719b272a5e4c..a5a2ee752726 100644 --- a/tests/hermes_cli/test_env_loader.py +++ b/tests/hermes_cli/test_env_loader.py @@ -276,6 +276,35 @@ def test_utf32_be_bom_leaves_file_untouched(tmp_path, caplog): assert any("UTF-32" in r.message for r in caplog.records) +def test_utf32_warning_fires_once_per_path(tmp_path, caplog, monkeypatch): + """Three sanitize calls on the same UTF-32 file → exactly one warning. + + Matches house style for warn-once (module-level seen-set, same class as + ``_WARNED_KEYS``): hot-reload / multi-entry load must not spam logs. + """ + import logging + + import hermes_cli.env_loader as env_loader + from hermes_cli.env_loader import _sanitize_env_file_if_needed + + # Isolate process-level seen-set so other tests' paths don't leak in. + monkeypatch.setattr(env_loader, "_WARNED_UTF32_PATHS", set()) + + env_file = tmp_path / ".env" + content = "HERMES_TEST_KEY=hello_utf32\nSECOND_KEY=world\n" + raw = codecs.BOM_UTF32_LE + content.encode("utf-32-le") + env_file.write_bytes(raw) + + with caplog.at_level(logging.WARNING, logger="hermes_cli.env_loader"): + _sanitize_env_file_if_needed(env_file) + _sanitize_env_file_if_needed(env_file) + _sanitize_env_file_if_needed(env_file) + + utf32_warnings = [r for r in caplog.records if "UTF-32" in r.message] + assert len(utf32_warnings) == 1 + assert env_file.read_bytes() == raw + + def test_leading_replacement_char_does_not_rewrite(tmp_path): """errors=replace FFFD-on-first-line guard: do not persist mangling. From 65bf42b669144a06ae8649d77e7c55b2d70f7e25 Mon Sep 17 00:00:00 2001 From: webtecnica <75556242+webtecnica@users.noreply.github.com> Date: Fri, 17 Jul 2026 23:04:25 -0300 Subject: [PATCH 10/12] fix(auxiliary): resolve key_env in _resolve_task_provider_model (#66641) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _resolve_task_provider_model() read api_key from the auxiliary task config but never consulted key_env (or api_key_env). When a user configured an auxiliary task with key_env instead of a plaintext api_key, the resolved API key was None, causing 401 on every call. Add the same key_env → os.getenv() resolution pattern already used in _fallback_entry_api_key() and named custom provider resolution. Closes #66641 --- agent/auxiliary_client.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 91523d76f8ee..da49a695180a 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -6253,6 +6253,13 @@ def _resolve_task_provider_model( cfg_model = str(task_config.get("model", "")).strip() or None cfg_base_url = str(task_config.get("base_url", "")).strip() or None cfg_api_key = str(task_config.get("api_key", "")).strip() or None + # Resolve key_env → env var when api_key is not set directly + if not cfg_api_key: + cfg_key_env = str( + task_config.get("key_env") or task_config.get("api_key_env") or "" + ).strip() + if cfg_key_env: + cfg_api_key = os.getenv(cfg_key_env, "").strip() or None cfg_api_mode = str(task_config.get("api_mode", "")).strip() or None # 'auto' is a sentinel meaning "inherit from main runtime / auto-detect", not From 2bae4df8bb29243fe6b627cb3942aa272edcffc2 Mon Sep 17 00:00:00 2001 From: webtecnica <75556242+webtecnica@users.noreply.github.com> Date: Fri, 17 Jul 2026 15:38:28 -0300 Subject: [PATCH 11/12] fix: replace hardcoded ~/.hermes with get_hermes_home() in system prompt (#66450) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The system prompt building code hardcoded '~/.hermes' paths instead of using get_hermes_home(). When HERMES_HOME is set to a custom location, the prompt text still referenced ~/.hermes, confusing the AI about where files actually live. Changed: - Import get_hermes_home from hermes_constants - Default profile hint: ~/.hermes/profiles// → /profiles// - Non-default profile hint: ~/.hermes/... → /... for all paths Closes #66450 --- agent/system_prompt.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/agent/system_prompt.py b/agent/system_prompt.py index 41337e4e9fbf..aab34cb799e1 100644 --- a/agent/system_prompt.py +++ b/agent/system_prompt.py @@ -46,6 +46,7 @@ from agent.prompt_builder import ( drain_truncation_warnings, ) from agent.runtime_cwd import resolve_context_cwd +from hermes_constants import get_hermes_home from utils import is_truthy_value @@ -395,7 +396,7 @@ def build_system_prompt_parts(agent: Any, system_message: Optional[str] = None) if active_profile == "default": stable_parts.append( "Active Hermes profile: default. Other profiles (if any) live " - "under ~/.hermes/profiles//. Each profile has its own " + "under " + str(get_hermes_home()) + "/profiles//. Each profile has its own " "skills/, plugins/, cron/, and memories/ that affect a different " "session than this one. Do not modify another profile's " "skills/plugins/cron/memories unless the user explicitly directs " @@ -404,9 +405,9 @@ def build_system_prompt_parts(agent: Any, system_message: Optional[str] = None) else: stable_parts.append( f"Active Hermes profile: {active_profile}. This session reads " - f"and writes ~/.hermes/profiles/{active_profile}/. The default " - f"profile's data lives at ~/.hermes/skills/, ~/.hermes/plugins/, " - f"~/.hermes/cron/, ~/.hermes/memories/ — those belong to a " + f"and writes {get_hermes_home()}/profiles/{active_profile}/. The default " + f"profile's data lives at {get_hermes_home()}/skills/, {get_hermes_home()}/plugins/, " + f"{get_hermes_home()}/cron/, {get_hermes_home()}/memories/ — those belong to a " f"different session run from a different shell. Do NOT modify " f"another profile's skills/plugins/cron/memories unless the user " f"explicitly directs you to. The cross-profile write guard will " From e30174fa173a4678cc730470e8f0aa3e19b5065a Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sat, 18 Jul 2026 19:15:57 -0400 Subject: [PATCH 12/12] perf(desktop): scope tool-diff subscriptions + narrow profile query invalidation Two structural fixes from the Desktop performance audit (P2 tier): 1. Scope live tool-diff subscriptions. `ToolEntry` subscribed to the whole `$toolDiffs` map via `useStore`, so one `recordToolDiff` re-rendered every mounted tool row. Add a cached per-toolCallId derived atom (`$toolInlineDiff(id)`, mirroring the existing `$toolDisclosureOpen` pattern); computed() only notifies when that id's diff string changes, so a live patch re-renders one row. 2. Narrow profile / gateway-switch query invalidation. Both the active-profile subscription and `wipeSessionListsForGatewaySwitch` called keyless `queryClient.invalidateQueries()`, refetching account/marketplace/onboarding caches on every switch. Add `invalidateProfileScopedQueries()` with a denylist of profile-independent roots (billing, marketplace-themes, onboarding-model-options, contrib-logs-tail). A denylist is correctness-safe: a root we forget just refetches (cheap), whereas an allowlist that misses a profile-scoped key would paint the previous profile's data. Tests: per-tool notify isolation, and real-QueryClient invalidation partition (profile-scoped invalidated, global left intact, unknown keys invalidated). --- .../components/assistant-ui/tool/fallback.tsx | 7 ++- apps/desktop/src/lib/query-client.test.ts | 58 +++++++++++++++++++ apps/desktop/src/lib/query-client.ts | 28 +++++++++ apps/desktop/src/store/gateway-switch.test.ts | 2 +- apps/desktop/src/store/gateway-switch.ts | 6 +- apps/desktop/src/store/profile.test.ts | 8 +-- apps/desktop/src/store/profile.ts | 6 +- apps/desktop/src/store/tool-diffs.test.ts | 47 +++++++++++++++ apps/desktop/src/store/tool-diffs.ts | 19 +++++- 9 files changed, 167 insertions(+), 14 deletions(-) create mode 100644 apps/desktop/src/lib/query-client.test.ts create mode 100644 apps/desktop/src/store/tool-diffs.test.ts diff --git a/apps/desktop/src/components/assistant-ui/tool/fallback.tsx b/apps/desktop/src/components/assistant-ui/tool/fallback.tsx index ae173d2eb5af..96db51f49550 100644 --- a/apps/desktop/src/components/assistant-ui/tool/fallback.tsx +++ b/apps/desktop/src/components/assistant-ui/tool/fallback.tsx @@ -39,7 +39,7 @@ import { useEnterAnimation } from '@/lib/use-enter-animation' import { cn } from '@/lib/utils' import { recordPreviewArtifact } from '@/store/preview-status' import { $activeSessionId, $currentCwd } from '@/store/session' -import { $toolInlineDiffs } from '@/store/tool-diffs' +import { $toolInlineDiff } from '@/store/tool-diffs' import { $toolRowDismissed, dismissToolRow } from '@/store/tool-dismiss' import { $toolDisclosureOpen, $toolViewMode, setToolDisclosureOpen } from '@/store/tool-view' @@ -283,8 +283,9 @@ function ToolEntry({ part }: ToolEntryProps) { const disclosureId = `tool-entry:${messageId}:${toolPartDisclosureId(stablePart)}` const dismissed = useStore($toolRowDismissed(disclosureId)) const isPending = messageRunning && result === undefined - const liveDiffs = useStore($toolInlineDiffs) - const sideDiff = toolCallId ? liveDiffs[toolCallId] || '' : '' + // Subscribe to this tool's diff only, so a live patch for one tool doesn't + // re-render every mounted tool row (the factory caches a per-id atom). + const sideDiff = useStore($toolInlineDiff(toolCallId ?? '')) const inlineDiff = stripInlineDiffChrome(sideDiff) || inlineDiffFromResult(result) const isFileEdit = isFileEditTool(toolName) const defaultOpen = Boolean(inlineDiff) diff --git a/apps/desktop/src/lib/query-client.test.ts b/apps/desktop/src/lib/query-client.test.ts new file mode 100644 index 000000000000..c2f0cc200702 --- /dev/null +++ b/apps/desktop/src/lib/query-client.test.ts @@ -0,0 +1,58 @@ +import { beforeEach, describe, expect, it } from 'vitest' + +import { invalidateProfileScopedQueries, queryClient } from './query-client' + +function invalidated(key: unknown[]): boolean { + return queryClient.getQueryState(key)?.isInvalidated ?? false +} + +describe('invalidateProfileScopedQueries', () => { + beforeEach(() => { + queryClient.clear() + }) + + it('invalidates profile-scoped caches and leaves account/global caches intact', () => { + const profileScoped = [ + ['hermes-config-record'], + ['hermes-config-schema'], + ['skills-list'], + ['toolsets-list'], + ['model-options', 'global'], + ['command-palette', 'sessions'], + ['session-picker', 'sessions'] + ] + + const global = [ + ['billing', 'state'], + ['billing', 'subscription'], + ['marketplace-themes', 'all'], + ['marketplace-themes-settings', 'x'], + ['onboarding-model-options', 'y'], + ['contrib-logs-tail'] + ] + + for (const key of [...profileScoped, ...global]) { + queryClient.setQueryData(key, { seeded: true }) + } + + invalidateProfileScopedQueries() + + for (const key of profileScoped) { + expect(invalidated(key), `${JSON.stringify(key)} should be invalidated`).toBe(true) + } + + for (const key of global) { + expect(invalidated(key), `${JSON.stringify(key)} should be left intact`).toBe(false) + } + }) + + it('invalidates unknown/non-string-rooted keys by default (correctness-safe)', () => { + queryClient.setQueryData(['some-future-profile-query'], 1) + queryClient.setQueryData([{ scope: 'weird' }], 1) + + invalidateProfileScopedQueries() + + expect(invalidated(['some-future-profile-query'])).toBe(true) + expect(invalidated([{ scope: 'weird' }])).toBe(true) + }) +}) diff --git a/apps/desktop/src/lib/query-client.ts b/apps/desktop/src/lib/query-client.ts index dd0df19941b7..cfea4f1054fd 100644 --- a/apps/desktop/src/lib/query-client.ts +++ b/apps/desktop/src/lib/query-client.ts @@ -18,3 +18,31 @@ export const writeCache = (key: QueryKey) => (next: T | undefined | ((prev: T | undefined) => T | undefined)): void => void queryClient.setQueryData(key, next) + +// Query-key roots that are NOT profile-scoped: account/billing, the theme +// marketplace, onboarding, and contrib log tails all read global or +// account-level state, so a profile/gateway swap must not refetch them. Any +// other key is treated as profile-scoped and invalidated -- a denylist is +// correctness-safe here: a root we forget to list just gets refetched (a small +// cost), whereas an allowlist that misses a profile-scoped key would paint the +// previous profile's data (a bug). +const PROFILE_INDEPENDENT_QUERY_ROOTS = new Set([ + 'billing', + 'marketplace-themes', + 'marketplace-themes-settings', + 'onboarding-model-options', + 'contrib-logs-tail' +]) + +// Invalidate profile-scoped query caches on a profile / gateway switch, leaving +// account/global caches intact. Replaces a keyless invalidateQueries() that +// refetched everything (billing, marketplace, onboarding) on every switch. +export function invalidateProfileScopedQueries(): void { + void queryClient.invalidateQueries({ + predicate: query => { + const root = query.queryKey[0] + + return typeof root !== 'string' || !PROFILE_INDEPENDENT_QUERY_ROOTS.has(root) + } + }) +} diff --git a/apps/desktop/src/store/gateway-switch.test.ts b/apps/desktop/src/store/gateway-switch.test.ts index 5b24c120ccc9..5513f4f4b290 100644 --- a/apps/desktop/src/store/gateway-switch.test.ts +++ b/apps/desktop/src/store/gateway-switch.test.ts @@ -19,7 +19,7 @@ import { import { $gatewaySwitching, wipeSessionListsForGatewaySwitch } from './gateway-switch' vi.mock('@/lib/query-client', () => ({ - queryClient: { invalidateQueries: vi.fn() } + invalidateProfileScopedQueries: vi.fn() })) describe('wipeSessionListsForGatewaySwitch', () => { diff --git a/apps/desktop/src/store/gateway-switch.ts b/apps/desktop/src/store/gateway-switch.ts index 0d1414adad00..2b6fb466e8e7 100644 --- a/apps/desktop/src/store/gateway-switch.ts +++ b/apps/desktop/src/store/gateway-switch.ts @@ -1,6 +1,6 @@ import { atom } from 'nanostores' -import { queryClient } from '@/lib/query-client' +import { invalidateProfileScopedQueries } from '@/lib/query-client' import { resetSessionsLimit } from '@/store/layout' import { $unreadFinishedSessionIds, @@ -57,5 +57,7 @@ export function wipeSessionListsForGatewaySwitch(): void { setMessages([]) setFreshDraftReady(true) - void queryClient.invalidateQueries() + // Narrowed: account/marketplace/onboarding caches are global, not gateway- + // scoped, so a mode swap must not refetch them. + invalidateProfileScopedQueries() } diff --git a/apps/desktop/src/store/profile.test.ts b/apps/desktop/src/store/profile.test.ts index b7489fe05566..49d6186a43d0 100644 --- a/apps/desktop/src/store/profile.test.ts +++ b/apps/desktop/src/store/profile.test.ts @@ -16,14 +16,14 @@ vi.mock('@/hermes', () => ({ getProfiles: vi.fn(async () => ({ profiles: [] })), setApiRequestProfile: vi.fn() })) -vi.mock('@/lib/query-client', () => ({ queryClient: { invalidateQueries: vi.fn() } })) +vi.mock('@/lib/query-client', () => ({ invalidateProfileScopedQueries: vi.fn() })) vi.mock('@/store/starmap', () => ({ resetStarmapGraph })) const { $activeGatewayProfile, $profiles, ensureGatewayProfile, prewarmProfileBackend, refreshProfiles } = await import('./profile') const { $connection } = await import('./session') -const { queryClient } = await import('@/lib/query-client') +const { invalidateProfileScopedQueries } = await import('@/lib/query-client') const { getProfiles } = await import('@/hermes') const profile = (name: string, isDefault = false): ProfileInfo => ({ @@ -53,7 +53,7 @@ beforeEach(() => { $connection.set(localConn()) $profiles.set([]) vi.stubGlobal('window', { hermesDesktop: { getConnection } }) - vi.mocked(queryClient.invalidateQueries).mockClear() + vi.mocked(invalidateProfileScopedQueries).mockClear() resetStarmapGraph.mockClear() }) @@ -114,7 +114,7 @@ describe('profile-scoped cache invalidation', () => { it('drops the memory graph cache when the active gateway profile changes', () => { $activeGatewayProfile.set('coder') - expect(queryClient.invalidateQueries).toHaveBeenCalled() + expect(invalidateProfileScopedQueries).toHaveBeenCalled() expect(resetStarmapGraph).toHaveBeenCalledTimes(1) }) }) diff --git a/apps/desktop/src/store/profile.ts b/apps/desktop/src/store/profile.ts index 1049d315311c..c97614884dca 100644 --- a/apps/desktop/src/store/profile.ts +++ b/apps/desktop/src/store/profile.ts @@ -1,7 +1,7 @@ import { atom, computed } from 'nanostores' import { getProfiles, setApiRequestProfile, STARTUP_REQUEST_TIMEOUT_MS } from '@/hermes' -import { queryClient } from '@/lib/query-client' +import { invalidateProfileScopedQueries } from '@/lib/query-client' import { arraysEqual, persistBoolean, @@ -177,7 +177,9 @@ $activeGatewayProfile.subscribe(value => { if (_lastRoutedProfile !== null && _lastRoutedProfile !== key) { // Profile-scoped settings + the unified session list are now stale. - void queryClient.invalidateQueries() + // Narrowed so account/marketplace/onboarding caches don't refetch on + // every profile switch. + invalidateProfileScopedQueries() resetStarmapGraph() } diff --git a/apps/desktop/src/store/tool-diffs.test.ts b/apps/desktop/src/store/tool-diffs.test.ts new file mode 100644 index 000000000000..c4421159ad69 --- /dev/null +++ b/apps/desktop/src/store/tool-diffs.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from 'vitest' + +import { $toolInlineDiff, getToolDiff, recordToolDiff } from './tool-diffs' + +describe('tool-diffs per-tool subscriptions', () => { + it('returns a stable cached atom per toolCallId', () => { + expect($toolInlineDiff('a')).toBe($toolInlineDiff('a')) + expect($toolInlineDiff('a')).not.toBe($toolInlineDiff('b')) + }) + + it('notifies only the tool whose diff changed', () => { + const aCalls: string[] = [] + const bCalls: string[] = [] + const unsubA = $toolInlineDiff('notify-a').listen(v => aCalls.push(v)) + const unsubB = $toolInlineDiff('notify-b').listen(v => bCalls.push(v)) + + recordToolDiff('notify-a', 'diffA') + expect(aCalls).toEqual(['diffA']) + expect(bCalls).toEqual([]) // the unrelated tool row is never notified + + recordToolDiff('notify-b', 'diffB') + expect(aCalls).toEqual(['diffA']) // still not re-notified + expect(bCalls).toEqual(['diffB']) + + unsubA() + unsubB() + }) + + it('does not re-notify when the same diff is recorded again', () => { + const calls: string[] = [] + const unsub = $toolInlineDiff('same').listen(v => calls.push(v)) + + recordToolDiff('same', 'x') + recordToolDiff('same', 'x') + + expect(calls).toEqual(['x']) + unsub() + }) + + it('reads the current diff for a tool and empty for unknown/blank ids', () => { + recordToolDiff('read-me', 'value') + expect(getToolDiff('read-me')).toBe('value') + expect(getToolDiff('missing')).toBe('') + expect(getToolDiff('')).toBe('') + expect($toolInlineDiff('').get()).toBe('') + }) +}) diff --git a/apps/desktop/src/store/tool-diffs.ts b/apps/desktop/src/store/tool-diffs.ts index 01678bc21c71..d4ca7d2ee41f 100644 --- a/apps/desktop/src/store/tool-diffs.ts +++ b/apps/desktop/src/store/tool-diffs.ts @@ -1,7 +1,13 @@ -import { atom } from 'nanostores' +import { atom, computed, type ReadableAtom } from 'nanostores' const $toolDiffs = atom>({}) +// Per-tool derived atoms, cached by toolCallId. A `ToolEntry` subscribes only +// to its own id's diff, so recording a diff for one tool re-renders that one +// row -- not every mounted tool row. computed() only notifies when the derived +// string actually changes, so unrelated writes to the map are inert here. +const inlineDiffCache = new Map>() + export function recordToolDiff(toolCallId: string, diff: string) { if (!toolCallId || !diff) { return @@ -20,4 +26,13 @@ export function getToolDiff(toolCallId: string): string { return toolCallId ? $toolDiffs.get()[toolCallId] || '' : '' } -export const $toolInlineDiffs = $toolDiffs +export function $toolInlineDiff(toolCallId: string): ReadableAtom { + let cached = inlineDiffCache.get(toolCallId) + + if (!cached) { + cached = computed($toolDiffs, diffs => (toolCallId ? diffs[toolCallId] || '' : '')) + inlineDiffCache.set(toolCallId, cached) + } + + return cached +}