fix(#6617): run the official Agent update transaction against one proven install - #6686
fix(#6617): run the official Agent update transaction against one proven install#6686rodboev wants to merge 7 commits into
Conversation
🎬 Cutter preview — PR #6686 |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Thanks @rodboev — delegating normal in-app Agent updates to the official updater is the right direction. Static exact-head review found four cross-layer transaction/ownership gaps that need correction.
- Bind command and source to one Agent install.
_find_agent_executable()acceptsagent_dirbut can select a sibling/global Python or PATHhermes; later verification reads_AGENT_DIR. WebUI can therefore execute install B while verifying install A. Resolve the updater from the exact target install and prove command→source identity; reject unrelated PATH fallback. - Do not turn documented non-fatal updater warnings into transaction failure.
_agent_update_output_failure()gives broad warning strings precedence over zero exit + official completion. The installed updater explicitly calls desktop-build and backup warnings non-fatal/continuing. The Agent may already be updated/restarted while WebUI reports failure and skips its own refresh, leaving stale modules. Prefer a machine-readable receipt; otherwise preserve degraded warnings without overriding authoritative completion. - Force recovery must run the full repair transaction. The Agent force path hard-resets source, then restarts gateway/WebUI without dependency repair, migrations, managed-runtime/config/skill/cron safeguards, or import validation. After the reset, run the exact install’s official repair/update transaction before any restart/success, or fail closed with manual recovery instructions.
- Resolve gateway lifecycle ownership before side effects. Health/verification can target a configured remote gateway while
_ensure_gateway_restart_for_agent_update()still spawns a local profile restart. Use one effective target owner: local/co-located may restart locally; remote must use an owner-supported operation or return honest unsupported/manual handoff, never spawn a local CLI.
Add composed two-install A/B tests, completion-plus-nonfatal-warning tests, post-reset repair-failure tests, and config/env remote target tests with local restart helpers set to fail-on-call.
Threat scan was SUSPICIOUS, so this was strict static-only. Green mocked CI does not exercise these install/target ownership compositions.
179e019 to
f305171
Compare
|
Thanks for the detailed review. The four findings pointed to one ownership defect, the WebUI adapter independently resolved the Agent install, transaction result, repair state, and gateway owner. I rebuilt that boundary around the existing official Agent transaction.
The installed Agent already owns the normal transaction at this revision, so this PR consumes that entry point through a narrow WebUI adapter rather than inventing a second receipt protocol. The composed regressions cover two Agent installs, exit-zero warnings, failed post-update health, incomplete state, force routing, and every configured remote gateway source with local mutation helpers set to fail on call. The WebUI update target, response handoff delay, browser timeout, and existing WebUI lock behavior stay unchanged. |
|
| Filename | Overview |
|---|---|
| api/agent_update.py | Adds the installation-proof, controlled-environment, supervised-process, health-validation, and typed-result adapter for the official Agent update transaction. |
| api/updates.py | Routes all Agent update actions through the new adapter while retaining existing WebUI Git, channel, stash, and restart behavior. |
| api/gateway_chat.py | Exposes effective chat-target resolution so lifecycle ownership follows the actual browser chat destination rather than health endpoints. |
| static/ui.js | Handles transaction contention separately from Git locks and reloads only when the backend schedules a restart. |
| tests/test_agent_update_transaction.py | Adds focused coverage for installation identity, health validation, transaction outcomes, bounded output, timeouts, and process quiescence. |
Sequence Diagram
sequenceDiagram
participant UI as Browser UI
participant Updates as api/updates.py
participant Adapter as api/agent_update.py
participant Agent as Official Agent transaction
UI->>Updates: Apply/force/retry Agent update
Updates->>Adapter: apply_agent_update()
Adapter->>Adapter: Resolve lifecycle owner and prove installation
Adapter->>Agent: python -m hermes_cli.main update --yes
Agent-->>Adapter: Exit status and diagnostics
Adapter->>Adapter: Verify quiescence, markers, and critical health
Adapter-->>Updates: Typed transaction result
alt Reload eligible
Updates->>Updates: Invalidate cache and schedule restart
Updates-->>UI: "restart_scheduled=true"
else Contended, incomplete, failed, or unsupported
Updates-->>UI: Typed non-restart outcome
end
Reviews (4): Last reviewed commit: "fix(#6617): make process proof optional-..." | Re-trigger Greptile
nesquena-hermes
left a comment
There was a problem hiding this comment.
Static re-gate: two prior ownership gaps remain, plus incomplete-state and lock-recovery defects
Thanks for rebuilding this around the official Agent transaction. Exact-head static review confirms that two important prior findings are fixed: documented non-fatal warnings no longer override exit-zero completion, and Agent force recovery no longer performs a WebUI-owned source reset/restart shortcut.
The exact head still needs four bounded corrections before another gate. The threat scan remains SUSPICIOUS (eval(snippet) in the browser harness), so I did not execute the PR or its tests.
1. Prove the exact venv, not only the source root
api/agent_update.py:_candidate() selects the lexical root/venv/.../python path, and _identity() proves that hermes_cli and PROJECT_ROOT import from root. It does not prove sys.prefix, the venv's pyvenv.cfg, or dependency ownership. A broken/shimmed venv interpreter can run an ambient Python whose cwd imports source A while updater/dependency authority belongs elsewhere; _run() also inherits PYTHONPATH/PYTHONHOME.
Fix: include interpreter/prefix metadata in _PROBE, require the expected install-root venv, and launch with a controlled import environment. Add a real-venv negative test for prefix/shim mismatch rather than mocking _identity() and _run().
2. Use one effective gateway target before side effects
_gateway_owner() returns immediately if api.agent_health._remote_gateway_base_url() yields any URL, including loopback. It then never checks the actual chat target from _gateway_base_url(get_config()). Those resolvers have different precedence: health accepts GATEWAY_HEALTH_URL / HERMES_GATEWAY_HEALTH_URL / HERMES_API_URL, while chat ownership comes from HERMES_WEBUI_GATEWAY_BASE_URL or config. A local health URL plus a configured remote chat gateway is classified local and can reach the local updater/restart path.
Fix: use one shared effective target/owner resolver. Add the requested precedence-conflict regression: local health env plus remote chat env/config must fail before interpreter lookup and every local lifecycle helper.
3. Honor the full official incomplete/health contract
_marker() checks only .update-incomplete. The official updater separately writes and retains .lazy-refresh-incomplete when lazy repair/import probes are indeterminate. The adapter's _PROBE imports common dependencies but omits the updater's critical startup modules (hermes_cli.main, run_agent, model_tools, toolsets). An exit-zero transaction can therefore leave the lazy marker or a critical import broken while WebUI reports updated and schedules reload.
Fix: consume both official incomplete markers and the official critical-module health result (or one authoritative transaction receipt). Add exit-zero + lazy-marker and missing-critical-import tests.
4. Do not map the Agent transaction marker to Git index-lock recovery
The adapter maps .hermes-update-in-progress refusal to lock_conflict=True. The frontend interprets every such result as “Clear lock and retry,” but /api/updates/clear_lock only inventories and tells the operator to remove _AGENT_DIR/.git/index.lock. That is unrelated to a live official Agent transaction and can immediately retry the still-running operation.
Fix: return a distinct transaction_in_progress state. Show wait/poll/retry-later guidance for it; reserve the clear-lock UI for actual .git/index.lock errors. Add an API-to-UI composition test.
One lower-priority hardening item also remains: the 1800-second subprocess.run(..., capture_output=True) timeout owns only the direct child, not the updater's descendant tree, and bounds diagnostics only after capture finishes. Please add process-tree/quiescence handling while revising the adapter.
Current disposition: needs contributor rework. No tests were run because the enforced threat gate returned SUSPICIOUS; this verdict is from exact-head static code/contract tracing.
2f68eba to
d4b52e1
Compare
|
The rework addresses the five remaining issues at the existing Agent and gateway ownership boundaries.
The WebUI update target, Agent force routing through the official transaction, genuine Git lock recovery, manual gateway restart, sequential browser flow, and WebUI reload ownership stay unchanged. |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Deep re-gate: identity proof + non-atomic upstream contract — five blockers, one a BRICK
Thanks @rodboev — real progress since the last round: force recovery now uses the official transaction (no WebUI-owned source reset), normal lock release is in finally with stale-PID recovery after forced termination, exit-zero warnings stay successful, and the frontend touches only the existing banner. Those prior findings are verified closed.
But adversarial reproduction (real venv + a real detached-child transaction) surfaced five issues the focused suite can't see — the 23 transaction tests pass because their identity fixture forces pyvenv.cfg home == base_prefix and there's no orderly-success detached-child case.
1. [CORE] Real POSIX venvs are rejected as unsupported — agent_update.py:231
The identity proof requires pyvenv_config["home"] == base_prefix. Verified against a real venv: pyvenv.cfg home=/home/hermes/.local/bin while sys.base_prefix=/home/hermes/.local/share/uv/python/…. These are not required to be equal (they differ for uv-built and many distro venvs), so _resolve_target() returns unsupported before launch — ordinary Agent installs can't use in-app update at all.
Fix: remove the home == base_prefix equality check; keep the launcher / sys.prefix / source-root / dependency-origin checks. Add a real POSIX-venv regression test (don't force home==base_prefix in the fixture).
2. [CORE] Optional psutil absent → every completed update reported indeterminate — agent_update.py:371
_load_process_observer() returns None without psutil, making descendants_quiescent unconditionally false, so a completed update reports indeterminate after it already mutated the install. requirements.txt:10-12 makes psutil explicitly optional.
Fix: for an orderly parent exit, define completion from the exited parent + drained pipes; require descendant observation only for timeout cleanup.
3. [CORE] Successful detached gateway lifecycle child gets killed — agent_update.py:372
Verified: _run_transaction() spawns a successful detached child → the wrapper terminates it and returns quiescent=True. The official updater deliberately launches detached gateway restart watchers/replacements, so the production impact is a previously-running gateway left offline after update.
Fix: never terminate descendants after an orderly official transaction exit; restrict process-tree termination to timeout cleanup. Add a success-with-detached-child test.
4. [BRICK] Fatal dependency-stage failure leaves source advanced + venv partially rewritten, no rollback — agent_update.py:398
Verified against the current official Agent updater (0c5f195ee238…): it rolls source back only for its syntax guard, writes the incomplete marker after changing source, and exits on later dependency failure without restoring source, venv, or migrated state. This adapter captures before but only reports failure at line 411 — it cannot restore what upstream left half-written.
Fix (architectural): this cannot be safely patched WebUI-side. Require an official, capability-versioned atomic transaction that restores all fatal-stage mutations and returns a verified receipt. Until that exists, refuse the operation rather than implement another WebUI-owned reset path (consistent with the round-2 direction of not owning recovery).
5. [CORE] Local update disabled by an inert remote gateway URL — agent_update.py:161
_gateway_owner() rejects a remote target without checking webui_chat_backend_mode(), but api/gateway_chat.py:257-278 establishes that only explicit gateway mode transfers execution ownership. So a configured-but-inactive remote URL blocks local update even while chat is on the legacy in-process backend.
Fix: reject a remote target only when webui_gateway_chat_enabled(config) is true; add legacy-mode and gateway-mode ownership cases.
Findings 1, 2, 5 independently break supported installs; 3 downs a running gateway; 4 is a brick with no WebUI-side remedy. The honest path: #4 waits on an atomic upstream contract (capability-versioned receipt), and 1/2/3/5 are bounded fixes on this branch. All reproduced at head da…/0c5f195ee238 (upstream). Happy to re-gate once the identity proof accepts real POSIX venv semantics, success no longer requires psutil or kills detached children, gateway ownership respects the active backend, and the fatal-stage path either gets an atomic upstream receipt or refuses.



Thinking Path
sys.prefix,pyvenv.cfg, dependency origins, source root, official health validator, and controlled environment.What Changed
api/agent_update.py: proves the selected Agent venv and controlled environment, consumes both official incomplete markers and the Agent critical-module validator, invokeshermes_cli.main update --yes, bounds output, supervises the owned process tree, and maps transaction contention separately from Git locks.api/gateway_chat.py: exposes the existing effective chat-target resolver to the Agent lifecycle adapter.api/updates.py: keeps Agent apply, force, and retry routed through the official transaction while preserving WebUI-target Git/channel/stash/lock behavior and WebUI reload ownership.static/ui.js: givestransaction_in_progresswait-and-retry guidance and hides Git lock recovery controls for that state; genuine Git lock results retain the existing clear-lock flow.Why It Matters
The WebUI cannot report success for the wrong Agent installation, a remote chat target, an incomplete transaction, or a live Agent transaction that has not reached process quiescence. Non-fatal updater warnings remain diagnostics after a successful official transaction.
Verification
git diff --check, and the invariant enumeration passed.The full Python suite and live installation or supervisor replacement remain CI and deployment-owner coverage.
Risks / Follow-ups
Live dependency installation and supervisor-managed replacement still require a real installation run. Managed, unsupported, or remote-owner targets return an explicit handoff before local mutation. A durable public Agent transaction receipt remains an Agent-side follow-up; this adapter consumes the current official validator and transaction contract without adding a second updater.
Upstream
Closes #6617.
Screenshots
These images show the retained genuine Git lock-recovery controls. The typed Agent transaction-in-progress state is process-owned and is covered by the headless API-to-UI composition tests; it intentionally does not expose the Git lock action.
Model Used
GPT-5.6 via Codex CLI