morluto/rea

Fork 0

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

[PR #4977] [CLOSED] fix(setup): remove every repeated REA-owned JSON member #6706

New issue

Closed

opened 2026-11-01 21:42:26 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:42:26 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4977
Author: @NikeGunn
Created: 10/18/2026
Status: ❌ Closed

Base: main ← Head: fix/uninstall-repeated-owned-json-members

📝 Commits (4)

5e25597 fix(setup): remove every repeated REA-owned JSON member
722af95 fix(setup): remove repeated JSON members from one syntax tree
939b31b fix(setup): delete repeated JSON members in one pass
4cd22d4 fix(setup): keep OpenCode registrations whose V1 fallback REA does not own

📊 Changes

6 files changed (+674 additions, -23 deletions)

View changed files

📝 docs/installation.md (+8 -1)
➕ src/application/ClientConfigurationDocument.duplicateMembers.test.ts (+251 -0)
📝 src/application/ClientConfigurationDocument.ts (+184 -3)
📝 src/application/SetupClientConfiguration.ts (+16 -8)
📝 src/application/Uninstall.ts (+60 -10)
📝 tests/acceptance/setup/duplicateJsonMemberSetup.test.ts (+155 -1)

📄 Description

Summary

Fixes #4976. Follow-up to #4957 (#4952).

#4957 made setup and uninstall edit the repeated JSON member that clients read. For the case where removal could activate an earlier repeat, it made the serializer refuse every repeated deletion, because the parsed document keeps only the last member and the ownership rule cannot see the others. Three cases from #4952 still fail on main:

Uninstall of repeated REA-owned rea members exits 1, although nothing foreign would become active. #4952 lists this as one of its four target failures.
That refusal happens while one client is being removed, after preflight. A client earlier in iteration order may already be changed, which leaves a partial uninstall.
OpenCode setup replaces a single V1 mcp.rea beside a native V2 table, but refuses when that member is repeated.
Found in review (Codex), and already reachable on main with one native entry: uninstall removes an owned OpenCode mcp.servers.rea even when the V1 mcp.rea that OpenCode falls back to is foreign. It reports removed while activating that launcher.

Problem and expected behavior

Smallest triggers on b83b9caf (isolated HOME; OWNED = npx -y rea-agents@6.6.0 mcp, classified only):

Input	main	this PR
`{"mcpServers": {"rea": OWNED, "peer": …, "rea": OWNED}}`, `rea uninstall`	exit 1, unchanged	exit 0, `removed`, only `peer` remains
`~/.claude.json` with OWNED, plus `~/.cursor/mcp.json` with `{"rea": FOREIGN, "rea": OWNED}`, `rea uninstall`	exit 1, `claude_code: removed`, `cursor: failed`	exit 1, `cursor: failed`, claude_code unchanged, no backups
OpenCode `{"mcp": {"servers": {…}, "rea": FOREIGN, "rea": FOREIGN}}`, `rea setup --client opencode`	exit 1 `failed/shadowed_member`	exit 0 `configured`, same as one V1 member
OpenCode `{"mcp": {"servers": {"rea": OWNED}, "rea": FOREIGN}}`, `rea uninstall`	exit 0 `removed`, OpenCode now loads FOREIGN	exit 1 before any change, no backup

Invariant: uninstall removes the registration the client reads only when every repeat it would reveal is REA-owned. It decides that in the ownership rule that preflight shares, so a refusal changes nothing.

Change and scope

ClientConfigurationDocument owns a new fact, shadowedClientServers(text, parsed, name): the earlier members of one name in the table a client reads, plus OpenCode V1 repeats beside a V2 table. It reuses #4957's last-member locator (effectiveJsonProperty). It builds a syntax tree only when the name's JSON string token occurs more than once. The pattern matches literal, \uXXXX (either hex case), and short-escape spellings, so it never misses a member, and parsing, setup and doctor keep their cost (measurements below).
serializeClientConfiguration keeps #4957's fail-closed shadowed_member default. It accepts { removeShadowedMembers: true } to delete every repeat at a deleted path in one pass over the text, using the syntax tree it has already parsed.
Uninstall's clientRegistrationRemoval (shared by preflight and removal) admits a removal only when REA owns the effective entry and every repeat. When an earlier repeat is not REA-owned, or OpenCode would fall back to a V1 member REA does not own, it reports failed in preflight with a specific reason, so no client, skill or backup changes. Removal then grants the serializer authority.
Setup grants the same authority for its only deletion: REA's OpenCode V1 name, which the native V2 entry supersedes whatever its launcher, as setup already does for a single member.
docs/installation.md documents the uninstall rule.
Not changed: repeats inside an inactive earlier copy of a server table, TOML/YAML/DSH (no repeated members), and the shadowed_member setup result and contract, which stays reachable for edits made without authority.

Contract and boundary impact

Semantic owner and earliest changed stage: ClientConfigurationDocument (repeated-member facts and JSON edit authority), then the uninstall ownership decision.
Contribution scope fit and linked maintainer decision: not applicable. This is a bug fix inside existing setup/uninstall authority, and it removes only REA-owned members or REA's superseded V1 name.
CLI and MCP/tool-catalog contract: uninstall now succeeds for owned repeats. The unowned-repeat refusal moves to preflight; its detail still contains "would activate an earlier duplicate" and now also says the repeat "is not REA-owned". OpenCode setup with repeated V1 members returns configured. Tool catalog unchanged.
Provider, bridge, target-format, or platform compatibility: JSON/JSONC client formats only.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details: foreign or unresolved repeats are never deleted or activated. A refused uninstall writes nothing, not even a backup.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.
Change process memory, registers, files, or configuration.

Evidence and regression coverage

Tests added or updated:
- tests/acceptance/setup/duplicateJsonMemberSetup.test.ts (public CLI, extends #4957's suite): owned repeats including an escaped "rea" spelling are removed and the backup equals the original; repeated owned OpenCode V2 and V1 members are removed and comments are kept; an unowned V1 repeat makes uninstall stop before Claude Code (iterated first) changes, with no backups; OpenCode setup replaces repeated V1 entries; uninstall keeps one or repeated native entries when the V1 fallback is foreign.
- src/application/ClientConfigurationDocument.duplicateMembers.test.ts: enumerates every layout of one to three repeated mcpServers tables built from seven server-table shapes (3,192 documents with LF/CRLF and BOM variants, for claude_code and JSONC) and checks the edited bytes with JSON.parse as an independent reader. It covers setup sets, the default refusal, removal with authority, and inactive tables kept byte for byte. A scale case removes 20,000 repeats interleaved with peers and comments in one pass. It also cross-checks that shadowedClientServers (prefiltered) and the serializer's full-tree refusal agree on every layout, and that every JSON escape spelling is found.
- #4957's five tests are unchanged and pass.
Base reproduction: on b83b9caf the six new acceptance tests and 14 of the 17 new module tests fail. The other three enumerate the layouts and check setup, which #4957 already fixed. All pass on this branch.
Worst-case CLI harness (23 scenarios with the real built CLI: setup/doctor/repeat/uninstall, escaped names, three repeated tables, BOM+CRLF, JSONC comments and trailing commas, nested repeats, owned and foreign shadowing, OpenCode V2/V1 fallback): b83b9caf 19/25, this branch 25/25. Before #4957, 2c4338db met 6 of the first 23 scenarios. Every setup case also checks that a second setup plans nothing and doctor reports aligned.
Performance (8 MiB .claude.json with 4,000 projects, WSL2 on a Ryzen 5 4600H): an earlier draft of this change built the tree on every parse and made it 13× slower (25 → 330 ms per parse). It was reworked to build on demand and only behind the prefilter. Results: parseClientConfiguration 33.4 ms main vs 34.6 ms branch; end-to-end rea uninstall median 3,453 ms main vs 3,353 ms branch (5 runs each, within noise). Removing repeats (review by Codex and Greptile): 2,000 repeats took 9,623 ms in the first commit, which re-parsed once per repeat. 20,000 repeats took 35,804 ms in 722af95b, which copied the text once per repeat, and take 455 ms in 939b31bb (one tree, one pass).
User-visible CLI/MCP output (refused uninstall):
Configuration repeats the "rea" member, and an earlier duplicate is not REA-owned; removing the registration the client reads would activate an earlier duplicate, so no change was made. Remove the duplicate members manually, then rerun uninstall.
Remaining proof gaps: no native client was launched for this change (#4952 holds the native last-member observation). macOS and Windows were not run. On this host two existing acceptance tests (dshClientSetup "previews, approves…" and unqualifiedDoctorSkill) exceed the 30 s timeout on both main and this branch, and both pass with --testTimeout 180000. Neither path reaches the new code.

Validation performed

All runs were on Linux x64 (WSL2 Ubuntu), Node 24.18.0, npm 11.16.0:

npm run test:focused -- 84 setup, uninstall, doctor and client-configuration test files: 1,003 passed, 8 skipped. The two timeouts above failed at 30 s; npx vitest run --project acceptance --testTimeout 180000 on those two files passed 10/10.
npm run test:focused -- src/application/ClientConfigurationDocument.duplicateMembers.test.ts tests/acceptance/setup/duplicateJsonMemberSetup.test.ts tests/boundary/filesystem/clientConfigurationEnvironment.test.ts src/application/ClientConfigurationDocument.capacity.test.ts: 52 passed
The same new tests on b83b9caf sources: 20 failed, 8 passed (expected: the 8 are #4957's five acceptance tests and the three module tests for setup and layout enumeration)
npm run check:fast: passed
npm run docs:check: passed
npm run verify:test-discovery: all 1,580 test files discovered once
npx oxfmt --check on changed files: clean
Lint: 0 errors. Two warnings of rules these files already exceed elsewhere: max-params (the new serializer options argument) and complexity 21 in removeClient.

Compatibility, safety, and release

Breaking changes or migration steps: none. Configurations without repeated members are edited exactly as before, and the serializer's default is unchanged.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; Linux only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: removal widens only to members REA owns, or to REA's superseded V1 name, which setup already replaces. The unowned case now stops earlier, before any client changes.

Author readiness

Human owner: @NikeGunn

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.

🤖 Generated with Claude Code

_{🔄 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/4977 **Author:** [@NikeGunn](https://github.com/NikeGunn) **Created:** 10/18/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/uninstall-repeated-owned-json-members` --- ### 📝 Commits (4) - [`5e25597`](https://github.com/morluto/rea/commit/5e255979f852117e088ba9b7f60cd3b0f84ff01f) fix(setup): remove every repeated REA-owned JSON member - [`722af95`](https://github.com/morluto/rea/commit/722af95b75a1b538629bf92d6b2f9381e759fcc4) fix(setup): remove repeated JSON members from one syntax tree - [`939b31b`](https://github.com/morluto/rea/commit/939b31bbbd83cfe79279f3cf288d3c61cd529a20) fix(setup): delete repeated JSON members in one pass - [`4cd22d4`](https://github.com/morluto/rea/commit/4cd22d4ec2f6ec9e6c8ebf98bf97e1d9d7f8ac23) fix(setup): keep OpenCode registrations whose V1 fallback REA does not own ### 📊 Changes **6 files changed** (+674 additions, -23 deletions) <details> <summary>View changed files</summary> 📝 `docs/installation.md` (+8 -1) ➕ `src/application/ClientConfigurationDocument.duplicateMembers.test.ts` (+251 -0) 📝 `src/application/ClientConfigurationDocument.ts` (+184 -3) 📝 `src/application/SetupClientConfiguration.ts` (+16 -8) 📝 `src/application/Uninstall.ts` (+60 -10) 📝 `tests/acceptance/setup/duplicateJsonMemberSetup.test.ts` (+155 -1) </details> ### 📄 Description ## Summary Fixes #4976. Follow-up to #4957 (#4952). #4957 made setup and uninstall edit the repeated JSON member that clients read. For the case where removal could activate an earlier repeat, it made the serializer refuse every repeated deletion, because the parsed document keeps only the last member and the ownership rule cannot see the others. Three cases from #4952 still fail on main: 1. Uninstall of **repeated REA-owned** `rea` members exits 1, although nothing foreign would become active. #4952 lists this as one of its four target failures. 2. That refusal happens while one client is being removed, **after preflight**. A client earlier in iteration order may already be changed, which leaves a partial uninstall. 3. OpenCode setup replaces a single V1 `mcp.rea` beside a native V2 table, but **refuses when that member is repeated**. 4. Found in review (Codex), and **already reachable on main with one native entry**: uninstall removes an owned OpenCode `mcp.servers.rea` even when the V1 `mcp.rea` that OpenCode falls back to is foreign. It reports `removed` while activating that launcher. ## Problem and expected behavior Smallest triggers on `b83b9caf` (isolated `HOME`; `OWNED` = `npx -y rea-agents@6.6.0 mcp`, classified only): | Input | main | this PR | | --- | --- | --- | | `{"mcpServers": {"rea": OWNED, "peer": …, "rea": OWNED}}`, `rea uninstall` | exit 1, unchanged | exit 0, `removed`, only `peer` remains | | `~/.claude.json` with OWNED, plus `~/.cursor/mcp.json` with `{"rea": FOREIGN, "rea": OWNED}`, `rea uninstall` | exit 1, **`claude_code: removed`**, `cursor: failed` | exit 1, `cursor: failed`, **claude_code unchanged, no backups** | | OpenCode `{"mcp": {"servers": {…}, "rea": FOREIGN, "rea": FOREIGN}}`, `rea setup --client opencode` | exit 1 `failed/shadowed_member` | exit 0 `configured`, same as one V1 member | | OpenCode `{"mcp": {"servers": {"rea": OWNED}, "rea": FOREIGN}}`, `rea uninstall` | exit 0 `removed`, **OpenCode now loads FOREIGN** | exit 1 before any change, no backup | Invariant: uninstall removes the registration the client reads only when every repeat it would reveal is REA-owned. It decides that in the ownership rule that preflight shares, so a refusal changes nothing. ## Change and scope - `ClientConfigurationDocument` owns a new fact, `shadowedClientServers(text, parsed, name)`: the earlier members of one name in the table a client reads, plus OpenCode V1 repeats beside a V2 table. It reuses #4957's last-member locator (`effectiveJsonProperty`). It builds a syntax tree only when the name's JSON string token occurs more than once. The pattern matches literal, `\uXXXX` (either hex case), and short-escape spellings, so it never misses a member, and parsing, setup and doctor keep their cost (measurements below). - `serializeClientConfiguration` keeps #4957's fail-closed `shadowed_member` default. It accepts `{ removeShadowedMembers: true }` to delete every repeat at a deleted path in one pass over the text, using the syntax tree it has already parsed. - Uninstall's `clientRegistrationRemoval` (shared by preflight and removal) admits a removal only when REA owns the effective entry and every repeat. When an earlier repeat is not REA-owned, or OpenCode would fall back to a V1 member REA does not own, it reports `failed` in preflight with a specific reason, so no client, skill or backup changes. Removal then grants the serializer authority. - Setup grants the same authority for its only deletion: REA's OpenCode V1 name, which the native V2 entry supersedes whatever its launcher, as setup already does for a single member. - `docs/installation.md` documents the uninstall rule. - Not changed: repeats inside an inactive earlier copy of a server table, TOML/YAML/DSH (no repeated members), and the `shadowed_member` setup result and contract, which stays reachable for edits made without authority. ## Contract and boundary impact - Semantic owner and earliest changed stage: `ClientConfigurationDocument` (repeated-member facts and JSON edit authority), then the uninstall ownership decision. - Contribution scope fit and linked maintainer decision: not applicable. This is a bug fix inside existing setup/uninstall authority, and it removes only REA-owned members or REA's superseded V1 name. - CLI and MCP/tool-catalog contract: uninstall now succeeds for owned repeats. The unowned-repeat refusal moves to preflight; its detail still contains "would activate an earlier duplicate" and now also says the repeat "is not REA-owned". OpenCode setup with repeated V1 members returns `configured`. Tool catalog unchanged. - Provider, bridge, target-format, or platform compatibility: JSON/JSONC client formats only. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: foreign or unresolved repeats are never deleted or activated. A refused uninstall writes nothing, not even a backup. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. - [x] Change process memory, registers, files, or configuration. ## Evidence and regression coverage - Tests added or updated: - `tests/acceptance/setup/duplicateJsonMemberSetup.test.ts` (public CLI, extends #4957's suite): owned repeats including an escaped `"rea"` spelling are removed and the backup equals the original; repeated owned OpenCode V2 and V1 members are removed and comments are kept; an unowned V1 repeat makes uninstall stop **before Claude Code (iterated first) changes**, with no backups; OpenCode setup replaces repeated V1 entries; uninstall keeps one or repeated native entries when the V1 fallback is foreign. - `src/application/ClientConfigurationDocument.duplicateMembers.test.ts`: enumerates **every** layout of one to three repeated `mcpServers` tables built from seven server-table shapes (3,192 documents with LF/CRLF and BOM variants, for `claude_code` and JSONC) and checks the edited bytes with **`JSON.parse` as an independent reader**. It covers setup sets, the default refusal, removal with authority, and inactive tables kept byte for byte. A scale case removes 20,000 repeats interleaved with peers and comments in one pass. It also cross-checks that `shadowedClientServers` (prefiltered) and the serializer's full-tree refusal agree on every layout, and that every JSON escape spelling is found. - #4957's five tests are unchanged and pass. - Base reproduction: on `b83b9caf` the six new acceptance tests and 14 of the 17 new module tests fail. The other three enumerate the layouts and check setup, which #4957 already fixed. All pass on this branch. - Worst-case CLI harness (23 scenarios with the real built CLI: setup/doctor/repeat/uninstall, escaped names, three repeated tables, BOM+CRLF, JSONC comments and trailing commas, nested repeats, owned and foreign shadowing, OpenCode V2/V1 fallback): `b83b9caf` 19/25, this branch 25/25. Before #4957, `2c4338db` met 6 of the first 23 scenarios. Every setup case also checks that a second setup plans nothing and `doctor` reports `aligned`. - Performance (8 MiB `.claude.json` with 4,000 projects, WSL2 on a Ryzen 5 4600H): an earlier draft of this change built the tree on every parse and made it 13× slower (25 → 330 ms per parse). It was reworked to build on demand and only behind the prefilter. Results: `parseClientConfiguration` 33.4 ms main vs 34.6 ms branch; end-to-end `rea uninstall` median 3,453 ms main vs 3,353 ms branch (5 runs each, within noise). Removing repeats (review by Codex and Greptile): 2,000 repeats took 9,623 ms in the first commit, which re-parsed once per repeat. 20,000 repeats took 35,804 ms in 722af95b, which copied the text once per repeat, and take 455 ms in 939b31bb (one tree, one pass). - User-visible CLI/MCP output (refused uninstall): `Configuration repeats the "rea" member, and an earlier duplicate is not REA-owned; removing the registration the client reads would activate an earlier duplicate, so no change was made. Remove the duplicate members manually, then rerun uninstall.` - Remaining proof gaps: no native client was launched for this change (#4952 holds the native last-member observation). macOS and Windows were not run. On this host two existing acceptance tests (`dshClientSetup` "previews, approves…" and `unqualifiedDoctorSkill`) exceed the 30 s timeout **on both main and this branch**, and both pass with `--testTimeout 180000`. Neither path reaches the new code. ## Validation performed All runs were on Linux x64 (WSL2 Ubuntu), Node 24.18.0, npm 11.16.0: - `npm run test:focused --` 84 setup, uninstall, doctor and client-configuration test files: 1,003 passed, 8 skipped. The two timeouts above failed at 30 s; `npx vitest run --project acceptance --testTimeout 180000` on those two files passed 10/10. - `npm run test:focused -- src/application/ClientConfigurationDocument.duplicateMembers.test.ts tests/acceptance/setup/duplicateJsonMemberSetup.test.ts tests/boundary/filesystem/clientConfigurationEnvironment.test.ts src/application/ClientConfigurationDocument.capacity.test.ts`: 52 passed - The same new tests on `b83b9caf` sources: 20 failed, 8 passed (expected: the 8 are #4957's five acceptance tests and the three module tests for setup and layout enumeration) - `npm run check:fast`: passed - `npm run docs:check`: passed - `npm run verify:test-discovery`: all 1,580 test files discovered once - `npx oxfmt --check` on changed files: clean - Lint: 0 errors. Two warnings of rules these files already exceed elsewhere: `max-params` (the new serializer options argument) and `complexity` 21 in `removeClient`. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Configurations without repeated members are edited exactly as before, and the serializer's default is unchanged. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; Linux only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: removal widens only to members REA owns, or to REA's superseded V1 name, which setup already replaces. The unowned case now stops earlier, before any client changes. ## Author readiness Human owner: @NikeGunn - [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 - [ ] 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <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:26 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns