morluto/rea

Fork 0

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

[GH-ISSUE #4952] [Bug] Setup edits the first duplicate JSON member while clients read the last one #3071

New issue

Closed

opened 2026-10-31 00:35:55 +00:00 by bsakaguchi · 2 comments

bsakaguchi commented

2026-10-31 00:35:55 +00:00

Owner

Originally created by @xu91102 on GitHub (Oct 18, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/4952

Before submitting

Searched open/closed reports and current related PR production diffs.
Only authored isolated configurations and finite local controls are included.

Area

Setup, packaging, or upgrade

Observed behavior

On current main 2c4338db1fdb7773e05376301c558cff2a0b4b80 (6.6.0), setup reads the last duplicate JSON member but edits the first one. Approved setup of a valid configuration with repeated mcpServers or rea members returns exit1, needs_human, client failed/readback. Uninstall of an owned registration under repeated parent members, or repeated identical owned rea members, also returns exit1 and restores the original. All observed failures retain the original and an exact backup; no destructive deletion was observed.

Reproduction

Built the public CLI with the pinned toolchain, then used a different isolated home for each call. One failing setup input is:

{
  "mcpServers": {"shadow": {"command": "/usr/bin/false"}},
  "mcpServers": {"peer": {"command": "/usr/bin/true"}}
}

Run rea setup --client claude_code --skill=false --yes --format json with that home as HOME and CLAUDE_CONFIG_DIR. The equivalent single-parent configuration succeeds. For the uninstall control, add an owned rea command npx -y rea-agents@6.6.0 mcp to the last parent and run rea uninstall --format json; that command is classified only, not executed.

Eight actual CLI calls gave:

Four target failures: duplicate-parent setup, duplicate-leaf setup, duplicate-parent uninstall, identical-owned-duplicate-leaf uninstall.
Three successful controls: ordinary setup, ordinary uninstall, unrelated duplicate setting during setup.
One safety control: an inactive foreign rea followed by an active owned rea remains unchanged after failed removal. A fix must not delete or activate the foreign entry.

Native Claude Code2.1.263 was tested separately with immutable input files and independent writable copies. Three claude mcp get rea calls (ordinary, repeated parent, repeated leaf) all admitted the configuration and actually launched our finite local Node marker with the last member's exact argv. The marker is intentionally not an MCP server, so connection-closed is expected; this is native admission/selection evidence, not a successful MCP or agent/model workflow. Claude normalized its own working copies; the original input bytes stayed unchanged.

Expected behavior

Use the same effective JSON member at the text-edit boundary that the accepted document and native client read. Preserve unrelated entries/comments, exact backups, repeat behavior and ownership rules. Removing repeated owned registrations must not accidentally reveal an inactive foreign or unresolved launcher; ambiguous removal should retain the configuration before mutation with a specific explanation.

Version and environment

REA6.6.0, source main 2c4338db1fdb7773e05376301c558cff2a0b4b80; Node24.18.0/npm11.16.0; macOS27.0.1arm64; Claude Code2.1.263. Authored configuration files only. No real user configuration, npm/registry request, agent/model, or target execution by REA.

Earliest failure stage and source reasoning

CLI setup/configuration publication readback. ClientConfigurationDocument parses object values with last-member semantics, while JSONC modify and findNodeAtLocation select the first matching AST property. Setup and uninstall share that serializer. This is distinct from strict JSON versus JSONC admission (#4940), retained entry fields (#1732), and ggcode's YAML migration (#4039); I read their relevant actual production diffs before selecting this root.

Impact and proposed checks

Deterministic failure on the four authored layouts above; external-user frequency is NOT_ENOUGH_INFO. No fix or upstream PR is claimed yet. Repair the shared parsed-member/text-edit owner, remove superseded first-member editing, and verify setup/doctor/repeat/uninstall, escaped member names, nested duplicates, comments/BOM/CRLF, owned-only removal and foreign/unresolved shadow retention. Windows/Linux, other native clients, full Claude agent/model and performance remain NOT RUN.

Follow-up: overlapping implementation, local experiment held

I read the complete actual diff of #4957, head b6bb75dfe8f3659152fced1b4550bef06bca25d4 by @kaoru0822-kitauji. It implements the same shared parsed-member/text-edit root, including last-member selection and refusal of duplicate removal before backup. Its representation and duplicate-removal policy differ from my local experiment. I have not run that head or established complete coverage of my test scope. It is OPEN and not merged at this read.

I am holding my nine-path local experiment read-only to avoid a duplicate contribution. It has no commit, fork push, upstream PR or CI. On its final local frozen inputs, 18 focused files457PASS plus README5 gave 19distinct462PASS/0skip, and one tarball passed58 installed CLI calls plus8 nativeClaude configuration/connection/absence probes. These passing controls do not establish a completed fix: independent review v1 found a setup migration admission gap; v2 corrected that gap but the Spec review found that removing duplicate OpenCode native entries can activate a retained foreign/unknown legacy fallback. I reproduced that latter case on the local installed experiment versus main with identical inputs. The experiment remains incomplete and will not be submitted or repackaged as leftover fallback/lifecycle work. No native OpenCode launcher, full agent/model workflow, Windows/Linux or performance qualification is claimed.

Update: #4957 merged; local experiment remains held

#4957 was merged on 2026-10-18 at 05:15:48 UTC as main commit b83b9caf4a5f0241a5fd370792f1aed1bffbb2cf, and this issue closed at 05:15:50 UTC. I read its complete eight-file merge patch and all changed production code. It addresses the same effective-member text-edit root and refuses deletion that would reveal an earlier duplicate before creating a backup or publishing configuration.

The merged implementation is the other contributor's PR, not a PR or commit from my local experiment; the merge commit does not list me as an author or coauthor. I have not run the merged head or borrowed its validation results. My earlier local tests remain tied to the original frozen experiment.

My nine-path experiment remains uncommitted and read-only. Its v2 Spec review failure concerning OpenCode legacy fallback remains recorded; it is not a completed fix and will not be submitted or repackaged as leftover legacy, fallback or lifecycle work. No close operation or reply to another contributor was performed by me.

Originally created by @xu91102 on GitHub (Oct 18, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/4952 ### Before submitting - [x] Searched open/closed reports and current related PR production diffs. - [x] Only authored isolated configurations and finite local controls are included. ### Area Setup, packaging, or upgrade ### Observed behavior On current main `2c4338db1fdb7773e05376301c558cff2a0b4b80` (6.6.0), setup reads the last duplicate JSON member but edits the first one. Approved setup of a valid configuration with repeated `mcpServers` or `rea` members returns exit1, `needs_human`, client `failed/readback`. Uninstall of an owned registration under repeated parent members, or repeated identical owned `rea` members, also returns exit1 and restores the original. All observed failures retain the original and an exact backup; no destructive deletion was observed. ### Reproduction Built the public CLI with the pinned toolchain, then used a different isolated home for each call. One failing setup input is: ```json { "mcpServers": {"shadow": {"command": "/usr/bin/false"}}, "mcpServers": {"peer": {"command": "/usr/bin/true"}} } ``` Run `rea setup --client claude_code --skill=false --yes --format json` with that home as `HOME` and `CLAUDE_CONFIG_DIR`. The equivalent single-parent configuration succeeds. For the uninstall control, add an owned `rea` command `npx -y rea-agents@6.6.0 mcp` to the last parent and run `rea uninstall --format json`; that command is classified only, not executed. Eight actual CLI calls gave: - Four target failures: duplicate-parent setup, duplicate-leaf setup, duplicate-parent uninstall, identical-owned-duplicate-leaf uninstall. - Three successful controls: ordinary setup, ordinary uninstall, unrelated duplicate setting during setup. - One safety control: an inactive foreign `rea` followed by an active owned `rea` remains unchanged after failed removal. A fix must not delete or activate the foreign entry. Native Claude Code2.1.263 was tested separately with immutable input files and independent writable copies. Three `claude mcp get rea` calls (ordinary, repeated parent, repeated leaf) all admitted the configuration and actually launched our finite local Node marker with the **last** member's exact argv. The marker is intentionally not an MCP server, so connection-closed is expected; this is native admission/selection evidence, not a successful MCP or agent/model workflow. Claude normalized its own working copies; the original input bytes stayed unchanged. ### Expected behavior Use the same effective JSON member at the text-edit boundary that the accepted document and native client read. Preserve unrelated entries/comments, exact backups, repeat behavior and ownership rules. Removing repeated owned registrations must not accidentally reveal an inactive foreign or unresolved launcher; ambiguous removal should retain the configuration before mutation with a specific explanation. ### Version and environment REA6.6.0, source main `2c4338db1fdb7773e05376301c558cff2a0b4b80`; Node24.18.0/npm11.16.0; macOS27.0.1arm64; Claude Code2.1.263. Authored configuration files only. No real user configuration, npm/registry request, agent/model, or target execution by REA. ### Earliest failure stage and source reasoning CLI setup/configuration publication readback. `ClientConfigurationDocument` parses object values with last-member semantics, while JSONC `modify` and `findNodeAtLocation` select the first matching AST property. Setup and uninstall share that serializer. This is distinct from strict JSON versus JSONC admission (#4940), retained entry fields (#1732), and ggcode's YAML migration (#4039); I read their relevant actual production diffs before selecting this root. ### Impact and proposed checks Deterministic failure on the four authored layouts above; external-user frequency is NOT_ENOUGH_INFO. No fix or upstream PR is claimed yet. Repair the shared parsed-member/text-edit owner, remove superseded first-member editing, and verify setup/doctor/repeat/uninstall, escaped member names, nested duplicates, comments/BOM/CRLF, owned-only removal and foreign/unresolved shadow retention. Windows/Linux, other native clients, full Claude agent/model and performance remain NOT RUN. ### Follow-up: overlapping implementation, local experiment held I read the complete actual diff of [#4957](https://github.com/morluto/rea/pull/4957), head `b6bb75dfe8f3659152fced1b4550bef06bca25d4` by @kaoru0822-kitauji. It implements the same shared parsed-member/text-edit root, including last-member selection and refusal of duplicate removal before backup. Its representation and duplicate-removal policy differ from my local experiment. I have not run that head or established complete coverage of my test scope. It is OPEN and not merged at this read. I am holding my nine-path local experiment read-only to avoid a duplicate contribution. It has no commit, fork push, upstream PR or CI. On its final local frozen inputs, 18 focused files457PASS plus README5 gave 19distinct462PASS/0skip, and one tarball passed58 installed CLI calls plus8 nativeClaude configuration/connection/absence probes. These passing controls do not establish a completed fix: independent review v1 found a setup migration admission gap; v2 corrected that gap but the Spec review found that removing duplicate OpenCode native entries can activate a retained foreign/unknown legacy fallback. I reproduced that latter case on the local installed experiment versus main with identical inputs. The experiment remains incomplete and will not be submitted or repackaged as leftover fallback/lifecycle work. No native OpenCode launcher, full agent/model workflow, Windows/Linux or performance qualification is claimed. ### Update: #4957 merged; local experiment remains held [#4957](https://github.com/morluto/rea/pull/4957) was merged on 2026-10-18 at 05:15:48 UTC as main commit [`b83b9caf4a5f0241a5fd370792f1aed1bffbb2cf`](https://github.com/morluto/rea/commit/b83b9caf4a5f0241a5fd370792f1aed1bffbb2cf), and this issue closed at 05:15:50 UTC. I read its complete eight-file merge patch and all changed production code. It addresses the same effective-member text-edit root and refuses deletion that would reveal an earlier duplicate before creating a backup or publishing configuration. The merged implementation is the other contributor's PR, not a PR or commit from my local experiment; the merge commit does not list me as an author or coauthor. I have not run the merged head or borrowed its validation results. My earlier local tests remain tied to the original frozen experiment. My nine-path experiment remains uncommitted and read-only. Its v2 Spec review failure concerning OpenCode legacy fallback remains recorded; it is not a completed fix and will not be submitted or repackaged as leftover legacy, fallback or lifecycle work. No close operation or reply to another contributor was performed by me.

bsakaguchi closed this issue

2026-10-31 00:35:56 +00:00

bsakaguchi commented

2026-10-31 00:35:57 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 18, 2026):

Reproduced the same read-last/edit-first inconsistency here — with the duplicated mcpServers example, setup returns exit 1 and leaves the client unable to read back the approved config. It's a genuine mismatch between the parsed-member owner (last one wins, per JSON semantics) and the text-edit owner (first occurrence), so the write never lands where clients look.

The #4957 direction (repair the shared parsed-member/text-edit owner and remove the superseded first-member editing) is right. One suggestion: since setup/doctor/uninstall all touch the same file, it'd be good to route them all through that single owner and add a regression case with a repeated parent member to each command's test, so this doesn't silently come back in a later refactor.

@XinwangTao commented on GitHub (Oct 18, 2026): Reproduced the same read-last/edit-first inconsistency here — with the duplicated mcpServers example, setup returns exit 1 and leaves the client unable to read back the approved config. It's a genuine mismatch between the parsed-member owner (last one wins, per JSON semantics) and the text-edit owner (first occurrence), so the write never lands where clients look. The #4957 direction (repair the shared parsed-member/text-edit owner and remove the superseded first-member editing) is right. One suggestion: since setup/doctor/uninstall all touch the same file, it'd be good to route them all through that single owner and add a regression case with a repeated parent member to each command's test, so this doesn't silently come back in a later refactor.

bsakaguchi commented

2026-10-31 00:35:58 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 18, 2026):

#4957 landing in 6.7.0 closes the read-last/edit-first gap, and routing setup/doctor/uninstall through the shared parsed-member owner was the right move. The follow-up #4976/#4977 is a worthwhile catch though: it shows the "refuse deletion when it would reveal an earlier duplicate" policy can still leave repeated REA-owned members blocking uninstall, and skipping preflight there makes it harder to diagnose. Suggest pinning a regression test in #4977 that covers the exact #4976 reproduction (repeated owned members + preflight check) so the removal path is proven end-to-end, not just at the AST level.

@XinwangTao commented on GitHub (Oct 18, 2026): #4957 landing in 6.7.0 closes the read-last/edit-first gap, and routing setup/doctor/uninstall through the shared parsed-member owner was the right move. The follow-up #4976/#4977 is a worthwhile catch though: it shows the "refuse deletion when it would reveal an earlier duplicate" policy can still leave repeated REA-owned members blocking uninstall, and skipping preflight there makes it harder to diagnose. Suggest pinning a regression test in #4977 that covers the exact #4976 reproduction (repeated owned members + preflight check) so the removal path is proven end-to-end, not just at the AST level.

bsakaguchi referenced this issue

2026-10-31 00:40:20 +00:00

[GH-ISSUE #5654] [Tracker] Open PRs that can't merge yet: conflicts and red CI (October 2026) #3425

bsakaguchi referenced this issue

2026-10-31 01:07:50 +00:00

[PR #3071] [CLOSED] Build #5591