Repository navigation
feat(dsh): support Desktop setup with profile selection - #1807
Conversation
AlexStocks
left a comment
There was a problem hiding this comment.
Reviewed against head 9e626eb3 (base 9db17534, 27 files, +1526/−155). The Desktop profile work itself is careful — preflight ordering, preserved user patches, and the new reconciliation validation are all solid. Requesting changes for three items, two of which are unchanged from my earlier pass at 08055f9c.
Blocking
1. The CI step that enables the dsh tests fails open
.github/workflows/master.yml:186:
export DSH_TEST_EXECUTABLE="$(node --input-type=module -e "import { dshBin } from './integrations/dsh/plugins/powercontext/tests/runtime/fixture.mjs'; process.stdout.write(dshBin)")"export is a shell builtin and always returns 0, so the non-zero exit of the command substitution is swallowed. Reproduced:
$ bash -ec 'export DSH_TEST_EXECUTABLE="$(node --input-type=module -e "import { dshBin } from \"./nonexistent-fixture.mjs\"; process.stdout.write(dshBin)")"; echo "exit=$? value=[$DSH_TEST_EXECUTABLE]"'
Node.js v22.22.2
exit=0 value=[]
The step is green and the variable is empty. dshBin depends on sdkRequire.resolve('@deepseek-ai/dsh/package.json') inside fixture.mjs:32, so any change to the runtime install layout silently turns this into "skip every dsh test" while the job stays green — exactly the failure mode this PR's own commit 9e626eb3 is trying to add validation for.
Fix: make the resolution its own step that fails the job, and assert non-empty before use:
- name: Resolve dsh test executable
run: |
bin="$(node --input-type=module -e "import { dshBin } from './integrations/dsh/plugins/powercontext/tests/runtime/fixture.mjs'; process.stdout.write(dshBin)")"
test -n "$bin" || { echo "dshBin unresolved"; exit 1; }
echo "DSH_TEST_EXECUTABLE=$bin" >> "$GITHUB_ENV"2. All three fix commits are duplicated with open PR #1799
diff <(git show A --format="") <(git show B --format=""):
| #1807 | #1799 | patch |
|---|---|---|
f4ff2e0c |
c1e934e6 |
byte-identical |
08055f9c |
c5ecc9b5 |
byte-identical |
9e626eb3 |
19e2410f |
same change, different context (profile param vs DSH_PROFILE) |
#1799 is titled fix(dsh): preserve unrelated user patches during setup and is still open. Whichever merges second will either conflict or silently re-apply. Please drop these from one of the two PRs (I'd keep the fixes in #1799 and leave #1807 focused on profile selection) and rebase.
3. @deepseek-ai/dsh-app-boot is imported but never declared
Introduced by f4ff2e0c at src/powercontext/cli/dsh_config.mjs:73:
const boot = await import(pathToFileURL(require.resolve('@deepseek-ai/dsh-app-boot')).href)Grepping the tree, the only occurrences outside lockfiles are that single require.resolve. Neither integrations/dsh/plugins/powercontext/package.json (which does declare nine @deepseek-ai/* peer deps) nor tests/runtime/package.json (which pins @deepseek-ai/dsh-sdk-client: 0.1.2-rc.1) mentions it; it only appears in tests/runtime/pnpm-lock.yaml as a transitive entry. So the module resolves today by luck of the transitive graph, with no declared version range.
For contrast, #1784 does this correctly: @deepseek-ai/dsh-mcp-client is an optional peerDependency in the plugin manifest and pinned in the runtime lock.
Fix: add @deepseek-ai/dsh-app-boot to the same place as the other @deepseek-ai/* peers with an explicit range, and pin it in the runtime lock.
Should fix
4. Coverage moved to a job that almost never exercises it
tests/test_dsh_transport.py on this head: 5 passed, 20 skipped locally (was 5/18 at 08055f9c; the +64 lines added by 9e626eb3 are also dsh-gated). The 23 tests are only really executed by the dsh-package job, which is the one job that depends on the fail-open variable from item 1. Meanwhile the PR deletes four unconditional tests that needed nothing but tmp_path. Net effect: the same assertions are now behind two gates instead of zero.
5. doctor dsh --profile desktop --json can emit a payload without plugin
src/powercontext/cli/system.py:1101-1103:
except SetupError as error:
diagnostics = {"dsh": Diagnostic(status=DiagnosticStatus.FAILED, detail=str(error))}Every other doctor subcommand returns a fixed key set, and the non---json rendering tolerates a missing key. A --json consumer that reads diagnostics["plugin"] gets a KeyError on exactly the path this PR adds (--profile desktop → resolve_dsh_target). Suggest seeding plugin with DiagnosticStatus.SKIPPED here, as run_dsh_diagnostics already does at dsh.py:179.
6. Unrelated plugin incompatibility aborts setup
dsh_config.mjs:142-145 calls evaluatePluginCompatibility for every bundle in the profile and fail(...)s on the first non-exempt issue. One third-party bundle with an unresolved version conflict is enough to block installing PowerContext, which is the opposite of what f4ff2e0c ("preserve unrelated user patches during setup") is going for. Suggest restricting the hard failure to the PowerContext bundle and downgrading others to a warning.
7. DSH_HOME is derived in three places with divergent semantics
cli/authorization.py:84—os.environ.get("DSH_HOME", Path.home() / ".dsh")(no.strip(), soDSH_HOME=""yieldsPath(""))cli/dsh_runtime.py:31—os.environ.get("DSH_HOME", "").strip() or Path.home() / ".dsh"cli/dsh_transport.py:41— same as above
Three copies of the same rule, one of which behaves differently on an empty value. Suggest a single helper (the profile-aware path now has a fourth consumer in this PR).
Verified as fine — do not re-check
assert "fully quit" in result.outputis reliable. typer 0.27.0 routes CLI output throughStreamMixer, which merges stdout and stderr intoresult.output, so stderr copy is captured.result.stdoutis the stdout-only view.- 24/24 CI green on this head, web profile unaffected.
9e626eb3genuinely tightens the reconciliation path (+64 test lines, +53 indsh_config.mjs); the concern is only that its CI gate (item 1) cannot fail.
9e626eb to
7999409
Compare
|
Thanks, @AlexStocks, for the detailed review. The changes are pushed in #1799 at
Native DSH installation/readback regressions pass. Compatibility tests exercise the real pinned 0.2 APIs through a synthetic carrier with a mocked installation boundary; they do not establish running Desktop acceptance. Validation status and the Windows SDK-context failure, which also reproduces without the Desktop changes, are documented in both PR descriptions. |
7999409 to
66afead
Compare
66afead to
212fa45
Compare
212fa45 to
ad63c62
Compare
Which issue or RFC does this PR close?
Closes #1804.
Rationale for this change
Desktop has separate dependencies and activation, and its reserved profile must be managed by the Desktop-installed CLI. Setup needs to inspect the selected host and profile before installation and saving connection settings.
What changes are included in this PR?
The PR contains Desktop-specific changes on master 1db8f3b. Shared native DSH setup, group/alias/include handling, home-directory and URL-bound credential behavior are supplied by master.
Are there any user-facing changes?
Open Desktop once to initialize its profile, register the launcher with Manage dsh Command…, fully quit the application before setup, and reopen afterward. Closing the window may only hide it. Use --dsh-command when the installed launcher is absent from PATH. Packaged Windows/macOS launcher layouts are supported.
JSON setup output includes profile and profile_dir. Saved hosts.dsh settings and DSH-home credentials are shared; use the same Server for both profiles. Standalone Doctor checks registration/configuration; use /pc doctor in a running session for readiness.
Transport conflicts fail before installation. Unexpected post-install configuration fails before saving connection settings or credentials; native package installation is not automatically rolled back.
How was this change tested?
Validated head: ad63c62.
Windows focused CLI/profile/setup/transport/authorization checks with the required pinned native DSH runtimes: 359 passed across focused executions, 4 filesystem/permission skips. The unchanged Claude statusline assertion requiring POSIX path separators is excluded on Windows.
Real pinned DSH 0.1.2-rc.1 setup suite: 7 passed, 1 Windows filesystem skip. It executes the public CLI and native DSH configuration in temporary homes, including repeated setup, group/include trees, inactive bundles and endpoint-bound HTTP refusal.
Native 0.2 configuration tests use dsh-app-boot 0.2.0-rc.2 through a synthetic carrier and mocked installation boundary. Desktop launcher tests use filesystem fixtures.
Whole-project Linux-target types, Windows types for the changed files, Ruff, all non-type pre-commit hooks and diff checks passed.
All 25 current-head CI checks passed, including the Python matrix and real DSH package/runtime acceptance, SQLite/OceanBase acceptance, website and native service checks.
uv run --locked python -m pytest -q tests/test_dsh_profiles.py tests/test_dsh_cli.py tests/test_dsh_transport.py tests/test_setup_transport.py tests/test_system_cli.py tests/test_authorization.py --require-dsh-runtime -k "not test_setup_claude_code_refreshes_an_existing_powercontext_statusline"
node --test integrations/dsh/plugins/powercontext/tests/runtime/setup.test.mjs
uv run --locked ty check --python-platform linux
Native fixture execution verifies Web configuration/setup. These checks do not establish real Desktop application execution, native Windows SDK conversation acceptance, macOS application execution or external-model acceptance.
AI usage statement
OpenAI Codex assisted with implementation, native source investigation, regression tests, validation, documentation and PR preparation. Open Code Review delegation resolved review file selection and rules; Codex reviewed the final diff, including tests and documentation.