Conversation
alt-glitch
added
type/bug
SGuibord
pushed a commit
to SGuibord/hermes-agent
that referenced
this pull request
May 7, 2026
All tool_calls synthesis removed. The patch now only strips JSON-wrapped text responses (content/text/message/response/answer keys + action:"text") for display in Discord. It never fabricates tool_calls. Root cause of ReAct output: Ollama bug NousResearch#15539 — system prompt + think:false + tools parser fails for gemma4, causing tool_calls to leak as plain JSON text. Fix is at the model level (switching hermes-primary to phi4:14b which has stable native tool calling in Ollama). Tool dispatch is Hermes' job; this patch must not interfere with it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
teknium1
reviewed
Jul 12, 2026
teknium1
left a comment
teknium1
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for identifying the shared-IPC collision. Current main still has the underlying overwrite path, but the submitted guard cannot make concurrent admission safe.
Problems
gateway/run.py:7829checks for a marker before the later write at PR lines 7843-7845. Two simultaneous handlers can both pass that check and both spawn an update process, so the reported race remains.- The handler moved to
gateway/slash_commands.pyin619bd7827; current main still overwrites.update_pending.jsonatgateway/slash_commands.py:4531-4549. - The new tests at PR
tests/gateway/test_update_command.py:218and:257pre-seed a marker, so they do not exercise the concurrent check-to-create gap.
Suggested changes
- Port the change to
gateway/slash_commands.pyand atomically reserve the profile-wide update state before writing routing metadata or spawning the updater. - Add a synchronized two-caller regression proving only one updater launches and the winner's metadata remains intact.
Automated hermes-sweeper review.
| exit_code_path = _hermes_home / ".update_exit_code" | ||
| session_key = self._session_key_for_source(event.source) | ||
|
|
||
| if pending_path.exists() or claimed_path.exists(): |
Collaborator
There was a problem hiding this comment.
This is a TOCTOU check: two handlers can both observe no marker before either reaches the later replace(pending_path), then both launch updates. Reserve the profile-wide update state with an exclusive atomic operation and test two interleaving callers.
teknium1
added
sweeper:risk-message-delivery
14 of 19 tasks
briandevans
added a commit
to briandevans/hermes-agent
that referenced
this pull request
Aug 14, 2026
`/update` wrote `.update_pending.json` and spawned a detached `hermes update --gateway` with no admission control of any kind. Two concurrent invocations — a user double-tapping because the update runs for minutes with no acknowledgement, or two platforms on one multiplexed gateway both triggering it — each wrote the marker and each spawned an updater against the same checkout and virtualenv. The second write also replaced the first requester's routing metadata, so that user never learned their update finished. Both handlers also stage through the same `.update_pending.tmp` path, so the losing rename raises FileNotFoundError out of the handler. Reserve the profile-wide slot with os.open(O_CREAT | O_EXCL) before any routing metadata is written and before the updater is spawned. That collapses observe-and-claim into a single atomic syscall, so exactly one caller can ever win. The losing caller gets an "update already running" reply and still gets `_schedule_update_notification_watch()`, so it learns the outcome of the update that is actually running. The reservation carries a TTL: an updater killed before it can write its exit code (host reboot, OOM kill) would otherwise wedge /update for the lifetime of the profile. Any failure between the claim and the spawn releases the slot immediately so the user can retry. Supersedes NousResearch#15539, which identified this collision. That guard patched `gateway/run.py`, where the handler no longer lives after 619bd78, and used a check-then-write `if pending_path.exists(): return` that both callers can pass; its tests pre-seeded the marker, so they never exercised the check-to-create gap.
This branch has not been deployed
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
3 participants
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.
Summary
Prevent duplicate gateway
/updaterequests from starting a second detached update process while another update is still active.The gateway update flow uses profile-global IPC files:
.update_pending.json.update_pending.claimed.json.update_output.txt.update_exit_code.update_prompt.json.update_responseBefore this change, a second
/updatecall could overwrite the active update metadata and spawn another detached update process against the same shared IPC files. That could misroute progress/prompts/final notifications and clobber the original run state.What changed
GatewayRunner._handle_update_command().update_pending.jsonor.update_pending.claimed.jsonalready exists, do not spawn another update processWhy this is a bug fix
The update IPC design is single-active-run per profile. The watcher and
hermes update --gatewayprompt bridge both assume a single shared set of marker files. The handler was missing the admission guard that enforces that invariant.Tests
Added regression coverage for both active-marker states:
test_rejects_duplicate_update_when_pending_marker_existstest_rejects_duplicate_update_when_claimed_marker_existsLocal/targeted result:
tests/gateway/test_update_command.py:28 passedRisk
Low.