fix(install): add browser stage to $InstallStages so desktop Update actually installs/upgrades agent-browser - #67835
Conversation
alt-glitch
added
type/bug
505c195 to
ab649be
Compare
|
@alt-glitch — thanks for flagging the #58687 interplay. Reconciling it explicitly so reviewers see the analysis: The two PRs install/skip in opposite directions on purpose
Not contradictory — install when wanted, skip when not — but there's a real asymmetry left in this PR that I want to own up to: The gap I deliberately did NOT fold here
I considered folding the config-awareness into this PR and chose not to, for three reasons:
Recommended sequencing
On the
|
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the missing staged-install path. The premise is verified on current main: scripts/install.ps1:3517-3543 has no browser stage, and apps/desktop/electron/bootstrap-runner.ts:933-977 runs only manifest stages and stops on a failure.
Problems
scripts/install.ps1:3606callsInstall-AgentBrowserwithout handling its npm failure. That helper throws on a non-zero npm exit (scripts/install.ps1:374-379), and the desktop runner aborts bootstrap for a failed stage (bootstrap-runner.ts:973-976). This makes an optional browser install fatal during an update.- The new stage at
scripts/install.ps1:3532is unconditional. As noted in the PR discussion, it will install browser dependencies even when the user disabled that toolset; current runtime resolution appliesagent.disabled_toolsetsas a final override (hermes_cli/tools_config.py:1988-1996). Please reconcile that policy before adding the stage. tests/test_install_ps1_browser_stage.py:47onward reads installer source and asserts regex shapes rather than exercising stage behavior. It cannot cover the failure path above.
Suggested changes
- Convert npm-install failures to a skipped stage with
_StageSkippedReason, and test the emitted JSON result in an isolated Windows fixture with controlled npm behavior. - Add the configuration-aware guard or obtain an explicit maintainer decision for the intended divergence.
- Replace the source-text test suite with behavioral PowerShell coverage.
Automated hermes-sweeper review.
| # browser tools are a general Hermes capability, not a desktop-only feature. | ||
| # Soft-skip when Node is unavailable (browser tools degrade gracefully); | ||
| # see the Stage-Node note at L3548-3558 for the same pattern. | ||
| $InstallStages += @{ Name = "browser"; Title = "Installing agent-browser"; Category = "install"; NeedsUserInput = $false; Worker = "Stage-Browser" } |
There was a problem hiding this comment.
This stage is added to every Windows manifest without consulting persisted browser-tool configuration. hermes_cli/tools_config.py:1988-1996 treats agent.disabled_toolsets as a final user override, and the PR discussion confirms this update would reinstall agent-browser after a user disabled browser tools. Please add the configuration-aware skip here or resolve the intended policy before merging.
There was a problem hiding this comment.
Resolved in cc6c08e83 (amended from 4bb103263 — same logical content, rebased to incorporate the behavioral test rewrite): Stage-Browser now consults agent.disabled_toolsets from config.yaml before installing. A new Get-HermesConfigDisabledToolsets helper (scripts/install.ps1, after Write-BrowserEnv) reads $HermesHome\config.yaml and returns the suppressed toolset names as a hashtable. If browser is in that set, the worker sets $script:_StageSkippedReason and returns — surfacing skipped=true, ok=true in the JSON frame, matching Stage-Node's soft-skip pattern. This mirrors the runtime resolver's behavior (hermes_cli/tools_config.py:2146-2154 — agent.disabled_toolsets applied as a final user override that runs last).
The helper uses a small line-oriented state machine rather than a YAML parser — PowerShell has no built-in YAML module, and importing one would widen the install bootstrap's dependency footprint for a single scalar key. It handles both inline ([browser, memory]) and block-sequence (- browser) YAML forms. When config.yaml is absent (fresh install), the function returns an empty set and the stage proceeds — the guard only applies during Update runs where config.yaml already exists.
| $script:_StageSkippedReason = 'Node.js not available; agent-browser install skipped (browser tools will be unavailable)' | ||
| return | ||
| } | ||
| Install-AgentBrowser -SkipChromium:$SkipChromium |
There was a problem hiding this comment.
Install-AgentBrowser throws when npm exits non-zero, while runBootstrap aborts on any failed stage (apps/desktop/electron/bootstrap-runner.ts:973-976). Since browser tooling is optional, catch this failure and emit a skipped stage via _StageSkippedReason rather than failing the desktop update.
There was a problem hiding this comment.
Resolved in cc6c08e83 (amended from 4bb103263 — same logical content, rebased): Stage-Browser now wraps Install-AgentBrowser in a try/catch that converts npm failures to a soft-skip via $script:_StageSkippedReason, so the JSON frame emits skipped=true, ok=true instead of ok=false. The desktop bootstrap pipeline (bootstrap-runner.ts:973-976 aborts on any stage that re-throws) will no longer abort an Update when the optional browser install fails.
The catch block sets the reason to "agent-browser install failed: $_" so the failure is still surfaced in the JSON frame for diagnostics — just without aborting the entire install flow.
A targeted Python invariant (test_install_ps1_stage_browser_converts_npm_failure_to_skip) pins that the try/catch structure exists and sets _StageSkippedReason. The npm-failure path is also covered behaviorally by the Pester suite's npm-install-failure-to-skip case, which overrides Install-AgentBrowser to throw and asserts $_StageSkippedReason is set.
| INSTALL_PS1 = REPO_ROOT / "scripts" / "install.ps1" | ||
|
|
||
|
|
||
| def _install_ps1() -> str: |
There was a problem hiding this comment.
Please replace this source-text/regex suite with a behavioral PowerShell fixture that executes -Stage browser against an isolated home and controlled npm. These assertions pin implementation shape and do not verify the stage JSON contract or the npm-failure behavior.
There was a problem hiding this comment.
Resolved in cc6c08e83 (amended from 4bb103263 — same logical content, rebased to incorporate the behavioral test rewrite): The source-text/regex suite has been replaced with behavioral PowerShell coverage in scripts/tests/test-install-ps1-browser-stage.ps1. The behavioral suite uses a -Manifest dot-source approach: it dot-sources install.ps1 -Manifest (which loads all functions then returns control via exit 0 without running any stages), overrides Test-Node and Install-AgentBrowser in the same scope (last-definition-wins in PowerShell), then invokes Stage-Browser directly and inspects $script:_StageSkippedReason. This avoids all real side effects (no npm installs, no PATH writes, no child processes). 8 cases:
- Node missing →
skipped=truereason mentions Node - disabled_toolsets block form (
- browser) →skipped=true, reason mentions config.yaml - disabled_toolsets inline form (
[browser, memory]) → same - complex config (other agent/model keys before the list) → same
- browser NOT in disabled_toolsets → stage proceeds, no skip reason
- no config.yaml (fresh install) → stage proceeds, no skip reason
- npm install failure → override
Install-AgentBrowserto throw, assert$_StageSkippedReasonis set with the error - manifest shape → browser stage present in
$InstallStages,category=install, after node, before configure
The Python file (tests/test_install_ps1_browser_stage.py) is slimmed to 3 host-independent invariants that Linux CI can run without PowerShell:
browserstage name exists in$InstallStages(structural existence, not shape)- Stage-Browser contains a try/catch that converts npm failures to
_StageSkippedReason(the npm-failure path also covered behaviorally by the Pester suite, but this invariant is the CI-feasible guard for Linux runners without PowerShell) - install.ps1 stays pure ASCII (regression guard for [Setup]: Installation didn't finish error #66994 / [Bug]: Installer log #67000)
All three focus on behavior contracts / invariants, not implementation shape — no regex assertions on field ordering or variable name presence.
ab649be to
4bb1032
Compare
…nstalls agent-browser
Adds a `browser` stage to $InstallStages so the desktop Update flow
(bootstrap-runner.ts L779 - drives install.ps1 -Stage <name> per-stage)
installs agent-browser into the Hermes-bundled npm prefix
($HERMES_HOME\node) on Windows, closing the asymmetry with install.sh's
ensure_browser() on Linux/macOS.
Stage-Browser is a thin wrapper delegating to the existing
Install-AgentBrowser function (extend, don't duplicate). Three soft-skip
paths surface $script:_StageSkippedReason so the JSON frame emits
skipped=true / ok=true, never ok=false:
1. Node.js unavailable -- browser tools degrade gracefully.
2. agent.disabled_toolsets contains "browser" in config.yaml -- the
runtime toolset resolver (hermes_cli/tools_config.py:2146-2154)
applies that field as a final user override; honoring it here
prevents a desktop Update from reinstalling a toolset the user
explicitly turned off (per teknium1 review on NousResearch#67835).
3. Install-AgentBrowser throws (npm non-zero exit, network failure) --
browser tooling is optional, so a failed install is converted to a
skip rather than aborting the bootstrap pipeline (per teknium1
review on NousResearch#67835).
Testing:
- scripts/tests/test-install-ps1-browser-stage.ps1: behavioral Pester
suite invoking install.ps1 -Stage browser -Json in isolated child pwsh
processes with temp $HERMES_HOME and config.yaml fixtures. Asserts the
JSON result frame (skipped=true, ok=true, reason) for each
disabled_toolsets soft-skip path (block, inline, complex config),
browser-not-disabled proceeding, no-config (fresh install), and
manifest shape.
- tests/test_install_ps1_browser_stage.py: slimmed to host-independent
invariants (stage existence, ASCII purity, try/catch structure for
rpm-failure-to-skip) for Linux CI without PowerShell.
- scripts/tests/test-install-ps1-stage-protocol.ps1: existing smoke test
extended to assert browser appears in -Manifest output, after node,
before configure, with category=install / needs_user_input=false.
4bb1032 to
cc6c08e
Compare
Review feedback addressed — updated to
|
| Test | Scenario | Expected |
|---|---|---|
| 1 | Node unavailable | skip, reason mentions "Node.js not available" |
| 2 | disabled_toolsets block form | skip, reason mentions config.yaml |
| 3 | disabled_toolsets inline form | skip, reason mentions config.yaml |
| 4 | browser NOT in disabled_toolsets | proceeds (no skip reason) |
| 5 | No config.yaml (fresh install) | proceeds (no skip reason) |
| 6 | Install-AgentBrowser throws | skip, reason mentions "install failed" |
| 7 | Complex config (other keys) | skip, reason mentions config.yaml |
| 8 | Manifest shape | browser present, correct category/ordering |
The Python file (tests/test_install_ps1_browser_stage.py) retains 3 host-independent invariants for Linux CI: (1) stage-name existence (structural, not shape-pinning), (2) try/catch structure that converts npm failures to _StageSkippedReason (the npm-failure path is also covered behaviorally by Pester test 6, but this invariant is the CI-feasible guard for Linux runners without PowerShell), and (3) ASCII purity (regression guard for #66994/#67000).
Verification
- Python tests: 3 passed (pytest 9.1.1, Python 3.11.15)
- Pester behavioral suite: 8/8 passed (pwsh 7.4)
- Pester smoke test: all passed (pwsh 7.4 and Windows PowerShell 5.1 parity)
- Merge probe against
origin/main:git merge --no-commit --no-ff origin/mainauto-mergedscripts/install.ps1cleanly with zero conflict files - Platforms: verified on Windows 11. Not directly verified on Linux/macOS (the Python invariants are designed to be host-independent but this was not exercised on a Linux runner).
teknium1
added
the
sweeper:blast-moderate
3 participants
Summary
Sibling to PR #65701 (
fix(browser): browser tools unusable after Hermes restart — zombie daemon holds port). #65701 fixes a runtime symptom — a zombieagent-browserdaemon holding a TCP port after Hermes crashes/closes on Windows. This PR fixes the install-flow bug that explains why the Windows desktop population commonly lands onagent-browser@0.17.1(the version with the zombie bug) instead ofagent-browser@^0.26.0(where the idle-timeout was wired in upstream commit284e084bcc).Sibling, not duplicate. #65701 is the runtime mitigation in the agent-browser-aug handling code; this PR is the install-path wiring so the bundled
^0.26.0actually gets installed on Windows desktop flows.The exact problem
The auto-driven stage list
$InstallStages(scripts/install.ps1L3501–L3527) contains 13 stages:There is no
browser/agent-browserstage.The only function in install.ps1 that runs
npm install -g --prefix $HERMES_HOME\node "agent-browser@^0.26.0"isInstall-AgentBrowser(L355). Its sole caller in the dispatch logic isInvoke-EnsureMode's"browser"case (L3680–L3688), which is reachable only via:install.ps1 -PostInstall→Invoke-PostInstallMode(L3703–L3706), orinstall.ps1 -Ensure browser(L3728–L3734).Neither is in the auto-driven
$InstallStagessequence.The desktop Update button reads the stage manifest (
install.ps1 -Manifest) and iterates each stage viainstall.ps1 -Stage <name> -NonInteractive -Json(seeapps/desktop/electron/bootstrap-runner.tsL779). Because nobrowserstage is declared, the desktop Update flow never invokesInstall-AgentBrowser. On Windows, this means$HERMES_HOME\node\bin\agent-browser(the Hermes-bundled 0.26.0 the install code intends to populate) is NEVER installed or upgraded by the desktop-installed Hermes flow. Hermes falls through to whateveragent-browseris on the user's bare PATH — commonly a stale NVM/Node global install.Evidence trail
Confirmed by direct inspection of the install.ps1 source (line numbers above) plus a live repro on a Windows + NVM + Node 24 environment while validating #65701's fix:
rg -n "function Install-AgentBrowser" scripts/install.ps1→ L355 (definition, exactly one).rg -n "Install-AgentBrowser" scripts/install.ps1→ L355 (def) + L3683 (sole pre-existing callsite inInvoke-EnsureMode). No callsite inInvoke-AllStages/Get-InstallStage.rg -n "PostInstall|-Ensure" apps/desktop/electron/bootstrap-runner.ts→ no matches. The desktop updater iterates stages via-Stage <name>, never via-PostInstall/-Ensure.$HERMES_HOME\node\bin\agent-browserdoes not exist on disk when Hermes was installed/upgraded solely via the desktop flow, even though the Hermes process's PATH includes$HERMES_HOME\node\bin(the bundled prefixInstall-AgentBrowseris supposed to populate).agent-browserinstall (in NVM'snode_modules/agent-browser/package.json) reports version0.17.1— the zombie-prone version.The Linux/macOS equivalent —
scripts/install.sh'sensure_browser()— IS in the install flow viaensure_mode(L2550–L2600). The Windows gap is asymmetric. The asymmetry was introduced in #27224, when the stage-manifest API was added; install.sh'sensure_browserpredates that.The fix
Add a
browserstage to$InstallStages, placed afternode(the worker callsResolve-NpmCmdwhich throws ifnpmis missing) and afterdesktop(when-IncludeDesktopis enabled, so a freshly-builtHermes.exepicks up the freshly-installed agent-browser on its first relaunch instead of inheriting a stale bare-PATH binary). The stage also runs for non-desktop installs (irm | iex) — browser tools are a general Hermes capability, not a desktop-only feature.Stage-Browserworker behavior (what the reviewer should verify)^0.26.0is a near-no-op.npm install -g --prefixis idempotent against a satisfied version range; protocol version is NOT bumped (stages are additive per L3492).Test-Nodefails, the worker sets$script:_StageSkippedReasonand returns — the install flow MUST NOT abort (browser tools are optional).Stage-Nodeat L3548–L3558 (same$_StageSkippedReasonchannel);Invoke-StageL3609–L3616 surfaces it asskipped: true, ok: truein the JSON frame.test_install_ps1_stage_browser_soft_skips_on_no_node.-SkipChromiumforwarding. Worker forwards the flag toInstall-AgentBrowser. install.ps1 has no top-level-SkipChromiumparam today, so the forwarded value is$null(falsy) — identical toInstall-AgentBrowser's own[switch]$SkipChromiumdefaulting to$false.Install-AgentBrowser -SkipChromium:$SkipChromiumform (forwarding-shape); body at L356/L383–L399 re-checksFind-SystemBrowserinternally.test_install_ps1_stage_browser_forwards_skipchromium_flag.$IncludeDesktopplacement. When-IncludeDesktopis set,Stage-Desktopruns first thenStage-Browser; both run for non-desktop installs since browser tools are a general capability.desktopstage insertion (L3517); also added afternode-deps(L3510).'browser' appears after 'node'.Stop-Process/taskkill/Terminate/.agent-browserreferences.test_install_ps1_stage_browser_does_not_terminate_process_pool.Coordination with #58687 (triage flag)
Triage helpfully flagged an interplay with #58687 (
fix(update): honor configured bootstrap state). #58687 makes the Linux/macOS side skip the browser install when thebrowsertoolset is configured off (agent.disabled_toolsets/platform_toolsets.cliexcludesbrowser). My PR adds thebrowserstage to the Windows side so the desktop Update flow does run the browser install when needed.These are not contradictory in intent — install when wanted, skip when not — but there is a real gap to own up to:
Stage-Browserdoes NOT currently consultagent.disabled_toolsets/platform_toolsetsconfig before spawningnpm install. A Windows desktop user who ranhermes tools disable browserand then hits the desktop Update flow would still get the bundledagent-browser@^0.26.0install via$InstallStagesiteration — which is exactly the unconditional-install behavior #58687 was filed to stop on Linux.Why the fix is not folded into this PR
has_existing_hermes_config) does not exist on the PowerShell side yet and pulling it across means duplicating fix(update): honor configured bootstrap state #58687's import (hermes_cli.dep_ensure) logic into PowerShell — that's a much larger change than this PR's scope.Install-AgentBrowserentirely — stands regardless of thedisabled_toolsetsinterplay.disabled_toolsets-aware skip is the (unmerged) fix(update): honor configured bootstrap state #58687 design.Recommended sequencing
#67835): adds the Windowsbrowserstage so the install-flow gap stops. The stage's necessary skip-paths (no Node, no Chromium override) are in place; the config-awareness skip is not.disabled_toolsets-aware skip. The natural shape is a symmetricStage-Browserguard that reads$HERMES_HOME\config.yaml'sagent.disabled_toolsetsfield and soft-skips whenbrowseris excluded — reusing the$_StageSkippedReasonchannel this PR wires the no-Node case through. A follow-up authoring this is straightforward once fix(update): honor configured bootstrap state #58687 lands or a maintainer indicates preference for folding it in.Existing-manual-install note (worth flagging for review)
A user with a manually-installed agent-browser 0.17.1 in their NVM continues to have that v0.17.1 in NVM. After this PR lands, Hermes-spawned subprocesses get 0.26.0 from the bundled prefix (
$HERMES_HOME\node\bin, prepended to PATH). The user's OLD 0.17.1 stays installed in NVM but is no longer spawned by Hermes — the bundled-prefix prepend order wins. This is the same ordering install.sh relies on for POSIX. The user is not silently upgraded on their bare PATH; the bundled-install wins via PATH precedence, which is the install.ps1 design intent (the prefix override is what makes^0.26.0the Hermes-curated version rather than whatever happens to be on PATH).Tests
Python source-level —
tests/test_install_ps1_browser_stage.py(new, 11 tests)Source-level by design: install.ps1 is Windows-only PowerShell; Linux CI cannot execute it. The existing
tests/test_install_ps1_*.pyfamily already pins install.ps1 contracts via source-text parsing (test_install_ps1_node_path_for_npm.py,test_install_ps1_ascii_only.py, etc). Following the same convention lets Linux CI verify the structural contract without running PowerShell.Covers:
browserstage is declared in$InstallStageswith the right shape (Name/Title/Category/NeedsUserInput/Worker).NeedsUserInput = $false(matches the manifest driver's contract — stages are driven with-NonInteractiveperbootstrap-runner.tsL779).browserappears afternodeANDnode-deps, and beforeconfigure(interactive group runs last).Stage-Browserworker is defined and delegates toInstall-AgentBrowser(extend, don't duplicate — assertsInstall-AgentBrowseris still defined exactly once and thenpm install -g --prefixcall still appears exactly once in the file).Test-Nodereturns false ($script:_StageSkippedReasonset,returnnotthrow).-SkipChromiumtoInstall-AgentBrowser(the future-proofing shape noted above).tests/test_install_ps1_ascii_only.pyinvariant for the lines added by this PR specifically).Pester smoke —
scripts/tests/test-install-ps1-stage-protocol.ps1(extended, +25 lines)Runtime-side cross-check on Windows where
install.ps1 -Manifestactually executes. New assertions:manifest contains stage 'browser''browser' stage appears after 'node' stage'browser' stage appears before 'configure' stage'browser' stage declares needs_user_input=false'browser' stage category is 'install'Test plan
pwsh):scripts/tests/test-install-ps1-stage-protocol.ps1— all 5 new assertions PASS, all pre-existing smoke assertions PASS (29 OK / 0 FAIL).powershell.exe); source-level Python tests use only stdlib regex +Path.read_bytes()so no host-specific behavior is expected, but is not asserted here.What this PR deliberately does NOT touch
Install-AgentBrowserbody (L355–L415) unchanged — extend, don't duplicate.Invoke-EnsureModecase"browser"(L3680–L3688) unchanged — the-PostInstall/-Ensure browserpaths continue to work as before.Invoke-PostInstallMode(L3703–L3706) unchanged — already fine.agent.disabled_toolsetsconfig-awareness — see "Coordination with fix(update): honor configured bootstrap state #58687" above; intended for the fix(update): honor configured bootstrap state #58687 follow-up, not bundled here.Cross-link / sibling PR
fix(browser): browser tools unusable after Hermes restart — zombie daemon holds portfix(update): honor configured bootstrap state(POSIX-sidedisabled_toolsets-aware skip — see Coordination section)ensure_browserpredates it)284e084bcc(upstream idle-timeout that 0.26.0 wires — not delivered to Windows desktop users until this PR)A top-level cross-link comment has been posted on #65701 so readers there can find this sibling.