Skip to content

fix: merge WebUI agent thread env before setting#1266

Closed
hi-friday wants to merge 1 commit intonesquena:masterfrom
hi-high:fix/webui-thread-env-duplicate-cwd
Closed

fix: merge WebUI agent thread env before setting#1266
hi-friday wants to merge 1 commit intonesquena:masterfrom
hi-high:fix/webui-thread-env-duplicate-cwd

Conversation

@hi-friday
Copy link
Copy Markdown

@hi-friday hi-friday commented Apr 29, 2026

Summary

  • merge profile runtime env and per-run WebUI env before calling _set_thread_env
  • let active workspace override profile TERMINAL_CWD while preserving profile terminal settings
  • add regression coverage for duplicate TERMINAL_CWD handling

Test Plan

  • ~/.hermes/hermes-agent/venv/bin/python -m pytest tests/test_profile_terminal_env.py -q
  • ~/.hermes/hermes-agent/venv/bin/python -m pytest -q

@nesquena-hermes
Copy link
Copy Markdown
Collaborator

nesquena-hermes commented Apr 29, 2026

Thanks for this fix, @friday!

What this addresses: Issue #1270 — when terminal.cwd is set in a profile's config.yaml, _set_thread_env() receives TERMINAL_CWD twice (once via **_profile_runtime_env and once as an explicit kwarg), causing a TypeError and a 500 on every message send.

Approach in this PR: Merging the profile runtime env and the per-run WebUI env before passing to _set_thread_env(), so explicit per-run values override profile values without duplication. This is cleaner than the pop()-before-call approach since it handles all env keys generically, not just TERMINAL_CWD.

Regression test: The PR includes coverage for the duplicate TERMINAL_CWD case — that's exactly the right safety net for this kind of kwarg merging logic.

Note on #1270: A commenter on issue #1270 references PR #1265 as an alternative fix using a _build_agent_run_env() helper. Maintainer may want to compare the two approaches before merging to avoid parallel fixes landing for the same root cause.

Looks like a solid, targeted fix. Leaving for maintainer review.

This was referenced Apr 29, 2026
Closed
Closed
@KingBoyAndGirl
Copy link
Copy Markdown

KingBoyAndGirl commented Apr 29, 2026

Just came across this PR while tracking the same issue (#1270). 👋

I have a similar fix in #1271 using the pop() approach, but I think this PR (#1266) is the better solution because:

✅ It handles all conflicting keys generically (not just TERMINAL_CWD)
✅ No need to repeat the fix for each new env var (HERMES_EXEC_ASK, HERMES_SESSION_KEY, etc.)
✅ Cleaner architecture with merge approach

My vote: If maintainers are happy with this approach, please merge this PR and close #1271. I'm totally fine with that — the goal is to get the fix in, not to push a specific implementation. 😊

Great work on the more generic solution! 🎉

@nesquena-hermes
Copy link
Copy Markdown
Collaborator

nesquena-hermes commented Apr 29, 2026

Thanks for the cross-reference, @KingBoyAndGirl — helpful context for the maintainer.

The three-PR comparison is now summarized in #1271 as well. The merge approach here (#1266) is the most future-proof since it prevents the duplicate-key problem for any env var, not just TERMINAL_CWD. Maintainer can pick whichever shape fits best; once one lands, the other two should be closed.

This was referenced Apr 30, 2026
Closed
Merged
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

nesquena-hermes commented Apr 30, 2026

✅ Merged into release branch — included in PR #1285 (v0.50.240). Thanks for the contribution!

nesquena-hermes added a commit that referenced this pull request Apr 30, 2026
@nesquena-hermes
release: v0.50.240
33a145a 
## Release v0.50.240

Batch release of 13 PRs that passed full triage + code review + test suite (3199 tests, 0 failures).

---

### Added

- **Compact tool activity mode** (`simplified_tool_calling`, default on) — groups tool calls and thinking traces into a single collapsed "Activity" disclosure card per assistant turn. Also adds a new **Calm Console** theme with earth/slate palette and serif prose. @Michaelyklam — #1282
- **PDF first-page preview** — `MEDIA:` `.pdf` files render a canvas thumbnail via PDF.js CDN (4 MB cap). **HTML sandbox iframe** — `.html`/`.htm` files render inline in a sandboxed `<iframe srcdoc>` (256 KB cap). 10 i18n keys × 7 locales. @bergeouss — #1280, closes #480 #482
- **Inline Excalidraw diagram preview** — `.excalidraw` files render as pure SVG (no external deps; rectangles, ellipses, diamonds, text, lines, arrows, freehand; 512 KB cap). @bergeouss — #1279, closes #479
- **Inline CSV table rendering** — fenced `csv` blocks and `MEDIA:` CSV files render as scrollable HTML tables with auto-separator detection. @bergeouss — #1277, closes #485
- **Inline SVG, audio, and video rendering** — SVG as `<img>`, audio as `<audio controls>`, video as `<video controls>`. @bergeouss — #1276, closes #481
- **Batch session select mode** — multi-select sessions for bulk Archive/Delete/Move. 11 i18n keys × 7 locales. @bergeouss — #1275, closes #568
- **Collapsible skill category headers** — click to collapse/expand without re-render; state persists across filter cycles. @bergeouss — #1281
- **`providers.only_configured` setting** — opt-in flag to restrict the model picker to explicitly configured providers. @KingBoyAndGirl — #1268
- **OpenCode Go model catalog** — adds Kimi K2.6, DeepSeek V4 Pro/Flash, MiMo V2.5/Pro, Qwen3.6/3.5 Plus. @nesquena-hermes — #1284, closes #1269

### Fixed

- **Profile `TERMINAL_CWD` TypeError** — `_build_agent_thread_env()` helper merges env before `_set_thread_env()` call. @hi-friday — #1266
- **Service worker subpath cache bypass** — regex now matches `/api/*` under any mount prefix. @Michaelyklam — #1278
- **SSE client disconnect leaks** — `TimeoutError`/`OSError` treated as clean disconnects; server backlog 64, threads daemonized; session list renders before saved-session restore. @KayZz69 — #1267
- **i18n locale corrections** — Korean MCP strings (23), Chinese MCP strings (23), zh-Hant missing keys (41), de missing keys (229). @bergeouss — #1274, closes #1273

---

### Test results

```
3199 passed, 2 skipped, 3 xpassed in 72.79s
```

### PRs on hold (not included)

#1265 (draft), #1271 (superseded by #1266), #1272 (skipped XSS tests), #1232 (partial test run), #1222 (review questions open), #1134 (live-server tests), #1132 (superseded by #1134), #1108 (negative UX review), #1084 (empty description)
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

nesquena-hermes commented Apr 30, 2026

Shipped in release v0.50.240 (PR #1285). Thanks!

This was referenced Apr 30, 2026
Closed
Closed
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hi-friday @nesquena-hermes @KingBoyAndGirl