Codex SHOULD-FIX: add a committed test proving the de-dup retry variant never
clobbers a user-source manual rename (retry goes through the same provenance
rank guard). Additive test-only; no behavior change to the contributor's fix.
Co-authored-by: webtecnica <webtecnica@users.noreply.github.com>
Codex round-2 SHOULD-FIX: add a committed test asserting the hinted bare-id
fallback returns null when two same-provider options normalize identically
(refuses to guess), plus single-match resolves and exact-match unaffected.
Additive test-only; no behavior change to the contributor's fix.
Co-authored-by: josephcy95 <josephcy95@users.noreply.github.com>
Codex round-1 SHOULD-FIX: the PR's only test change mocked the new write to a
no-op. Add real-SessionDB tests proving generated-title persistence, manual-rename
no-overwrite, and cli/gateway source preservation on ensure_session. Additive
test-only; no behavior change to the contributor's fix.
Co-authored-by: liuguangyong93 <liuguangyong93@users.noreply.github.com>
#3964 [security] gate first-password bootstrap (_set_password on POST /api/settings
while auth disabled) to local clients — blocks remote unauth first-run ownership.
Uses request-start auth snapshot (auth_enabled_before), so no mid-request TOCTOU.
Self-rebased onto v0.51.357 (8-behind, 3-dot fidelity verified byte-identical).
#3970 (oauth single-flight) DROPPED from this stage: Codex+Opus both caught a
check-then-insert race — _pending_oauth_flow_for releases the lock before the
device-code request + flow insertion, so concurrent unauth starts still spawn
multiple workers (Codex empirically reproduced w/ 2 threads). Returned to author
w/ the atomic per-(provider,home) start-lock fix. Re-gating the (N-1) stage.
Co-authored-by: Hinotoi-agent <Hinotoi-agent@users.noreply.github.com>
Both reviewers noted the _preservedLiveTurn.dataset.sessionId stamp was a dead
write in the segment-swap path (only the segment enters the DOM there; the
rebuilt turn was already stamped at build time). Move the stamp into the two
whole-turn paths where the preserved turn actually enters the DOM. No behavior
change; removes the dead write.
Codex round 2 caught that _structuralCount() only enumerated .tool-call-group/
.tool-card-row/.tool-worklog-group but NOT the live Worklog shell
(.live-worklog[data-live-worklog-shell=1]) or its .wl-reason content (built by
_flushPendingSegmentRender / ensureLiveWorklogContainer, see messages.js:2386 +
ui.js:6712/6655). So a turn whose live-ahead structure is a Worklog (not a bare
tool-call-group) undercounted preserved structure → wrongly took the segment-swap
path → detached the visible live Worklog container for a frame.
Fix: add .live-worklog[data-live-worklog-shell=1] and .wl-reason to the structural
count (mirrors the canonical 'has current worklog content' selector set at
sessions.js:1125-1131). Added a structural-test assertion + verified live: a
preserved turn with a live worklog .wl-reason the rebuild lacks now takes the
whole-turn restore path and the worklog + parser both survive.
Convergence: Codex findings narrowing each round (tie-guard → tail-segment →
worklog-selector), all real, all fixed + empirically verified.
Opus flagged a tool-card-frame-loss edge in the segment-only swap: the premise of
#3877 is that the live DOM can be AHEAD of S.messages, so a tool/worklog group can
land in the live turn between the last throttled persist and a mid-stream rebuild.
The rebuild (built from the lagging S.messages) would then lack that group, and a
segment-only swap would drop it for a frame (empirically confirmed: a live-only
tool card present before the rebuild was absent after).
Fix: count structural blocks ([data-live-assistant=1] + tool/worklog/thinking
groups) on both turns. Segment-swap only when the rebuild is the structural
SUPERSET (rebuilt >= preserved); otherwise restore the WHOLE preserved turn so
live-only structure the user already saw never vanishes. Verified live: tool-card-
ahead → whole-turn restore keeps the card + parser; single-segment tie → segment
swap, parser connected; multi-live-segment tail → tail parser connected, both
segments kept.
Also addresses Opus's dead-code note implicitly — the dataset.sessionId stamp now
matters on the whole-turn restore path (it's the branch that actually inserts the
preserved wrapper).
Codex caught an asymmetry in the first cut: the rebuilt side selected the tail
live segment ([...].pop()) but the preserved side used querySelector() = the
FIRST [data-live-assistant=1] segment. In a multi-live-segment turn (reconnect /
post-tool activity boundaries — see messages.js ensureAssistantRow re-attaching
to the LAST live segment), the smd parser writes into the tail segment, so
swapping the first preserved segment would move the wrong node and leave the
parser-owned tail detached — re-introducing the flicker for multi-segment turns.
Fix: select the preserved segment from querySelectorAll, defaulting to the LAST
(tail), and prefer the one whose data-live-segment-seq matches the rebuilt tail;
compute _preservedLen from that segment. Added a structural regression test
pinning the tail selection.
Verified live: a 2-live-segment turn (seq 1 + seq 2, parser on seq 2) keeps the
parser-owned tail connected across a mid-stream rebuild (was orphaned with the
first-segment selection).
The #3877 fix (#3892) preserves the live assistant turn's DOM node across a
mid-stream renderMessages() rebuild and swaps it back in when the rebuilt turn
has less streamed text than the preserved (smd-parser-referenced) node. But the
guard was strict: _rebuiltLen < _preservedLen. At the throttled session
write-back boundary the rebuilt turn's live segment can carry EXACTLY the same
text length as the preserved node, so the strict guard skipped the swap and left
the streaming parser writing into the now-detached original node — the residual
'disappears, then reappears' frame the reporter still saw on v0.51.347.
Fix:
- Relax the guard to _rebuiltLen <= _preservedLen so the equal-length tie also
restores the parser-referenced node (on a tie the preserved node is strictly
preferable: it holds the live parser reference and nothing is lost). When the
rebuilt turn genuinely has MORE content (reconnect where S.messages caught up
past the parser) the guard still skips and the parser re-resolves to the fuller
node.
- Swap at the SEGMENT level (replace only the rebuilt live segment with the
preserved one) instead of a whole-turn replaceWith, so a multi-segment turn
(earlier settled segments + tool/worklog groups built by the rebuild) keeps
that rebuilt-only structure. Whole-turn replace remains the fallback when the
rebuilt turn has no live segment to target.
Verified live (isolated server on the edited ui.js, real shipped renderMessages):
- single-segment tie: parser node stays connected (was orphaned on master);
- multi-segment tie: parser connected AND both assistant segments preserved;
- rebuilt-longer: correctly keeps the fuller rebuilt node, no content loss.
Updated the #3877 structural tests to pin the <= guard + segment-level swap.
Closes#3877.
- _load_models_cache_from_disk: resolve _get_models_cache_path() once
(was called twice — exists() then open()) (greptile P2).
- conftest: use a _MISSING sentinel for the _active_profile restore guard so
it restores whenever api.profiles was importable pre-test, independent of the
value (greptile P2; _active_profile defaults to 'default' so the prior
is-not-None guard was functionally fine but technically value-dependent).
Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
Codex + Opus both independently caught a CORE gap in the first cut: the
bounded /api/models rebuild runs on a detached 'models-catalog-rebuild' daemon
thread that inherits neither the request-profile thread-local (#798) nor
os.environ. So on a non-default profile the worker probed the DEFAULT profile's
credentials and, when the 4s budget was exceeded, published the rebuilt catalog
to the DEFAULT profile's disk cache (cross-contamination) — exactly the slow
path a non-default cold rebuild takes.
Fix:
- profile_scope_for_detached_worker(profile_name): sets the request-profile TLS
AND applies the profile .env on the worker thread, restoring both on exit
(no-op for default). Distinct from profile_env_for_active_request (which reads
the current thread's TLS and must not clear it).
- get_available_models() captures the active profile on the request thread and
wraps the rebuild worker body (probe + over-budget publish + disk save +
fingerprint) in that scope; the legacy synchronous rebuild applies the profile
env on the foreground. /api/models route no longer wraps (the work moved into
get_available_models so ALL callers — chat/start, resolution — are fixed).
- 2 new regression tests incl. the worker-thread before/inside/after assertion.
Empirically verified: a fresh worker thread resolved models_cache.json/default
WITHOUT the scope (the bug) and models_cache.work.json + the work .env + the
work auth.json WITH it.
Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
On a non-default profile, Settings → Providers timed out and the model
picker showed only the default profile's models. WebUI profile switching is
per-client/cookie-scoped (#798), but two read-only paths resolved from the
process-global default profile:
- Facet A: /api/providers + /api/models did not apply the active profile's
.env around the read, so get_auth_status() / provider_model_ids() / custom
key lookups resolved the default profile's credentials. On a non-default
profile the auth probes could stall past the 30s frontend abort.
- Facet B: the /api/models disk cache was a single import-time
STATE_DIR/models_cache.json shared across every profile, while the cache
fingerprint is profile-specific -> a non-default profile rejected the shared
snapshot every read and cold-rebuilt (the slow serial-probe path).
Fix:
- api.profiles.profile_env_for_active_request(): applies the active
per-request profile's .env for the duration of the read (delegates to the
existing profile_env_for_background_worker used by streaming). No-op for the
default/root profile, so single-profile deployments are byte-identical.
- api.config._get_models_cache_path(): profile-keys the disk cache filename
(models_cache.<profile>.json) derived from the default path; default profile
keeps models_cache.json unchanged (no file migration).
- routes.py: wrap both GET handlers in profile_env_for_active_request.
- conftest: restore api.profiles._active_profile + clear request-profile TLS
after each test (a pre-existing isolation hole that profile-keyed cache
paths newly surface under sharding).
Tested: 10 new regression tests; live before/after on isolated servers shows
a non-default profile now surfaces its configured provider (deepseek) with
its own credentials + its own cache file, where master showed only the
default profile's providers and one shared cache.
Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
Per maintainer UX direction: drop the text badge pill ("APPROVAL" /
"N QUESTIONS") in favor of color-coding the existing right-side status dot
plus the colored left rail. Red dot (--error) for pending approvals, amber
dot (--warning) for pending clarifies; theme-driven, visible even when the
session is not streaming/unread. Approval dot gently pulses (disabled under
prefers-reduced-motion). Quieter and consistent with the existing pin/unread
dot family — the row no longer needs to truncate its title to fit a pill.
Updated test_session_attention_badges to assert the dot classes
(is-attention-approval/clarify) instead of the removed badge element/styles.
Co-authored-by: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com>
The new CLI/gateway insights pass counted ALL state.db sessions, but
WebUI-native sessions are persisted to state.db with source='webui' AND
already counted from the sidecar _index.json first pass — double-counting
them in totals, model breakdown, and daily charts. Add
`AND COALESCE(source, '') != 'webui'` so only CLI/gateway/cron/tui rows are
added by the second pass.
Adds regression tests proving (a) CLI + Telegram sessions appear in totals
and (b) a webui-source state.db row is not double-counted against its
_index.json entry.
Co-authored-by: wind-chant <wind-chant@users.noreply.github.com>
The Docker smoke gate added in this same PR caught a real production
regression on its very first CI run. v0.51.84 (PR #2470) mounted
hermes-agent-src read-only on the WebUI side and widened the chown
prune to keep the read-only walk happy, but missed that the WebUI's
startup also runs:
uv pip install "$_agent_src[all]"
against the same now-read-only mount. setuptools' egg_info step writes
hermes_agent.egg-info/ inside the source tree even under PEP 517 build
isolation (this is by design -- PEP 517 isolates the BUILD environment,
not the source tree's metadata directory). On a :ro mount this returns
EROFS, the install fails, error_exit fires, and every multi-container
deploy dies at startup. The smoke gate flagged it on both the
two-container and three-container variants.
The fix
-------
Stage the agent source into a writable build dir under /tmp BEFORE
invoking pip install, then point pip at the staged copy.
_stage_src="/tmp/hermes-agent-build"
rm -rf "$_stage_src" && mkdir -p "$_stage_src"
rsync -a --exclude='*.egg-info' --exclude='build' --exclude='dist' \
--exclude='__pycache__' --exclude='.git' \
"$_agent_src"/ "$_stage_src"/
uv pip install "$_stage_src[all]" ...
rm -rf "$_stage_src"
The exclusion list matters: when setuptools sees a pre-baked *.egg-info,
build, or dist directory, it takes a timestamp-update code path that
also reads/writes inside that directory -- which itself fails on a :ro
source. Excluding them keeps the build on the fresh-build path
unconditionally.
rsync is in the production image (Dockerfile line 41-44). For users
running custom WebUI images without rsync, the script falls back to
cp -a + post-copy rm -rf of the same artifacts.
Tests
-----
Two new source-level invariants in tests/test_docker_docs_and_readonly.py:
test_docker_init_stages_agent_source_for_writable_install
-- asserts _stage_src=... is declared
-- asserts every `uv pip install ...[all]` line uses _stage_src,
NOT raw $_agent_src
test_docker_init_excludes_egg_info_during_staging
-- asserts the staging path excludes *.egg-info (rsync exclude
form or cp-fallback's explicit rm -rf both pass)
These would have caught the v0.51.84 regression at the source level
(once written; they're new). The Docker runtime smoke gate is the
durable defence for the broader class of :ro x init-script
interactions, since source-level invariants only catch what they're
written to catch.
Verification
------------
- pytest tests/test_docker_docs_and_readonly.py: 11 passed (9 existing
+ 2 new)
- pytest tests/ -q --timeout=60: 5891 passed, 6 skipped (was 5889;
delta is exactly the 2 new tests)
- bash -n docker_init.bash: clean
Once this lands, the Docker smoke gate's two/three-container variants
should go green, completing the self-validating loop.
Closes the source-only-test gap that let v0.51.84's :ro-mount x chown -h
{} + startup regression reach review with 5800+ green pytests. Adds a
new GitHub Actions workflow .github/workflows/docker-smoke.yml that
actually runs 'docker compose up' against each compose variant.
Triggers
--------
Path-filtered on pull_request + push to master:
Dockerfile, docker_init.bash, docker-compose*.yml, .dockerignore,
.env.docker.example, .github/workflows/docker-smoke.yml itself.
Also workflow_dispatch for manual runs.
Jobs
----
1. compose-config -- preflight that 'docker compose config' parses each
of the three compose files. Cheap, fast, catches schema/interpolation
drift in parallel before any container starts.
2. smoke (matrix: single / two-container / three-container) -- for each
variant:
a. Reap any leftover hermes-smoke-* containers/volumes/networks from
prior runs (defence-in-depth on self-hosted runners; hosted runners
are fresh).
b. docker build -t ghcr.io/nesquena/hermes-webui:latest .
Critical: the multi-container compose files reference the GHCR
image. Without this retag, multi-container smoke would test the
previously-released image, NOT the PR's docker_init.bash / Dockerfile
changes. With the retag, Compose's default pull_policy=missing keeps
the local build in place and the PR is genuinely exercised.
c. mktemp -d for ephemeral HERMES_HOME + HERMES_WORKSPACE so the
runner's host filesystem is never touched.
d. docker compose up -d --wait --wait-timeout 120 (Dockerfile carries a
HEALTHCHECK so --wait blocks on 'healthy', not just 'running').
e. curl /health probe with a 30-attempt x 2s poll loop as headroom for
the multi-container variants' Python dep install phase.
f. grep startup logs for known-bad signatures:
EROFS | Read-only file system | Traceback | PermissionError |
error_exit | groupmod: cannot | usermod: cannot |
Failed to set (UID|GID|owner|permissions|ownership)
These are the exact patterns that would have flagged #2470 in real
time. Failed-to-set is anchored to specific objects to avoid false
positives on benign locale/library bootstrap warnings.
g. trap on EXIT: docker compose down -v --remove-orphans + rm -rf the
ephemeral host paths, regardless of how the job exited.
Safety
------
- permissions: contents: read only -- no GITHUB_TOKEN write scope.
- Fork PRs run with no secrets (standard pull_request, not
pull_request_target).
- No host bind mounts; no ~/.hermes exposure; no network egress beyond
what compose itself needs to pull the agent image.
- timeout-minutes: 15 on the smoke job as a hard ceiling against a
hung docker build.
- Per-run COMPOSE_PROJECT name (hermes-smoke-VARIANT-RUNID-ATTEMPT)
so concurrent runs or reruns can't clobber each other.
Out of scope for v1 (per design review)
---------------------------------------
- HERMES_WEBUI_SMOKE_TEST env flag in docker_init.bash -- production-code
footgun that would let any leaked env var silently exit before
serving traffic.
- --user 60000:60000 -- incompatible with the image's root-init phase
and would skip the very chown branch we are guarding against.
- Local-runnable scripts/docker-smoke-test.sh -- defer until CI gating
ships and we see what contributors actually trip over.
- Hadolint / yamllint -- separate lint workflow, follow-up PR.
- Podman runtime smoke -- defer until a podman-specific bug ships.
Pre-merge verification
----------------------
- actionlint: clean
- YAML parse: clean (3 triggers, 2 jobs, 3-variant matrix)
- bash -n on all 6 run-blocks: clean
- pytest tests/ -q --timeout=60: 5889 passed, 6 skipped (no test impact;
workflow-only change)
- Opus design review on the brief (REVISE -> minimum scope adopted)
- Opus implementation review on this workflow (APPROVE)
The hermes-agent-src named volume in the two- and three-container compose
files is initialised from the agent image's /opt/hermes on first `up` and
Docker reuses it verbatim on every subsequent `up` — even after a fresh
`docker pull` of the agent image. This was the root cause of #1416 (the
'missing entrypoint' symptom was a stale cached volume hiding the new
image's source tree).
Changes:
- Add an 'Upgrading the agent container' section to docs/docker.md with
the canonical `down → docker volume rm → pull → up -d` recipe, plus the
same pointer as a comment block in both multi-container compose files
near the volume declarations.
- Switch the WebUI's hermes-agent-src mount to `:ro` in both multi-container
compose files. The WebUI only reads this volume to install the agent's
Python deps at startup; mounting it read-only enforces that at the kernel
layer and brings the actual mount mode in line with the existing
docs/docker.md architecture diagram (which already labelled this edge as
read-only).
- Align the workspace bind default in both multi-container compose files
with the single-container convention — `${HERMES_WORKSPACE:-${HOME}/workspace}`
instead of `${HERMES_WORKSPACE:-~/workspace}` — so the default resolves
the same way across Linux, macOS, WSL2, and Docker Desktop on Windows.
- Add a 'What the multi-container setup isolates (and what it doesn't)'
section to docs/docker.md to frame the two/three-container setups as
process/network/resource isolation, not filesystem isolation, so users
don't reach for multi-container expecting a trust boundary it doesn't
provide.
- Cross-link #1416 from the Related issues section.
Adds 9 regression tests in tests/test_docker_docs_and_readonly.py covering:
- :ro on the WebUI side of hermes-agent-src in both files
- agent side stays read-write (still needs to populate /opt/hermes on first run)
- ${HOME} (not ~) in workspace bind defaults in both files
- single-container file already uses ${HOME} (pin to prevent drift)
- docs/docker.md has the 'Upgrading the agent container' section + recipe
- compose files reference docs/docker.md + show the upgrade step inline
- docs/docker.md frames the isolation model honestly
Test suite: 42 passed (33 existing Docker tests + 9 new). No behaviour
change for users who set HERMES_WORKSPACE explicitly, and no migration is
required for existing deployments — Docker rebinds the existing volume
read-only on next `up`. Users upgrading the agent image should now follow
the documented `docker volume rm hermes-agent-src` recipe.
Closes#1416 (documented upgrade procedure) and addresses the read-only
half of the multi-container coupling concern raised on #2453.
Opus advisor caught this on stage-369 review — PR #2347 left a 'PR TBD'
placeholder in CHANGELOG that should reference its own number. One-line
attribution fix, no behavior change.
PR #2347 hoisted the inline state object to a `state` variable so the
auto-compression handler could share it with appendLiveCompressionCard.
Behavior is identical — same setCompressionUi() dispatch, same calm
compression-card path — but tests/test_run_journal_frontend_static.py
pinned the literal substring `setCompressionUi({` to verify the call
site. Relax the assertion to accept either inline (`{...}`) or hoisted
(`state`) argument form. Both forms route through the same compression
card path; the over-specific substring was the bug.
api.updates._schedule_restart() spawns a daemon thread that calls
os.execv() after a short sleep. Tests in test_update_banner_fixes.py
monkeypatch os.execv to a no-op, but monkeypatch teardown can win the
race against the daemon thread — when the thread wakes up after
teardown, the real os.execv is back, and it re-execs pytest with the
original argv. From the outside this looked like pytest hanging at 99%
and then restarting the entire suite from 0% in a loop.
The fix shadows os.execv with a permanent no-op wrapper at conftest
module-import time, so late-firing daemon threads can't escape. Tests
that need to verify execv was called still patch it themselves; their
patches sit on top of the wrapper for their lifetime.
Also adds tests/test_pytest_execv_guard.py to pin the guard against
future conftest refactors.
