fix(packaging): bundle audio capture for mirror installs - #5747
Conversation
|
Re-run at HEAD Template looks good ✓ On direction: still squarely in scope. Mirror registries lagging on On approach: scope is tight — 5 files, each doing one job. The release workflow reorder (download prebuilds before prepare-package) is the root-cause fix for the CI ordering bug. The No concerns since initial triage. Moving on to code review. 🔍 中文说明在 HEAD 模板完整 ✓ 方向：仍然完全在范围内。镜像源在同步 方案：范围紧凑——5 个文件，各司其职。release workflow 的步骤重排（在 prepare-package 之前下载 prebuilds）是 CI 排序 bug 的根因修复。 自初次 triage 以来无新顾虑。进入代码审查。🔍 — Qwen Code · qwen3.7-max |
Code review (re-run at
|
|
Re-run at Stepping back: this is a focused distribution fix that does exactly what the title says. The implementation is tight, the error handling is correctly scoped, and every change earns its place. Key things that hold up on re-review:
Independent proposal was: copy the package subtree when artifacts exist, add to All 23 unit tests pass (11 packaging + 12 recorder). The 6 error-rewrite scenarios all behave correctly at the function level, and the PR's own unit tests verify the call-site guards. No reservations. Approving. 中文说明在 退一步看：这是对分发痛点的聚焦修复，实现紧凑，错误处理范围正确，每个改动都说得过去。 重新审查中仍然成立的关键点：
独立方案是：产物存在时复制包子树、加进 全部 23 个单元测试通过（11 个打包 + 12 个录音器）。6 个错误改写场景在函数级别都行为正确，PR 自己的单元测试验证了调用点守卫。 无保留意见。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Release workflow ordering prevents bundling from working. In .github/workflows/release.yml, npm run prepare:package runs at line 397, but the "Download audio capture prebuilds" step is at lines 399–406 — after the package is already prepared. Since packages/audio-capture/prebuilds/ doesn't exist when copyNativeAudioCapturePackage runs, the fs.existsSync guard returns false and bundledDependencies is never written to the dist package.json. The bundling feature that is the PR's primary goal is effectively dead code in release builds. The prebuild download step needs to move before "Build Bundle and Prepare Package" (or prepare:package needs to re-run after the download).
| ); | ||
|
|
||
| const nodeGypBuildSrc = path.dirname( | ||
| nodeRequire.resolve('node-gyp-build/package.json'), |
There was a problem hiding this comment.
[Critical] nodeRequire.resolve('node-gyp-build/package.json') is not preflighted — it runs after fs.rmSync(addonDest, ...) has deleted the destination and partial copies of dist/, prebuilds/, and package.json have already been written. If resolution fails (e.g., hoisting issue, lockfile drift), the build crashes with an unhandled error and leaves dist/node_modules/@qwen-code/audio-capture/ in a partial state. A subsequent retry would see the partially-written files passing existsSync checks but ship a broken tarball missing node-gyp-build.
Resolve node-gyp-build in the preflight block before any destructive operations:
let nodeGypBuildSrc;
try {
nodeGypBuildSrc = path.dirname(
nodeRequire.resolve('node-gyp-build/package.json'),
);
} catch {
console.warn('Warning: node-gyp-build not resolvable from build context');
return false;
}— qwen3.7-max via Qwen Code /review
| nodeRequire.resolve('node-gyp-build/package.json'), | ||
| ); | ||
| fs.cpSync( | ||
| nodeGypBuildSrc, |
There was a problem hiding this comment.
[Suggestion] The transitive dependency copy hardcodes node-gyp-build by name, even though addonPkg (parsed from packages/audio-capture/package.json) already contains the full dependencies map. If anyone adds a new runtime dependency to audio-capture, the bundling silently skips it — the published tarball would declare the dependency in package.json but not include it in node_modules/, producing a confusing runtime Cannot find module error that doesn't match explainMissingNativePackage (which checks for @qwen-code/audio-capture, not the transitive dep).
Derive the copy list from addonPkg.dependencies:
for (const dep of Object.keys(addonPkg.dependencies ?? {})) {
const depSrc = path.dirname(nodeRequire.resolve(`${dep}/package.json`));
fs.cpSync(
depSrc,
path.join(addonDest, 'node_modules', dep),
copyOpts,
);
}— qwen3.7-max via Qwen Code /review
| } catch (error) { | ||
| this.starting = false; | ||
| throw error; | ||
| throw explainMissingNativePackage(error); |
There was a problem hiding this comment.
[Suggestion] The catch block wraps ALL errors from the try block through explainMissingNativePackage, including errors from backend.startRecording() — not just this.loadBackend(). While isMissingNativePackageError checks for both the package name and specific error patterns (making false matches unlikely), startRecording() errors would never be module-not-found errors, so the wrapping is conceptually broader than intended.
Consider narrowing the scope to only wrap loadBackend() errors:
try {
try {
backend = await this.loadBackend();
} catch (loadError) {
throw explainMissingNativePackage(loadError);
}
// ... startRecording and rest of try block
} catch (error) {
this.starting = false;
throw error;
}— qwen3.7-max via Qwen Code /review
|
|
||
| function explainMissingNativePackage(error: unknown): unknown { | ||
| if (!(error instanceof Error) || !isMissingNativePackageError(error)) { | ||
| return error; |
There was a problem hiding this comment.
[Suggestion] No debugLogger call before throwing the enhanced error. The original error's stack trace, error.code, and the specific resolution failure reason are only preserved via { cause: error }, which most logging frameworks and error reporters don't surface by default. At runtime, the operator sees "check your mirror registry" with no structured log of the actual underlying error.
Add a debug log before the throw:
debugLogger.warn(
'[voice] native package missing:',
error.message,
(error as NodeJS.ErrnoException).code,
);— qwen3.7-max via Qwen Code /review
| ), | ||
| ).toBe(true); | ||
| expect( | ||
| existsSync( |
There was a problem hiding this comment.
[Suggestion] The fixture creates packages/audio-capture/dist/index.test.js with sentinel content 'throw new Error("should not copy tests")', but no assertion verifies this file was excluded by the copy filter regex. If the filter in copyNativeAudioCapturePackage is accidentally loosened or removed, test files would leak into the published tarball with no test catching it.
Add a negative assertion:
expect(
existsSync(
path.join(
rootDir,
'dist',
'node_modules',
'@qwen-code',
'audio-capture',
'dist',
'index.test.js',
),
),
).toBe(false);— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No critical issues found. Two test coverage suggestions below.
Skipped 3 stale comments from prior commits, found 2 prior comments with no overlap.
— qwen3.7-max via Qwen Code /review
| ); | ||
|
|
||
| expect(distPackageJson.files).toContain('examples'); | ||
| expect(distPackageJson.bundledDependencies).toContain( |
There was a problem hiding this comment.
[Suggestion] The fixture in createFixtureRoot() always creates all audio-capture artifacts (dist/, prebuilds/, package.json), so copyNativeAudioCapturePackage always returns true in tests. There is no test for the return false path — the case where artifacts are missing and bundledDependencies should be absent from dist/package.json.
Consider adding a test that omits the prebuilds directory and asserts the fallback:
it('omits bundledDependencies when audio-capture artifacts are missing', () => {
const rootDir = createFixtureRoot();
rmSync(path.join(rootDir, 'packages', 'audio-capture', 'prebuilds'), { recursive: true, force: true });
createBundleArtifacts(rootDir);
stubConsole();
preparePackage({ rootDir });
const distPackageJson = JSON.parse(
readFileSync(path.join(rootDir, 'dist', 'package.json'), 'utf8'),
);
expect(distPackageJson.bundledDependencies).toBeUndefined();
});— qwen3.7-max via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| function isMissingNativePackageError(error: Error): boolean { |
There was a problem hiding this comment.
[Suggestion] isMissingNativePackageError has four OR-branches (Cannot find package, Cannot find module, ERR_MODULE_NOT_FOUND, MODULE_NOT_FOUND), but the test only exercises the first one. Consider adding test cases for the code-based branches to guard against future Node.js error message changes:
it('recognises ERR_MODULE_NOT_FOUND code for the audio-capture package', async () => {
const err = Object.assign(
new Error("some message @qwen-code/audio-capture"),
{ code: 'ERR_MODULE_NOT_FOUND' },
);
const recorder = createNativeAudioRecorder({ loadBackend: () => { throw err; } });
await expect(recorder.start()).rejects.toThrow(/mirror or private registry/);
});— qwen3.7-max via Qwen Code /review
| `Warning: audio capture package artifact not found at ${requiredPath}`, | ||
| ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Critical] copyNativeAudioCapturePackage returns false on missing artifacts or unresolvable dependencies with only a console.warn, and the build continues — producing a publishable package without native audio. There is no build-time gate to prevent publishing a degraded package.
If the prebuilds download step silently fails or produces nothing, the package ships without voice capture and the only signal is a console.warn buried in CI logs. The runtime explainMissingNativePackage error points users at mirror/registry issues, but the root cause was a build-time failure.
| } | |
| if (!fs.existsSync(requiredPath)) { | |
| console.error( | |
| `Error: audio capture package artifact not found at ${requiredPath}`, | |
| ); | |
| throw new Error( | |
| `Required audio capture artifact missing: ${requiredPath}. ` + | |
| 'Cannot publish package without native voice capture.', | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| ); | ||
|
|
||
| expect(distPackageJson.files).toContain('examples'); | ||
| expect(distPackageJson.bundledDependencies).toContain( |
There was a problem hiding this comment.
[Suggestion] The test fixture includes scripts: { install: 'node install.js' } and devDependencies: { typescript: ... } in the addon's package.json, and copyNativeAudioCapturePackage explicitly strips them via delete addonPkg.scripts; delete addonPkg.devDependencies. But no assertion reads the bundled package.json to verify these fields were removed.
If the stripping code is accidentally removed, the published package would include an install script pointing to a nonexistent file (node install.js), causing npm install failures for end users.
Consider adding assertions after the existing bundledDependencies check:
const distAudioPkgJson = JSON.parse(
readFileSync(
path.join(rootDir, 'dist', 'node_modules', '@qwen-code', 'audio-capture', 'package.json'),
'utf8',
),
);
expect(distAudioPkgJson.scripts).toBeUndefined();
expect(distAudioPkgJson.devDependencies).toBeUndefined();— qwen3.7-max via Qwen Code /review
| name: 'audio-capture-prebuilds' | ||
| path: 'packages/audio-capture/prebuilds' | ||
|
|
||
| - name: 'Build Bundle and Prepare Package' |
There was a problem hiding this comment.
[Critical] Fork release pipeline will hard-fail. copyNativeAudioCapturePackage throws when packages/audio-capture/prebuilds/ is missing, but the "Download audio capture prebuilds" step above (line 397) is gated on github.repository == 'QwenLM/qwen-code' while this "Build Bundle and Prepare Package" step runs unconditionally. Any fork running the release workflow will crash at prepare:package with Required audio capture artifact missing.
Two options:
- Make
copyNativeAudioCapturePackagereturnfalse(instead of throwing) when artifacts are missing, and letwriteDistPackageJsonomitbundledDependenciesaccordingly — thebundleNativeAudioCapture = falsedefault at line 222 already exists for this. - Gate this step on the same
github.repositorycondition, with a fallback step that runsnpm run bundlealone for forks.
— qwen3.7-max via Qwen Code /review
| 'dist', | ||
| 'node_modules', | ||
| '@qwen-code', | ||
| 'audio-capture', |
There was a problem hiding this comment.
[Critical] The happy-path test verifies dist/index.js, package.json sanitization, node-gyp-build copying, and test-file exclusion — but never asserts that prebuilds/ was actually copied to dist/node_modules/@qwen-code/audio-capture/prebuilds/. The prebuilt binaries are the single most critical artifact this entire function exists to ship. If the fs.cpSync for prebuilds were accidentally removed or misdirected, the test suite would still pass while the published package silently lacks native binaries.
| 'audio-capture', | |
| ).toBe(false); | |
| expect( | |
| existsSync( | |
| path.join( | |
| rootDir, | |
| 'dist', | |
| 'node_modules', | |
| '@qwen-code', | |
| 'audio-capture', | |
| 'prebuilds', | |
| 'darwin-arm64', | |
| '@qwen-code+audio-capture.node', | |
| ), | |
| ), | |
| ).toBe(true); |
— qwen3.7-max via Qwen Code /review
| let backend: NativeAudioCaptureBackend; | ||
| try { | ||
| backend = await this.loadBackend(); | ||
| try { |
There was a problem hiding this comment.
[Suggestion] The explainMissingNativePackage wrapping is only applied here in start(). The warmup() method (line 46) and microphoneStatus() method (line 52) also call this.loadBackend() directly without wrapping errors. When the native package is missing, those methods throw the raw "Cannot find package" error instead of the helpful mirror-registry explanation.
Consider extracting the load-and-wrap into a private helper so all three call sites stay consistent:
private async loadBackendWithExplanation(): Promise<NativeAudioCaptureBackend> {
try {
return await this.loadBackend();
} catch (loadError) {
throw explainMissingNativePackage(loadError);
}
}— qwen3.7-max via Qwen Code /review
| await expect(recorder.start()).rejects.toThrow(/@qwen-code\/audio-capture/); | ||
| }); | ||
|
|
||
| it.each(['ERR_MODULE_NOT_FOUND', 'MODULE_NOT_FOUND'])( |
There was a problem hiding this comment.
[Suggestion] isMissingNativePackageError has four OR-branches (Cannot find package, Cannot find module, ERR_MODULE_NOT_FOUND, MODULE_NOT_FOUND), but the tests only exercise the Cannot find package message branch (line 165) and the two error code branches (this it.each). The Cannot find module message branch — a distinct condition used for CJS require() resolution failures — has no dedicated test.
Consider adding 'Cannot find module' to the it.each array, or adding a separate test:
it('explains mirror registry installs for Cannot find module errors', async () => {
const recorder = createNativeAudioRecorder({
loadBackend: () => {
throw new Error("Cannot find module '@qwen-code/audio-capture'");
},
});
await expect(recorder.start()).rejects.toThrow(/mirror or private registry/);
});— qwen3.7-max via Qwen Code /review
| try { | ||
| dependencySources.push([ | ||
| dependencyName, | ||
| path.dirname(nodeRequire.resolve(`${dependencyName}/package.json`)), |
There was a problem hiding this comment.
[Suggestion] Two concerns with the dependency-copying loop:
(1) The error path for unresolvable dependencies is not tested. No test in package-assets.test.js creates a fixture with an unresolvable dependency name to verify this throw fires correctly.
(2) The loop only copies direct dependencies of @qwen-code/audio-capture. If any dependency ever gains transitive sub-dependencies, they will be silently absent from the published tarball. Currently safe since node-gyp-build has zero sub-dependencies, but fragile.
Consider adding a test for the unresolvable-dependency path, and either recursively walking the dependency tree or validating the bundled tree post-copy.
— qwen3.7-max via Qwen Code /review
| function writeDistPackageJson( | ||
| rootDir, | ||
| distDir, | ||
| { bundleNativeAudioCapture = false } = {}, |
There was a problem hiding this comment.
[Suggestion] copyNativeAudioCapturePackage always returns true on success (line 217) or throws on failure (lines 157, 174). It never returns false, making the bundleNativeAudioCapture = false default and the conditional spread at line 284 dead code. This misleads future readers into thinking a graceful-degradation path exists.
If the intent is to always require native bundling, simplify:
function writeDistPackageJson(rootDir, distDir) {
// ...
bundledDependencies: ['@qwen-code/audio-capture'],
// ...
}If the intent is to support fork builds without prebuilds (per the CI gate at release.yml:397), change copyNativeAudioCapturePackage to return false instead of throwing when artifacts are missing.
— qwen3.7-max via Qwen Code /review
| path.dirname(nodeRequire.resolve(`${dependencyName}/package.json`)), | ||
| ]); | ||
| } catch { | ||
| throw new Error( |
There was a problem hiding this comment.
[Critical] The catch block in the dependency resolution loop throws unconditionally, ignoring the required flag. Lines 159-169 above correctly check if (required) before throwing on missing source artifacts and gracefully return false when required=false. But this block hard-throws on unresolvable dependencies without checking required. This breaks the graceful-degradation contract for non-official builds: if packages/audio-capture/{dist,prebuilds,package.json} all exist but a declared dependency can't be resolved (e.g., partial npm ci --omit=optional on a fork), the build crashes with "Cannot publish package without native voice capture" even though the fork has no intention to publish.
| throw new Error( | |
| } catch { | |
| const message = `audio capture dependency not resolvable: ${dependencyName}`; | |
| if (required) { | |
| throw new Error( | |
| `Required ${message}. ` + | |
| 'Cannot publish package without native voice capture.', | |
| ); | |
| } | |
| console.warn(`Warning: ${message}`); | |
| return false; | |
| } |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No critical issues found. Four suggestions identified; two posted inline, two skipped due to overlap with existing comments (line-158 false-positive concern and line-181 transitive-dep concern already covered by prior review threads).
— qwen3.7-max via Qwen Code /review
| ); | ||
| fs.cpSync(path.join(addonSrc, 'dist'), path.join(addonDest, 'dist'), { | ||
| ...copyOpts, | ||
| filter: (src) => !/\.test\.(d\.)?[mc]?[jt]s(\.map)?$/.test(src), |
There was a problem hiding this comment.
[Suggestion] The filter regex excludes .test.* files but not .spec.* files. Vitest supports both .test.* and .spec.* naming conventions. If a .spec.ts is ever compiled into packages/audio-capture/dist/, it would ship in the published tarball alongside production code.
| filter: (src) => !/\.test\.(d\.)?[mc]?[jt]s(\.map)?$/.test(src), | |
| filter: (src) => !/\.(test|spec)\.(d\.)?[mc]?[jt]s(\.map)?$/.test(src), |
— qwen3.7-max via Qwen Code /review
| it('fails packaging when required audio-capture artifacts are missing', () => { | ||
| const rootDir = createFixtureRoot(); | ||
| rmSync(path.join(rootDir, 'packages', 'audio-capture', 'prebuilds'), { | ||
| recursive: true, |
There was a problem hiding this comment.
[Suggestion] The required: true + unresolvable dependency path (prepare-package.js:186-189, throwing "Required audio capture dependency not resolvable") is untested. The existing tests cover required: true + missing artifact and required: false + unresolvable dependency, but not this combination. A regression here could cause the official release to silently publish without audio-capture dependencies instead of failing loudly.
Consider adding a test analogous to the one above but injecting an unresolvable dependency and asserting .toThrow(/Required audio capture dependency not resolvable/).
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
| ]; | ||
|
|
||
| for (const requiredPath of requiredPaths) { | ||
| if (!fs.existsSync(requiredPath)) { |
There was a problem hiding this comment.
[Suggestion] copyNativeAudioCapturePackage validates that dist/, prebuilds/, and package.json exist as paths, but not that they have usable contents. An empty dist/ (e.g. a stale tsconfig.tsbuildinfo no-opping the addon's tsc build, or a wrong outDir) or an empty prebuilds/ directory passes all three existsSync checks, so bundledDependencies: ['@qwen-code/audio-capture'] is written and a broken addon ships — even under the required (main-repo release) gate. The sibling standalone packaging path already guards against exactly this with hasNativePrebuild() (scripts/create-standalone-package.js:363), which requires an actual .node file. Worse, at runtime an empty dist/ produces an ERR_MODULE_NOT_FOUND whose message contains both @qwen-code/audio-capture and the not-found code, so isMissingNativePackageError matches and the user is told to "reinstall from a mirror/registry" — advice that can't fix a present-but-empty package. Consider validating contents (e.g. the resolved main/entry under dist/ exists, and prebuilds/ contains at least one .node, reusing hasNativePrebuild).
— claude-opus-4-8[1m] via Qwen Code /qreview
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Two new findings not covered by the existing 19 inline comments on this PR. Both are in code paths the prior comments did not reach (false-positive error rewrite; unhandled JSON.parse in the non-required path).
|
|
||
| function isMissingNativePackageError(error: Error): boolean { | ||
| return ( | ||
| error.message.includes(AUDIO_CAPTURE_PACKAGE) && |
There was a problem hiding this comment.
[Critical] False-positive error rewrite. packages/audio-capture/src/index.ts:62-70 (loadBinding()) wraps any inner failure from node-gyp-build or prebuild loading with 'Native audio capture addon could not be loaded. Reinstall @qwen-code/audio-capture, ... (' + error.message + ')'. When the inner error is itself a Cannot find module 'node-gyp-build' (e.g. the bundled dep was not copied, or its files are corrupted), the resulting message contains both @qwen-code/audio-capture and Cannot find module, so this substring check passes and the error is rewritten to "package is missing, reinstall from npmjs.org".
That confidently points the operator at the registry mirror when the package is installed and the real root cause is an incomplete bundled node_modules/ tree (precisely the failure mode the bundling added in this PR is supposed to prevent). The original Cannot find module 'node-gyp-build' detail ends up buried in parentheses at the end of the rewritten message.
| error.message.includes(AUDIO_CAPTURE_PACKAGE) && | |
| function isMissingNativePackageError(error: Error): boolean { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code === 'ERR_MODULE_NOT_FOUND' || code === 'MODULE_NOT_FOUND') { | |
| return error.message.includes(AUDIO_CAPTURE_PACKAGE); | |
| } | |
| return ( | |
| error.message.includes(AUDIO_CAPTURE_PACKAGE) && | |
| (error.message.startsWith('Cannot find package') || | |
| error.message.startsWith('Cannot find module')) | |
| ); | |
| } |
The startsWith guard rejects loadBinding()'s wrapped message (which starts with Native audio capture addon could not be loaded) while still matching the ESM/CJS loader's genuine resolution failures (which start with Cannot find package '@qwen-code/audio-capture' ...).
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| const addonPkg = JSON.parse( |
There was a problem hiding this comment.
[Suggestion] JSON.parse(fs.readFileSync(..., 'package.json')) has no try/catch. Every other early-return path in copyNativeAudioCapturePackage honors the required contract — warn + return false when not required, throw when required — but a malformed packages/audio-capture/package.json (invalid JSON, truncated file, merge-conflict markers) bypasses all of that and throws a raw SyntaxError unconditionally, crashing npm run prepare:package for every caller regardless of required.
| const addonPkg = JSON.parse( | |
| let addonPkg; | |
| try { | |
| addonPkg = JSON.parse( | |
| fs.readFileSync(path.join(addonSrc, 'package.json'), 'utf8'), | |
| ); | |
| } catch { | |
| const message = `audio capture package.json is not valid JSON at ${addonSrc}`; | |
| if (required) { | |
| throw new Error( | |
| `Required ${message}. ` + | |
| 'Cannot publish package without native voice capture.', | |
| ); | |
| } | |
| console.warn(`Warning: ${message}`); | |
| return false; | |
| } |
Matches the required handling pattern used on lines 161, 183, and 207 for other artifact failures.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Independent verification of the current HEAD (5ab7349). The prior round's Critical findings appear addressed on this commit:
- Release ordering / fork hard-fail —
prepare:packagenow runs after the "Download audio capture prebuilds" step, andQWEN_REQUIRE_AUDIO_CAPTURE_PREBUILDis'1'only forQwenLM/qwen-code, so forks getrequired=false(warn + skip, no throw). - False-positive error rewrite — the real
loadBinding()throws a code-lessErrorwhose message starts with"Native audio capture addon could not be loaded…", soisMissingNativePackageErrorreturns false for it (thestartsWithguards + the no-codepath). A dedicated test asserts the wrapped addon-load failure is passed through unchanged. - Bundling actually works end-to-end — verified via
npm pack+ offlinenpm install:bundledDependenciesshipsdist/node_modules/@qwen-code/audio-captureplus the nestednode-gyp-buildand the.nodeprebuild even though the bundled package's ownfileswhitelist omitsnode_modules, and runtimerequireresolves both. Package versions are lockstep (0.19.0), so theoptionalDependencies/bundledDependenciesentry matches the bundled copy (no registry refetch).
One minor, non-blocking observation (adjacent to the existing comment on the dependency-copy loop at prepare-package.js:223): that loop resolves the addon's runtime deps via the module-level nodeRequire (rooted at the script), not the rootDir parameter the rest of the function is keyed on. It's harmless in the real release (rootDir == repo root), but it couples the happy-path packaging test to the host having node-gyp-build hoisted — when it isn't, the test fails with a confusing expect(undefined).toContain(...) rather than a clear message. Resolving deps relative to rootDir (e.g. createRequire rooted at rootDir) would make the function self-contained and the test hermetic.
— claude-opus-4-8[1m] via Qwen Code /qreview
| createBundleArtifacts(rootDir); | ||
| stubConsole(); | ||
|
|
||
| preparePackage({ rootDir }); |
There was a problem hiding this comment.
[Suggestion] Tests that exercise the graceful-degradation path (e.g. this one, line 293, and the happy-path test at line 57) call preparePackage({ rootDir }) without passing requireNativeAudioCapture. The parameter defaults to process.env.QWEN_REQUIRE_AUDIO_CAPTURE_PREBUILD === '1' (set in prepare-package.js:25). If a developer runs the test suite with that env var set in their shell (e.g. after a local release dry-run), these tests would unexpectedly throw instead of asserting graceful degradation.
Consider explicitly passing requireNativeAudioCapture: false in the non-required test cases to decouple test determinism from ambient environment state:
| preparePackage({ rootDir }); | |
| preparePackage({ rootDir, requireNativeAudioCapture: false }); |
— qwen3.7-max via Qwen Code /review
| ); | ||
| fs.cpSync(path.join(addonSrc, 'dist'), path.join(addonDest, 'dist'), { | ||
| ...copyOpts, | ||
| filter: (src) => !/\.(test|spec)\.(d\.)?[mc]?[jt]s(\.map)?$/.test(src), |
There was a problem hiding this comment.
[Suggestion] The copy filter only excludes test/spec files (.(test|spec).*) but does not exclude .d.ts declaration files or .js.map source maps. The packages/audio-capture/dist/ directory contains index.d.ts, platform.d.ts, index.js.map, and platform.js.map — all of which are copied into the published tarball. For a bundled CLI tool, declaration files and source maps are unused (declarations reference .ts sources not shipped in the bundle, and source maps point at files outside the tarball).
Tightening the filter to only copy runtime JS files would reduce the npm tarball size:
| filter: (src) => !/\.(test|spec)\.(d\.)?[mc]?[jt]s(\.map)?$/.test(src), | |
| filter: (src) => /\.[cm]?js$/.test(src) && !/\.(test|spec)\.(d\.)?[mc]?[jt]s(\.map)?$/.test(src), |
— qwen3.7-max via Qwen Code /review
|
|
||
| function isMissingNativePackageError(error: Error): boolean { | ||
| return ( | ||
| error.message.includes(AUDIO_CAPTURE_PACKAGE) && |
There was a problem hiding this comment.
[Suggestion] isMissingNativePackageError can produce false positives because error.message.includes(AUDIO_CAPTURE_PACKAGE) also matches when the package name appears in the importer path. For example, "Cannot find package 'other-dep' imported from .../node_modules/@qwen-code/audio-capture/dist/index.js" contains both the package name and Cannot find package, so it would be misclassified as a missing-package error.
Similarly, error.message.includes('Cannot find module') is overly broad — it matches CJS sub-dependency failures within the audio-capture package (e.g., require('./missing-helper')), sending users down the wrong troubleshooting path.
| error.message.includes(AUDIO_CAPTURE_PACKAGE) && | |
| function isMissingNativePackageError(error: Error): boolean { | |
| const msg = error.message; | |
| const namesPackage = | |
| msg.includes(`'${AUDIO_CAPTURE_PACKAGE}'`) || | |
| msg.includes(`"${AUDIO_CAPTURE_PACKAGE}"`); | |
| return ( | |
| namesPackage && | |
| (msg.includes('Cannot find package') || | |
| (error as NodeJS.ErrnoException).code === 'ERR_MODULE_NOT_FOUND' || | |
| (error as NodeJS.ErrnoException).code === 'MODULE_NOT_FOUND') | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| delete addonPkg.scripts; | ||
| delete addonPkg.devDependencies; |
There was a problem hiding this comment.
[Suggestion] Only scripts and devDependencies are stripped from the copied package.json. Several other fields should also be removed:
optionalDependencies/peerDependencies/bundledDependencies— these are preserved verbatim and could cause npm to attempt fetching unexpected packages during install of the published tarball.files— references paths (native/audio_capture.cc,binding.gyp,install.js) that are not present in the bundle, creating misleading metadata.
| delete addonPkg.devDependencies; | |
| delete addonPkg.scripts; | |
| delete addonPkg.devDependencies; | |
| delete addonPkg.optionalDependencies; | |
| delete addonPkg.peerDependencies; | |
| delete addonPkg.bundledDependencies; | |
| delete addonPkg.files; |
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| function hasFileMatching(dir, predicate) { | ||
| for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { |
There was a problem hiding this comment.
[Suggestion] hasFileMatching uses entry.isFile() and entry.isDirectory() from Dirent, which return false for symbolic links. But the copy step uses fs.cpSync with dereference: true, which follows symlinks. This means validation and copy disagree on symlinked entries — validation could reject valid symlinked artifacts (e.g., from pnpm content-addressable stores or CI artifact symlinks).
Consider using fs.statSync(entryPath) which follows symlinks, keeping validation consistent with the copy behavior:
| for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { | |
| function hasFileMatching(dir, predicate) { | |
| for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { | |
| const entryPath = path.join(dir, entry.name); | |
| let stat; | |
| try { stat = fs.statSync(entryPath); } catch { continue; } | |
| if (stat.isDirectory()) { | |
| if (hasFileMatching(entryPath, predicate)) return true; | |
| } else if (stat.isFile() && predicate(entryPath)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
— qwen3.7-max via Qwen Code /review
| filter: (src) => !/\.(test|spec)\.(d\.)?[mc]?[jt]s(\.map)?$/.test(src), | ||
| }); | ||
| fs.cpSync( | ||
| path.join(addonSrc, 'prebuilds'), |
There was a problem hiding this comment.
[Suggestion] The prebuilds and dependency fs.cpSync calls use dereference: true with no filter function, unlike the dist/ copy above which filters test files. This means any file in prebuilds/ (not just .node files) and any symlink target in dependency directories will be silently bundled into the published tarball.
Consider adding filter functions to restrict what gets copied:
| path.join(addonSrc, 'prebuilds'), | |
| fs.cpSync( | |
| path.join(addonSrc, 'prebuilds'), | |
| path.join(addonDest, 'prebuilds'), | |
| { | |
| ...copyOpts, | |
| filter: (src) => | |
| fs.statSync(src, { throwIfNoEntry: false })?.isDirectory() || | |
| src.endsWith('.node'), | |
| }, | |
| ); |
— qwen3.7-max via Qwen Code /review
| : {}), | ||
| config: rootPackageJson.config, | ||
| dependencies: {}, | ||
| optionalDependencies: { |
There was a problem hiding this comment.
[Suggestion] optionalDependencies always includes '@qwen-code/audio-capture': rootPackageJson.version regardless of whether bundleNativeAudioCapture is true or false. When bundling is skipped (fork builds, local dev, artifact validation failure), the published package.json still tells npm to attempt a separate install of this optional dependency.
This creates a mismatch: the runtime error message says the package "should have been bundled" and tells users to reinstall from npmjs.org, but the install metadata says it's an optional separate dependency. Consider conditionally including it:
| optionalDependencies: { | |
| optionalDependencies: { | |
| ...(bundleNativeAudioCapture | |
| ? { '@qwen-code/audio-capture': rootPackageJson.version } | |
| : {}), | |
| '@lydell/node-pty': '1.2.0-beta.10', |
— qwen3.7-max via Qwen Code /review
| 'https://registry.npmjs.org or make sure the configured registry ' + | ||
| `provides ${AUDIO_CAPTURE_PACKAGE}. (${error.message})`, | ||
| { cause: error }, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] ${error.message} is interpolated directly into the user-facing error string. Node.js module-resolution errors embed full filesystem paths (e.g., "Cannot find package '@qwen-code/audio-capture' imported from /opt/mycompany/qwen/dist/cli.js"), which leaks internal installation paths to end users.
Additionally, the original error is already attached via { cause: error }, making the inline message redundant. Consider removing it or sanitizing:
| ); | |
| return new Error( | |
| `Native voice capture package '${AUDIO_CAPTURE_PACKAGE}' is missing. ` + | |
| 'If Qwen Code was installed from a mirror or private registry, the ' + | |
| 'registry may not have synced this optional package. Reinstall from ' + | |
| 'https://registry.npmjs.org or make sure the configured registry ' + | |
| `provides ${AUDIO_CAPTURE_PACKAGE}.`, | |
| { cause: error }, | |
| ); |
— qwen3.7-max via Qwen Code /review
✅ Local verification —
|
| Check | Result |
|---|---|
…/audio-capture/dist/index.js present |
✅ |
real prebuilds/linux-x64/@qwen-code+audio-capture.node (locally compiled) |
✅ |
runtime dep node-gyp-build bundled |
✅ |
scripts / devDependencies stripped from copied package.json |
✅ |
*.test.* / *.spec.* filtered out of the copy |
✅ |
dist/package.json → bundledDependencies: ['@qwen-code/audio-capture'] |
✅ |
optionalDependencies['@qwen-code/audio-capture'] = 0.19.0 preserved |
✅ |
2. Mirror-install load test under tmux (the actual fix)
Copied only the bundled node_modules into an isolated /tmp dir (the workspace root has @qwen-code/audio-capture as a symlink, so isolation is required to avoid a false pass), then import('@qwen-code/audio-capture'):
===== CASE 1: WITH bundled audio-capture (this PR) =====
RESOLVE: OK — imported @qwen-code/audio-capture
getPlatformBackendName(): alsa-pulse
createNativeAudioCaptureBackend(): OK — real native addon loaded
microphoneAuthorizationStatus(): unknown
NATIVE: OK
===== CASE 2: WITHOUT bundle (pre-fix mirror install) =====
RESOLVE: FAIL — ERR_MODULE_NOT_FOUND Cannot find package '@qwen-code/audio-capture'
CASE 2 is exactly the failure mode this PR fixes — and the precondition the new error-rewrite targets.
3. Packaging gate A/B (prebuild missing)
| Mode | Behavior |
|---|---|
QWEN_REQUIRE_AUDIO_CAPTURE_PREBUILD=1 (official) |
fails hard → Required audio capture package artifact not found … Cannot publish package without native voice capture. (exit 1) |
| default (fork / mirror) | warns, omits bundledDependencies, packaging still succeeds (exit 0) |
4. Tests + error-rewrite A/B
native-audio-recorder.test.ts— 12/12 ✅ ·scripts/tests/package-assets.test.js— 10/10 ✅ ·tsc --noEmit(cli) — 0 errors ✅- A/B: disabling
explainMissingNativePackageflips the 3 mirror-message tests to red (they receive the rawCannot find package/missing @qwen-code/audio-captureerror), while the 2 "does not rewrite wrapped-addon / start failures" guards stay green — the rewrite is load-bearing and correctly narrowed.
Notes (merge reference, not blockers)
- The gate is per-build-platform: required mode only checks the prebuild for the current platform.
release.ymlnow downloads all platform prebuilds as a CI artifact before bundling (the step reorder in this PR), so the official multi-arch release bundles every platform. My local run had only the linux-x64 prebuild I compiled, so it bundled that one — the mechanism is verified; the matrix is covered by CI. @qwen-code/audio-captureis listed in bothbundledDependenciesandoptionalDependencies— intentional: npm uses the bundled copy (no registry fetch needed) while the optional entry keeps non-bundled installs working.
Verdict: ✅ Safe to merge. The bundled package is self-contained and loads its real native addon in an isolated mirror-install environment; the official-release gate fails closed; forks degrade gracefully; the new error message is correctly targeted; all tests pass and the A/Bs confirm both behaviors are load-bearing.
🇨🇳 中文版（合并参考）
✅ 本地验证 — fix(packaging): bundle audio capture for mirror installs
在隔离 worktree（commit 5ab73495b）上用真实 npm ci + 真实 npm run bundle + 真实 prepare:package 完成验证，随后在 tmux 中、于隔离的「镜像源安装」环境里（无 workspace node_modules）加载打包进去的原生插件。原生插件在本地编译，因此整条链路是端到端真实跑通的。
这个修复做了什么： 把 @qwen-code/audio-capture（它的 dist/、对应平台的 prebuilds/*.node、以及运行时依赖 node-gyp-build）复制进 dist/node_modules/@qwen-code/audio-capture，并标记为 bundledDependencies，从而即使从未同步该可选原生包的镜像源安装也能正常使用语音采集。官方发布在缺少 prebuild 时会让打包硬失败（QWEN_REQUIRE_AUDIO_CAPTURE_PREBUILD=1），fork 则优雅降级。同时把原始的「找不到模块」错误改写为镜像源提示。
1. 真实打包产物 —— required 模式（官方发布）—— 8/8 ✅
| 检查项 | 结果 |
|---|---|
…/audio-capture/dist/index.js 存在 |
✅ |
真实 prebuilds/linux-x64/@qwen-code+audio-capture.node（本地编译） |
✅ |
运行时依赖 node-gyp-build 一并打包 |
✅ |
复制的 package.json 中 scripts / devDependencies 已剥离 |
✅ |
*.test.* / *.spec.* 已从复制中过滤 |
✅ |
dist/package.json → bundledDependencies: ['@qwen-code/audio-capture'] |
✅ |
保留 optionalDependencies['@qwen-code/audio-capture'] = 0.19.0 |
✅ |
2. tmux 中的镜像源安装加载测试（修复本身）
只把打包出的 node_modules 复制到隔离的 /tmp 目录（workspace 根目录里 @qwen-code/audio-capture 是符号链接，必须隔离才能避免假通过），再 import('@qwen-code/audio-capture')：
===== CASE 1: 带 bundle（本 PR）=====
RESOLVE: OK — imported @qwen-code/audio-capture
getPlatformBackendName(): alsa-pulse
createNativeAudioCaptureBackend(): OK — 真实原生插件已加载
microphoneAuthorizationStatus(): unknown
NATIVE: OK
===== CASE 2: 不带 bundle（修复前的镜像源安装）=====
RESOLVE: FAIL — ERR_MODULE_NOT_FOUND Cannot find package '@qwen-code/audio-capture'
CASE 2 正是本 PR 要修复的故障，也是新错误改写逻辑所针对的前置条件。
3. 打包 gate 的 A/B（缺少 prebuild 时）
| 模式 | 行为 |
|---|---|
QWEN_REQUIRE_AUDIO_CAPTURE_PREBUILD=1（官方） |
硬失败 → Required audio capture package artifact not found … Cannot publish package without native voice capture.（退出码 1） |
| 默认（fork / 镜像） | 警告，省略 bundledDependencies，打包仍成功（退出码 0） |
4. 测试 + 错误改写 A/B
native-audio-recorder.test.ts— 12/12 ✅；scripts/tests/package-assets.test.js— 10/10 ✅；tsc --noEmit(cli) — 0 错误 ✅- A/B： 关闭
explainMissingNativePackage后，3 个镜像源提示测试变红（拿到的是原始的Cannot find package/missing @qwen-code/audio-capture错误），而 2 个「不改写 包装过的插件加载/启动失败」守护测试仍为绿 —— 说明改写既是必要的，又被正确收窄。
说明（合并参考，非阻塞项）
- 该 gate 是按构建平台判定的：required 模式只检查当前平台的 prebuild。
release.yml现在会在打包之前把所有平台的 prebuild 作为 CI artifact 下载下来（本 PR 的步骤重排），因此官方多架构发布会打包所有平台。我本地只有自己编译的 linux-x64 prebuild，所以只打包了它 —— 机制已验证，矩阵由 CI 覆盖。 @qwen-code/audio-capture同时出现在bundledDependencies和optionalDependencies中 —— 这是有意为之：npm 会使用打包进来的副本（无需从 registry 拉取），而 optional 条目让未打包的安装方式仍可工作。
结论：✅ 可以合并。 打包出的包是自包含的，并能在隔离的镜像源安装环境中加载其真实原生插件；官方发布 gate 失败即拦截（fail-closed）；fork 优雅降级；新错误信息定位准确；全部测试通过，A/B 证明两处行为都是必要的。
| }); | ||
| fs.cpSync( | ||
| path.join(addonSrc, 'prebuilds'), | ||
| path.join(addonDest, 'prebuilds'), |
There was a problem hiding this comment.
[Suggestion] The prebuilds cpSync copies the entire prebuilds/ directory without a filter, while the dist/ copy above explicitly filters out test files. Any non-.node file that ends up in prebuilds/ (debug logs, .env files, source maps, build metadata) ships verbatim into the published tarball.
| path.join(addonDest, 'prebuilds'), | |
| fs.cpSync( | |
| path.join(addonSrc, 'prebuilds'), | |
| path.join(addonDest, 'prebuilds'), | |
| { | |
| ...copyOpts, | |
| filter: (src) => | |
| fs.statSync(src, { throwIfNoEntry: false })?.isDirectory() || | |
| src.endsWith('.node'), | |
| }, | |
| ); |
— qwen3.7-max via Qwen Code /review
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
| const defaultRootDir = path.resolve(__dirname, '..'); | ||
| const nodeRequire = createRequire(import.meta.url); |
There was a problem hiding this comment.
[Suggestion] createRequire(import.meta.url) anchors module resolution to scripts/prepare-package.js, so nodeRequire.resolve('dep/package.json') walks up from scripts/ to the monorepo root node_modules/. This happens to work in a hoisted npm workspace but would break in pnpm with node-linker=isolated, Yarn PnP, or any layout where the dependency is installed inside packages/audio-capture/node_modules/ but not at the root. Even in hoisted setups, a different version at the root vs. what audio-capture pinned would silently bundle the wrong version.
Anchor the require to the audio-capture package directory to match the resolution context the runtime code itself uses:
| const nodeRequire = createRequire(import.meta.url); | |
| const nodeRequire = createRequire(path.join(defaultRootDir, 'packages', 'audio-capture', 'package.json')); |
— qwen3.7-max via Qwen Code /review
| JSON.stringify(addonPkg, null, 2) + '\n', | ||
| ); | ||
| fs.cpSync(path.join(addonSrc, 'dist'), path.join(addonDest, 'dist'), { | ||
| ...copyOpts, |
There was a problem hiding this comment.
[Suggestion] The test-file exclusion regex /\.(test|spec)\.(d\.)?[mc]?[jt]s(\.map)?$/ appears in two places: once inside the hasFileMatching validation predicate (line 177) and again here in the fs.cpSync filter. They must stay in sync — if the pattern needs updating, both locations need to change, and a missed update in either direction creates a silent inconsistency (validation passes but files leak into the tarball, or vice versa).
Extract to a named constant:
| ...copyOpts, | |
| // (at module level) | |
| const TEST_FILE_RE = /\.(test|spec)\.(d\.)?[mc]?[jt]s(\.map)?$/; | |
| // ...then use in both places: | |
| filter: (src) => !TEST_FILE_RE.test(src), |
— qwen3.7-max via Qwen Code /review
| verbatimSymlinks: false, | ||
| }; | ||
|
|
||
| fs.rmSync(addonDest, { recursive: true, force: true }); |
There was a problem hiding this comment.
[Suggestion] fs.rmSync(addonDest, ...) runs after all the validation return false paths. If a prior successful preparePackage run already created dist/node_modules/@qwen-code/audio-capture/, a subsequent run that bails early (e.g., missing prebuilds in non-required mode) leaves those stale files on disk. The dist package.json correctly omits bundledDependencies, but printPackageStructure() still lists the stale directory in the packaging summary — potentially misleading developers iterating locally.
Move the cleanup to the top of the function so stale artifacts are always removed:
| fs.rmSync(addonDest, { recursive: true, force: true }); | |
| fs.rmSync(addonDest, { recursive: true, force: true }); | |
| for (const requiredPath of requiredPaths) { |
— qwen3.7-max via Qwen Code /review
| }, | ||
| ); | ||
|
|
||
| for (const [dependencyName, dependencySrc] of dependencySources) { |
There was a problem hiding this comment.
[Suggestion] The dependency copy loop only copies direct dependencies of @qwen-code/audio-capture — it never recurses into their sub-dependencies. This works today because node-gyp-build declares zero runtime dependencies, but that invariant is neither validated nor documented. A future npm update that introduces transitive deps would silently produce a broken published package.
Consider adding a validation step that asserts each copied dependency has no sub-dependencies:
for (const [dependencyName, dependencySrc] of dependencySources) {
const depPkgPath = path.join(dependencySrc, 'package.json');
if (fs.existsSync(depPkgPath)) {
const depPkg = JSON.parse(fs.readFileSync(depPkgPath, 'utf8'));
const subDeps = Object.keys(depPkg.dependencies ?? {});
if (subDeps.length > 0) {
throw new Error(
`audio capture dependency '${dependencyName}' has sub-dependencies ` +
`(${subDeps.join(', ')}). Recursive bundling is not implemented.`
);
}
}
fs.cpSync(
dependencySrc,
path.join(addonDest, 'node_modules', dependencyName),
copyOpts,
);
}— qwen3.7-max via Qwen Code /review
|
|
||
| it('does not rewrite wrapped native addon load failures as missing packages', async () => { | ||
| const loadError = new Error( | ||
| "Native audio capture addon could not be loaded. Reinstall @qwen-code/audio-capture, or use the SoX fallback. (Cannot find module 'node-gyp-build')", |
There was a problem hiding this comment.
[Suggestion] The synthetic error uses "or use the SoX fallback", but the real loadBinding() error in packages/audio-capture/src/index.ts says "or run \"npm run build\" in packages/audio-capture". The test passes because isMissingNativePackageError returns false based on the startsWith check, not because it exercises the real error chain. If a future change introduces false positives on the actual error format, this test would not catch it.
| "Native audio capture addon could not be loaded. Reinstall @qwen-code/audio-capture, or use the SoX fallback. (Cannot find module 'node-gyp-build')", | |
| "Native audio capture addon could not be loaded. Reinstall @qwen-code/audio-capture, or run \"npm run build\" in packages/audio-capture. (Cannot find module 'node-gyp-build')", |
— qwen3.7-max via Qwen Code /review
✅ Local real-build verification — PR #5747Maintainer verification before merge. This is a packaging / release fix with no TUI surface, so the meaningful "real test" is running the actual publish-prep script (
1. Real
|
| Injected error | Result |
|---|---|
ERR_MODULE_NOT_FOUND mentioning @qwen-code/audio-capture |
✅ rewritten to the mirror/registry hint; keeps pkg name; original message preserved in (…) |
MODULE_NOT_FOUND (CJS) for the pkg |
✅ rewritten |
generic Cannot find package '@qwen-code/audio-capture' (no code) |
✅ rewritten |
EACCES: permission denied (unrelated) |
✅ passed through unchanged — not rewritten |
ERR_MODULE_NOT_FOUND for a different package |
✅ not rewritten — no over-matching |
Actual rewritten text: "Native voice capture package '@qwen-code/audio-capture' is missing. If Qwen Code was installed from a mirror or private registry, the registry may not have synced this optional package. Reinstall from https://registry.npmjs.org … (<original error>)".
3. release.yml — the root-cause fix (reviewed)
The "Download audio capture prebuilds" step is moved before "Build Bundle and Prepare Package" — previously prepare ran first, so the prebuilds were never present to bundle. It also sets QWEN_REQUIRE_AUDIO_CAPTURE_PREBUILD to 1 only when github.repository == 'QwenLM/qwen-code' (official release fails loudly; forks stay graceful). Coherent and matches the script behavior verified above. (CI-only; not run locally.)
4. Unit tests — 23 pass
scripts/tests/package-assets.test.js→ 11 (realpreparePackagevs fixtures: bundles, omits when missing, removes stale, fails when required+missing for prebuilds / empty dist / empty prebuilds)packages/cli/src/ui/voice/native-audio-recorder.test.ts→ 12
5. Static checks — eslint exit 0, prettier --check clean, tsc --noEmit (cli) exit 0.
Coverage note
Not run (out of scope, matching the PR): a full real-prebuild release publish across every platform (CI/release matrix territory), and a live mic/voice run. The native prebuild here was a stub since .node binaries require a platform build/download — but the publish-prep copy logic is byte-agnostic, so the bundling is faithfully exercised; CI supplies the real prebuilds.
Verdict
Reproduces and behaves as described. The real publish-prep bundles the native package (incl. node-gyp-build, prebuilds, stripped manifest, no tests) when artifacts are present, skips + cleans up when absent, and the required-mode gate fails the release loudly; the error rewrite is helpful and correctly targeted; the release.yml reorder addresses the root cause. Tests / typecheck / lint / prettier are green. Safe to merge from this verification's standpoint.
🇨🇳 中文版（完整对应）
✅ 本地真实构建验证 — PR #5747
维护者合并前验证。这是一个打包 / 发布修复，没有 TUI 可见面,所以有意义的"真实测试"是在真实 bundle 上运行实际的发布准备脚本 (prepare-package.js) 并检查真实产物,外加驱动真实错误路径。我正是这么做的——真实构建 CLI bundle,在三种产物状态下跑真实发布准备,并驱动真实的 native-recorder 错误改写。
环境: PR head
ba21195e(merge-based350dd8d),独立 worktree + 独立npm ci,真实npm run bundle。native 预构建用的是桩.node(发布准备脚本逐字节复制.node文件,从不校验二进制——作者自己的测试也这么做;CI 提供真实的各平台 prebuilds)。macOS,node v22。
1. 真实 prepare-package.js 运行 —— 头号打包行为(3 种产物状态)
A —— 产物存在: 跑 node scripts/prepare-package.js;真实 dist/ 产物:
dist/package.json→bundledDependencies: ["@qwen-code/audio-capture"](同时仍在optionalDependencies: "0.19.0"中保留回退路径)dist/node_modules/@qwen-code/audio-capture/被填充:dist/*.js、prebuilds/darwin-arm64/*.node,以及内嵌的node_modules/node-gyp-build/(运行时依赖被打进去)- 内嵌的
package.json→scripts被剥离、devDependencies被剥离,保留dependencies: {node-gyp-build} - 测试文件被排除(
platform.test.js未被复制)
B —— 产物缺失、非 required: 重跑 → Warning: audio capture package artifact not found …;bundledDependencies 不存在;陈旧的 dist/node_modules/@qwen-code/audio-capture/ 被移除(顶部 rmSync(addonDest) 清理);optionalDependencies 回退保留。端到端的陈旧清理也确认了(打包 → 删产物 → 重新 prepare → 消失)。
C —— QWEN_REQUIRE_AUDIO_CAPTURE_PREBUILD=1 + 产物缺失: 发布闸门触发——
Error: Required audio capture package artifact not found at …/prebuilds. Cannot publish package without native voice capture.
进程以 1 退出(官方仓库的真实发布会大声失败,而不是发出坏掉的语音 CLI)。
2. 真实错误改写 —— native-audio-recorder.ts(独立 harness）
用注入的失败 loadBackend 驱动真实的 createNativeAudioRecorder({ loadBackend }),读回 start() 的真实错误。改写正确且有针对性:
| 注入的错误 | 结果 |
|---|---|
ERR_MODULE_NOT_FOUND 含 @qwen-code/audio-capture |
✅ 改写为镜像/registry 提示;保留包名;原始消息在 (…) 中保留 |
MODULE_NOT_FOUND(CJS)含该包 |
✅ 改写 |
通用 Cannot find package '@qwen-code/audio-capture'(无 code) |
✅ 改写 |
EACCES: permission denied(无关) |
✅ 原样透传——未改写 |
ERR_MODULE_NOT_FOUND 但是别的包 |
✅ 未改写——无过度匹配 |
3. release.yml —— 根因修复(已审阅）
"Download audio capture prebuilds" 步骤被移到 "Build Bundle and Prepare Package" 之前——此前 prepare 先跑,所以 prebuilds 根本不在场可供打包。同时仅当 github.repository == 'QwenLM/qwen-code' 时把 QWEN_REQUIRE_AUDIO_CAPTURE_PREBUILD 设为 1(官方发布大声失败;fork 保持优雅)。与上面验证的脚本行为一致连贯。(仅 CI;本地未跑。)
4. 单元测试 —— 23 通过
scripts/tests/package-assets.test.js→ 11(真实preparePackage对 fixtures:打包、缺失时省略、清理陈旧、required+缺失时失败——prebuilds / 空 dist / 空 prebuilds 三种)packages/cli/src/ui/voice/native-audio-recorder.test.ts→ 12
5. 静态检查 —— eslint exit 0、prettier --check clean、tsc --noEmit（cli）exit 0。
覆盖说明
未做(范围外,与 PR 一致):带真实 prebuild 的全平台完整 release 发布(CI/release matrix 范畴),以及真机麦克风/语音运行。这里 native 预构建用的是桩,因为 .node 二进制需要平台构建/下载——但发布准备的复制逻辑是逐字节的,所以打包被忠实地验证了;CI 提供真实 prebuilds。
结论
完整复现、行为符合描述。 产物存在时真实发布准备会打包 native 包(含 node-gyp-build、prebuilds、剥离后的 manifest、无测试),缺失时跳过 + 清理,required 模式闸门让发布大声失败;错误改写有用且精准定向;release.yml 的重排解决了根因。测试 / typecheck / lint / prettier 全绿。就本次验证而言,可以安全合并。
🤖 Verified locally with the real build. Method: isolated worktree + self-contained npm ci + real npm run bundle + running the real scripts/prepare-package.js across 3 artifact states + an independent harness driving the real native-recorder error path + unit suites + eslint/prettier/tsc.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
3 participants
What this PR does
Bundles the native voice capture package into the prepared npm package when its build artifacts are available, so the published CLI package can carry the native recorder instead of relying on mirror registries to resolve the package separately. It also rewrites missing native package errors into an actionable message that points users at mirror/private registry optional dependency sync issues.
Why it's needed
A mirror registry can make the main CLI package available before
@qwen-code/audio-captureis synced. Since the native package is optional, npm can complete the install while skipping it, leaving/voiceto fail at runtime and then fall back to SoX. Bundling the package with the main publish artifact makes mirror installs more reliable, and the improved error text makes the remaining failure mode clear.Reviewer Test Plan
How to verify
Confirm that preparing a publish package with native audio capture artifacts copies the native package into the package's bundled dependencies and keeps its runtime
node-gyp-builddependency available. Confirm that a missing@qwen-code/audio-captureimport reports a mirror/private registry install hint instead of only returning Node's generic package resolution error.Evidence (Before & After)
Before: the prepared package only referenced
@qwen-code/audio-captureas an optional dependency, so mirrors that had the main package but not the native package could produce successful installs with broken native voice capture. Missing native package errors surfaced as genericCannot find package '@qwen-code/audio-capture'messages.After: focused tests verify the prepared package declares and copies the native capture package when artifacts are present, including
node-gyp-build, and verify the native recorder explains the mirror/private registry failure mode when the package is missing.Tested on
Environment (optional)
Local macOS worktree with Node.js/npm workspace install.
Risk & Scope
Linked Issues
Fixes #5742
中文说明
What this PR does
当 native 语音采集构建产物存在时，把 native 语音采集包打进准备发布的 npm 包里，让 CLI 主包自己携带 native recorder，而不是依赖镜像源再单独解析这个包。同时，把 native 包缺失错误改写成可操作的提示，明确指出镜像源或私有 registry 可能没有同步 optional dependency。
Why it's needed
镜像源可能先同步 CLI 主包，但还没有同步
@qwen-code/audio-capture。因为 native 包是 optional dependency，npm 可能跳过它但仍然安装成功，导致/voice运行时失败并回退到 SoX。把 native 包随主发布产物一起携带，可以让镜像安装更可靠；改进错误文案也能让剩余失败模式更容易定位。Reviewer Test Plan
How to verify
确认在存在 native audio capture 构建产物时，准备发布包会把 native 包复制进 bundled dependencies，并保留它运行时需要的
node-gyp-build。确认当@qwen-code/audio-captureimport 缺失时，错误会提示镜像源或私有 registry 同步问题，而不是只返回 Node 的通用包解析错误。Evidence (Before & After)
Before：准备发布的包只把
@qwen-code/audio-capture作为 optional dependency 引用。如果镜像源有主包但没有 native 包，安装会成功但 native 语音采集会坏。缺少 native 包时，错误只是通用的Cannot find package '@qwen-code/audio-capture'。After：聚焦测试验证准备发布包会在产物存在时声明并复制 native capture 包，包括
node-gyp-build；同时验证 native recorder 在包缺失时会解释镜像源或私有 registry 的失败模式。Tested on
Environment (optional)
本地 macOS worktree，使用 Node.js/npm workspace install。
Risk & Scope
Linked Issues
Fixes #5742