[DRAFT - DO NOT MERGE] CI smoke test: standalone-update Windows fix#4697
Closed
yiliang114 wants to merge 18 commits into
Closed
[DRAFT - DO NOT MERGE] CI smoke test: standalone-update Windows fix#4697yiliang114 wants to merge 18 commits into
yiliang114 wants to merge 18 commits into
Conversation
Standalone installs (via install-qwen-standalone.sh) previously fell through to npm global detection, causing auto-update to run `npm install -g` which doesn't update the standalone binary. Now the CLI detects standalone installs by looking for manifest.json in ancestor directories, then performs a self-update: download archive from OSS/GitHub, verify SHA256, extract, and atomically replace the installation directory. Includes: input validation (semver + target allowlist), lockfile for concurrency, Windows deferred replacement via helper script, and tar path traversal protection. Closes QwenLM#4627
- Smoke test: spawn new binary with --version after extraction, before replacing the installation. Rejects broken/wrong-arch packages. - Rollback: keep .old directory after updates. Export rollbackStandaloneUpdate() for future /doctor rollback integration. - Signature: Ed25519 verification of SHA256SUMS via node:crypto. Embeds public key; CI signs with scripts/sign-release.sh. Graceful degradation when .sig is not yet published (transition period). Set QWEN_REQUIRE_SIGNATURE=1 to enforce strict mode.
- Fix TS4111: use bracket notation for process.env index signature - Fix CodeQL: validate paths for cmd.exe metacharacters before bat script - Fix Windows lock race: keep lock alive during deferred swap, release in bat script after move completes
- handleAutoUpdate: handleUpdateFailed/handleUpdateSuccess now read the message from the event payload instead of hardcoding text. Windows deferred users now see the correct "applied after exit" message. - Add standalone-update.test.ts with 4 tests covering rollback scenarios.
1. SHA256SUMS: handle GNU coreutils binary-mode `*` prefix in filename 2. acquireLock: treat NaN (empty/corrupt lock file) as stale, not held 3. tryFetch: consume response body on non-OK status to release sockets 4. Windows bat: add 30s timeout to PID wait loop preventing infinite hang 5. Finally block: clean orphaned .new directory on Windows error path
Tests previously asserted hardcoded handler text. After making handlers read messages from event payload, the assertions need to reflect the emitted payload values instead.
…itable When a global npm install requires sudo (prefix not writable), the auto-update now falls back to the standalone path instead of spawning a doomed `npm install -g` that always EACCES. - installationInfo.ts: detect writable prefix with fs.accessSync; return isStandalone=true with fallback dir when not writable - standalone-update.ts: support first-time migration (no existing manifest), detect platform target, create bin wrapper, refuse to overwrite unmanaged directories Closes QwenLM#4643
…headers - Fix Windows deferred update: move pendingDir cleanup to catch block so it only runs on error, not on successful deferred path - Add ensurePathInShellRc: auto-append ~/.local/bin to shell rc on npm→standalone migration so the wrapper is actually discoverable - Wire rollbackStandaloneUpdate into /doctor rollback subcommand - Add license headers to all 4 new source files - Fix spawnAndCapture double-settle with settled flag - Add tests for ensureBinWrapper and ensurePathInShellRc - Clarify signature verification comments (test key, not production)
The mustTranslateKeys test requires zh-CN and zh-TW translations for all built-in command descriptions. Add translations for the new rollback subcommand and its user-facing messages.
The ensureBinWrapper Unix-wrapper test asserts on POSIX file mode bits (mode & 0o111) which Windows NTFS does not implement. Skip Unix-specific tests when running on Windows runners.
- Add assertPathWithin() and resolve install paths before spawning the smoke-test process. Closes the CodeQL "shell command built from environment values" finding by guaranteeing nodeBin/cliBin stay inside the freshly extracted install directory. - Add performStandaloneUpdate test coverage for the four pre-flight failure cases (bad version, missing manifest, unknown target, concurrent lock) so the core update path is no longer untested.
…ck safety, shell injection, lock race, test coverage Five issues raised in PR QwenLM#4629 review (2026-06-01): 1. Windows bat: add errorlevel checks after each move, rollback on second-move failure, timestamped log to qwen-update.log 2. Unix rollback: inner try/catch on recovery rename; compound error with manual mv instructions if both renames fail 3. Shell injection: assertSafeForShellEmbed() validates paths before embedding in /bin/sh wrappers and shell rc (platform-aware: allows backslash on Windows) 4. Lock-file race: bat writes .swap sentinel before rename; acquireLock refuses to reclaim stale-PID lock while sentinel exists 5. Test coverage: 4 new handleAutoUpdate standalone-path tests (done, deferred, error, npm-not-entered) + 3 shell-safety rejection tests
Replace fixed-name .qwen-code-update-staging with mkdtempSync random suffix — an attacker who can write to parentDir can no longer pre-create a symlink to hijack recursive deletion. Also adds lstat guard before cleanup rmSync (defense-in-depth) and fixes a potential tempDir leak if mkdtempSync for extractDir fails.
…coverage, cleanup logging - Add MAX_DOWNLOAD_BYTES (512 MB) streaming size guard in downloadToFile to prevent resource exhaustion from oversized responses - Extend UNSAFE_SHELL_META_UNIX to also reject semicolon and single-quote - Add debugLogger.warn when extractDir cleanup is skipped (unexpected type) - Add Windows backslash acceptance test for assertSafeForShellEmbed
…l, shell-rc validation Defense-in-depth improvements from independent code review: - Explicitly set preservePaths:false on tar extract to prevent symlink path traversal - Check err.signal in spawnAndCapture for cross-platform timeout detection - Validate binDir in ensurePathInShellRc independently of calling context
…ollback result type 1. Separate ARCHIVE_TIMEOUT_MS (5 min) from FETCH_TIMEOUT_MS (30s) so large archive downloads on slow networks don't time out prematurely 2. Capture stderr from smoke-test execFile and include it in the error message so users see the actual crash reason (missing lib, etc.) 3. Change rollbackStandaloneUpdate return from boolean to discriminated result type (ok/reason/detail) so doctorCommand shows accurate messages for each failure mode instead of a generic "not found"
The 'rejects standaloneDir with embedded double-quote' test called fs.mkdirSync on a path containing `"`, which ENOENTs on Windows since NTFS forbids `"` in path components. The validator assertSafeForShellEmbed runs synchronously before any FS access in ensureBinWrapper, so the pre-mkdir was vestigial — removing it makes the test pass on Windows without weakening the assertion.
Contributor
📋 Review SummaryThis PR is marked as [DRAFT - DO NOT MERGE] and exists solely as a CI smoke test to validate a Windows fix before cherry-picking into #4629. However, there is a critical discrepancy: the PR description states only 🔍 General Feedback
🎯 Specific Feedback🔴 CriticalIssue: PR scope mismatch requires investigation
Issue: Referenced files do not exist
Issue: Package-lock.json change appears unrelated
🟡 HighIssue: Production code changes in a "CI smoke test" PR
Issue: Localization file changes without context
🟢 MediumIssue: Test file changes cannot be verified
🔵 LowSuggestion: Clarify PR intent and scope
Suggestion: Separate unrelated changes
✅ Highlights
📝 RecommendationDO NOT MERGE this PR in its current state. The discrepancy between the stated scope (1 test file, +3/-1 lines) and the actual changes (16 files including production code) requires investigation before proceeding. Recommended actions:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+487
to
+491
| spawn('cmd.exe', ['/c', scriptPath], { | ||
| detached: true, | ||
| stdio: 'ignore', | ||
| windowsHide: true, | ||
| }).unref(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Verify on Windows CI that removing the
fs.mkdirSyncsetup from the'rejects standaloneDir with embedded double-quote'test (one line inpackages/cli/src/utils/standalone-update.test.ts) resolves the ENOENT failure onTest (windows-latest, Node 22.x)introduced by #4629.Background
#4629's most recent commit (47dc004) added a test that pre-creates a directory named
qwen"code. NTFS forbids"in path components, sofs.mkdirSyncENOENTs before the actual assertion runs. The validatorassertSafeForShellEmbedis synchronous and runs inensureBinWrapperbefore any FS access (seestandalone-update.ts:526), so the pre-creation is vestigial.Diff
+3 / -1 in
packages/cli/src/utils/standalone-update.test.tsonly.Local verification (darwin)
vitest run standalone-update.test.ts→ 20/20 passtsc --noEmit -p packages/cli→ 0 errorseslint→ 0 warningsPlan
Once
Test (windows-latest, Node 22.x)here passes:worktree-swift-elm-iqh0branchcc — auto-close after CI smoke test