[PR #6724] [CLOSED] fix(uninstall): keep the reason when valid Cordis YAML is refused #7399

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6724
Author: @pablodiazjorge
Created: 10/23/2026
Status: ❌ Closed

Base: main ← Head: fix/uninstall-cordis-ownership-diagnostic


📝 Commits (2)

  • 4f5df4a fix(uninstall): keep the reason when valid Cordis YAML is refused
  • 41f4728 Merge upstream main into fix/uninstall-cordis-ownership-diagnostic

📊 Changes

3 files changed (+55 additions, -3 deletions)

View changed files

📝 src/application/DshCordisYaml.ts (+4 -1)
📝 src/application/Uninstall.ts (+7 -1)
📝 tests/boundary/filesystem/dshClientConfiguration.test.ts (+44 -1)

📄 Description

Summary

Fixes #5439. For a DeepSeek Harness Cordis patch whose entry ID is an expression, rea uninstall reported Configuration is not valid YAML and was not changed, although the YAML is valid. Setup and doctor already explain the real reason: expression-backed IDs have unknown ownership and need literal IDs. Uninstall now keeps that reason; a YAML syntax failure still reads as invalid YAML.

Thanks to @coygeek for the report and the syntax-error control.

Problem and expected behavior

DshCordisYaml admits the !!js tag and only then does DshCordisDocument reject the expression ID with a ConfigurationError ("Expression-backed Cordis IDs have unknown ownership; use literal IDs before changing registrations"). The catch around parseClientConfiguration in Uninstall.ts kept the message only for ClientConfigurationShapeError and turned everything else into the format's invalid-syntax message. Syntax failures in DshCordisYaml are also ConfigurationErrors, so the catch could not tell the two apart.

Expected, as the issue asks: keep the failed dsh item, the unchanged file and the missing backup, report the ownership reason, and keep the invalid-YAML wording for real syntax errors.

Change and scope

  • src/application/DshCordisYaml.ts: the YAML parse and warning failure ("Invalid or unsupported Cordis YAML") is now a CordisYamlSyntaxError, a ConfigurationError subclass owned by the YAML reader, so callers can tell syntax from content.
  • src/application/Uninstall.ts: besides shape errors, any other ConfigurationError keeps its message (Configuration was not changed: <reason>. Repair it, then rerun uninstall.); CordisYamlSyntaxError and non-configuration errors keep the existing invalid-format message. Setup already reports every ConfigurationError message (SetupClientConfiguration.ts), so both commands now give the same reason.
  • In uninstall only the Cordis reader raises ConfigurationError; the other formats' parse failures are SyntaxErrors or shape errors and are unchanged. The other Cordis content refusals (for example a patch root that is not an operation list, or an unresolved alias) now also show their reason instead of "not valid YAML".
  • Not included: changing what setup or doctor report, or the uninstall remediation sentence.

Contract and boundary impact

  • Semantic owner and earliest changed stage: the Cordis YAML reader (DshCordisYaml), which now types its syntax failure; uninstall's configuration read (Uninstall.ts) consumes it.
  • Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable.
  • CLI and MCP/tool-catalog contract: the failed dsh uninstall item's detail names the content refusal instead of "not valid YAML"; status, exit code and the no-removal behavior are unchanged. No catalog change.
  • Provider, bridge, target-format, or platform compatibility: DeepSeek Harness only.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): uninstall still refuses before any removal, leaves the file byte-identical and writes no backup.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): 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: tests/boundary/filesystem/dshClientConfiguration.test.ts, new describe "DeepSeek Harness uninstall diagnostics", through runUninstall with the system uninstall host, two cases with the same patch except the id line:
    • the issue's expression ID: a failed dsh item whose detail names the unknown ownership and the literal-ID remediation;
    • [unterminated (the issue's syntax-error control): the existing "not valid YAML" detail;
    • both: overall status failed, the file unchanged and no .rea.backup.
  • Base reproduction or other evidence: on the base revision the expression case fails (the item reads "Configuration is not valid YAML and was not changed…"); the syntax control passes on both.
  • User-visible CLI/MCP output (if applicable): the new dsh item detail, from the test: Configuration was not changed: Expression-backed Cordis IDs have unknown ownership; use literal IDs before changing registrations. Repair it, then rerun uninstall.
  • Remaining proof gaps: the CLI acceptance suite (clientConfigurationDiagnosticsCli.test.ts) was not run here; its dsh syntax case should be unaffected because that failure is now a CordisYamlSyntaxError.

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

Windows 11 x64 10.0.26300, Node 24.18.0, on main 19520d000:

  • npx vitest run tests/boundary/filesystem/dshClientConfiguration.test.ts -t "uninstall diagnostics": 2 passed; on the base the expression case fails and the syntax control passes.
  • npx vitest run tests/boundary/filesystem/dshClientConfiguration.test.ts tests/boundary/filesystem/clientConfigurationEnvironment.test.ts tests/boundary/filesystem/clientLifecycle.test.ts tests/boundary/filesystem/hermesClientConfiguration.test.ts: 233 passed, 7 failed, 2 skipped. The 7 failures are host-dependent (five symlink cases in clientLifecycle, which need symlink privilege on Windows, and two Hermes path cases) and identical on the base: those two files give the same 7 failures with and without the change.
  • npm run check:fast: typecheck and lint pass.
  • npx oxfmt --check on the changed files: pass.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Only the wording of a failed uninstall item changes, for valid Cordis patches that REA refuses to edit.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider involved. Windows only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none; uninstall still refuses before removing anything, and Cordis expressions are never evaluated.

Author readiness

Human owner: @pablodiazjorge

  • 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/6724 **Author:** [@pablodiazjorge](https://github.com/pablodiazjorge) **Created:** 10/23/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/uninstall-cordis-ownership-diagnostic` --- ### 📝 Commits (2) - [`4f5df4a`](https://github.com/morluto/rea/commit/4f5df4a823a322f42237911bbb398020de1abf2d) fix(uninstall): keep the reason when valid Cordis YAML is refused - [`41f4728`](https://github.com/morluto/rea/commit/41f4728e92472fa25d2a0e1d2b02a641c232258c) Merge upstream main into fix/uninstall-cordis-ownership-diagnostic ### 📊 Changes **3 files changed** (+55 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `src/application/DshCordisYaml.ts` (+4 -1) 📝 `src/application/Uninstall.ts` (+7 -1) 📝 `tests/boundary/filesystem/dshClientConfiguration.test.ts` (+44 -1) </details> ### 📄 Description ## Summary Fixes #5439. For a DeepSeek Harness Cordis patch whose entry ID is an expression, `rea uninstall` reported `Configuration is not valid YAML and was not changed`, although the YAML is valid. Setup and doctor already explain the real reason: expression-backed IDs have unknown ownership and need literal IDs. Uninstall now keeps that reason; a YAML syntax failure still reads as invalid YAML. Thanks to @coygeek for the report and the syntax-error control. ## Problem and expected behavior `DshCordisYaml` admits the `!!js` tag and only then does `DshCordisDocument` reject the expression ID with a `ConfigurationError` ("Expression-backed Cordis IDs have unknown ownership; use literal IDs before changing registrations"). The catch around `parseClientConfiguration` in `Uninstall.ts` kept the message only for `ClientConfigurationShapeError` and turned everything else into the format's invalid-syntax message. Syntax failures in `DshCordisYaml` are also `ConfigurationError`s, so the catch could not tell the two apart. Expected, as the issue asks: keep the failed `dsh` item, the unchanged file and the missing backup, report the ownership reason, and keep the invalid-YAML wording for real syntax errors. ## Change and scope - `src/application/DshCordisYaml.ts`: the YAML parse and warning failure ("Invalid or unsupported Cordis YAML") is now a `CordisYamlSyntaxError`, a `ConfigurationError` subclass owned by the YAML reader, so callers can tell syntax from content. - `src/application/Uninstall.ts`: besides shape errors, any other `ConfigurationError` keeps its message (`Configuration was not changed: <reason>. Repair it, then rerun uninstall.`); `CordisYamlSyntaxError` and non-configuration errors keep the existing invalid-format message. Setup already reports every `ConfigurationError` message (`SetupClientConfiguration.ts`), so both commands now give the same reason. - In uninstall only the Cordis reader raises `ConfigurationError`; the other formats' parse failures are `SyntaxError`s or shape errors and are unchanged. The other Cordis content refusals (for example a patch root that is not an operation list, or an unresolved alias) now also show their reason instead of "not valid YAML". - Not included: changing what setup or doctor report, or the uninstall remediation sentence. ## Contract and boundary impact <!-- Short answers are welcome. Complete applicable lines; mark unresolved facts as "unknown". Use "none" or "not applicable" only when established. --> - Semantic owner and earliest changed stage: the Cordis YAML reader (`DshCordisYaml`), which now types its syntax failure; uninstall's configuration read (`Uninstall.ts`) consumes it. - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: the failed `dsh` uninstall item's `detail` names the content refusal instead of "not valid YAML"; status, exit code and the no-removal behavior are unchanged. No catalog change. - Provider, bridge, target-format, or platform compatibility: DeepSeek Harness only. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): uninstall still refuses before any removal, leaves the file byte-identical and writes no backup. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): 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. - [ ] 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: `tests/boundary/filesystem/dshClientConfiguration.test.ts`, new describe "DeepSeek Harness uninstall diagnostics", through `runUninstall` with the system uninstall host, two cases with the same patch except the `id` line: - the issue's expression ID: a failed `dsh` item whose detail names the unknown ownership and the literal-ID remediation; - `[unterminated` (the issue's syntax-error control): the existing "not valid YAML" detail; - both: overall status `failed`, the file unchanged and no `.rea.backup`. - Base reproduction or other evidence: on the base revision the expression case fails (the item reads "Configuration is not valid YAML and was not changed…"); the syntax control passes on both. - User-visible CLI/MCP output (if applicable): the new `dsh` item detail, from the test: `Configuration was not changed: Expression-backed Cordis IDs have unknown ownership; use literal IDs before changing registrations. Repair it, then rerun uninstall.` - Remaining proof gaps: the CLI acceptance suite (`clientConfigurationDiagnosticsCli.test.ts`) was not run here; its `dsh` syntax case should be unaffected because that failure is now a `CordisYamlSyntaxError`. 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 Windows 11 x64 10.0.26300, Node 24.18.0, on main `19520d000`: - `npx vitest run tests/boundary/filesystem/dshClientConfiguration.test.ts -t "uninstall diagnostics"`: 2 passed; on the base the expression case fails and the syntax control passes. - `npx vitest run tests/boundary/filesystem/dshClientConfiguration.test.ts tests/boundary/filesystem/clientConfigurationEnvironment.test.ts tests/boundary/filesystem/clientLifecycle.test.ts tests/boundary/filesystem/hermesClientConfiguration.test.ts`: 233 passed, 7 failed, 2 skipped. The 7 failures are host-dependent (five symlink cases in `clientLifecycle`, which need symlink privilege on Windows, and two Hermes path cases) and identical on the base: those two files give the same 7 failures with and without the change. - `npm run check:fast`: typecheck and lint pass. - `npx oxfmt --check` on the changed files: pass. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only the wording of a failed uninstall item changes, for valid Cordis patches that REA refuses to edit. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider involved. Windows only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; uninstall still refuses before removing anything, and Cordis expressions are never evaluated. ## Author readiness Human owner: @pablodiazjorge - [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:47:34 +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#7399
No description provided.