Skip to content

fix(directory-picker-native): run win32 dialog worker under plain node - #37

Open
takoyakiwhite wants to merge 10 commits into
whitelonng:masterfrom
takoyakiwhite:fix/win32-dialog-worker-source-launch-upstream
Open

fix(directory-picker-native): run win32 dialog worker under plain node#37
takoyakiwhite wants to merge 10 commits into
whitelonng:masterfrom
takoyakiwhite:fix/win32-dialog-worker-source-launch-upstream

Conversation

@takoyakiwhite

Copy link
Copy Markdown
Contributor

Summary

Fixes the Windows source-plane folder dialog failure reported in deepseek-ai/deepseek-harness discussions #154 / #197: the Web UI showed win32 folder dialog worker exited before reporting a result.

Root cause

packages/host/directory-picker-native/src/win32-dialog-host.ts launched the source-plane worker as:

node --import tsx/esm <absolute .ts path>

With a loader registered via --import, Node's ESM loader resolves the entry as a URL. On Windows, the absolute path E:\... becomes an e: scheme URL and Node rejects it with ERR_UNSUPPORTED_ESM_URL_SCHEME before the worker posts its first IPC message, so the driver only surfaces its generic exit error. Packaged consumers were unaffected because they launch the built worker.cjs under plain node.

CI also missed it: the built/source arm test used import.meta.url.endsWith('.ts'), and under Vitest/Vite the URL carries a query string, so the win32 smoke test silently exercised the built arm.

Change

  • Launch the source worker directly under plain node with native type stripping (stable since Node 22.18, inside the engines range ^22.19.0 || >=24.0.0). The worker's three modules use only erasable TS syntax, so no tsx bootstrap is needed — this does not conflict with the CLI source graph's tsx decision.
  • Switch the arm test to new URL(import.meta.url).pathname.endsWith('.ts') so bundler query strings cannot misclassify source modules as built.
  • Add a spawn-args regression pin (worker path passed positionally, no --import) and the real-dialog win32 smoke now covers the source arm.
  • Remove the now-unused tsx devDependency from the package.

Verification

  • packages/host/directory-picker-native suite: 48 passed, 1 skipped (win32-skipped built-worker e2e).
  • The win32 smoke test opens and abort-closes a real dialog through the source launch (verified on Windows, Node 24).
  • pnpm run typecheck passes.

The source-plane worker launched through `--import tsx/esm` with a Windows
absolute path, which Node's ESM loader misparses as an `e:` scheme URL and
rejects with ERR_UNSUPPORTED_ESM_URL_SCHEME before the first IPC message —
the UI then only sees "win32 folder dialog worker exited before reporting
a result". The worker's modules use only erasable TS syntax, so launch it
directly under Node's native type stripping (stable since 22.18, inside
the engines range) with no loader chain. Also switch the built/source arm
test to `pathname.endsWith('.ts')` so bundler query strings cannot
misclassify source modules as built, and pin the spawn args plus a real
dialog smoke against the source arm.

Generated with Codebuff 🤖
Co-Authored-By: Codebuff <noreply@codebuff.com>
@whitelonng

Copy link
Copy Markdown
Owner

感谢PR,但是不能直接合并,还存在一个阻塞问题必须修改:
新增 Agent Note 只有 .md 与 .zh.md,缺 .i18n.yaml。verify-translation-pairing 直接失败,而它属于 doc-sync —— fork CI 在 push master 时会跑,且 fork-ci.yml 没有 pull_request 触发,所以这个 PR 上没有任何 lint/test/doc-sync 信号,合进去就红。

实质问题(非阻塞,但建议改)
N1 · 「Vitest 查询串导致 CI 走了 built 分支」这条根因在 POSIX 上复现不出来

N2 · 没引用仓库内既有先例,且擦除前提写得不完整

N3 · 还有一条未记录的前提:worker 依赖图不能出现 workspace 包导入

N4 · 继承的 NODE_OPTIONS=--no-experimental-strip-types 会以同一个模糊症状打回原形

N5 · 同类站点没有一起处理

@whitelonng whitelonng left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review. This review was produced and posted by a DSH agent (deepseek-v4-pro) acting for @whitelonng. Every finding below was verified by running the named gate locally against this exact head; no claim is inferred from reading the diff alone.

Re-reviewed at f16e1fb84. The .i18n.yaml record fixes the pairing gate, but the feedback commit introduces two new gate failures — one of them a regression against the previous revision. Requesting changes on those two; everything else is a suggestion.

Gates run locally with this head applied to a clean tree (macOS, Node 25.9.0, Vitest 4.1.8):

gate result
verify-translation-pairing pass — 977 pairs consistent
verify-agent-note-classification pass — 577 notes
vitest run packages/host/directory-picker-native pass — 49 passed, 1 skipped
typecheck pass (exit 0)
lint pass — 0 warnings, 0 errors
verify-agent-note-format fail
test:coverage (per-file 100%) fail

Blocker 1 — the note lost two mandatory sections (regression vs bad70fa44)

verify-agent-note-format: violations found:
  ...-win32-dialog-worker-source-launch.md — missing the required `## Consequences` section
  ...-win32-dialog-worker-source-launch.md — missing `## Alternatives considered`

bad70fa44 had both and passed this gate, which is doc-sync gate 22. .agents/notes/README.md § The file format makes ## Alternatives considered mandatory for every note — "A decision recorded without what it beat invites re-litigation — the failure Agent Notes exist to prevent" — and the implemented/ skeleton requires ## Consequences.

Both deleted alternatives were genuinely recorded and worth restoring verbatim: the file://-URL variant (tsx's tsconfig-paths hook rewrites it to <cwd>\file:\<path>) and the koffi-version scoping (3.1.1 predates the broken 3.1.3/3.1.4 win32-x64 prebuilds). My earlier review questioned only the Vitest query-string causal claim; the alternatives and consequences were not part of that.

Blocker 2 — test:coverage fails on the new sanitizer

packages/host/directory-picker-native/src/win32-dialog-host.ts:27:29 uncovered branch (cond-expr, path 1/2)
ERROR: Coverage for branches (75%) does not meet global threshold (100%)
       for packages/host/directory-picker-native/src/win32-dialog-host.ts

vitest.config.ts sets perFile: true with branches: 100, and test:coverage — not test — is the CI coverage gate. Verified one-case fix inline at line 27.

Suggestions

  1. Scope the sanitizer to the source arm; the built arm has no type-stripping dependency (inline).
  2. Pin the NODE_OPTIONS-unset case — the path that runs on nearly every host (inline).
  3. The erasability precondition is still half-stated and the in-repo precedent is still uncited (inline).
  4. "Related launch paths" narrows the earlier point out of existence. The site I named is packages/workflow/workflow-worker-thread/src/host.ts:69, still on the raw import.meta.url.endsWith('.ts') check. It has no e:-scheme exposure — it boots from a data: URL with a proper file:// href — so it is not a bug, but it is the same arm-detection fragility, and the repository's symmetry rule wants either the fix or the stated reason. One sentence naming that file settles it; this PR does not need to touch that package.
  5. ## Verification lost its numbers. bad70fa44 recorded "48 passed, 1 skipped"; the present bullets state less. Current reality is 49 passed / 1 skipped, or 50 / 1 with the coverage case added.
  6. packages/host/directory-picker-native/README.md § Known Limitations and Deferred Work. The source plane now depends on host NODE_OPTIONS hygiene, native type stripping, and a package-local erasable graph; packages/AGENTS.md puts durable non-obvious maintainer constraints there.

Verified, so these are not re-litigated

  • The flag pair is right. node --help on 25.9 lists --experimental-strip-types, --no-strip-types, and the legacy --no-experimental-strip-types is still accepted (exit 0). Removing both spellings is the correct superset; no change needed.
  • NODE_OPTIONS: undefined is safe through a real spawn. Node's normalizeSpawnArguments skips undefined env values, so the child sees no NODE_OPTIONS rather than the literal string "undefined" ({"has":false}, exit 0).
  • Erasability is genuinely gated already. Appending export enum ProbeKind { A = 1 } to win32-dialog-logic.ts makes rejects through the real worker where the Win32 surface is unavailable fail with ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX'worker exited before reporting a result', which does not match 'win32 folder dialog failed'. POSIX CI therefore catches both syntax drift and a missing import type in this graph — worth recording in the note as the standing guard.
  • fork-ci.yml triggers on push: [master] and workflow_dispatch only, so this PR carries no lint/typecheck/test/doc-sync signal; both blockers would land red on master.

Comment thread packages/host/directory-picker-native/src/win32-dialog-host.ts
Comment thread packages/host/directory-picker-native/src/win32-dialog-host.ts Outdated
Comment thread packages/host/directory-picker-native/src/win32-dialog-host.ts Outdated
Comment thread packages/host/directory-picker-native/src/win32-dialog-host.ts Outdated
Comment thread .agents/notes/implemented/bug-fix/2026-08-19-win32-dialog-worker-source-launch.md Outdated
@takoyakiwhite
takoyakiwhite force-pushed the fix/win32-dialog-worker-source-launch-upstream branch from 1863680 to f16e1fb Compare August 19, 2026 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants