[PR #5120] [MERGED] fix(setup): keep client and skill setup when the Hopper install fails #6774

Closed
opened 2026-11-01 21:42:56 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5120
Author: @bercedev
Created: 10/18/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @morluto

Base: main ← Head: fix/optional-provider-install-continues


📝 Commits (6)

  • 8ac00a4 fix(setup): keep client and skill setup when the Hopper install fails
  • 47f395e fix(setup): keep the Hopper failure when a client write or the skill install fails too
  • e5e15de fix(setup): keep client and skill setup when the Hopper install fails
  • 79d9c54 fix(setup): keep the Hopper failure when a client write or the skill install fails too
  • 763ac6f fix(setup): reconcile optional Hopper recovery with current main
  • fedcc79 chore: integrate concurrent setup branch refresh

📊 Changes

3 files changed (+100 additions, -54 deletions)

View changed files

📝 docs/installation.md (+6 -4)
📝 src/application/Setup.configuration.test.ts (+57 -14)
📝 src/application/Setup.ts (+37 -36)

📄 Description

Summary

Fixes #5113.

When the optional Hopper package install fails, rea setup now still applies the other approved changes (client registrations and the skill) and reports needs_human with the Hopper failure. Before, runSetup returned at the Hopper step, so a package conflict that only concerns Hopper cost every other change in the plan.

Problem and expected behavior

On Arch Linux and CachyOS, zlib is provided by zlib-ng-compat, and pacman refuses Hopper's versioned zlib dependency. Setup then stopped at the Hopper step: no MCP registration and no skill were written. I hit this myself on CachyOS on 2026-10-14 with rea-agents 6.5.0; nothing from the plan was applied until I reran setup without Hopper. The issue also shows that removing zlib-ng-compat isn't realistic there.

Expected (the second option in the issue): skip Hopper, report it as unavailable, apply the rest.

Change and scope

installHopperAction returns the install failure instead of a finished SetupResult. runSetup records it and carries on with the clients and skill the user approved, without a Hopper launcher in the registrations (the same registrations a plan without Hopper writes). The final result keeps the install's code and remediation, so needs_human and the reason are unchanged for callers; the generic "rerun with --install-hopper" hint is left out in that case, since a rerun would hit the same conflict. The inputs installHopperAction only needed to build the early result are removed.

This changes a tested contract: reports a failed Hopper installation without configuring clients becomes reports a failed Hopper installation and still configures the selected clients. The docs/installation.md paragraph on the package-manager failure says what setup does now and names Arch as well as CachyOS.

Not included: detecting the conflict before running pacman (the first option in the issue), and the Ghidra/IDA install lanes, which the issue left uninvestigated.

Follow the contribution scope. Unresolved scope, authority, side effects, or required verification block integration. Ordinary fix within existing boundaries: not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: runSetup in src/application/Setup.ts, at the Hopper install step.
  • Contribution scope fit and linked maintainer decision: ordinary setup fix, not applicable.
  • CLI and MCP/tool-catalog contract: no tool or catalog change; rea setup writes the approved client and skill changes after a failed Hopper install instead of none.
  • Provider, bridge, target-format, or platform compatibility: Hopper install failure on any platform; most visible on Arch and CachyOS.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable; only changes that were already planned and approved are applied.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: docs/installation.md paragraph updated.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: Setup.configuration.test.ts, the failed-install case now selects a client and the skill and checks both are applied, with no HOPPER_LAUNCHER_PATH and no installed_hopper, while status, code and remediation stay as before.
  • Base reproduction or other evidence: on main the updated test fails with expected +0 to be 1 (no client configured).
  • User-visible CLI/MCP output (if applicable): source-derived, not captured: the Hopper progress event is still failed, and the client and skill progress follows it instead of setup exiting.
  • Remaining proof gaps: not run end to end against a real pacman conflict; the fix was checked through FakeSetupHost.

Validation performed

In a Node 24.18.0 container with no network, after npm ci:

  • npx vitest run on Setup.configuration, Setup.planning, Setup.recovery, SetupInstallFailure and setupEnvironment tests: 71 passed, 2 failed. The 2 are in setupEnvironment.test.ts and fail the same way on main in the same container.
  • npx tsc --noEmit: no errors.
  • oxlint on the two changed source files: 0 errors, 3 warnings (max-lines, complexity, max-lines-per-function on Setup.ts), the same 3 as on main.
  • oxfmt --check on the changed files: clean.
  • npm run verify:module-boundaries: passed.
  • npm run docs:check: passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none; the result shape is unchanged.
  • Real Hopper/Ghidra, browser, or OS coverage: the original failure was observed on CachyOS (rea-agents 6.5.0); the fix was not run against real Hopper.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new writes; it applies writes the user already approved in the plan.

Merge follow-up at 763ac6fd5: integrated main e83e3cead. The recovery composition retains the failed Hopper installation details while using main's single eligibility decision for generic install advice. The existing mixed Hopper/client and Hopper/skill failure regressions remain covered; they now live in their own test group to satisfy lint size limits without weakening assertions. On Node 24.18.0, nine focused setup/CLI/conflict/rollback files passed 162 tests; the final configuration-file rerun passed 24 tests. check:ci:fast passed all five tasks and docs:check passed. No real pacman/Hopper installation or native Windows run was performed; hosted checks are pending.

Author readiness

Human owner: @bercedev

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

Note: I used Claude Code while working on this.


🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/5120 **Author:** [@bercedev](https://github.com/bercedev) **Created:** 10/18/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/optional-provider-install-continues` --- ### 📝 Commits (6) - [`8ac00a4`](https://github.com/morluto/rea/commit/8ac00a4aec32472c37fd278e9914f2c894679bc4) fix(setup): keep client and skill setup when the Hopper install fails - [`47f395e`](https://github.com/morluto/rea/commit/47f395ee4a894d39699305ca8d2b19dcad56c4e3) fix(setup): keep the Hopper failure when a client write or the skill install fails too - [`e5e15de`](https://github.com/morluto/rea/commit/e5e15de915df56b226545d3b66bdcae8cec28bb9) fix(setup): keep client and skill setup when the Hopper install fails - [`79d9c54`](https://github.com/morluto/rea/commit/79d9c54a88837ff44c80ce44e5ef3a7d56a0faaa) fix(setup): keep the Hopper failure when a client write or the skill install fails too - [`763ac6f`](https://github.com/morluto/rea/commit/763ac6fd5d539fa461413043da19451b4a2ba9f2) fix(setup): reconcile optional Hopper recovery with current main - [`fedcc79`](https://github.com/morluto/rea/commit/fedcc792382910a9406f4a51333881ee48747daa) chore: integrate concurrent setup branch refresh ### 📊 Changes **3 files changed** (+100 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `docs/installation.md` (+6 -4) 📝 `src/application/Setup.configuration.test.ts` (+57 -14) 📝 `src/application/Setup.ts` (+37 -36) </details> ### 📄 Description ## Summary Fixes #5113. When the optional Hopper package install fails, `rea setup` now still applies the other approved changes (client registrations and the skill) and reports `needs_human` with the Hopper failure. Before, `runSetup` returned at the Hopper step, so a package conflict that only concerns Hopper cost every other change in the plan. ## Problem and expected behavior On Arch Linux and CachyOS, `zlib` is provided by `zlib-ng-compat`, and pacman refuses Hopper's versioned `zlib` dependency. Setup then stopped at the Hopper step: no MCP registration and no skill were written. I hit this myself on CachyOS on 2026-10-14 with rea-agents 6.5.0; nothing from the plan was applied until I reran setup without Hopper. The issue also shows that removing `zlib-ng-compat` isn't realistic there. Expected (the second option in the issue): skip Hopper, report it as unavailable, apply the rest. ## Change and scope `installHopperAction` returns the install failure instead of a finished `SetupResult`. `runSetup` records it and carries on with the clients and skill the user approved, without a Hopper launcher in the registrations (the same registrations a plan without Hopper writes). The final result keeps the install's `code` and `remediation`, so `needs_human` and the reason are unchanged for callers; the generic "rerun with --install-hopper" hint is left out in that case, since a rerun would hit the same conflict. The inputs `installHopperAction` only needed to build the early result are removed. This changes a tested contract: `reports a failed Hopper installation without configuring clients` becomes `reports a failed Hopper installation and still configures the selected clients`. The `docs/installation.md` paragraph on the package-manager failure says what setup does now and names Arch as well as CachyOS. Not included: detecting the conflict before running pacman (the first option in the issue), and the Ghidra/IDA install lanes, which the issue left uninvestigated. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Ordinary fix within existing boundaries: not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `runSetup` in `src/application/Setup.ts`, at the Hopper install step. - Contribution scope fit and linked maintainer decision: ordinary setup fix, not applicable. - CLI and MCP/tool-catalog contract: no tool or catalog change; `rea setup` writes the approved client and skill changes after a failed Hopper install instead of none. - Provider, bridge, target-format, or platform compatibility: Hopper install failure on any platform; most visible on Arch and CachyOS. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable; only changes that were already planned and approved are applied. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: `docs/installation.md` paragraph updated. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [x] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `Setup.configuration.test.ts`, the failed-install case now selects a client and the skill and checks both are applied, with no `HOPPER_LAUNCHER_PATH` and no `installed_hopper`, while `status`, `code` and `remediation` stay as before. - Base reproduction or other evidence: on `main` the updated test fails with `expected +0 to be 1` (no client configured). - User-visible CLI/MCP output (if applicable): source-derived, not captured: the Hopper progress event is still `failed`, and the client and skill progress follows it instead of setup exiting. - Remaining proof gaps: not run end to end against a real pacman conflict; the fix was checked through `FakeSetupHost`. ## Validation performed In a Node 24.18.0 container with no network, after `npm ci`: - `npx vitest run` on `Setup.configuration`, `Setup.planning`, `Setup.recovery`, `SetupInstallFailure` and `setupEnvironment` tests: 71 passed, 2 failed. The 2 are in `setupEnvironment.test.ts` and fail the same way on `main` in the same container. - `npx tsc --noEmit`: no errors. - `oxlint` on the two changed source files: 0 errors, 3 warnings (`max-lines`, `complexity`, `max-lines-per-function` on `Setup.ts`), the same 3 as on `main`. - `oxfmt --check` on the changed files: clean. - `npm run verify:module-boundaries`: passed. - `npm run docs:check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none; the result shape is unchanged. - Real Hopper/Ghidra, browser, or OS coverage: the original failure was observed on CachyOS (rea-agents 6.5.0); the fix was not run against real Hopper. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new writes; it applies writes the user already approved in the plan. Merge follow-up at `763ac6fd5`: integrated main `e83e3cead`. The recovery composition retains the failed Hopper installation details while using main's single eligibility decision for generic install advice. The existing mixed Hopper/client and Hopper/skill failure regressions remain covered; they now live in their own test group to satisfy lint size limits without weakening assertions. On Node 24.18.0, nine focused setup/CLI/conflict/rollback files passed 162 tests; the final configuration-file rerun passed 24 tests. `check:ci:fast` passed all five tasks and `docs:check` passed. No real pacman/Hopper installation or native Windows run was performed; hosted checks are pending. ## Author readiness <!-- The human owner completes these declarations before requesting review. The PR author is the owner by default. A different owner must explicitly accept ownership on this PR. AI agents must leave these declarations to a human to dictate to them or there could be catastrophic consequences for many users of REA. Draft PRs may leave them unchecked. These declarations record initial readiness when opening the PR for review; later pushes do not require a new human sign-off. Keep the description and validation evidence current as the work changes. See CONTRIBUTING.md#pull-request-ownership-and-readiness. --> Human owner: @bercedev - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge <!-- Draft PRs may leave pending items unchecked. Resolve material scope and authority questions before integration. --> - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. Note: I used Claude Code while working on this. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:42:56 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#6774
No description provided.