fix(terminal): no crash on absolute-path executables in lifecycle_guard (#76762) - #76773
Open
webtecnica wants to merge 33 commits into
Open
fix(terminal): no crash on absolute-path executables in lifecycle_guard (#76762)#76773webtecnica wants to merge 33 commits into
webtecnica wants to merge 33 commits into
Conversation
alt-glitch
added
type/bug
Collaborator
Related to #76762, the reproduced absolute-path lifecycle-guard failure this PR repairs. |
Contributor
|
Thanks for the focused regression fix. Current main still has the unhandled path at PR commit Automated hermes-sweeper review. |
teknium1
added
the
sweeper:blast-moderate
|
Independent validation of commit
This addresses the direct terminal failure from #76762 without removing the intended gateway-lifecycle protection. |
alt-glitch
removed
the
sweeper:risk-security-boundary
alt-glitch
added
the
sweeper:risk-security-boundary
Contributor
Author
|
Collaboration note: this PR supersedes @blut-agent's #76813, which was consolidated here after triage flagged it as a duplicate. The ValueError handling in the lifecycle_guard resolve/open paths originates from his work — credit for that part belongs to him. |
alt-glitch
removed
the
sweeper:risk-security-boundary
Replaces the synchronous per-sentence streamer.stream() loop in stream_tts_to_speaker with a per-sentence prefetch pipeline. Each sentence gets its own background thread that fires the HTTP request immediately, buffering PCM chunks into a per-segment queue (capped at 3 concurrent via semaphore). A single playback worker drains segments in FIFO order with PortAudio error recovery (reinit up to 3 attempts, then temp-file fallback). Also fixes PCM chunk alignment for odd-byte HTTP chunks and increases the worker join timeout from 30s to 300s. Salvage of PR NousResearch#71084 onto current main. Closes NousResearch#71084
- carry mark_audio_output_active into playback worker (df093bf) - close temp WAV handle before playback (555d4e1) - move sentinel + join into finally block (exception-path deadlock) - update _pcm_leftover before continue on reinit+rewrite success - remove dead _playback_done event - extract shared _create_output_stream helper (dedup) - extract shared _align_int16_chunks generator (dedup)
Start from main's 13 tests (renamed test_openai_available_reflects_key to test_openai_available_reflects_audio_key_resolution, added 4 new tests for xai oauth, elevenlabs secret resolver, openai configured key, stream cap). Append 12 new regression tests from PR NousResearch#71084 for the prefetch pipeline, PCM misalignment, and PortAudio resilience. Patch platform.system in stream-path tests for main's macOS guard.
…eadonly - Merge _aux_free_only() + _aux_openrouter_model() into single _aux_openrouter_settings() that reads config once via load_config_readonly (avoids double deepcopy). - Remove 15-line block comment and 5-line inline comment that restated what the code already says. - Trim module docstring from 10 lines to 3. - Update test patches to target load_config_readonly.
Multiplexed gateways resolve credentials through the fail-closed
per-profile secret scope (agent/secret_scope.py, Workstream A): any
get_secret() read outside a set_secret_scope(...) block raises
UnscopedSecretError. The agent turn installs the scope via _run_agent's
profile-scoping wrapper, but slash-command dispatch does not — so manual
/compress reached provider resolution unscoped and every invocation on a
gateway.multiplex_profiles: true deployment failed with:
Manual compress failed: get_secret('OPENROUTER_BASE_URL') called with
no profile secret scope active while multiplexing is on.
Same bug class as the cron scheduler (NousResearch#57692) and the /v1/runs agent
path — an un-migrated call site the fail-closed design is meant to catch.
Two changes, both required:
- _handle_compress_command becomes a profile-scoping wrapper around the
existing handler (renamed _handle_compress_command_inner), mirroring
_run_agent: gated on multiplex_profiles, resolves the source profile's
home and runs the whole handler inside _profile_runtime_scope. Covers
the coroutine-side read (_resolve_session_agent_runtime).
- The compressor call switches from a bare loop.run_in_executor(None, …)
to the existing _run_in_executor_with_context helper, so the scope
contextvar survives the thread hop into _compress_context, where the
aux-client provider resolution reads credentials.
Single-profile gateways take the pass-through branch — zero behavior
change (pinned by test).
Tests: 2 added (scoped read inside the executor under fail-closed
multiplexing reproduces the field failure pre-fix; single-profile
pass-through). 162 gateway compress/multiplex-scope tests green.
…ltiplex profiles Fix NousResearch#75349 Root cause: Under multiplex_profiles, secondary profiles run inside _profile_runtime_scope which installs a per-profile secret scope via set_secret_scope. The WhatsApp adapter (and the shared WhatsAppBehaviorMixin + Cloud API adapter) read WHATSAPP_MODE, WHATSAPP_DM_POLICY, etc. via raw os.getenv(), bypassing the secret scope. Since os.environ doesn't contain secondary profile .env values, the bridge silently falls back to 'self-chat' and rejects all inbound messages with self_chat_mode_rejects_non_self. Fix: - Add _wenv() helper in adapter.py that reads WHATSAPP_* vars through get_secret() (agent.secret_scope), which honors the active scope. - Replace all os.getenv('WHATSAPP_*') calls in adapter.py, whatsapp_common.py, and whatsapp_cloud.py with get_secret()-based equivalents. - Inject resolved WHATSAPP_* values into the bridge subprocess environment so the Node.js bridge (which reads process.env) sees the profile's own configuration. Changes: - plugins/platforms/whatsapp/adapter.py: 37 lines (+ helper, bridge_env injection, 2 os.getenv→_wenv) - gateway/platforms/whatsapp_common.py: 13 lines (6 os.getenv→_get_wsecret) - gateway/platforms/whatsapp_cloud.py: 21 lines (9 os.getenv→_get_wsecret) - New regression test: 6 test cases covering scope isolation, fallback, and cross-profile non-leakage.
…idge env set Follow-ups on the NousResearch#75382 salvage (review findings): - _wenv/_get_wsecret now catch UnscopedSecretError and fall back to os.getenv for the DEFAULT profile's adapter, which constructs and sends outside any _profile_runtime_scope under multiplexing — a bare get_secret would crash its WhatsApp path (fixing one profile by breaking another). Same pattern as Slack SLACK_APP_TOKEN (NousResearch#59739) and the Matrix recovery key. Scoped misses still return the default — no cross-profile borrow. - bridge_env overlay extended to the full WHATSAPP_* set bridge.js consumes (DEBUG, FORWARD_OWNER_MESSAGES, REPLY_PREFIX, MAX_MESSAGE_LENGTH, CHUNK_DELAY_MS, SEND_TIMEOUT_MS). - Removed the always-true conditional on WHATSAPP_MODE injection.
The salvaged hydrate_profile_secret_sources (NousResearch#74549) seeded its profile-local env from <home>/.env only, but the documented 1Password bootstrap flow puts OP_SERVICE_ACCOUNT_TOKEN in the gitignored <home>/.op.env (mirrored from load_hermes_dotenv). A cold profile using that flow still failed 1Password hydration — the one unaddressed item from the sweeper review on NousResearch#74549. Seed .op.env via setdefault so .env values win; never touches os.environ. Two regression tests.
Addresses the hermes-sweeper review on NousResearch#76188: 1. task_id is session-scoped (task_id == session_id), not turn-scoped, and the reap runs on a detached thread. A replacement turn could claim the same session and spawn a legitimate process before the previous turn's reaper thread actually enumerates its targets, killing that new process by mistake. Fixed by gating the reap on the existing run_generation mechanism (_is_session_run_current) instead of inventing a new ownership token: the timeout path captures its own run_generation at turn start, the interrupt path captures the generation immediately after invalidating it. If a newer turn has since claimed the session, the reap is skipped — that newer turn owns its own baseline, so nothing is left permanently unreaped. 2. gateway/platforms/api_server.py's SSE handlers for chat-completions and the /api/sessions responses endpoint run their own agent lifecycle via _run_agent() and never passed through TurnRunner, so client-disconnect abandonment there had no baseline and no reap — contradicting the PR's stated disconnect coverage. Both disconnect handlers now snapshot/reap through the same tools.process_registry primitives, via a small _reap_disconnected_agent_processes() helper shared by both call sites.
…ct reap dbbb10d shipped without direct test coverage for its own new logic — the same gap teknium's review flagged on the competing PR. Close it: - _reap_gateway_turn_processes: skips when is_still_current() is False, proceeds when True, fails open (reaps) if the check itself raises rather than silently disabling the leak fix. - _abandon_timed_out_gateway_turn: still marks the turn abandoned (interrupt fires) even when the reap itself is skipped. - api_server._reap_disconnected_agent_processes: reaps the baseline-diff for an owned turn, no-ops when the agent never recorded ownership markers. - APIServerAdapter._run_agent: markers are populated with the right task_id/baseline during the turn and cleared once it completes, closing the same race window fixed in gateway/run.py for this separate agent-lifecycle surface.
…r's result Two follow-ups from review of the salvaged fix: - _reap_gateway_turn_processes now returns 0 for a blank task_id. ProcessSession.task_id defaults to empty for sessionless callers, so a blank turn id would match (and kill) every unrelated empty-task process instead of the turn's own. - The asyncio poll loop checks executor completion BEFORE the watchdog's timeout flag. When both race in the same window, the completed run has already persisted its real reply to session history; surfacing the 'agent inactive' diagnostic would contradict the stored transcript. This matches _abandon_timed_out_gateway_turn's own worker-done-wins tiebreak.
…s sibling
The API server intentionally lets concurrent runs share a client-provided
session_id (= process task_id), so the SSE-disconnect reap could kill a
process a still-live concurrent run spawned after the disconnecting run's
baseline — the same stale-reaper bug class the gateway path gates via
run_generation.
- Per-task-id run epochs (monotonic counter): each run claims the epoch at
publish; a reaper holding a superseded epoch declines to kill. A missing
entry (the run's own clear pruned it) still reaps, so the leak fix isn't
silently disabled.
- _publish_turn_process_ownership / _clear_turn_process_ownership helpers
replace the copy-pasted marker set/clear blocks, so attribute names and
epoch bookkeeping can't drift between surfaces.
- /v1/runs — the third own-lifecycle surface — now records ownership and
reaps on POST /v1/runs/{id}/stop and on server-side SSE cancellation,
closing the remaining sibling paths of NousResearch#76115.
… exclude_ids kill_started_since duplicated kill_all's collect-under-lock/kill-outside-lock loop line for line; it is now a thin delegate through new kill_all kwargs (exclude_ids, source, consume_output). Public signatures unchanged — existing callers and test monkeypatch seams keep working. kill_process's docstring now names the deliberate consume_output=True exception for abandoned-turn reaping so the deviation isn't 'fixed' later.
…ig.yaml
agent_import.py carries a private load_yaml_file/dump_yaml_file pair that
returned {} for an absent file AND for a present file it could not read or
parse. Three importers -- import_permission_allowlist, import_permission_denylist
and import_mcp_servers -- read config.yaml through it, merge one section into
the result, and write the whole mapping straight back. So a YAML syntax error,
a permission problem or a broken mount meant the importer replaced every
setting the user had with only the one to three keys it merged, and still
reported the item as "imported". The write was a bare path.write_text(), so an
interrupted import truncated the file instead.
Distinguish the two cases at the read. Absent, or present but empty, still
yields {} so first-time creation works. Present but unreadable, unparseable, or
not a mapping raises ConfigReadError; the three sites funnel through a new
load_target_config() that records the refusal as a per-item error and leaves the
file byte-identical. Dry-run refuses too, rather than previewing an "imported"
that would destroy the config. dump_yaml_file now writes through
utils.atomic_write_text, which the module already imports and uses for the
memory store.
This is the invariant hermes_cli/config.py already enforces for its own writers
via require_readable_config_before_write / atomic_config_write, whose docstring
names this exact root cause and calls itself "the single chokepoint every
config-update path should use". agent_import.py has its own helper pair and so
was never covered; it was the last config.yaml writer without the guard.
The identical helper pair lives in openclaw_to_hermes.py, the script this module
was ported from, where twelve config.yaml read-modify-write sites share the same
defect; fixed there too. Its refusal is recorded at the run_if_selected dispatch
point, which flips the existing _config_apply_blocked flag so the remaining
config-mutating options short-circuit instead of each rediscovering the same
unreadable file. The atomic write is inlined with tempfile + os.replace because
that script runs standalone with only the stdlib on its path.
…omic write The inlined temp-file + os.replace in openclaw_to_hermes.dump_yaml_file replaced a symlinked config.yaml with a regular file, silently detaching managed deployments that symlink ~/.hermes/config.yaml into a dotfiles repo or profile package. The bare path.write_text it replaced followed the link, and utils.atomic_replace -- which the hermes_cli twin reaches through atomic_write_text -- resolves the link for exactly this reason (NousResearch#16743). Mirror that here: resolve the symlink before creating the temp file so the rename lands on the real file, and fall back to copyfile on EXDEV/EBUSY now that the target can live on another device. Covered by a regression test that fails when the resolution is removed. Also guard the permission-denied test for Windows: os.geteuid does not exist there and chmod-based denial is unreliable, so skip on non-POSIX.
- agent_import.dump_yaml_file now calls utils.atomic_yaml_write instead of hand-rolling safe_dump + atomic_write_text — same temp+fsync+atomic rename and symlink preservation, plus mode/owner preservation a 0600-secured config.yaml needs - openclaw script: the EXDEV/EBUSY copy fallback gains copystat + target fsync so the docstring's 'mirrors utils.atomic_replace' durability claim is true on cross-device deployments - trim load_yaml_file's docstring to the behavior contract
Exact parity with utils.atomic_replace: its target fsync is wrapped in try/except OSError. A failed fsync after a successful copy must not surface the already-completed write as an error (Windows can raise on fsync of a read-only handle).
webtecnica
force-pushed
the
fix/76762-lifecycle-guard-nullbyte
branch
from
August 4, 2026 11:35
40d3e2d to
840fe02
Compare
alt-glitch
added
comp/gateway
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
14 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.
Closes #76762