Skip to content

fix(runtime): report unverified Agent updates without automatic restart - #7160

Closed
hrdwdmrbl wants to merge 3 commits into
nesquena:masterfrom
hrdwdmrbl:agent/agent-runtime-update-restart
Closed

hrdwdmrbl wants to merge 3 commits into
nesquena:masterfrom
hrdwdmrbl:agent/agent-runtime-update-restart

Conversation

@hrdwdmrbl

@hrdwdmrbl hrdwdmrbl commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Thinking Path

A known Agent revision change must keep returning a manual-restart 409. Agent marker absence and a readable Git revision cannot prove that an update completed with a healthy environment. An external updater can also start after a final readiness check and before process replacement.

This PR ships the safe WebUI subset requested in the maintainer review: report update diagnostics, reject the stale in-process runtime, and leave restart to the operator. It makes no claim that WebUI can verify Agent update success or coordinate an atomic restart.

What Changed

  • api/agent_runtime.py: shared manual-restart payload with restart_scheduled: false and diagnostic agent_update_state. Missing markers report unverified; dead or over-age markers report stale; malformed, unreadable, or unclassifiable state reports unknown. Active and recovery markers remain diagnostic observations. Invalid UTF-8 and oversized PIDs fail closed; symlinked venv interpreters retain the configured installation's recovery-marker path.
  • api/routes.py: preserve the same payload across admission, commit-message generation, compression, handoff summaries, and asynchronous compression error/status paths.
  • tests/test_agent_runtime_revision_guard.py, tests/test_sprint46.py, and tests/test_issue1013_handoff_dock.py: observable manual-409, diagnostic, no-restart, and session-mutation coverage.
  • docs/troubleshooting.md: explain the diagnostic states, manual recovery, and missing Agent-owned restart contract.

The final diff contains these six files. The automatic scheduler, polling loop, final-readiness callback, browser reload wiring, and tests asserting marker absence means completion have been removed from the proposal. api/updates.py, static/ui.js, static/workspace.js, and tests/test_update_banner_fixes.py match the PR base.

Why It Matters

An interrupted or failed Agent update must never cause WebUI to restart into an unverified checkout or venv. The operator receives an actionable error while WebUI preserves its existing admission boundary and reports what its marker reads actually establish.

Contract Routing

  • Family: in-process Agent source identity and WebUI runtime admission; asynchronous compression terminal error metadata.
  • References: docs/CONTRACTS.md, docs/architecture/agent-api-contract.md, docs/rfcs/webui-run-state-consistency-contract.md, ARCHITECTURE.md, and TESTING.md.
  • State ownership: WebUI keeps its loaded revision identity and compression job error metadata. Agent owns all update and recovery markers; WebUI only reads them. No restart worker or additional lock is created by a revision mismatch.
  • Invariants: a known stale runtime cannot materialize a new chat session; diagnostic observations cannot authorize automatic restart; asynchronous status preserves the manual-restart error. Gateway- and runner-owned chat retain their existing admission ownership.

Contract Change

The base branch's manual 409 agent_runtime_stale behavior remains. Responses add explicit restart_scheduled: false and marker diagnostics; no complete or ready state is inferred. The original automatic-recovery proposal is withdrawn because its Agent-owned prerequisite is unverified. Troubleshooting guidance and response assertions are updated together.

Verification

All tests used ./scripts/test.sh, a supported Python 3.11 repo-local .venv, isolated state directories, and a synthetic Agent fixture. Agent code and live services were not inspected or exercised.

Fail-before evidence against c3757641bffa92cae5a7cd950c51eef0d717eb5a:

./scripts/test.sh tests/test_agent_runtime_revision_guard.py -k 'unverified_update_keeps or final_read_cannot' -q --tb=short
16 failed, 34 deselected

Every selected test observed an automatic restart worker being queued. The tests use real temporary Git commits and marker files, including removed failed/interrupted markers, a dead child PID, missing/stale/malformed state, recovery markers, and a marker created after the revision read. Only the scheduler's thread boundary is captured to prevent actual process replacement; the revision guard and Git reads execute.

Two further diagnostic regressions failed before correction: an oversized PID raised OverflowError, and resolving a symlinked venv Python hid its recovery marker. The first focused run also exposed two neighboring compression assertions needing the explicit manual-restart field; those expectations are updated.

Merged current master (6e6893f3) to resolve GitHub's conflict in the handoff summary error handling. The resolution preserves both the shared stale-runtime 409 and upstream's ambiguous-custom-provider 400. The expanded suite found one additional handoff assertion needing restart_scheduled: false; it is updated.

Final focused and neighboring run after the merge:

./scripts/test.sh tests/test_agent_runtime_revision_guard.py tests/test_update_banner_fixes.py tests/test_sprint46.py tests/test_updates.py tests/test_health_restart.py tests/test_update_apply_ui.py tests/test_issue4626_windows_restart_no_console.py tests/test_pycache_purge.py tests/test_issue1013_handoff_dock.py -q --tb=short
333 passed in 8.60s

python scripts/ruff_lint.py --diff origin/master
5 changed Python files; 20 existing findings; 0 on added/modified lines — passed

node --check static/ui.js
node --check static/workspace.js
git diff origin/master --check
All passed

A Node execution probe of the actual shared api() helper confirmed that manual stale-runtime HTTP 409 and asynchronous HTTP 200 error payloads preserve the error/status without triggering reload or retry (2 cases passed). No frontend diff remains against the base; no screenshots were produced.

Risks / Follow-ups

  • Automatic restart remains blocked on an Agent-owned terminal success receipt bound to the exact transaction, final revision, and healthy environment, plus an atomic handoff/lease excluding mutations across replacement, or an Agent updater that performs the restart itself. No receipt or handoff protocol is invented or implemented here.
  • Marker diagnostics are snapshots and cannot establish success, failure cause, or exclusive ownership. Operators must check the Agent update outcome/environment before manually restarting WebUI.
  • Explicit updates initiated through WebUI retain their existing scheduler behavior and are outside this revision-mismatch fix. Non-Git installs retain their existing behavior.
  • Not verified: a real Agent updater/restart integration, native Windows PID inspection/process replacement, live services, the full repository test suite, or browser layout screenshots. Synthetic tests establish WebUI behavior, not an Agent-core success contract.

Release-note wording

Hermes WebUI reports Agent update diagnostics and requires a manual restart after detecting a stale Agent revision. Missing or stale update markers cannot trigger automatic restart.

Model Used

OpenAI GPT-6 (Codex) for this follow-up, using repository tools, pytest, Ruff, Node, Git, and GitHub CLI. The earlier implementation disclosed OpenAI GPT-5 Codex.

@hrdwdmrbl
hrdwdmrbl marked this pull request as ready for review August 19, 2026 22:32
@greptile-apps

greptile-apps Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR keeps Agent revision mismatches fail-closed and operator-controlled while adding marker-based diagnostics to the existing manual-restart response.

  • Centralizes the agent_runtime_stale payload with restart_scheduled: false.
  • Classifies active, stale, incomplete, unknown, and unverified Agent update observations.
  • Preserves diagnostics through synchronous routes and asynchronous compression status.
  • Expands regression coverage and documents the manual recovery contract.

Confidence Score: 5/5

The PR appears safe to merge, with one non-blocking diagnostic accuracy issue for custom Agent virtualenv layouts.

The manual-restart boundary remains intact and stale runtime use is rejected before session mutation. The only accepted issue affects whether an interrupted update is labeled incomplete or the more general unverified; it does not authorize a restart or permit stale runtime execution.

Files Needing Attention: api/agent_runtime.py

Important Files Changed

Filename Overview
api/agent_runtime.py Adds shared stale-runtime payload construction and read-only update-marker diagnostics; custom virtualenv names can currently hide recovery markers.
api/routes.py Routes stale-runtime failures through the shared payload and preserves diagnostics in asynchronous compression job status.
docs/troubleshooting.md Documents diagnostic states, their evidentiary limits, and the required manual recovery procedure.
tests/test_agent_runtime_revision_guard.py Adds broad observable coverage for marker states and no-restart behavior, but does not cover explicitly configured virtualenvs with custom directory names.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Local Agent-backed action] --> B{Loaded revision still current?}
    B -->|Yes| C[Continue existing action]
    B -->|No| D[Read Agent-owned markers]
    D --> E[Classify diagnostic state]
    E --> F[Return typed 409]
    F --> G[restart_scheduled: false]
    G --> H[Operator verifies update and restarts WebUI]
Loading

Reviews (3): Last reviewed commit: "chore: merge master and preserve runtime..." | Re-trigger Greptile

@nesquena-hermes nesquena-hermes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep gate: this can restart WebUI into a half-applied Agent checkout — the design needs an Agent-owned success receipt that doesn't exist yet

Thanks @hrdwdmrbl — the intent is right (today's guard stopping at a manual-restart 409 genuinely leaves a gap), and the implementation is careful (WebUI stays a read-only observer of the Agent markers, the client changes are clean — no global collision, no const reassignment, no new visible surface, no untrusted HTML; the manual-409 fallback and pre-dispatch stale-revision rejection still work; 156 changed-area tests + the #6677/#7132 regression checks pass). But the automatic-restart path has two reproduced safety failures that make it unshippable as-is, and they're architectural rather than fixable-in-place.

[BRICK] api/agent_runtime.py:299 — marker-absence is treated as "complete", so an interrupted update restarts into a half-applied checkout

_agent_update_transaction_state() returns "complete" when there is no live-active marker and no recovery marker, and _agent_restart_readiness() then treats any readable Git revision as "ready". But the Agent removes its .hermes-update-in-progress marker on every exit, including errors/interruptions — so marker-absence does not prove success. Reproduced: with a fresh live marker the restart correctly deferred; once the marker was removed, the same probe restarted with no success receipt — state became "complete", readiness "ready", and execution reached execv against a potentially half-applied checkout/venv.
Fix: require an Agent-owned terminal success receipt bound to the exact transaction, the final revision, and a healthy environment. Missing / malformed / stale / revision-mismatched receipts must fail closed and must not schedule a restart.

[CORE] api/updates.py:1780 — TOCTOU: an external Agent update can acquire its lock after the last readiness check but before execv

_apply_lock only serializes WebUI's own updates. An external Agent updater can acquire its lock after the final readiness callback and before os.execv() (line 1860), so WebUI can still restart while that updater owns and is mutating the checkout. Reproduced: a synchronized run created the Agent marker after the third readiness check; it existed when execv was called.
Fix: replace the polling with an Agent-owned atomic handoff/lease that excludes new mutations across the process replacement — or have the Agent updater perform the WebUI restart after completing its transaction.

Why this is a kick-back, not a nit

Both fixes require an authoritative, revision-bound Agent success receipt plus an atomic updater→WebUI restart handoff. WebUI cannot safely implement automatic-restart-on-revision-mismatch while remaining only a reader of the Agent-owned update lifecycle — the missing contract is on the Agent side. This is the same shape as #7079 (a correct WebUI change blocked on an Agent-core contract that isn't shipped). The tests don't catch it because test_agent_runtime_revision_guard.py:284 explicitly expects a dead/old marker → "complete" and the readiness tests mock "complete" without ever requiring a success receipt; the concurrent test only exercises repeated callbacks, not lock acquisition between the last check and execv.

Suggested path

Either (a) land only the safe subset — keep the improved diagnostics + the manual-409 path, but do NOT auto-schedule a restart on revision mismatch without an Agent success receipt (fail closed to the existing 409 until the Agent-side receipt/handoff contract exists); or (b) pair this with an Agent-core PR that writes a revision-bound terminal success receipt + provides the atomic handoff, and gate the two together. Happy to re-gate whichever way you take it. If a receipt/handoff contract already exists on the Agent side that I'm not seeing, point me at it and I'll re-verify against it.

@hrdwdmrbl hrdwdmrbl changed the title fix(runtime): restart after Agent update transaction fix(runtime): report unverified Agent updates without automatic restart Sep 8, 2026
Comment thread api/agent_runtime.py
Comment on lines +256 to +259
if python_path.parent.name.lower() in {"bin", "scripts"}:
venv_dir = python_path.parent.parent
if venv_dir.name.lower() in {"venv", ".venv"}:
candidates.append(venv_dir.parent)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Custom virtualenv markers missed

When HERMES_WEBUI_PYTHON points to a supported custom virtualenv whose directory is not named venv or .venv, this check omits the installation root. As a result, .update-incomplete and .lazy-refresh-incomplete are not observed, and an interrupted update is reported as unverified instead of the more actionable incomplete state. Derive the installation root without limiting explicitly configured interpreters to the two auto-discovery directory names.

Knowledge Base Used: Agent runtime and gateway

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

nesquena-hermes added a commit that referenced this pull request Sep 10, 2026
…ad (#7502)

* rebase #7160 onto master (report unverified Agent updates, no auto-restart)

Co-authored-by: hrdwdmrbl <hrdwdmrbl@users.noreply.github.com>

* harden agent-update marker read: no-follow, fstat regular-file, bounded (fixes gate CORE)

The Agent update marker is attacker-adjacent shared state. _read_live_agent_update()
used Path.read_text(), which follows symlinks, blocks forever on a FIFO, and reads an
unbounded regular file — a rejected/stale-runtime request could hang or OOM. Now opens
O_RDONLY|O_NONBLOCK|O_NOFOLLOW, fstat-verifies a small regular file, and reads a bounded
max, classifying anything else as 'unknown' (fail-closed). Adds FIFO/oversized/symlink/
happy-path regression tests.

Co-authored-by: hrdwdmrbl <hrdwdmrbl@users.noreply.github.com>

* fix marker-read portability: gate os.open fast-path on O_NONBLOCK+O_NOFOLLOW

os.O_NONBLOCK is Unix-only; accessing it unconditionally raised AttributeError
on native Windows, breaking every stale-revision barrier (chat/compression/etc).
Now both flags resolve via getattr and the os.open() fast path is taken only
when BOTH are available (_MARKER_SAFE_OPEN_AVAILABLE); otherwise fall back to an
lstat-only classification (absent vs unknown) so a fallback platform can never
symlink-traverse or crash. Adds a Windows-fallback regression test.

Co-authored-by: hrdwdmrbl <hrdwdmrbl@users.noreply.github.com>

* docs(changelog): stamp #7160 stale-runtime report + hardened marker read

---------

Co-authored-by: n <a@n>
Co-authored-by: hrdwdmrbl <hrdwdmrbl@users.noreply.github.com>
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in exp-v0.52.289 (experimental channel) — thanks @hrdwdmrbl! 🎉

Your de-scoped safe subset is live: a stale WebUI runtime after an Agent update is reported + rejected with the manual-restart 409, no unsafe self-restart into a half-applied checkout.

During the release gate, Codex reproduced two issues in the marker-read path that I fixed on the branch (preserving your attribution):

  1. _read_live_agent_update() used Path.read_text() → could hang forever on a FIFO marker and read unbounded files. Now opens O_RDONLY|O_NONBLOCK|O_NOFOLLOW, fstat-verifies a small regular file, and reads a bounded max (fail-closed unknown otherwise).
  2. That fix used os.O_NONBLOCK unconditionally (Unix-only → AttributeError on native Windows). Now both flags resolve via getattr and the os.open fast path is gated on both being available, with an lstat-only fallback.

Final gate: Codex SAFE TO SHIP, full suite 15,211 passed, 51/51 in the revision-guard module (incl. FIFO/oversized/symlink/Windows-fallback regression tests).

itsdarklikehell pushed a commit to itsdarklikehell/hermes-webui that referenced this pull request Sep 16, 2026
…arker read (nesquena#7502)

* rebase nesquena#7160 onto master (report unverified Agent updates, no auto-restart)

Co-authored-by: hrdwdmrbl <hrdwdmrbl@users.noreply.github.com>

* harden agent-update marker read: no-follow, fstat regular-file, bounded (fixes gate CORE)

The Agent update marker is attacker-adjacent shared state. _read_live_agent_update()
used Path.read_text(), which follows symlinks, blocks forever on a FIFO, and reads an
unbounded regular file — a rejected/stale-runtime request could hang or OOM. Now opens
O_RDONLY|O_NONBLOCK|O_NOFOLLOW, fstat-verifies a small regular file, and reads a bounded
max, classifying anything else as 'unknown' (fail-closed). Adds FIFO/oversized/symlink/
happy-path regression tests.

Co-authored-by: hrdwdmrbl <hrdwdmrbl@users.noreply.github.com>

* fix marker-read portability: gate os.open fast-path on O_NONBLOCK+O_NOFOLLOW

os.O_NONBLOCK is Unix-only; accessing it unconditionally raised AttributeError
on native Windows, breaking every stale-revision barrier (chat/compression/etc).
Now both flags resolve via getattr and the os.open() fast path is taken only
when BOTH are available (_MARKER_SAFE_OPEN_AVAILABLE); otherwise fall back to an
lstat-only classification (absent vs unknown) so a fallback platform can never
symlink-traverse or crash. Adds a Windows-fallback regression test.

Co-authored-by: hrdwdmrbl <hrdwdmrbl@users.noreply.github.com>

* docs(changelog): stamp nesquena#7160 stale-runtime report + hardened marker read

---------

Co-authored-by: n <a@n>
Co-authored-by: hrdwdmrbl <hrdwdmrbl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L Large PR (>10 files or >250 LOC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants