Conversation
Teingi
left a comment
Teingi
left a comment
There was a problem hiding this comment.
One transport-validation issue reproduced with the real DSH 0.1.2-rc.1 installer; details inline.
| const layers = bundles.map(name => { | ||
| if (name === plugin && candidate) return bundle(candidate) | ||
| const packageDir = read(manifestPath, () => boot.resolveBundleDir('powercontext', name, anchor, dir)) | ||
| return bundle(packageDir) | ||
| }) | ||
| if (!bundles.includes(plugin)) { | ||
| if (candidate) layers.push(bundle(candidate)) |
There was a problem hiding this comment.
[P2] Validate the bundle stack produced by DSH reconciliation
This projection assumes that dsh plugin add only replaces or appends PowerContext, but DSH reconciles every installed dependency and enables packages declaring dsh.bundle, including those absent from the current bundle list.
With real DSH 0.1.2-rc.1, an existing PowerContext bundle, another installed but inactive bundle overriding its baseUrl, and a nonempty disabled: false profile patch, I reproduced setup dsh --server-url https://selected.example --json returning success. The installer activated the other bundle, so the resulting configuration used https://unexpected.example while clients.json still saved https://selected.example.
Please account for the full native reconciliation when validating the prospective configuration, so successful setup preserves the selected endpoint and HTTP consent.
There was a problem hiding this comment.
Thank you for the detailed reproduction. You were right: the preflight missed inactive dependencies enabled by native reconciliation. I’ve updated it to validate the complete resulting bundle stack and added real DSH regressions for endpoint and HTTP-consent overrides. Setup now also verifies the installed configuration before saving connection settings or credentials.
19e2410 to
b06a35e
Compare
Teingi
left a comment
Teingi
left a comment
There was a problem hiding this comment.
The earlier inactive-dependency reconciliation issue is fixed. A named group can still bypass transport validation, and there is a smaller DSH_HOME path regression. Details inline.
| } | ||
| matches.push(row) | ||
| } | ||
| if (row.group && Array.isArray(row.config)) visit(row.config, unavailable) |
There was a problem hiding this comment.
[P2] Traverse named groups when validating PowerContext entries
Native DSH also accepts name: "@deepseek-ai/cordis-plugin-group" without group: true, so this traversal skips valid group children. With a normal PowerContext entry plus a second one inside such a group, I reproduced setup succeeding and saving https://selected.example, while real DSH 0.1.2-rc.1 loaded both the selected and https://unexpected.example configurations (verified using an instrumented plugin). Both preflight and post-install readback miss the ambiguity. Please traverse every group form supported by the selected host and reject multiple PowerContext entries.
There was a problem hiding this comment.
The shared traversal now recognizes both group: true and name: '@deepseek-ai/cordis-plugin-group', including nested groups. Preflight and post-install validation use the same traversal, so duplicate PowerContext entries are rejected before connection settings or credentials are saved. Added native configuration regressions and a real DSH setup acceptance case for named-group duplicates. Verified in DSH CI.
There was a problem hiding this comment.
已独立验证修复，并确认这条回归护栏真的能检出修复前的行为。
我在隔离副本里把 visit() 的遍历条件退回修复前的 row.group && Array.isArray(row.config)，用真实 @deepseek-ai/dsh-app-boot@0.2.0-rc.2（npm 安装真实包，非合成物）跑本文件的 group 用例：
FAILED test_setup_rejects_duplicate_powercontext_inside_native_groups[False-group1-dsh_native_api_profile]
FAILED test_setup_rejects_duplicate_powercontext_inside_native_groups[True-group1-dsh_native_api_profile]
FAILED test_a_single_powercontext_inside_a_named_group_is_inspected[dsh_native_api_profile]
3 failed, 2 passed, 6 skipped
失败信息正是你当时看到的那一类：The composed DSH profile does not contain PowerContext。对照之下 group0（group: true）两条在变异下仍通过，说明 || row.name === '@deepseek-ai/cordis-plugin-group' 这一支正是命名组形态所必需的，且不是冗余判断。
补充一个可直接复现的形状确认：真实 0.2.0-rc.2 的 composeEntries 对 {name: '@deepseek-ai/cordis-plugin-group', config: [...]} 返回的行不带 group 字段，config 保持为子行数组 —— 所以只看 row.group 必然漏掉该子树。
在未变异的 head 上，装好 tests/config-runtime 依赖后：20 passed, 23 skipped（23 条是需要真实 DSH CLI 的 dsh_profile 变体）。这条已可关闭。
|
|
||
| def dsh_home() -> Path: | ||
| """Use the same home for profile configuration and URL-bound credentials.""" | ||
| return Path(os.environ.get("DSH_HOME", "").strip() or Path.home() / ".dsh").expanduser() |
There was a problem hiding this comment.
[P3] Preserve nonblank DSH_HOME paths verbatim
For a valid POSIX path such as DSH_HOME='/data/dsh ', native DSH preserves the trailing space, but this helper changes the path to /data/dsh. With real DSH 0.1.2-rc.1, the same public setup succeeds on the base commit; here plugin add succeeds, then readback fails with PowerContext is not enabled in this profile and no client settings are saved. Use trimming only to detect an empty value, and preserve a nonblank path when resolving the home.
There was a problem hiding this comment.
DSH_HOME is now trimmed only to detect blank values; nonblank paths retain their original spaces in the CLI, credential storage/readback, and plugin peer lookup. Saved-credential readback also uses the existing URL normalization helper, preserving endpoint binding. Fresh and repeated setup with a literal trailing-space POSIX home, plus credential readback, passed in DSH CI.
There was a problem hiding this comment.
已独立验证修复，并确认它有不依赖 DSH 运行时的回归护栏。
src/powercontext/cli/dsh_runtime.py 现在的语义是「只把 strip 结果用于判空，非空则原样保留」：
configured = os.environ.get("DSH_HOME", "")
return Path(configured if configured.strip() else Path.home() / ".dsh").expanduser()/data/dsh （尾空格）、/data/dsh（首尾空格）、relative 、 relative 都保留原值；未设置、空串、纯空白回落到 ~/.dsh，与你在真实 0.1.2-rc.1 上观测到的 host 行为一致。三个消费点也统一走同一函数：cli/authorization.py 的 credential_path("dsh")、cli/dsh_transport.py 的 read_dsh_settings()、以及插件侧的 peer 查找。
新增的 tests/test_authorization.py::test_dsh_credential_home_preserves_nonblank_path_whitespace 与 test_dsh_credentials_and_configuration_share_the_same_home 是无条件用例（纯 tmp_path，不需要 DSH），在本机 9 passed；除尾空格那条按 POSIX 语义在 Windows 跳过外，其余在 Python 矩阵里都会跑。这点比放进 test_dsh_transport.py 更稳妥，建议保留这个位置。
|
@Teingi Both review findings are fixed in All 24 checks on Could you please review the updated PR again? The formal re-review request endpoint is unavailable to this account, so I am requesting re-review here. |
| if not executable: | ||
| if request.config.getoption("--require-dsh-runtime"): | ||
| pytest.fail("Required DSH runtime is unavailable; set DSH_TEST_EXECUTABLE") | ||
| pytest.skip("Native DSH composition requires DSH; set DSH_TEST_EXECUTABLE to select a runtime") |
There was a problem hiding this comment.
[P2] DSH 传输校验现在只有装好宿主运行时才可见，主测试矩阵里 33/43 条会静默跳过
--require-dsh-runtime 的设计本身是对的（缺运行时会 pytest.fail 而不是静默 skip，我实测过：本机无 DSH 时 10 failed, 23 errors，门禁确实是 fail-closed）。问题在覆盖位置：
- 不带任何 flag 跑
tests/test_dsh_transport.py：10 passed, 33 skipped。 - 只装
tests/config-runtime依赖、没有真实 DSH CLI：20 passed, 23 skipped—— 也就是说dsh_native_api_profile那一半只需要那个 npm fixture，不需要 DSH 安装。 - 目前唯一带
--require-dsh-runtime的步骤在dsh-package（仅 ubuntu）。而tests (3.11)–(3.14)矩阵按testpaths=["tests"]收集该文件时，两个运行时都不可用，于是 33 条一律 skip。
同时，本 PR 删掉的 4 个无条件用例（纯 tmp_path，全平台必跑）在 head 上没有任何同名或等价替代：
test_dsh_runtime_overlays_are_not_assumed_to_use_loopback（3 例）test_dsh_empty_generated_overlay_does_not_hide_saved_transport（3 例）test_dsh_setup_preflight_accepts_inert_defaults_and_requires_manual_custom_configuration（2 例）test_dsh_setup_checks_the_web_profile_even_with_another_runtime_profile（1 例）
它们所固定的场景现在只由运行时条件用例覆盖。我理解其中一部分语义是本 PR 有意替换的（preflight 从「人工核对」改为真实 composition），但覆盖从「矩阵 + 开发者本机全跑」退化为「只有 dsh-package 跑」，是这次改动带来的实质变化。
建议：把 pnpm --dir .../tests/config-runtime install --frozen-lockfile 与 DSH_TEST_CONFIG_BOOT 的解析也放进主测试 job（或单开一个不依赖 DSH CLI 的 job），让需要真实 CLI 的那 23 条继续只在 dsh-package 跑。这样不改断言、不新增依赖类型，就能让 20 条原生配置用例回到每次 push 都执行的集合里。
另外提一句与本 PR 无冲突但需要协调的事：本 PR 的两个 usage 提交（310cf21c、2e3fdd57）修改的 tests/builtin/runtime/test_model_usage_recorder.py 与仍然开放的 #1838 改的是同一个 flaky 缺陷（1835 的 tests (3.14) 就是挂在 test_queue_capacity_drops_without_sql_or_sensitive_logs 上）。建议在两处互相引用，或把 usage 那两个提交拆成独立 PR，避免同一缺陷在两处各修一半。
There was a problem hiding this comment.
Thank you for independently verifying the fixes and pointing out the coverage gap. The Python 3.11–3.14 unit matrix now installs and resolves the pinned native configuration package and requires it with --require-dsh-config-runtime; missing APIs fail instead of skipping. CLI-specific tests remain required in dsh-package, and the existing regression assertions are unchanged.
I also linked #1838, which is now merged and addresses recorder attempt expiry. This PR's usage changes remain limited to test synchronization and explicit write/flush budgets.
Verified on the updated head in CI.
Teingi
left a comment
Teingi
left a comment
There was a problem hiding this comment.
The earlier reconciliation, named-group, and DSH_HOME issues are fixed. One remaining native-include validation gap is detailed inline.
| if ((row.group || row.name === '@deepseek-ai/cordis-plugin-group') && Array.isArray(row.config)) { | ||
| visit(row.config, unavailable) | ||
| } |
There was a problem hiding this comment.
[P2] Inspect or reject native include trees before accepting setup
This traversal skips @deepseek-ai/cordis-plugin-include, DSH's native YAML/JSON loader. With a top-level PowerContext entry and an include file containing another powercontext-dsh entry at https://unexpected.example, setup dsh --source <fixture> --server-url https://selected.example --json succeeds and saves the selected URL. On real DSH 0.1.2-rc.1, the recording plugin fixture receives both endpoint configurations at startup. Both preflight and installed readback miss the second instance.
Please inspect native include trees using their path/patch semantics, or reject configurations that cannot be verified before accepting setup.
There was a problem hiding this comment.
Thank you for the reproduction. Fixed in 32f2357f: preflight and installed readback inspect native include trees using DSH's parser, path rules and include patches, rejecting unverifiable trees before saving settings or credentials. Real DSH startup tests cover the duplicate endpoints and successful UI/model includes.
This branch has not been deployed
3 participants
Which issue or RFC does this PR close?
Closes #1798. Supplies the shared DSH configuration and reconciliation behavior required by #1807.
Rationale for this change
powercontext setup dshmust validate the configuration DSH will actually load before saving an endpoint or HTTP consent. Ordinary UI/model customization should remain usable, while a second PowerContext instance or an unverifiable transport configuration must fail.What changes are included in this PR?
DSH_HOMEpaths including surrounding spaces. Normalize saved-credential URLs through the shared policy so matching credentials can be read back.Are there any user-facing changes?
Unrelated UI/model patches and verifiable native includes allow setup. Conflicting endpoints, HTTP consent and ambiguous plugin instances fail before installation. Configuration drift introduced during installation fails the readback check before saving new settings or credentials.
Include files must exist and have a statically inspectable tree. Include paths use the selected profile as their outer base and the containing file's directory for nested includes. Setup inspects preserved expressions without evaluating them. Native package installation is not automatically rolled back after a failed readback; running-session environment and extra
--patcharguments are checked with/pc doctor.How was this change tested?
Validated commit:
32f2357ff61fc7f3d97950a973d7cee73d2c24a0.The 0.2.0-rc.2 tests execute its actual configuration APIs through a synthetic carrier, and post-install drift is injected at the installation boundary. The recording-plugin startup establishes native include behavior; it does not establish full Desktop or 0.2 host acceptance.
Commands run:
From
integrations/dsh/plugins/powercontext/tests/runtime:node --test setup.test.mjs.AI usage statement
OpenAI Codex assisted with implementation, independent diff review, native-source inspection, documentation and tests. Actual native startup/configuration and HTTP execution are distinguished from synthetic carriers and injected installation boundaries.