fix(directory-picker-native): run win32 dialog worker under plain node - #37
Conversation
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>
|
感谢PR,但是不能直接合并,还存在一个阻塞问题必须修改: 实质问题(非阻塞,但建议改) N2 · 没引用仓库内既有先例,且擦除前提写得不完整 N3 · 还有一条未记录的前提:worker 依赖图不能出现 workspace 包导入 N4 · 继承的 NODE_OPTIONS=--no-experimental-strip-types 会以同一个模糊症状打回原形 N5 · 同类站点没有一起处理 |
whitelonng
left a comment
There was a problem hiding this comment.
🤖 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
- Scope the sanitizer to the source arm; the built arm has no type-stripping dependency (inline).
- Pin the
NODE_OPTIONS-unset case — the path that runs on nearly every host (inline). - The erasability precondition is still half-stated and the in-repo precedent is still uncited (inline).
- "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 rawimport.meta.url.endsWith('.ts')check. It has noe:-scheme exposure — it boots from adata:URL with a properfile://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. ## Verificationlost its numbers.bad70fa44recorded "48 passed, 1 skipped"; the present bullets state less. Current reality is 49 passed / 1 skipped, or 50 / 1 with the coverage case added.packages/host/directory-picker-native/README.md§ Known Limitations and Deferred Work. The source plane now depends on hostNODE_OPTIONShygiene, 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 --helpon 25.9 lists--experimental-strip-types, --no-strip-types, and the legacy--no-experimental-strip-typesis still accepted (exit 0). Removing both spellings is the correct superset; no change needed. NODE_OPTIONS: undefinedis safe through a real spawn. Node'snormalizeSpawnArgumentsskips undefined env values, so the child sees noNODE_OPTIONSrather than the literal string"undefined"({"has":false}, exit 0).- Erasability is genuinely gated already. Appending
export enum ProbeKind { A = 1 }towin32-dialog-logic.tsmakesrejects through the real worker where the Win32 surface is unavailablefail withERR_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 missingimport typein this graph — worth recording in the note as the standing guard. fork-ci.ymltriggers onpush: [master]andworkflow_dispatchonly, so this PR carries no lint/typecheck/test/doc-sync signal; both blockers would land red on master.
1863680 to
f16e1fb
Compare
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.tslaunched the source-plane worker as:With a loader registered via
--import, Node's ESM loader resolves the entry as a URL. On Windows, the absolute pathE:\...becomes ane:scheme URL and Node rejects it withERR_UNSUPPORTED_ESM_URL_SCHEMEbefore the worker posts its first IPC message, so the driver only surfaces its generic exit error. Packaged consumers were unaffected because they launch the builtworker.cjsunder 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
^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.new URL(import.meta.url).pathname.endsWith('.ts')so bundler query strings cannot misclassify source modules as built.--import) and the real-dialog win32 smoke now covers the source arm.tsxdevDependency from the package.Verification
packages/host/directory-picker-nativesuite: 48 passed, 1 skipped (win32-skipped built-worker e2e).pnpm run typecheckpasses.