fix(update): honor configured bootstrap state - #58687
konsisumer wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting unnecessary bootstrap work; the current main paths do still perform browser setup and Node/UI refresh without consulting tool configuration.
Problems
hermes_cli/main.py:9928uses the browser toolset setting to skip_update_node_dependencies()and_build_web_ui(). Those are not browser-only: current main installs theui-tuiandwebworkspaces athermes_cli/main.py:8233-8239, and_build_web_ui()is the dashboard build path athermes_cli/main.py:4871-4955. A disabled browser toolset must not suppress enabled TUI/dashboard updates.hermes_cli/dep_ensure.py:138applies the setting to everyensure_dependency("browser")caller. ACP explicitly requests that dependency inacp_adapter/entry.py:206; with this change, an explicit browser setup fails when the CLI browser toolset is disabled.
Suggested changes
- Scope the opt-out to automatic browser-only bootstrap, preserving shared Node/UI refresh and explicit setup commands. Add regressions for disabled-browser updates that still refresh TUI/dashboard paths and for ACP explicit browser setup.
Automated hermes-sweeper review.
| @@ -9906,8 +9925,11 @@ def _cmd_update_impl(args, gateway_mode: bool): | |||
|
|
|||
| _refresh_active_lazy_features() | |||
|
|
|||
| _update_node_dependencies() | |||
| _build_web_ui(PROJECT_ROOT / "web") | |||
| if _browser_toolset_enabled_for_update(): | |||
There was a problem hiding this comment.
Blocking: _update_node_dependencies() is shared TUI/web maintenance, not browser-only—current main installs ui-tui and web workspaces (hermes_cli/main.py:8233-8239), and the next call builds the dashboard. Disabling browser must not skip those independent update paths; isolate only browser-specific bootstrap work.
| @@ -112,6 +135,10 @@ def ensure_dependency( | |||
| if check is None: | |||
| # Unknown dep — don't silently forward to install script. | |||
| return False | |||
| if dep == "browser" and _browser_toolset_disabled_by_config(): | |||
There was a problem hiding this comment.
Blocking: this generic gate also affects explicit callers. ACP's requested browser bootstrap calls ensure_dependency("browser") at acp_adapter/entry.py:206, so --setup-browser would fail merely because the CLI browser toolset is disabled. Scope the check to automatic/lazy browser setup or pass caller intent.
d03baf1 to
d0fe27e
Compare
|
Thanks for the review. Addressed in this push:
Validated with the focused CLI tests, the full |
|
Rebased onto current main and resolved the scoped installer-test conflict. Updated the Node-refresh regression for the extracted update module and corrected the reported Windows encoding diagnostics in the existing scoped test files. Verified: the three exact regressions pass (3 passed); Ruff, The full |
d0fe27e to
74fa839
Compare
|
Rebased onto current main and resolved the scoped dependency-bootstrap test conflict, preserving upstream's platform-specific test behavior and the ACP explicit-browser setup regression. Verified: the three affected regressions pass (3 passed); Ruff, |
0e00e55 to
063898c
Compare
|
Rebased onto current Verified: 3 focused regressions pass; Ruff, |
|
Rebased onto current Verified: 3 focused regressions pass; Ruff, Windows-footguns, |
063898c to
0b9bcc5
Compare
|
Rebased onto current main and resolved the test verification unavailable: the recorded local test command exited 3 |
0b9bcc5 to
fb4b29b
Compare
|
Rebased cleanly onto current main. The PR remains limited to its original install/update scope (with the upstream-renamed install test). Three focused in-scope regression tests, ruff, and the Windows-footgun scan pass. The full local suite could not collect because this sandbox lacks fastapi/uvicorn and its lazy dependency install cannot write the uv cache. test verification unavailable: the recorded local test command exited 3 |
fb4b29b to
7a03e64
Compare
|
Closing — on reflection, |
What does this PR do?
Update/bootstrap orchestration now respects existing configuration before running heavyweight browser dependency installs or interactive setup stages.
Related Issue
Type of Change
Changes Made
hermes_cli/dep_ensure.py: skipinstall.sh --ensure browserwhen the browser toolset is disabled viaagent.disabled_toolsetsor primary CLI toolset config.hermes_cli/main.py: skiphermes updateNode/browser/web refresh when the primary browser toolset is disabled, while preserving the existing Termux Python dependency path.scripts/install.sh: make staged bootstrap manifests incremental by omitting setup/gateway stages when an existing config and credential file are present; non-interactive skipped stage JSON now reports the skip reason.install.sh --manifest --include-desktopbehavior.Shared root cause
How this fixes each issue
hermes tools disable browsershould also skip browser dependency installation on gateway startup/update #37231:hermes tools disable browsernow prevents lazy browser dependency ensure from spawning the browser installer, andhermes updateskips Node/browser refresh when the browser toolset is disabled.install.sh --include-desktopmanifests omit setup/gateway stages for already configured installs, so desktop/bootstrap drivers do not rerun interactive setup work just to add the desktop build.Supersedes
N/A - no member PRs were provided for this spanning group.
How to Test
/opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/hermes_cli/test_dep_ensure.py tests/hermes_cli/test_cmd_update.py tests/test_install_sh_browser_install.py -q --timeout=60'{ git diff --name-only --diff-filter=d origin/main; git ls-files --others --exclude-standard; } | grep -E '\.pyi?$' | sort -u | xargs ruff checkbash -n scripts/install.sh && git diff --check/opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/ -q -x --timeout=60 "$@"' shwas attempted; collection aborted before changed tests because this local Python environment lacksfastapi, and lazy install failed under Homebrew's PEP 668 externally-managed environment.What platforms tested on
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (attempted; blocked during collection by missingfastapi/PEP 668 local environment)Documentation & Housekeeping
cli-config.yaml.exampleupdate N/ACONTRIBUTING.md/AGENTS.mdupdate N/AScreenshots / Logs
Focused regression tests: 60 passed, 12 pre-existing warnings.
This coordinated PR bundles a fix that spans several issues. Happy to split it back into focused per-issue PRs if you'd prefer to review them separately.
Refs #37231
Refs #37874
Refs #39411