[PR #2730] [MERGED] fix(setup): keep a Cordis operation's own fields when uninstall empties it #5348

New issue

Closed

opened 2026-10-31 01:05:54 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:05:54 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2730
Author: @JayOfTheKeyboard
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/cordis-uninstall-keep-user-keys

📝 Commits (3)

291b035 fix(setup): keep a Cordis operation's own fields when uninstall empties it
358f774 test(setup): keep an operation's own fields when setup collapses a second REA row
5ffb7a5 chore: merge upstream CI baseline fixes

📊 Changes

2 files changed (+142 additions, -4 deletions)

View changed files

📝 src/application/DshCordisPatch.ts (+10 -4)
📝 tests/boundary/filesystem/dshClientConfiguration.test.ts (+132 -0)

📄 Description

Summary

rea uninstall deletes a whole Cordis patch operation in ~/.dsh/cordis.patch.yml when REA's rea-mcp row was the only row in its insert list. If that operation also carries the user's own fields, such as a group patch with id and config, those fields and the comment above them are lost. Uninstall still reports removed. No issue is linked: I found this while reviewing the DeepSeek Harness support from #1929, and the fix is small enough to explain here.

Problem and expected behavior

Setup writes REA's row as its own operation, - insert: [rea-mcp], and uninstall correctly drops that operation once the row is gone. A user can move the row into an operation that does more than insert, though, for example one that also patches a group:

- insert:
    - id: user-a
      name: plugin-a
# caller group patch
- id: tools
  config:
    sandbox: strict
  insert:
    - id: rea-mcp
      name: "@deepseek-ai/dsh-mcp-client"
      config: { serverName: rea, transport: stdio, command: ..., args: [...] }

DshCordisDocument.serialize drops any operation whose insert list ends up empty, so uninstall removes id: tools, its config and the comment with it. REA's own parser reads a no-insert { id: rea-mcp, config } operation as a patch of REA's row (the direct patch branch in the constructor), so by the same shape { id: tools, config } is the user's patch of tools. .rea.backup is written on first setup and kept after that, so it may not hold the user's later edit. For setup, docs/client-configuration.md promises to keep "unrelated patches and comments", and uninstall should keep them too.

Measured with the built CLI under a scratch HOME on main at 0043d9b1 (src/application/Dsh* and the test file are unchanged on current main, 4276aaf1): rea setup --client dsh --yes, then the hand edit above, then rea uninstall. Uninstall reports dsh: removed, and the file is left with only the user-a operation. The backup holds only the original user-a operation as well.

Expected: remove REA's row and keep the rest of the operation, dropping the operation only when REA's row was everything in it.

Change and scope

In DshCordisDocument.serialize (src/application/DshCordisPatch.ts):

When REA's rows empty an operation's insert list, delete only the insert key.
Keep the operation when it still holds a field besides its id or name selector. The kept fields keep their comments and tags, since the operation is the same detached copy as before.
Drop it as before when nothing but the selector is left. Setup's own shape, - insert: [rea-mcp], and - id: tools with only REA's row are still removed whole.
Also drop it when its id is rea-mcp. Kept without insert, it would read back as a direct patch of REA's row, and uninstall's readback check would then report failed.

Not changed, and yours to decide:

A comment on REA's removed row, or on the insert: key itself, is removed with it, as it was before.
.rea.backup is still written once, on first setup. Refreshing it on each uninstall would cover other hand edits too, but it changes the backup rule for every client.
The keep rule names the selector keys (id, name) and REA's own id directly. Reusing the constructor's direct classification on the kept operation would be the more general check if more key kinds turn up.
Merge keys (<<: *anchor) are not read by the parser, so a REA row reached only through one is left in place on uninstall. That was already true before this change.

Contract and boundary impact

Semantic owner and earliest changed stage: Cordis patch serialization in DshCordisDocument.serialize, used by rea uninstall (and by setup when it collapses duplicate REA rows).
Contribution scope fit and linked maintainer decision: not applicable. A bug fix inside the existing DeepSeek Harness client support.
CLI and MCP/tool-catalog contract: none.
Provider, bridge, target-format, or platform compatibility: DeepSeek Harness client configuration only.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details (if applicable): uninstall now removes less from a caller-owned file. Setup's own shape is cleaned up as before.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

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: 4 uninstall cases in tests/boundary/filesystem/dshClientConfiguration.test.ts, each through systemUninstallHost(home).removeClient:
- a group patch (id, config, a comment above it) whose only row was REA's keeps id, config and the comment, and loses insert;
- an emptied operation with id: rea-mcp is dropped and uninstall reports removed;
- an emptied operation left with only its target selector (id, name, or both) is dropped;
- setup's own insert-only operation is still dropped.
- setup, collapsing a second REA row out of an operation that has its own field (disabled: false) and a comment, keeps both, leaves one REA row and reads back as aligned. Setup refuses a REA row inside a group patch, so this is the only way setup reaches the rule. It fails on main, which drops the field and the comment (added after review).
Base reproduction: on main at 4276aaf1 the group patch case fails with expected [ { insert: [ { …(2) } ] } ] to deeply equal [ { insert: [ { …(2) } ] }, …(1) ]. The other 5 pass there too. They pin the cases where the operation must still go, which a plain "keep it if anything is left" rule gets wrong. Each part of the fix turns a test red when removed: keeping the empty insert key fails 10 tests, dropping the rea-mcp check fails 1, counting selectors as caller fields fails 3, leaving id or name out of the selector keys fails 2 each, and the old drop rule fails the group patch case.
User-visible CLI/MCP output: the built CLI under a scratch HOME. rea setup --client dsh --yes on a file holding one user-a operation, then the hand edit from above, then rea uninstall.

	`main` at `0043d9b1`	This branch
uninstall result for `dsh`	`removed`	`removed`
`user-a` operation	kept	kept
`# caller group patch`	lost	kept
`id: tools` with `config.sandbox: strict`	lost	kept, without `insert`
`cordis.patch.yml.rea.backup`	only the `user-a` operation	only the `user-a` operation

Remaining proof gaps: no real DeepSeek Harness was run. I have not checked how Harness itself treats a { id, config } operation with no insert, or an operation left with no selector at all (- insert: [rea-mcp] plus disabled: false becomes - disabled: false). Both keep what the user wrote rather than guess.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npx vitest run over the 20 test files whose path names setup, uninstall, dsh or cordis: 227 tests passed.
npm run check:fast: passed.
npm run verify:test-discovery: "Discovered all 961 test files exactly once."
npx oxfmt --check on the 2 changed files: passed.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: Linux x64 (Fedora 44) only. No real DeepSeek Harness was run.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none. Uninstall now deletes less from a user file, and only REA's row is removed.

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/2730 **Author:** [@JayOfTheKeyboard](https://github.com/JayOfTheKeyboard) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/cordis-uninstall-keep-user-keys` --- ### 📝 Commits (3) - [`291b035`](https://github.com/morluto/rea/commit/291b035caa741b29bbf1b3e0529e04947471c0cc) fix(setup): keep a Cordis operation's own fields when uninstall empties it - [`358f774`](https://github.com/morluto/rea/commit/358f77483d98fe62c6f3e5d3924df6117c02c151) test(setup): keep an operation's own fields when setup collapses a second REA row - [`5ffb7a5`](https://github.com/morluto/rea/commit/5ffb7a573613fe6dc078e4ee5024d7ad469e4b43) chore: merge upstream CI baseline fixes ### 📊 Changes **2 files changed** (+142 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/application/DshCordisPatch.ts` (+10 -4) 📝 `tests/boundary/filesystem/dshClientConfiguration.test.ts` (+132 -0) </details> ### 📄 Description ## Summary `rea uninstall` deletes a whole Cordis patch operation in `~/.dsh/cordis.patch.yml` when REA's `rea-mcp` row was the only row in its `insert` list. If that operation also carries the user's own fields, such as a group patch with `id` and `config`, those fields and the comment above them are lost. Uninstall still reports `removed`. No issue is linked: I found this while reviewing the DeepSeek Harness support from #1929, and the fix is small enough to explain here. ## Problem and expected behavior Setup writes REA's row as its own operation, `- insert: [rea-mcp]`, and uninstall correctly drops that operation once the row is gone. A user can move the row into an operation that does more than insert, though, for example one that also patches a group: ```yaml - insert: - id: user-a name: plugin-a # caller group patch - id: tools config: sandbox: strict insert: - id: rea-mcp name: "@deepseek-ai/dsh-mcp-client" config: { serverName: rea, transport: stdio, command: ..., args: [...] } ``` `DshCordisDocument.serialize` drops any operation whose `insert` list ends up empty, so uninstall removes `id: tools`, its `config` and the comment with it. REA's own parser reads a no-insert `{ id: rea-mcp, config }` operation as a patch of REA's row (the direct patch branch in the constructor), so by the same shape `{ id: tools, config }` is the user's patch of `tools`. `.rea.backup` is written on first setup and kept after that, so it may not hold the user's later edit. For setup, `docs/client-configuration.md` promises to keep "unrelated patches and comments", and uninstall should keep them too. Measured with the built CLI under a scratch `HOME` on `main` at `0043d9b1` (`src/application/Dsh*` and the test file are unchanged on current `main`, `4276aaf1`): `rea setup --client dsh --yes`, then the hand edit above, then `rea uninstall`. Uninstall reports `dsh: removed`, and the file is left with only the `user-a` operation. The backup holds only the original `user-a` operation as well. Expected: remove REA's row and keep the rest of the operation, dropping the operation only when REA's row was everything in it. ## Change and scope In `DshCordisDocument.serialize` (`src/application/DshCordisPatch.ts`): - When REA's rows empty an operation's `insert` list, delete only the `insert` key. - Keep the operation when it still holds a field besides its `id` or `name` selector. The kept fields keep their comments and tags, since the operation is the same detached copy as before. - Drop it as before when nothing but the selector is left. Setup's own shape, `- insert: [rea-mcp]`, and `- id: tools` with only REA's row are still removed whole. - Also drop it when its `id` is `rea-mcp`. Kept without `insert`, it would read back as a direct patch of REA's row, and uninstall's readback check would then report `failed`. Not changed, and yours to decide: - A comment on REA's removed row, or on the `insert:` key itself, is removed with it, as it was before. - `.rea.backup` is still written once, on first setup. Refreshing it on each uninstall would cover other hand edits too, but it changes the backup rule for every client. - The keep rule names the selector keys (`id`, `name`) and REA's own id directly. Reusing the constructor's `direct` classification on the kept operation would be the more general check if more key kinds turn up. - Merge keys (`<<: *anchor`) are not read by the parser, so a REA row reached only through one is left in place on uninstall. That was already true before this change. ## Contract and boundary impact - Semantic owner and earliest changed stage: Cordis patch serialization in `DshCordisDocument.serialize`, used by `rea uninstall` (and by setup when it collapses duplicate REA rows). - Contribution scope fit and linked maintainer decision: not applicable. A bug fix inside the existing DeepSeek Harness client support. - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: DeepSeek Harness client configuration only. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): uninstall now removes less from a caller-owned file. Setup's own shape is cleaned up as before. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [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. ## Evidence and regression coverage - Tests added or updated: 4 uninstall cases in `tests/boundary/filesystem/dshClientConfiguration.test.ts`, each through `systemUninstallHost(home).removeClient`: - a group patch (`id`, `config`, a comment above it) whose only row was REA's keeps `id`, `config` and the comment, and loses `insert`; - an emptied operation with `id: rea-mcp` is dropped and uninstall reports `removed`; - an emptied operation left with only its target selector (`id`, `name`, or both) is dropped; - setup's own insert-only operation is still dropped. - setup, collapsing a second REA row out of an operation that has its own field (`disabled: false`) and a comment, keeps both, leaves one REA row and reads back as aligned. Setup refuses a REA row inside a group patch, so this is the only way setup reaches the rule. It fails on `main`, which drops the field and the comment (added after review). - Base reproduction: on `main` at `4276aaf1` the group patch case fails with `expected [ { insert: [ { …(2) } ] } ] to deeply equal [ { insert: [ { …(2) } ] }, …(1) ]`. The other 5 pass there too. They pin the cases where the operation must still go, which a plain "keep it if anything is left" rule gets wrong. Each part of the fix turns a test red when removed: keeping the empty `insert` key fails 10 tests, dropping the `rea-mcp` check fails 1, counting selectors as caller fields fails 3, leaving `id` or `name` out of the selector keys fails 2 each, and the old drop rule fails the group patch case. - User-visible CLI/MCP output: the built CLI under a scratch `HOME`. `rea setup --client dsh --yes` on a file holding one `user-a` operation, then the hand edit from above, then `rea uninstall`. | | `main` at `0043d9b1` | This branch | | --- | --- | --- | | uninstall result for `dsh` | `removed` | `removed` | | `user-a` operation | kept | kept | | `# caller group patch` | lost | kept | | `id: tools` with `config.sandbox: strict` | lost | kept, without `insert` | | `cordis.patch.yml.rea.backup` | only the `user-a` operation | only the `user-a` operation | - Remaining proof gaps: no real DeepSeek Harness was run. I have not checked how Harness itself treats a `{ id, config }` operation with no `insert`, or an operation left with no selector at all (`- insert: [rea-mcp]` plus `disabled: false` becomes `- disabled: false`). Both keep what the user wrote rather than guess. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run` over the 20 test files whose path names setup, uninstall, `dsh` or `cordis`: 227 tests passed. - `npm run check:fast`: passed. - `npm run verify:test-discovery`: "Discovered all 961 test files exactly once." - `npx oxfmt --check` on the 2 changed files: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: Linux x64 (Fedora 44) only. No real DeepSeek Harness was run. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none. Uninstall now deletes less from a user file, and only REA's row is removed. ## 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-10-31 01:05:54 +00:00