Skip to content

Don't eager-install camofox in bootstrap installers - #34732

Open
rp-keeran wants to merge 1 commit into
NousResearch:mainfrom
rp-keeran:fix/installers-no-eager-camofox
Open

rp-keeran wants to merge 1 commit into
NousResearch:mainfrom
rp-keeran:fix/installers-no-eager-camofox

Conversation

@rp-keeran

Copy link
Copy Markdown

install.sh / install.ps1 eagerly installed @askjo/camofox-browser, violating the lazy-install contract documented in test_package_json_lazy_deps (Camofox should be opt-in via the post_setup handler). Drops it from both installers and extends the lazy-deps test to cover the bootstrap scripts.

install.sh / install.ps1 eagerly installed @askjo/camofox-browser,
violating the lazy-install contract documented in
test_package_json_lazy_deps (Camofox should be opt-in via post_setup).
Drop it from both installers; extend the lazy-deps test to cover the
bootstrap scripts.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard tool/browser Browser automation (CDP, Playwright) labels May 29, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for identifying the bootstrap regression. The eager installs are still present on current origin/main at scripts/install.sh:2481 and scripts/install.ps1:371.

Problems

  • The deletion needs its lazy-install counterpart restored. hermes_cli/tools_config.py:1258-1274 checks for Camofox below checkout-local PROJECT_ROOT/node_modules, but its npm install --silent --workspaces=false command at :1265-1268 has no Camofox package argument. Current package.json:34-37 declares agent-browser only. After removing the installer arguments, selecting Camofox would not explicitly install @askjo/camofox-browser.

Suggested changes

  • Preserve the installer deletions, but make the post_setup_key == "camofox" branch explicitly install Camofox on demand and align its path check with that install location.
  • Keep the proposed bootstrap-script regression guard once that opt-in path is complete.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 13, 2026

@GottZ GottZ left a comment

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.

This was generated by AI during triage.

Summary

Two PRs address the eager Camofox installation path: #27055 removed Camofox from the root dependency graph and established opt-in installation, while #34732 extends that contract to the bootstrap installers but currently removes their install path without restoring a functional on-demand counterpart.

Related pull requests

  • #27055 [merged] related — (+110/-2642) — merged reference implementation: removed @askjo/camofox-browser and its transitive tree from the root package files, explicitly installed it from Camofox post-setup, exposed download progress, and added regression coverage for the lazy-install contract. It remains relevant because #34732 extends the contract established by this merged change to bootstrap installers.
  • #34732 related — (+16/-6) — keep open, needs changes: correctly removes the eager Camofox argument from both bootstrap installers and adds regression coverage, but does not restore the required lazy-install counterpart. As the contributor keep_open review notes, the current post-setup command has no Camofox package argument, so selecting Camofox would not install @askjo/camofox-browser after this deletion.

Suggested consolidation

Do not merge #34732 yet; preserve its installer deletions and regression guard, then explicitly install Camofox on demand in the post_setup_key == "camofox" path and align the existence check with that install location. #27055 is the merged reference implementation rather than a duplicate, so no PR should be closed as duplicate.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 107 kB of PR diffs, 3 kB of issue/PR text, 3 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/browser Browser automation (CDP, Playwright) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants