morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #5508] [MERGED] fix(setup): preserve explicit clients with all-detected selection #6941

New issue

Closed

opened 2026-11-01 21:44:13 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:44:13 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5508
Author: @shoyann
Created: 10/20/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @morluto

Base: main ← Head: fix/setup-explicit-detected

📝 Commits (6)

f2cec38 fix(setup): preserve explicit clients with all-detected selection
1ae16cb Merge branch 'main' into fix/setup-explicit-detected
cd3136f chore: merge main to refresh CI baseline
bea49b1 test(filesystem): align staged cancellation cleanup assertions
5221c6e chore: sync main and resolve cancellation test conflict
1411d5e chore: sync main and preserve cleanup retry coverage

📊 Changes

6 files changed (+157 additions, -15 deletions)

View changed files

📝 docs/installation.md (+5 -3)
📝 src/application/Setup.planning.test.ts (+55 -0)
📝 src/application/Setup.ts (+4 -4)
📝 src/cli/setupCommands.ts (+1 -2)
📝 tests/acceptance/setup/repeatedClientSetup.test.ts (+78 -0)
📝 tests/boundary/filesystem/jsonOutputOwnership.test.ts (+14 -6)

📄 Description

Summary

Fixes #5443. Preserve explicitly selected clients when rea setup --client ... --all-detected also selects detected clients.

Problem and expected behavior

With only Codex detectable, rea setup --client cursor --all-detected --yes --json previously returned ready after configuring Codex alone. A fresh dry run also omitted Cursor. Both selections should be honored before reporting success.

Change and scope

Pass the explicit client IDs through the CLI boundary, then combine them with detected IDs at the existing application selection point. Keep first-selected order and deduplicate clients. The existing plan and final readiness scope consume that selection. Document the additive flag behavior.

Synced with main 7590b508, retaining the upstream fixes from #5601 and #5852. The filesystem test keeps both the cleanup-retry/error-projection assertions and the additional exact cancellation-cause, retained-content and resource assertions. It removes the injected file in finally even if an earlier assertion fails. No filesystem production code changes.

Contract and boundary impact

Semantic owner and earliest changed stage: CLI option forwarding and application setup selection.
Contribution scope fit and linked maintainer decision: ordinary fix within existing setup boundaries; a new capability scope decision is not applicable.
CLI and MCP/tool-catalog contract: existing setup flags now compose; no schema or MCP changes.
Provider, bridge, target-format, or platform compatibility: no provider/bridge changes; selection logic is shared across hosts.
Evidence, artifact, provenance, or reconstruction contract: unchanged.
Target selection, authority, upstream behavior, and cleanup: local client selection only; existing approval, backups, and cleanup remain in place. Explicit skill-client selection and the initial all-detected diagnostic scope retain their existing behavior.
Generated metadata, package, or installation impact: setup includes explicitly selected undetected clients. No generated files or release metadata changes.

Expected effects:

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.

The changed behavior concerns local client configuration; it introduces no target execution or new network access.

Evidence and regression coverage

Executed the initial new regressions against base c374c2fe: 3 failed, 28 passed. Both application selection cases and the real CLI plan omitted Cursor.
Application coverage checks explicit/readiness-scope selection, detected-only control, deduplication, and final client/skill scope.
The isolated-HOME CLI regression checks explicit-only and detected-only controls, mixed dry-run without writes, both configuration files, and final doctor scope. In the mixed case, Cursor comes only from explicit selection and Codex only from detection.
On the original fix commit f2cec38e, replayed the original command with default skill installation in a fresh isolated HOME on Ubuntu/WSL. Observed:

dry run: planned, configure_client:cursor, configure_client:codex, install_skill
apply:   ready, configured_cursor, configured_codex, installed_skill
         doctor.scope.clients = [codex, cursor], skill_clients = [codex, cursor]
repeat:  ready, plannedActions = [], appliedActions = []; config bytes unchanged

Remaining proof gaps: actual Cursor/Codex application loading and native macOS/Windows execution were not tested. No real analysis engine is involved in this selection fix.

Validation performed

Revision 1411d5e9 merges main 7590b508 and resolves the single filesystem test conflict. Executed on Ubuntu/WSL with Node 24.18.0 and npm 11.16.0:

npm run test:focused -- <paths below> — 124 passed across 12 files, including real CLI selection, repeated setup, skill selection, provider-readiness controls and staged-output cleanup.
npm run check — passed, including typecheck, lint, formatting, metadata and architecture/module guards.
npm run docs:check — passed, including 9 README consistency tests.
npm run verify:package — passed, including install/update, read-only setup planning, idempotent setup, backup readback, failure recovery, uninstall and MCP checks.
git diff --cached --check and the normal check:fast pre-push gate — passed.
Independent static review found no conflict-resolution or setup/CLI integration issue. It confirmed that both upstream cleanup-retry assertions and the additional cancellation/content checks remain.

The committed tree exactly matches the validated tree. Hosted CI must run for this new revision; previous green checks do not establish its result. Local platform and application-loading gaps are recorded above.

Focused test paths

tests/boundary/filesystem/jsonOutputOwnership.test.ts
src/application/Setup.planning.test.ts
src/application/Setup.configuration.test.ts
src/application/Setup.initialSnapshot.test.ts
src/application/Setup.recovery.test.ts
tests/acceptance/setup/repeatedClientSetup.test.ts
tests/acceptance/setup/selectedSkillSetup.test.ts
tests/boundary/cli/cliSetup.test.ts
tests/boundary/cli/filteredFailureOutput.test.ts
src/cliJsonPaths.test.ts
tests/acceptance/applications/electronInvalidPaths.test.ts
tests/boundary/cli/setupProviderReadiness.test.ts

Compatibility, safety, and release

No migration or release metadata changes. Combined flags now configure all requested clients, including undetected explicit choices; callers can inspect the existing dry-run plan before applying. Default selection, skill-only selection, approval and backup behavior remain unchanged. Validation scope and platform gaps are recorded above.

Author readiness

Human owner: @shoyann

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.

_{🔄 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/5508 **Author:** [@shoyann](https://github.com/shoyann) **Created:** 10/20/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/setup-explicit-detected` --- ### 📝 Commits (6) - [`f2cec38`](https://github.com/morluto/rea/commit/f2cec38ea5ac32a0edf7b8c31052d4c2871bba38) fix(setup): preserve explicit clients with all-detected selection - [`1ae16cb`](https://github.com/morluto/rea/commit/1ae16cbe69a4e1f61eaeed8b05f9c320a32ed11d) Merge branch 'main' into fix/setup-explicit-detected - [`cd3136f`](https://github.com/morluto/rea/commit/cd3136f5ee71d311f1f1f208370e58fb6b2da3d2) chore: merge main to refresh CI baseline - [`bea49b1`](https://github.com/morluto/rea/commit/bea49b1c01849b906e504bb463d102f44eff24b7) test(filesystem): align staged cancellation cleanup assertions - [`5221c6e`](https://github.com/morluto/rea/commit/5221c6e24a84f51a205df5fec8487f768441e06a) chore: sync main and resolve cancellation test conflict - [`1411d5e`](https://github.com/morluto/rea/commit/1411d5e912b44218bf5aab4408f091486a2ffbb1) chore: sync main and preserve cleanup retry coverage ### 📊 Changes **6 files changed** (+157 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `docs/installation.md` (+5 -3) 📝 `src/application/Setup.planning.test.ts` (+55 -0) 📝 `src/application/Setup.ts` (+4 -4) 📝 `src/cli/setupCommands.ts` (+1 -2) 📝 `tests/acceptance/setup/repeatedClientSetup.test.ts` (+78 -0) 📝 `tests/boundary/filesystem/jsonOutputOwnership.test.ts` (+14 -6) </details> ### 📄 Description ## Summary Fixes #5443. Preserve explicitly selected clients when `rea setup --client ... --all-detected` also selects detected clients. ## Problem and expected behavior With only Codex detectable, `rea setup --client cursor --all-detected --yes --json` previously returned `ready` after configuring Codex alone. A fresh dry run also omitted Cursor. Both selections should be honored before reporting success. ## Change and scope Pass the explicit client IDs through the CLI boundary, then combine them with detected IDs at the existing application selection point. Keep first-selected order and deduplicate clients. The existing plan and final readiness scope consume that selection. Document the additive flag behavior. Synced with main `7590b508`, retaining the upstream fixes from #5601 and #5852. The filesystem test keeps both the cleanup-retry/error-projection assertions and the additional exact cancellation-cause, retained-content and resource assertions. It removes the injected file in `finally` even if an earlier assertion fails. No filesystem production code changes. ## Contract and boundary impact - Semantic owner and earliest changed stage: CLI option forwarding and application setup selection. - Contribution scope fit and linked maintainer decision: ordinary fix within existing setup boundaries; a new capability scope decision is not applicable. - CLI and MCP/tool-catalog contract: existing setup flags now compose; no schema or MCP changes. - Provider, bridge, target-format, or platform compatibility: no provider/bridge changes; selection logic is shared across hosts. - Evidence, artifact, provenance, or reconstruction contract: unchanged. - Target selection, authority, upstream behavior, and cleanup: local client selection only; existing approval, backups, and cleanup remain in place. Explicit skill-client selection and the initial all-detected diagnostic scope retain their existing behavior. - Generated metadata, package, or installation impact: setup includes explicitly selected undetected clients. No generated files or release metadata changes. Expected effects: - [x] 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. The changed behavior concerns local client configuration; it introduces no target execution or new network access. ## Evidence and regression coverage - Executed the initial new regressions against base `c374c2fe`: 3 failed, 28 passed. Both application selection cases and the real CLI plan omitted Cursor. - Application coverage checks explicit/readiness-scope selection, detected-only control, deduplication, and final client/skill scope. - The isolated-HOME CLI regression checks explicit-only and detected-only controls, mixed dry-run without writes, both configuration files, and final doctor scope. In the mixed case, Cursor comes only from explicit selection and Codex only from detection. - On the original fix commit `f2cec38e`, replayed the original command with default skill installation in a fresh isolated HOME on Ubuntu/WSL. Observed: ```text dry run: planned, configure_client:cursor, configure_client:codex, install_skill apply: ready, configured_cursor, configured_codex, installed_skill doctor.scope.clients = [codex, cursor], skill_clients = [codex, cursor] repeat: ready, plannedActions = [], appliedActions = []; config bytes unchanged ``` Remaining proof gaps: actual Cursor/Codex application loading and native macOS/Windows execution were not tested. No real analysis engine is involved in this selection fix. ## Validation performed Revision `1411d5e9` merges main `7590b508` and resolves the single filesystem test conflict. Executed on Ubuntu/WSL with Node 24.18.0 and npm 11.16.0: - `npm run test:focused -- <paths below>` — 124 passed across 12 files, including real CLI selection, repeated setup, skill selection, provider-readiness controls and staged-output cleanup. - `npm run check` — passed, including typecheck, lint, formatting, metadata and architecture/module guards. - `npm run docs:check` — passed, including 9 README consistency tests. - `npm run verify:package` — passed, including install/update, read-only setup planning, idempotent setup, backup readback, failure recovery, uninstall and MCP checks. - `git diff --cached --check` and the normal `check:fast` pre-push gate — passed. - Independent static review found no conflict-resolution or setup/CLI integration issue. It confirmed that both upstream cleanup-retry assertions and the additional cancellation/content checks remain. The committed tree exactly matches the validated tree. Hosted CI must run for this new revision; previous green checks do not establish its result. Local platform and application-loading gaps are recorded above. <details> <summary>Focused test paths</summary> ```text tests/boundary/filesystem/jsonOutputOwnership.test.ts src/application/Setup.planning.test.ts src/application/Setup.configuration.test.ts src/application/Setup.initialSnapshot.test.ts src/application/Setup.recovery.test.ts tests/acceptance/setup/repeatedClientSetup.test.ts tests/acceptance/setup/selectedSkillSetup.test.ts tests/boundary/cli/cliSetup.test.ts tests/boundary/cli/filteredFailureOutput.test.ts src/cliJsonPaths.test.ts tests/acceptance/applications/electronInvalidPaths.test.ts tests/boundary/cli/setupProviderReadiness.test.ts ``` </details> ## Compatibility, safety, and release No migration or release metadata changes. Combined flags now configure all requested clients, including undetected explicit choices; callers can inspect the existing dry-run plan before applying. Default selection, skill-only selection, approval and backup behavior remain unchanged. Validation scope and platform gaps are recorded above. ## Author readiness Human owner: @shoyann - [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 - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:44:13 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:48:01 +00:00

[PR #6941] [MERGED] fix(test): reserve heap headroom for shared CLI workers #7460

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#6941

No description provided.

Rows
Columns