fix(gateway): stop leaking PYTHONPATH to subprocesses; TUI composer-token fixes - #15
Conversation
Select-all + Cut emptied the text and left the composer blank — no draft, no prompt. Delete had the same hole. The placeholder is painted on `:empty`, and a cleared editor keeps a scaffolding <br> so the contenteditable can't collapse to a sliver. Those two facts collide: the moment the break lands the editor has a child, `:empty` goes false, and the prompt never comes back. CSS can't infer emptiness on its own either. A text node is invisible to selectors, so `one<br>` and a lone `<br>` are the same shape — a structural rule like `:has(> br:only-child)` paints the placeholder straight over the user's text. The code that empties the editor is what knows, so it marks the root and the condition reads `:is(:empty, [data-empty])`. Both writers that reshape that root maintain the marker through one helper: the normalizer, and renderComposerContents for a restored draft or an undo. The message-edit composer shares the slot and the rule, so it takes the same shared class instead of drifting on its own copy. NousResearch#74815 fixed the draft this stashed; the placeholder is a separate seam.
A collapsed paste and an attached image are the same idea: a `[[ … ]]` marker sitting in the input line that stands in for a payload resolved at submit. Model both as ComposerToken and give them one expander. Image tokens resolve to nothing — the gateway already holds the file in attached_images — so expandTokens eats an adjacent space to avoid leaving a gap mid-sentence. nextImageIndex never reuses an index after a delete, or two files would collide on one label.
…attach Every attach path now drops an `[[ Image N ]]` token where you are typing: drag-drop, clipboard (bracketed and hotkey), /image, /paste. The composer owns clipboard attach directly instead of calling back out to useMainApp. Deleting the token is how you unattach — there is no second control. updateInput is the one choke point every keystroke passes through, so syncTokens reconciles there and detaches anything erased. That also fixes a stale image riding along on the next unrelated turn. Tokens and the input line get refs alongside state: paste-then-immediately -Enter submits before React has re-rendered, and the submit path has to see the token that was just added.
The token in the input line is the whole receipt. Drop the notices that duplicated it somewhere the user was not looking: the drag-drop and clipboard sys() lines, and the attachedImageNotice / "detected file: X" activity rows above the status bar. attachedImageNotice and imageTokenMeta have no callers left.
…ut-placeholder The placeholder comes back when you clear the composer
…achments TUI attachments live in the composer, not above the status bar
_ensure_windows_gateway_venv_imports() lives at gateway/run.py:247 and runs once at gateway startup, just before MCP tool discovery. Its job is to make the in-process Python see the hermes-agent source tree + the venv site-packages — for that it adjusts sys.path and calls site.addsitedir. It also wrote os.environ['PYTHONPATH'] = '<project_root>;<site-packages>' on the theory that any downstream subprocess would inherit it. That mutation has no in-tree consumer: MCP discovery runs in-process and uses the sys.path mutation above, and hermes_cli.gateway_windows._build_gateway_argv scopes its own PYTHONPATH env overlay per call via _prepend_pythonpath, the same pattern the desktop Electron side uses (apps/desktop/electron/main.ts). The os.environ mutation was dead ambient state — and worse, it leaked PYTHONPATH to every subprocess the gateway spawned thereafter, including the bash terminals the chat session opens for the user. The leaked PYTHONPATH points at the hermes-agent source tree plus the cp311 venv site-packages. Cross-version Python tools spawned from those shells (uvx, uv tool, honcho-cli, mcp-server-*) inherit cp311 .pyd binaries under their own cp313 interpreter and crash with: ModuleNotFoundError: No module named 'pydantic_core._pydantic_core' Fix: - Drop the os.environ['PYTHONPATH'] write. Keep the in-process sys.path and site.addsitedir setup unchanged (those are what MCP discovery actually depends on). - Keep os.environ['VIRTUAL_ENV'] (a benign Python-only env signal). - Update the docstring to document the root cause and pointer to the correct scoped-env helper for any future subprocess consumer. Verified post-fix: a Windows Python process with PYTHONPATH explicitly unset gets through _ensure_windows_gateway_venv_imports() without the variable being written, and a bash subprocess spawned afterward reports PYTHONPATH=[UNSET_FALLBACK]. VIRTUAL_ENV is still set as intended. Belt-and-suspenders on the consumer side: ~/.bash_env (added 2026-07-31 in this same effort) sources unset PYTHONPATH for every non-interactive bash session via the Windows-user-level BASH_ENV var, so any user-side attack surface is shut even before the gateway fix lands. Repo-local cleanup: branch fix/gateway-pythonpath-ambient-leak on bbasketballer75/hermes-agent forks off origin/main dbe1442 with this single-file diff; no other files touched.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR primarily removes an ambient PYTHONPATH mutation in the Windows gateway startup path to prevent leaking the gateway venv’s site-packages into downstream subprocesses (notably bash terminals), which can break cross-version Python tooling. It also includes a sizable TUI composer attachment-token refactor (pastes + images as [[ … ]] tokens) and a desktop composer placeholder/emptiness-marker adjustment.
Changes:
- Stop exporting
PYTHONPATHfrom_ensure_windows_gateway_venv_imports()while keeping in-processsys.path/site.addsitedirsetup. - Refactor the TUI composer to manage both paste snippets and attached images as unified
ComposerTokens, with token expansion at submit time. - Fix desktop contenteditable placeholder rendering by marking “emptiness” via
data-emptyrather than relying on:empty.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
gateway/run.py |
Removes ambient PYTHONPATH export; expands rationale in docstring/comments. |
ui-tui/src/domain/messages.ts |
Removes image attachment notice helpers from messages domain. |
ui-tui/src/domain/attachments.ts |
Introduces shared token helpers (image token labels, token expansion, dropped-token detection). |
ui-tui/src/app/useComposerState.ts |
Converts composer state from paste-snips to generalized tokens; adds attach/detach flows. |
ui-tui/src/app/useSubmission.ts |
Switches submit-time expansion to token-based expansion sourced from refs. |
ui-tui/src/app/submissionCore.ts |
Removes out-of-band “detected file/image” activity notices on submit path. |
ui-tui/src/app/useMainApp.ts |
Wires token reconciliation on every input change; routes slash commands to new attach actions. |
ui-tui/src/app/useSessionLifecycle.ts |
Clears composer tokens on session transitions (replacing paste-snips clearing). |
ui-tui/src/app/interfaces.ts |
Updates composer interfaces to token model and new attach APIs. |
ui-tui/src/app/slash/commands/core.ts |
Updates /paste to attach clipboard images via composer actions. |
ui-tui/src/app/slash/commands/session.ts |
Updates /image to attach by path via composer actions. |
ui-tui/src/app/useSubmission.test.ts |
Removes tests tied to the old expandSnips helper. |
ui-tui/src/__tests__/attachments.test.ts |
Adds tests for token expansion, image indexing, and dropped-token detection. |
apps/desktop/src/styles.css |
Updates placeholder selector to include [data-empty] marker. |
apps/desktop/src/app/chat/composer/rich-editor.ts |
Adds emptiness marker helper and shared placeholder class constant. |
apps/desktop/src/app/chat/composer/index.tsx |
Uses shared placeholder class constant for the main composer. |
apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx |
Uses shared placeholder class constant for the “edit message” composer. |
apps/desktop/src/app/chat/composer/empty-composer.test.ts |
Extends tests to assert placeholder marker behavior across normalize/render paths. |
Suppressed comments (1)
gateway/run.py:312
- Same issue in the inline comment: “See fix/gateway-pythonpath-ambient-leak” reads like a branch pointer. Keeping the comment self-contained avoids stale references.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Tokens whose label is no longer anywhere in the composer text. */ | ||
| export const droppedTokens = (tokens: ComposerToken[], value: string) => { | ||
| const live = new Set(value.match(PASTE_SNIPPET_RE) ?? []) | ||
|
|
||
| return tokens.filter(t => !live.has(t.label)) | ||
| } |
| return (value: string) => | ||
| value | ||
| .replace(new RegExp(`[ \\t]?(?:${PASTE_SNIPPET_RE.source})`, 'g'), match => { | ||
| const token = byLabel.get(match.trimStart())?.shift() | ||
|
|
||
| if (!token) { | ||
| return match | ||
| } | ||
|
|
||
| return token.kind === 'paste' ? match.slice(0, match.length - token.label.length) + token.text : '' | ||
| }) | ||
| .trim() | ||
| } |
| Note: previous revisions also exported ``PYTHONPATH=<project_root>; | ||
| <site-packages>`` to ``os.environ`` on the theory that downstream | ||
| subprocesses would inherit it. That mutation had no in-tree consumer (MCP | ||
| discovery runs in-process) and leaked ``PYTHONPATH`` into every subprocess | ||
| spawned afterward, including the bash terminals the chat session opens — | ||
| which broke cross-version Python tools (``uvx``, ``uv tool``, | ||
| ``honcho-cli``, ``mcp-server-*``) that inherit cp311 site-packages under | ||
| their own cp313 interpreters and crash with ``ModuleNotFoundError: No | ||
| module named 'pydantic_core._pydantic_core'``. Removed in | ||
| fix/gateway-pythonpath-ambient-leak; any future subprocess can build its | ||
| own scoped env block via | ||
| ``hermes_cli.gateway_windows._prepend_pythonpath`` like the NSSM service | ||
| wrapper already does. |
| def _ensure_windows_gateway_venv_imports() -> None: | ||
| """Make detached Windows gateway runs see the Hermes venv packages. | ||
|
|
||
| Some Windows restart paths run the gateway under uv's base ``pythonw.exe`` | ||
| to avoid the venv launcher respawning a visible console interpreter. That |
| (attached: ImageAttachResponse & { path?: string }, value: string, cursor: number): ComposerPasteResult => { | ||
| const index = nextImageIndex(tokensRef.current) | ||
| const label = imageToken(index) | ||
|
|
||
| setComposerTokens(prev => trimTokens([...prev, { index, kind: 'image', label, path: attached.path ?? '' }])) |
Required for the upstream contributor-attribution check (CI gate) to green for PR #15 (fix(gateway): stop leaking PYTHONPATH to subprocesses). One-line entry, format matches the existing files in contributors/emails/.
|
CI Diagnostic — Python test slices 1/4/5/7/8 fail (environmental, not a regression from this PR) The failing checks are environmental, not caused by this PR:
The PR's actual change is a 4-line net diff (PYTHONPATH ambient mutation removed from a Windows-gateway-startup helper; sys.path.insert / site.addsitedir / VIRTUAL_ENV semantics unchanged). It doesn't load or call Tuple, memory_tool._read_raw_checked, or any of the failing test paths. Suggested fix paths (in order of preference):
cc anyone reviewing — would appreciate a re-run before deeper investigation since local repro is fully green. |
Two defects in the composer token handling, plus type/comment cleanup.
droppedTokens() tested set membership, so it could not tell that one of
several identical labels had been deleted. Repeated labels are explicitly
supported (expandTokens resolves them left to right), so deleting one of
three [[ Image 1 ]] tokens has to drop exactly one; membership saw the
label still present and dropped none. Now counts occurrences and matches
survivors left to right, mirroring expandTokens' shift() order.
expandTokens() called .trim() unconditionally, so submitted text differed
from the transcript bubble even when nothing expanded — anyone who typed
deliberate leading or trailing whitespace had it silently rewritten. The
trim exists to clean up the gap an image token leaves behind, so it is now
gated on an expansion having actually occurred rather than removed.
Also adds path/attached to ClipboardPasteResponse and ImageAttachResponse
in gatewayTypes.ts — the server really returns them (see
tui_gateway/methods_prompt.py) — and drops the three ad-hoc
`& { path?: string }` intersections that were widening the types at the
call sites. Replaces two references to this PR's temporary branch name in
gateway/run.py comments with #15.
Both fixes verified as real regression tests: restoring set membership
fails the duplicate-label test, and restoring the unconditional trim fails
the token-free passthrough tests.
|
Addressed all five findings in
Dead branch names — both references in Scope complaint — fair, and the description was simply inaccurate. Retitled and rewrote the body to describe all three scopes honestly instead of claiming a single-file change. The gateway fix is ~4 net lines and self-contained; happy to split it out if you'd rather review it alone. Verified both TUI fixes are genuine regression tests by reverting each: set membership fails the duplicate-label case, unconditional trim fails the token-free passthrough cases. 22 passed, 🤖 Addressed by Claude Code |
bbasketballer75
changed the title
|
Ran down the CI failure. It isn't a stale cache, and it isn't this PR.
I reproduced it deterministically by stamping Two corrections to the earlier diagnosis on this PR:
Fixed upstream in NousResearch#75837 (a suite-wide autouse fixture in 🤖 Investigated by Claude Code |
…h-ambient-leak # Conflicts: # contributors/emails/bbasketballer75@gmail.com
Three related-but-distinct scopes. Flagging that plainly up front — the original description claimed a single-file change, which was wrong (19 files).
1. Gateway: stop leaking PYTHONPATH to subprocesses
The core fix, ~4 net lines in
gateway/run.py._ensure_windows_gateway_venv_imports()setos.environ["PYTHONPATH"]at startup. Nothing in-tree consumed it — MCP discovery runs in-process viasys.path— but it leaked to every subprocess the gateway spawned, so cross-version Python tools launched from chat-session terminals picked up a cp311site-packagesand died withModuleNotFoundError: No module named 'pydantic_core._pydantic_core'.2. TUI composer tokens (
ui-tui/)droppedTokens()tested set membership, so deleting one of several identical[[ … ]]labels dropped none. Now counts occurrences and matches survivors left to right, mirroringexpandTokens()'sshift()order.expandTokens()called.trim()unconditionally, so submitted text diverged from the transcript bubble for input with deliberate leading/trailing whitespace. Now gated on an expansion actually happening.path/attachedadded toClipboardPasteResponseandImageAttachResponse; the three ad-hoc& { path?: string }intersections at the call sites are gone.3. Desktop composer placeholder
Small placeholder-text fix in the desktop composer.
Verification
ui-tui: 22 passed inattachments.test.ts;tsc --noEmitclean.py_compileclean ongateway/run.py.Happy to split (1) out on its own if maintainers prefer — it's self-contained and would merge trivially.
🤖 Updated with Claude Code