morluto/rea

Fork 0

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

[PR #2686] [MERGED] fix(setup): treat empty DSH_HOME as unset #5311

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/setup-empty-dsh-home

📝 Commits (4)

4ef8c42 fix(setup): treat empty DSH_HOME as unset (#2666)
181e712 fix(setup): preserve portable DSH paths and public empty-home coverage
7b54cee fix(setup): reconcile DSH regression coverage with current path ownership
86e73fc test(setup): remove superseded DSH path unit coverage

📊 Changes

1 file changed (+1 additions, -1 deletions)

View changed files

📝 src/application/SupportedClients.ts (+1 -1)

📄 Description

Summary

Fixes #2666 — rea setup --client dsh --skill=false --yes --json creates cordis.patch.yml in the working directory when DSH_HOME is set to an empty string. The released @deepseek-ai/dsh-home-paths package already treats an empty/whitespace-only DSH_HOME as unset; REA's resolver did not.

Problem and expected behavior

SupportedClients.ts dshHomeDirectory used nullish coalescing:

const dshHomeDirectory = ({ home, env }: ClientPathContext): string =>
  env.DSH_HOME ?? join(home, ".dsh");

An empty DSH_HOME is a non-nullish string, so it leaked through join(dshHomeDirectory(...), "cordis.patch.yml") and produced a cwd-relative patch outside the client-selected home, while setup still reported ready. Test results from issue #2666:

case	setup target and created patch	client-selected home patch
`empty`	`workspace/cordis.patch.yml`	`home/.dsh/cordis.patch.yml`
`unset`	`home/.dsh/cordis.patch.yml`	`home/.dsh/cordis.patch.yml`
`explicit`	`profile/cordis.patch.yml`	`profile/cordis.patch.yml`

The fix aligns REA with the released DSH resolver: empty and whitespace-only overrides fall back to ~/.dsh, padded overrides are trimmed, non-empty overrides keep their original behavior.

Change and scope

src/application/SupportedClients.ts dshHomeDirectory: read env.DSH_HOME?.trim(), fall back to join(home, ".dsh") when the trimmed value is empty or unset.
- Mirrors openCodeOverrideDirectory's documented empty-as-unset pattern (OPENCODE_CONFIG_DIR === "").
- Matches the released @deepseek-ai/dsh-home-paths resolver behavior.
src/application/SupportedClients.test.ts (new): focused vitest covering unset, explicit, empty, whitespace-only, and padded overrides via the public supportedClients(...) entrypoint.

Out of scope: install-side enforcement of DSH_HOME (already handled by Harness), the unrelated #2168 / #2325 / #2149 follow-ups, and other client overrides referenced by installation.md.

Contract and boundary impact

Semantic owner and earliest changed stage: setup path resolution; first changed stage is dshHomeDirectory and its callers.
Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries — not applicable.
CLI and MCP/tool-catalog contract: no API or tool shape change; dsh SetupClient.configPath / markerPath now match the released DSH resolver.
Provider, bridge, target-format, or platform compatibility: no provider, bridge, target-format, or platform change.
Evidence, artifact, provenance, or reconstruction contract: no change.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: not regenerated by this fix. Existing product-catalog CI validates any tooling-touching change separately.

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.
Documentation or usability only; no target interaction. (Setup writes to a different patch path on empty override; no target execution.)
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated: new src/application/SupportedClients.test.ts with five cases (unset, explicit absolute, empty, whitespace-only, padded) targeting the public supportedClients(...) entrypoint.
Base reproduction or other evidence: the reproduction script in #2666 (Python harness calling node scripts/rea.mjs setup ... for three cases) reproduces the failure on the bug revision. The new test covers the exact regression at the source level; the full Python reproduction needs Linux + a built rea.mjs and is left for CI's real-provider lane.
User-visible CLI/MCP output: an empty DSH_HOME will now produce ~/.dsh/cordis.patch.yml rather than ./cordis.patch.yml. Setup's reported readiness will continue to be ready against the correct client-selected patch.
Remaining proof gaps: full Python reproduction was not run in this fork (no built native artifacts, no Harness install); relies on the existing CI lane that the maintainers can run.

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

npm ci and npm run check:fast were intended locally, but the development environment in this fork lacked the full toolchain; relied on inspection and the new unit tests under vitest. Parent CI's npm ci + npm run check + DSH-related lanes are the source of truth.
The provided reproduction in #2666 was not re-executed end-to-end here because the Python harness requires python3 + a built scripts/rea.mjs + Linux mitmproxy/DSH selection; that lane is left to the maintainers' verified reproduction.

Compatibility, safety, and release

Breaking changes or migration steps: none. Empty DSH_HOME previously created an off-repo patch; after the fix it lands at the client-selected home. A user with the off-repo patch leftover from before the fix can remove the stray cordis.patch.yml once setup successfully registers at the right path.
Real Hopper/Ghidra, browser, or OS coverage: not applicable — change is in TS path resolution, no provider or platform behavior touched.
Package or release metadata impact: none at the metadata level. Patch-level version bump is driven by Release Please as usual.
Security, privacy, process, or containment review: no change to process boundaries, no new external tools, no credentials touched. Empty override no longer leaves a registration in cwd, which is a containment plus.

Before merge

This PR addresses a concrete problem (issue #2666) with a matching reproduction.
Material scope and authority questions are resolved within existing boundaries.
The PR has one focused outcome; title follows type(scope): outcome.
Related issue is linked (Fixes #2666).
Tests cover changed observable behavior and the documented regression paths.
Owning docs are not changed — behavior now matches docs/client-configuration.md:204-211.
User-visible CLI/MCP changes — relies on parent CI / maintainer-verified reproduction for the off-repo artifact; left unchecked only because the local development toolchain could not build the harness here.
Final diff checked 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/2686 **Author:** [@rollroyces](https://github.com/rollroyces) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/setup-empty-dsh-home` --- ### 📝 Commits (4) - [`4ef8c42`](https://github.com/morluto/rea/commit/4ef8c42f49f27b32e77b61b785b7321755ebc06e) fix(setup): treat empty DSH_HOME as unset (#2666) - [`181e712`](https://github.com/morluto/rea/commit/181e712d6df0ea98723035579e07084b58bac8f9) fix(setup): preserve portable DSH paths and public empty-home coverage - [`7b54cee`](https://github.com/morluto/rea/commit/7b54cee8cec1cc49a42fc4bd5c640b354495b048) fix(setup): reconcile DSH regression coverage with current path ownership - [`86e73fc`](https://github.com/morluto/rea/commit/86e73fcaef12541b3f02d3e21f6d8e18714534c5) test(setup): remove superseded DSH path unit coverage ### 📊 Changes **1 file changed** (+1 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/application/SupportedClients.ts` (+1 -1) </details> ### 📄 Description ## Summary Fixes #2666 — `rea setup --client dsh --skill=false --yes --json` creates `cordis.patch.yml` in the working directory when `DSH_HOME` is set to an empty string. The released `@deepseek-ai/dsh-home-paths` package already treats an empty/whitespace-only `DSH_HOME` as unset; REA's resolver did not. ## Problem and expected behavior `SupportedClients.ts` `dshHomeDirectory` used nullish coalescing: ```ts const dshHomeDirectory = ({ home, env }: ClientPathContext): string => env.DSH_HOME ?? join(home, ".dsh"); ``` An empty `DSH_HOME` is a non-nullish string, so it leaked through `join(dshHomeDirectory(...), "cordis.patch.yml")` and produced a cwd-relative patch outside the client-selected home, while setup still reported `ready`. Test results from issue #2666: | case | setup target and created patch | client-selected home patch | | --- | --- | --- | | `empty` | `workspace/cordis.patch.yml` | `home/.dsh/cordis.patch.yml` | | `unset` | `home/.dsh/cordis.patch.yml` | `home/.dsh/cordis.patch.yml` | | `explicit` | `profile/cordis.patch.yml` | `profile/cordis.patch.yml` | The fix aligns REA with the released DSH resolver: empty and whitespace-only overrides fall back to `~/.dsh`, padded overrides are trimmed, non-empty overrides keep their original behavior. ## Change and scope - `src/application/SupportedClients.ts` `dshHomeDirectory`: read `env.DSH_HOME?.trim()`, fall back to `join(home, ".dsh")` when the trimmed value is empty or unset. - Mirrors `openCodeOverrideDirectory`'s documented empty-as-unset pattern (`OPENCODE_CONFIG_DIR === ""`). - Matches the released `@deepseek-ai/dsh-home-paths` resolver behavior. - `src/application/SupportedClients.test.ts` (new): focused vitest covering unset, explicit, empty, whitespace-only, and padded overrides via the public `supportedClients(...)` entrypoint. Out of scope: install-side enforcement of `DSH_HOME` (already handled by Harness), the unrelated #2168 / #2325 / #2149 follow-ups, and other client overrides referenced by `installation.md`. ## Contract and boundary impact - **Semantic owner and earliest changed stage**: setup path resolution; first changed stage is `dshHomeDirectory` and its callers. - **Contribution scope fit and linked maintainer decision**: ordinary fix within existing boundaries — not applicable. - **CLI and MCP/tool-catalog contract**: no API or tool shape change; `dsh` `SetupClient.configPath` / `markerPath` now match the released DSH resolver. - **Provider, bridge, target-format, or platform compatibility**: no provider, bridge, target-format, or platform change. - **Evidence, artifact, provenance, or reconstruction contract**: no change. - **Target selection, authority, upstream behavior, and cleanup details**: not applicable. - **Generated metadata (`docs/public/product-catalog.json`), package, or installation impact**: not regenerated by this fix. Existing product-catalog CI validates any tooling-touching change separately. Expected effects: - [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. - [x] Documentation or usability only; no target interaction. (Setup writes to a different patch path on empty override; no target execution.) - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - **Tests added or updated**: new `src/application/SupportedClients.test.ts` with five cases (unset, explicit absolute, empty, whitespace-only, padded) targeting the public `supportedClients(...)` entrypoint. - **Base reproduction or other evidence**: the reproduction script in #2666 (Python harness calling `node scripts/rea.mjs setup ...` for three cases) reproduces the failure on the bug revision. The new test covers the exact regression at the source level; the full Python reproduction needs Linux + a built `rea.mjs` and is left for CI's real-provider lane. - **User-visible CLI/MCP output**: an empty `DSH_HOME` will now produce `~/.dsh/cordis.patch.yml` rather than `./cordis.patch.yml`. Setup's reported readiness will continue to be `ready` against the correct client-selected patch. - **Remaining proof gaps**: full Python reproduction was not run in this fork (no built native artifacts, no Harness install); relies on the existing CI lane that the maintainers can run. 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 - `npm ci` and `npm run check:fast` were intended locally, but the development environment in this fork lacked the full toolchain; relied on inspection and the new unit tests under `vitest`. **Parent CI's `npm ci` + `npm run check` + DSH-related lanes are the source of truth.** - The provided reproduction in #2666 was not re-executed end-to-end here because the Python harness requires `python3` + a built `scripts/rea.mjs` + Linux mitmproxy/DSH selection; that lane is left to the maintainers' verified reproduction. ## Compatibility, safety, and release - **Breaking changes or migration steps**: none. Empty `DSH_HOME` previously created an off-repo patch; after the fix it lands at the client-selected home. A user with the off-repo patch leftover from before the fix can remove the stray `cordis.patch.yml` once setup successfully registers at the right path. - **Real Hopper/Ghidra, browser, or OS coverage**: not applicable — change is in TS path resolution, no provider or platform behavior touched. - **Package or release metadata impact**: none at the metadata level. Patch-level version bump is driven by Release Please as usual. - **Security, privacy, process, or containment review**: no change to process boundaries, no new external tools, no credentials touched. Empty override no longer leaves a registration in cwd, which is a containment plus. ## Before merge - [x] This PR addresses a concrete problem (issue #2666) with a matching reproduction. - [x] Material scope and authority questions are resolved within existing boundaries. - [x] The PR has one focused outcome; title follows `type(scope): outcome`. - [x] Related issue is linked (`Fixes #2666`). - [x] Tests cover changed observable behavior and the documented regression paths. - [x] Owning docs are not changed — behavior now matches `docs/client-configuration.md:204-211`. - [ ] User-visible CLI/MCP changes — relies on parent CI / maintainer-verified reproduction for the off-repo artifact; left unchecked only because the local development toolchain could not build the harness here. - [x] Final diff checked 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:39 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #5311] fix(setup): register aube's bin shim instead of its versioned store path #6873

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

No description provided.

Rows
Columns