fix(ui-tui): widget-grid hardening — review fast-follow for #20379 - #68999
Merged
Merged
Conversation
4 tasks
Contributor
૮ >ﻌ< ა ci reviewrunning on 5c714be
|
OutThisLife
force-pushed
the
bb/widget-grid-hardening
branch
from
July 22, 2026 00:27
f5e7073 to
7707a49
Compare
OutThisLife
force-pushed
the
bb/widget-grid-hardening
branch
from
July 22, 2026 01:01
dfbf0d7 to
892d0da
Compare
…as LOADED Review on #20379, finding 1 (High). Two ways an MCP config revision could be silently acknowledged without ever being applied: Client: the poll advanced its accepted mcp_rev BEFORE calling reload.mcp, and quietRpc collapses failures to null — a reload that failed against a temporarily broken server left the revision recorded as applied, and no subsequent poll retried until an unrelated MCP edit. The handshake is now syncMcpReload(): send the observed rev with the request, advance `accepted` only when the server answers status=reloaded (to the server's loaded_rev, falling back to the requested rev on older gateways), and re-compare on EVERY poll tick — decoupled from mtime — so a transient failure heals on the next tick. An in-flight guard stops the 5s poll from stacking requests behind a slow reload. Server: generation-only coalescing let a follower triggered by revision B ack against revision A's registry when the config changed under a slow leader. The leader now re-hashes the MCP-relevant config after discovery and repeats until stable (bounded), records _mcp_reload_loaded_rev, and a follower coalesces only when the revision it was asked to load matches — otherwise it re-runs the full reload itself. Responses carry loaded_rev. Deterministic tests for the exact failure sequences: failed reload → no ack, no generation advance; A-then-B overlap → follower re-runs; matching rev → coalesces; failed leader → follower re-runs; legacy no-rev callers keep generation-only coalescing (thread ordering via an instrumented lock, no sleeps). Client: 6 vitest cases on the ack/retry/in-flight contract.
…n the terminal Review on #20379, finding 2 (High). The boot cache seeded the previous session's background into HERMES_TUI_BACKGROUND, the same slot a CURRENT OSC-11 answer occupies — so a cache written on a light terminal pinned a now-pure-black terminal to light forever: the new OSC-11 #000000 answer is distrusted by design, the pure-white OSC-10 foreground is distrusted too, and the macOS appearance fallback refuses to run while the slot is set. Seeding now records provenance (themeBoot.seedBootEnvironment, extracted and testable against a passed env). When the current terminal answers the background probe with the untrusted fingerprint, the gateway handler calls invalidateBootBackground(): the slot clears ONLY while it still holds the seeded value (a trusted answer that overwrote it is authoritative), OSC-10 gets first claim in the same startup batch, and a short settle pass re- derives from the live fallback chain if nothing answered. The cache is also pin-coherent now: commitTheme persists the config mode pin (display.tui_theme) alongside the resolved theme + physical background. Previously "/theme light" on a dark terminal cached a light theme next to a dark background — the next launch painted light, flipped dark when the skin resolved against the seeded background, then flipped light again on config hydration: the exact multi-stage flash the cache exists to eliminate. The seeded pin counts as config-owned (bootSeededPin), so a later 'auto' can still clear it instead of mistaking it for a user shell export. Tests cover the review's sequences: stale-light cache vs current dark terminal (invalidate → fallback chain), stale-dark vs ambiguous light, pinned light on a dark physical background across restart (and the inverse), trusted-overwrite protection, and the explicit-signal guards.
Review on #20379, finding 3 (Medium). /grid-test's `d` opens a dialog on top without clearing the grid, but the grid's input branch ran FIRST — so Esc/q/Enter mutated the hidden grid (close/unzoom/promote) instead of closing the visible dialog, contradicting its "Esc/q/Enter close" hint. Input routing now follows visual stacking: the dialog/grid dispatch is extracted into handleStackedModalInput() with the dialog branch first, the hook consumes through it, and tests drive the real dispatch against the overlay store — each advertised close key closes only the dialog (grid byte-identical), grid keys don't leak through while the dialog is up, and the same keys route to the grid again after it closes.
OutThisLife
force-pushed
the
bb/widget-grid-hardening
branch
from
July 22, 2026 01:27
892d0da to
e5fa519
Compare
Review on #20379, finding 4 (Medium). Three portability defects in the visual verification harness: - `FORCE_COLOR=3 COLORTERM=truecolor tsx ...` POSIX env assignment does not work under the Windows npm command shell. The script is now a plain Node launcher (scripts/visual/run.mjs) that sets the env itself and spawns tsx via require.resolve('tsx/cli') — no cross-env, no shell syntax. - Hardcoded /tmp/tui-visual.{html,png} resolve to a drive-root path like C:\tmp on native Windows (and fail when that directory doesn't exist). Both scripts now derive the output directory from a shared paths.mjs helper: os.tmpdir()/hermes-tui-visual (created recursively; HERMES_TUI_VISUAL_DIR overrides for CI or side-by-side runs). - electron was undeclared by ui-tui and only worked via hoisting luck. The launcher now resolves it EXPLICITLY from the install tree the desktop workspace already provides (require('electron') in plain Node returns the binary path), with an ELECTRON_BIN override and a clear error pointing at the repo-root install when it's absent — instead of declaring a second ~100MB dependency on a TUI workspace for a dev-only harness. Also fixes the trailing-whitespace line in render.tsx that `git diff --check` flags. Verified end-to-end: render writes the HTML scene sheet and the electron shot step produces the screenshot from the tmpdir path; the missing-electron error path prints the guidance message.
…d period Review on #20379, finding 5 (Perf). Every ShimmerRows mounted its own 90 ms setInterval — the session panel can show lazy skills AND lazy tools at once, and a lazy watch session stays lazy indefinitely, so an otherwise- idle TUI ran ~22 React state updates per second forever. All shimmer compositions now subscribe to a single module-level clock: one interval regardless of how many skeletons are on screen, updates delivered in one timer callback so React batches them into a single render pass, and the interval is torn down with the last subscriber. Each mount's animation is also bounded (SHIMMER_ANIMATE_MS, 30 s): after the budget the skeleton freezes in place — it still reads as "loading" — and stops costing renders entirely. Tests: fake-timer coverage that N subscribers share one timer in lockstep, the interval stops with the last unsubscribe, and a late subscriber restarts the clock cleanly.
Worktrees symlink node_modules to the main checkout; the dir-only node_modules/ pattern doesn't match symlinks, so one slipped into a commit and broke npm ci on CI (ENOTDIR). Dropping the trailing slash matches both.
OutThisLife
force-pushed
the
bb/widget-grid-hardening
branch
from
July 22, 2026 01:29
e5fa519 to
502939e
Compare
OutThisLife
enabled auto-merge
July 22, 2026 01:30
austinpickett
approved these changes
Jul 22, 2026
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
…d-hardening fix(ui-tui): widget-grid hardening — review fast-follow for NousResearch#20379
prmartinow
pushed a commit
to prmartinow/hermes-agent
that referenced
this pull request
Aug 26, 2026
…d-hardening fix(ui-tui): widget-grid hardening — review fast-follow for NousResearch#20379
melon-xf
added a commit
to melon-xf/hermes-agent
that referenced
this pull request
Sep 3, 2026
…d-hardening fix(ui-tui): widget-grid hardening — review fast-follow for NousResearch#20379
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fast-follow to #20379 (now merged), addressing every finding from the deep review.
Finding 1 (High) — MCP revision acked without being loaded
reload.mcpis now a revision-aware handshake on both sides:useConfigSync.ts):syncMcpReload()sends the observedmcp_revwith the request and advances its accepted revision only when the server answersstatus: reloaded— to the server-reportedloaded_rev, falling back to the requested rev on older gateways. The comparison runs on every poll tick (decoupled from mtime), so a reload that failed against a temporarily broken server retries on the next tick instead of being lost until an unrelated MCP edit. An in-flight guard stops the 5 s poll from stacking requests behind a slow reload.tui_gateway/server.py): the leader re-hashes the MCP-relevant config after discovery and repeats until the hash is stable (bounded at 3 passes), then records_mcp_reload_loaded_rev. A follower coalesces only when the revision it was asked to load matches what the leader actually loaded — the A-then-B race from the review now makes the B follower re-run the full reload instead of acking B against A's registry. Responses carryloaded_rev.Tests:
tests/tui_gateway/test_mcp_reload_rev.pycovers the exact failure sequences deterministically (instrumented lock for thread ordering, no sleeps): failed reload → error + no generation advance; A-then-B overlap → follower re-runs; matching rev → coalesces; failed leader → follower re-runs; legacy no-rev callers keep generation-only coalescing. Client side: 6 vitest cases on the ack/retry/in-flight contract.Finding 2 (High) — boot theme cache could pin the terminal indefinitely
The cache now has provenance:
seedBootEnvironment()(extracted, testable against a passed env) records what it seeded. When the current terminal answers OSC-11 with the distrusted#000000fingerprint, the gateway handler callsinvalidateBootBackground(): the slot clears only while it still holds the seeded value, OSC-10 gets first claim in the same startup batch, and a short settle pass re-derives from the live fallback chain. The review's stale-light-cache + pure-black-terminal sequence now lands on dark, and the macOS appearance fallback is no longer suppressed by a stale hint.commitThemepersists thedisplay.tui_themepin alongside the resolved theme and physical background, and boot replays it./theme lighton a dark terminal no longer flashes light → dark → light across restarts. The seeded pin counts as config-owned (bootSeededPin), so a laterautocan still clear it instead of mistaking it for a user shell export.Tests (
themeBoot.test.ts) cover the review's sequences: stale-light vs current-dark, stale-dark vs ambiguous-light, pinned light on dark background across restart (and the inverse), trusted-overwrite protection, explicit-signal guards.Finding 3 (Medium) — stacked dialog didn't receive input
Input routing now follows visual stacking: the dialog/grid dispatch is extracted into
handleStackedModalInput()with the dialog branch first. Tests drive the real dispatch against the overlay store — each advertised close key (Esc/q/Enter/Ctrl+C) closes only the dialog with the grid byte-identical underneath, grid keys don't leak through while the dialog is up, and routing returns to the grid after it closes.Finding 4 (Medium) —
npm run visualportability (zero new deps)scripts/visual/run.mjs) that setsFORCE_COLOR/COLORTERMitself and spawns tsx viarequire.resolve('tsx/cli')— no POSIX shell env assignments, and nocross-envdependency needed.os.tmpdir()/hermes-tui-visualvia a sharedpaths.mjs(override withHERMES_TUI_VISUAL_DIR) — no more drive-root/tmpwrites on native Windows.require('electron')in plain Node returns the binary path), with anELECTRON_BINoverride and a clear error pointing at the repo-root install when absent — the review's "invoked through the workspace that owns it" option, instead of declaring a second ~100MB dependency on the TUI workspace for a dev-only harness.package-lock.jsonis untouched.render.tsxthatgit diff --checkflags is fixed.Verified end-to-end: render writes the HTML scene sheet, the electron step produces the screenshot from the tmpdir path, and the missing-electron error path prints the guidance message.
Finding 5 (Perf) — permanent shimmer repaint loop
All shimmer compositions now share one module-level clock: a single interval regardless of how many skeletons are mounted, updates delivered in one timer callback (React batches them into one render pass), interval torn down with the last subscriber. Each mount's animation is bounded (
SHIMMER_ANIMATE_MS, 30 s) — after the budget the skeleton freezes in place instead of repainting an idle TUI forever. Fake-timer tests cover single-timer sharing, teardown with the last unsubscribe, and clean restart.Product note from the review (not changed here)
/grid-testand/dialog-testremain in the production registry for the TUI alpha — flagging that this is now an explicit decision rather than a leftover. Happy to gate them behind a debug flag in a follow-up if preferred.Verification
ui-tui: typecheck, eslint, and the full vitest suite green (121 files, 1312 tests).scripts/run_tests.sh tests/tui_gateway/green (37 files, 401 tests), including the newtest_mcp_reload_rev.py.npm run visualexercised end-to-end on macOS.