fix(review): 按独立 review 结论修正 PR C 的注释、文档与用例保真度
独立 review(对照606339f/4099700静态复核 + 日志逐行核对)提出的问题,逐条修正: 1. **补回被我自己抹掉的源码注释**:上次验证「未修复代码上 4/4 红」时用了 `git checkout606339f-- src/client/native/index.ts`,随后 `git checkout HEAD -- <同一文件>` 把**尚未提交**的 `disposeSafely` 注释一起还原掉了 —— PR 描述里承诺的 「说明释放失败为何只走 console.error」因此在 diff 里根本不存在。已重新落盘。 2. **事故文档三处事实修正**:同一毫秒的 `already registered` 是 **7 条**(.339 一条 + .340 六条,合计 burst 9 条),不是 6;验证段的「114 files / 1136 passed」是修复分支 (base 早于 #781)的数字,补上口径并给出合入 main 后的 122/1292;`index.ts:353` 标注 为 v0.22.0 行号(main 上因 #777 顺移)。 3. **撤回一处措辞过头**:「部分回滚分支从未被执行」→ 回收集合虽然进了、但 disposer 列表恒为空(释放动作未执行),改后能真正覆盖。 4. **e2e 门不再可能静默通过**:`test.skip(CLIENT_JS === undefined, …)` 改成硬失败—— 这个门的价值就在于 harness 以后不再导出 DSH_HOME 时必须报错而不是变成 skip。 同时把「无 `[data-sidebar-right-unavailable]`」移到文件渲染断言**之后**(前者在空 pane 上恒真,不是真空但也没重量)。 5. **测试替身补齐保真度**:假注册表的 disposer 改为幂等(真宿主的 disposer 是幂等的 ——cordis effect 二次释放返回同一个 task),避免「二次 release 又被记一笔」这种与 真实宿主不一致的行为被后续用例当成事实。 6. **AGENTS §2 的 HMR 机制加版本限定**:`mtime/ctime/size` 的 sha1 rev 是**钉住的宿主 0.1.7-rc.1** 的行为;DSH 源码树新版本改成只比 `mtime+size` 并对内容取 sha1,pin 上调 时该触发器必须同步复核(否则会红在替换断言上,而不是静默失效)。 验证:typecheck / lint 通过;pnpm test 122 files / 1292 passed / 9 skipped(重启前机器上有 两个 scratch dsh 服务时的 fs-watch 超时是负载抖动,单独跑 5/5 绿、清空负载后全量绿)。
This commit is contained in:
@@ -17,7 +17,7 @@
|
||||
|
||||
## 2. CI 挂载冒烟(`plugin-mount` job / `pnpm test:mount`)
|
||||
|
||||
「npm 打包 → 真实挂载 → 无头渲染」门禁(证明打包产物在真实 DSH 挂载后不 crash):`pnpm build && pnpm pack` 产 tarball → `scripts/e2e-mount.sh` 装进全新 scratch profile(`dsh plugin --profile web add <tarball>`)并启动真实 `dsh web`(keyless,`--port 0`)→ `tests/e2e/mount.e2e.ts`(Playwright)断言 `[data-dsh-better-sidebar]` 挂载、无错误条/pageerror/console 错误,展开 DSH 原生右侧栏后经其 guide 页逐个打开本插件贡献的 tab 类型**以及宿主自己的终端条目**(必须恰好 1 条——多于 1 条即意味着插件又在遮蔽宿主),再经插件文件树(原生 `files` kind 接管)打开 seed 文件强制加载 editor chunk(`client-editor.js`),并跑 mermaid / README 预览与 sidechat 宿主路由烟测。**`tests/e2e/native-reload.e2e.ts` 是同一 lane 里的第二条门**:它 `utimesSync` 已安装的 `lib/client.js`(宿主 HMR 的 500ms stat-poll 以 `mtime/ctime/size` 的 sha1 作 rev)触发**页面内条目替换**,断言替换前后都无 `native register … error`、无诊断条、且 `files` 接管重新可用——这是唯一能在部署层复现「teardown 期间注册把 id 孤儿化」(见 §3.4 第 9 条)的用例,v0.22.0 上实测 3/3 红、修复后 3/3 绿。**`browser` 不在这个清单里,而是显式断言缺席**:DSH 0.1.7 的 `ui-sidebar-browser` 只在 desktop profile 挂载(`web-app` 的 patch 里 `disabled: ctx.get('profileContext')?.name !== 'desktop'`),lane 全走 web profile,钉住「没有」比默默放过更能暴露宿主立场的变化。
|
||||
「npm 打包 → 真实挂载 → 无头渲染」门禁(证明打包产物在真实 DSH 挂载后不 crash):`pnpm build && pnpm pack` 产 tarball → `scripts/e2e-mount.sh` 装进全新 scratch profile(`dsh plugin --profile web add <tarball>`)并启动真实 `dsh web`(keyless,`--port 0`)→ `tests/e2e/mount.e2e.ts`(Playwright)断言 `[data-dsh-better-sidebar]` 挂载、无错误条/pageerror/console 错误,展开 DSH 原生右侧栏后经其 guide 页逐个打开本插件贡献的 tab 类型**以及宿主自己的终端条目**(必须恰好 1 条——多于 1 条即意味着插件又在遮蔽宿主),再经插件文件树(原生 `files` kind 接管)打开 seed 文件强制加载 editor chunk(`client-editor.js`),并跑 mermaid / README 预览与 sidechat 宿主路由烟测。**`tests/e2e/native-reload.e2e.ts` 是同一 lane 里的第二条门**:它 `utimesSync` 已安装的 `lib/client.js`(**在钉住的宿主 0.1.7-rc.1 上**:HMR 每 500ms stat-poll,rev = `mtime/ctime/size` 的 sha1;DSH 源码树的新版本改成只比 `mtime+size` 并对**内容**取 sha1——pin 上调时这个触发器必须同步复核,否则该用例会红在替换断言上而不是静默失效)触发**页面内条目替换**,断言替换前后都无 `native register … error`、无诊断条、且 `files` 接管重新可用——这是唯一能在部署层复现「teardown 期间注册把 id 孤儿化」(见 §3.4 第 9 条)的用例,v0.22.0 上实测 3/3 红、修复后 3/3 绿。**`browser` 不在这个清单里,而是显式断言缺席**:DSH 0.1.7 的 `ui-sidebar-browser` 只在 desktop profile 挂载(`web-app` 的 patch 里 `disabled: ctx.get('profileContext')?.name !== 'desktop'`),lane 全走 web profile,钉住「没有」比默默放过更能暴露宿主立场的变化。
|
||||
|
||||
本地:`pnpm build && pnpm pack && pnpm exec playwright install chromium && pnpm test:mount`(`DSH_CMD` 未设时 `scripts/e2e-common.sh` 回退的 npx 钉版是 `DSH_NPX_SPEC`,默认 `@deepseek-ai/dsh@0.1.7-rc.1`——**不钉就会解析 `latest`,而 `latest` 仍是插件已不支持的 0.1.5 线**)。CI 钉 `@deepseek-ai/dsh@0.1.7-rc.1`(**pin 一律写精确版本号,不要写 `@alpha`**:rc.1 上线时 npm `alpha` 指的是 **0.1.7-alpha.2**,rc.1 走的是 `next`;`latest` 仍是 0.1.5-rc.3;peer 下限 `^0.1.7-rc.1`)。该步骤用 `NODE_OPTIONS=--max-old-space-size=4096`:钉版自身的传递依赖是浮动 `^` 范围,上游分阶段发布预发布版时(rc.2 于 2026-09-10 的 14:43–14:57 逐包上线)npm 会组出混合 peer 图,3062 条 ERESOLVE 后 OOM(exit 134);另一个失败模式是对未发布兄弟包 ETARGET,靠「钉一个已完整发布的版本」修掉。**不要加 `--legacy-peer-deps`**:它跳过的正是全局安装必须提供的 peer,`@deepseek-ai/dsh-app-boot` 在 boot 时 require 的 `@deepseek-ai/cordis-plugin-group` 是 peer 而非 dependency,加了它 CLI 直接 `ERR_MODULE_NOT_FOUND`(实测过一次,见 rc.2 计划 C 节)。`ci-windows` 的 `Test` 跑 `pnpm test:windows`(`--maxWorkers=1`)而非 `pnpm test`——多个 spec 真起进程(`smoke` / `install-powershell` / `git`),2 核 runner 上并行起 ConPTY / 冷启 `powershell.exe` 是超时与 worker 静默死亡的放大器;`vitest.config.ts` 的全局 `testTimeout: 15_000`(默认 5000 在 Windows 上对真起进程的用例太低,三个不同文件先后翻车)是配套的一半。e2e spec 命名 `*.e2e.ts` + vitest `exclude` 双保险;**改 `exclude` 必须保留默认排除项**(exclude 整体替换默认值)。
|
||||
|
||||
|
||||
@@ -21,12 +21,12 @@ native register files error: Error: cannot create effect on inactive context
|
||||
assertActive ← effect ← inject(slots) ← registerSlots ← registerFilesKind ← sync ← notify
|
||||
```
|
||||
|
||||
随后同一毫秒(.339–.340)**6 条** `already registered`(栈:`register ← registerFilesKind ← sync ← notify`),13 秒后新激活的 `_reload` 路径又报 2 条。链条:
|
||||
随后同一毫秒(.339–.340)**7 条** `already registered`(栈:`register ← registerFilesKind ← sync ← notify`),13 秒后新激活的 `_reload` 路径又报 2 条(该 burst 合计 9 条,与「7 个内置描述符注销 → 7 次 notify」一一对应)。链条:
|
||||
|
||||
1. `service.notify()` 是同步内联(`src/client/service.ts:696`),而 `registerNativeSurface` 把 `sync()` 订阅到它和 store 上(`src/client/native/index.ts:353`)。
|
||||
1. `service.notify()` 是同步内联(`src/client/service.ts:696`),而 `registerNativeSurface` 把 `sync()` 订阅到它和 store 上(`src/client/native/index.ts:353`;本文行号按 v0.22.0,main 因 #777 的插入而顺移)。
|
||||
2. `sync()` 的清理循环把 `live` 里**两类归属不同的条目**一视同仁:描述符类型(键 = descriptor id)与 `files` 接管(键 = `FILES_KIND`,**永远不在 `wanted` 里**)。于是**每次**通知都会释放接管、并在同一轮里重建它。
|
||||
3. 插件 teardown 时,内置描述符逐个注销 → 每次注销 notify → `sync()` 在**已经 inactive** 的插件 ctx 上运行。此时 `tabs.register` 建在**宿主** ctx 上(宿主 ctx 仍活着)→ id 被取走且返回了 disposer;紧随的 `ctx.slots.inject` 建在**插件** ctx 上 → cordis `assertActive()` 抛 `INACTIVE_EFFECT`(`@deepseek-ai/cordis@4.0.4` `effect()` 首行)→ `live.set(FILES_KIND, …)` 永不执行 → **该 id 在本页生命周期内再也不能注册**。
|
||||
4. 之后每次 notify(日志里还有 6 次)都重试同一个被占用的 id → `already registered` 刷屏;新一次激活同样注册不上 → 文件树空态。
|
||||
4. 之后每次 notify(同一 burst 里还有 6 次)都重试同一个被占用的 id → `already registered` 刷屏;新一次激活同样注册不上 → 文件树空态。
|
||||
|
||||
**两条一般化的不变量**(已写进 [AGENTS.md](../../AGENTS.md) §3.4 第 9 条):
|
||||
|
||||
@@ -41,8 +41,9 @@ PR #777(贡献者 @yanzhaohui1999)落地上面两条:`sync()` 的清理循
|
||||
|
||||
## 验证
|
||||
|
||||
- **单测红→绿(实测)**:`tests/native-registration.spec.ts` 在 v0.22.0 源码上 4/4 红,在 PR head 上 4/4 绿;head 上 `pnpm typecheck` / `pnpm lint` / `pnpm test`(114 files / 1136 passed / 9 skipped)全绿。
|
||||
- 首版用例的两处断言(「失败后还能重新注册」)写在**活着的 id 集合**上,在未修复代码上会**同样通过**(泄漏的 id 恰好构成同一个集合)。跟进 PR 把它们改写到注册表的**事件日志**上,并把槽位失败判据从 `key` 改成 `name::key` —— 两个槽位共用一个 `key`,按 `key` 失败永远只能失败第一个槽,`registerSlots` 的部分回滚分支从未被执行;改后该分支有覆盖,且 4 例在 v0.22.0 上仍全部红。
|
||||
- **单测红→绿(实测)**:`tests/native-registration.spec.ts` 在 v0.22.0 源码上 4/4 红,在修复分支上 4/4 绿;`pnpm typecheck` / `pnpm lint` / `pnpm test` 全绿(修复分支的 base 早于 #781,故为 114 files / 1136 passed;合入 main 后为 122 files / 1292 passed / 9 skipped)。
|
||||
- **部署级红→绿(实测)**:`tests/e2e/native-reload.e2e.ts` 在 scratch profile 里对 npm 上的 `dsh-better-sidebar@0.22.0` 触发页面内条目替换(`utimesSync` 已安装的 `lib/client.js`),**3/3 红**且每次都是本文那两条栈;换成本地打包的修复版 **3/3 绿**。该用例已随本 PR 进入挂载 lane(CI `plugin-mount` 绿),触发方式依赖钉住的宿主 0.1.7-rc.1 的 rev 规则(`mtime/ctime/size` 的 sha1)——pin 上调时需同步复核。
|
||||
- 首版用例的两处断言(「失败后还能重新注册」)写在**活着的 id 集合**上,在未修复代码上会**同样通过**(泄漏的 id 恰好构成同一个集合)。跟进 PR 把它们改写到注册表的**事件日志**上,并把槽位失败判据从 `key` 改成 `name::key` —— 两个槽位共用一个 `key`,按 `key` 失败时**第一个**槽就抛,回收循环虽然进了、但手里的 disposer 列表永远是空的(即 `registerSlots` 的部分回滚**释放动作**从未被执行);改后可以让某个描述符的**第二个**槽失败,回收列表非空、释放路径真正被覆盖,且 4 例在 v0.22.0 上仍全部红。
|
||||
|
||||
## 已知限制 / 未做
|
||||
|
||||
|
||||
@@ -160,6 +160,13 @@ interface Registration {
|
||||
* Run one registration disposer, reporting a failure instead of propagating
|
||||
* it: releasing a registration must never mask the error being handled, and
|
||||
* one broken entry must not leave the others registered.
|
||||
*
|
||||
* A release failure is logged, NOT routed through `reportFailure` (the visible
|
||||
* diagnostic strip), on purpose: both callers run while the surface is being
|
||||
* taken down — the drop loop and the seat teardown — where a strip would just
|
||||
* be noise, and an id that really stays taken cannot hide anyway: the next
|
||||
* registration attempt for it goes through the strip as
|
||||
* `register … error: … already registered`.
|
||||
* @param dispose - the disposer to run.
|
||||
* @param what - the registration's name, for the log line.
|
||||
*/
|
||||
|
||||
@@ -65,7 +65,11 @@ test.afterAll(async () => {
|
||||
})
|
||||
|
||||
test('the native tab types survive an in-page client entry replacement', async ({ page }) => {
|
||||
test.skip(CLIENT_JS === undefined, 'set DSH_HOME or DSH_E2E_PLUGIN_CLIENT_JS to locate the installed bundle')
|
||||
// A hard failure, not a skip: this gate's whole value is that it cannot
|
||||
// pass quietly when the harness stops telling us where the bundle lives.
|
||||
if (CLIENT_JS === undefined) {
|
||||
throw new Error('DSH_HOME (or DSH_E2E_PLUGIN_CLIENT_JS) must point at the profile running this server')
|
||||
}
|
||||
const consoleErrors: string[] = []
|
||||
page.on('console', (message) => {
|
||||
if (message.type() === 'error') consoleErrors.push(message.text())
|
||||
@@ -145,6 +149,12 @@ test('the native tab types survive an in-page client entry replacement', async (
|
||||
const filesEntry = page.locator('[data-sidebar-right-guide-entry="files"]')
|
||||
await expect(filesEntry, 'the Files guide row must be offered again').toHaveCount(1, { timeout: 30_000 })
|
||||
await filesEntry.click()
|
||||
await expect(pane.locator('[data-sidebar-right-unavailable]')).toHaveCount(0)
|
||||
// The explorer must really render again — assert the CONTENT first (a bare
|
||||
// "no unavailable marker" check passes on an empty pane), then the host's
|
||||
// "a kind with no registrant" fallback must be absent.
|
||||
await expect(pane.locator(`[role="button"][title$="${SEEDED_FILE}"]:visible`)).toHaveCount(1, { timeout: 30_000 })
|
||||
await expect(
|
||||
pane.locator('[data-sidebar-right-unavailable]'),
|
||||
'the host must not fall back to its "nothing can view this kind" face',
|
||||
).toHaveCount(0)
|
||||
})
|
||||
|
||||
@@ -51,8 +51,14 @@ function createRegistry() {
|
||||
throw new Error(`sidebarRight: tab type id "${definition.id}" is already registered`)
|
||||
}
|
||||
events.push(`register ${definition.id}`)
|
||||
// The host's disposer is IDEMPOTENT (a cordis effect disposer returns
|
||||
// the same task on a second call), so a repeated release must neither
|
||||
// log again nor pretend to free something twice.
|
||||
let released = false
|
||||
const dispose = (): void => {
|
||||
if (released) return
|
||||
if (failReleaseOf === definition.id) throw new Error(`cannot release ${definition.id}`)
|
||||
released = true
|
||||
registered.delete(definition.id)
|
||||
events.push(`release ${definition.id}`)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user