From 3a46420d59a76409b267da65d30833fe06cd39ce Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 26 Aug 2026 16:12:36 +0000 Subject: [PATCH] fix: repair the Windows test lane's standing failures (#10064) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: repair the Windows test lane's standing failures Fixes the eight failure clusters that keep Test (windows-latest) red on the ECS nightly lane — three product bugs and five test bugs. Product: - extension-git-client: GIT_CONFIG_GLOBAL was os.devNull, whose win32 spelling ('\\.\nul') Git for Windows rejects with "Invalid argument", failing every restricted-env git command on Windows. Use the literal '/dev/null', which git's compat layer maps on every platform. - sessionArtifacts: the pre-open/post-open identity check compared numeric dev/ino. NTFS file ids are 64-bit and lose precision above 2^53 as Numbers, so two files created close together can round to the same ino and defeat the swap (TOCTOU) detection. Compare bigint stats. - dws-event-stream: clearing a stale stderr error on a healthy stdout line was gated on the child still being alive, but on Windows the pipe routinely drains only after exitCode is set, so the final healthy line never cleared it and close reported the stale error. A clean (code 0) exit now certifies recorded errors as stale at close. Tests: - daemon-git-worktree-guard: fixtures pasted native Windows paths into command strings unquoted; on the bash lane the tokenizer consumes the backslashes (C:\t\repo -> C:trepo), so resolvable targets tested as unresolvable ones and cd shifts silently failed in place. Interpolate through cmdPath(), which spells them with forward slashes on win32 — what bash preserves and Windows resolves. Assertions keep the native spelling; the guard canonicalizes targets through realpath. - review/lib/worktree: git marks a linked worktree's .git gitfile hidden on Windows and CREATE_ALWAYS on a hidden file fails with EPERM; rewrite gitfiles via unlink-then-write. - config: compare getCwd() against path.resolve(targetDir) — the constructor resolves it, so the POSIX fixture comes back drive-qualified on win32. - SessionMessageHandler: complete the pathToFileURL shim's mirror case — parse POSIX-spelled fixture paths with { windows: false } on win32 so expected file:// URLs read the same on every host. - node-repl.semantics: dispose the kernel before removing its cwd (a process's cwd cannot be removed on Windows) and retry the removal across the child's asynchronous teardown. * fix: witness the Windows-lane fixes per review feedback (#10064) - Document why the workspace-status identity fstat cannot be folded into the Number stat (bigint stats truncate mtimeMs and return BigInt fields). - Add a clean-exit fixture + test proving the dws consumer reports the code-0 process error instead of a stale stderr error. - Add an injected-stats test detecting swaps whose file ids differ only above 2^53, so a numeric identity revert cannot ship green. - Route the remaining unquoted/eval'd env-var assignment payloads in the worktree guard tests through cmdPath so they stay resolvable on the win32+bash lane. * test(acp-bridge): consolidate the duplicated forwarding lstat spy (#10064) --------- Co-authored-by: qwen-code-ci-bot --- .../acp-bridge/src/sessionArtifacts.test.ts | 114 +++++++++++++++++- packages/acp-bridge/src/sessionArtifacts.ts | 20 ++- .../channels/dws/src/dws-event-stream.test.ts | 23 ++++ packages/channels/dws/src/dws-event-stream.ts | 10 +- .../fixtures/dws-event-clean-exit-source.mjs | 15 +++ .../src/commands/review/lib/worktree.test.ts | 17 ++- .../serve/daemon-git-worktree-guard.test.ts | 113 ++++++++++------- packages/core/src/config/config.test.ts | 4 +- .../src/extension/extension-git-client.ts | 7 +- .../node-repl/src/node-repl.semantics.test.ts | 18 ++- .../handlers/SessionMessageHandler.test.ts | 7 ++ 11 files changed, 284 insertions(+), 64 deletions(-) create mode 100644 packages/channels/dws/src/fixtures/dws-event-clean-exit-source.mjs diff --git a/packages/acp-bridge/src/sessionArtifacts.test.ts b/packages/acp-bridge/src/sessionArtifacts.test.ts index 8e43369525..ffb0375058 100644 --- a/packages/acp-bridge/src/sessionArtifacts.test.ts +++ b/packages/acp-bridge/src/sessionArtifacts.test.ts @@ -6,7 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { createHash } from 'node:crypto'; -import { promises as fs } from 'node:fs'; +import { promises as fs, type BigIntStats, type Stats } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { pathToFileURL } from 'node:url'; @@ -46,6 +46,30 @@ describe('SessionArtifactStore', () => { .slice(0, 16); } + // Forward the options through: the store lstats with { bigint: true } + // for the identity check, and a spy that dropped it would hand back + // numeric stats that never equal the bigint fstat — hiding a broken + // comparison behind a type mismatch. + function spyOnLstatForwarding( + hook: ( + entry: Parameters[0], + stat: Stats | BigIntStats, + ) => void | Promise, + ) { + const originalLstat = fs.lstat.bind(fs); + return vi.spyOn(fs, 'lstat').mockImplementation((async ( + entry: Parameters[0], + options?: Parameters[1], + ) => { + const stat = await originalLstat( + entry, + options as Parameters[1], + ); + await hook(entry, stat); + return stat; + }) as typeof fs.lstat); + } + it('lists, removes, and idempotently ignores missing artifact deletes', async () => { const store = new SessionArtifactStore({ sessionId: 's1', @@ -3244,16 +3268,13 @@ describe('SessionArtifactStore', () => { vi.useFakeTimers(); vi.setSystemTime(new Date(Date.now() + 6_000)); - const originalLstat = fs.lstat.bind(fs); let swapped = false; - const lstatSpy = vi.spyOn(fs, 'lstat').mockImplementation(async (entry) => { - const stat = await originalLstat(entry); + const lstatSpy = spyOnLstatForwarding(async (entry) => { if (!swapped && String(entry) === realTarget) { swapped = true; await fs.writeFile(replacement, 'after'); await fs.rename(replacement, target); } - return stat; }); try { @@ -3266,6 +3287,89 @@ describe('SessionArtifactStore', () => { } }); + it('detects a swap whose file ids differ only above 2^53', async () => { + const store = new SessionArtifactStore({ + sessionId: 's7-bigint-identity', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'big.txt'), 'content'); + const created = await store.upsertMany([ + { title: 'Big', workspacePath: 'big.txt' }, + ]); + const artifactId = created.changes[0]!.artifactId; + const realTarget = await fs.realpath(path.join(workspace, 'big.txt')); + + // Injected ids on both sides of 2^53: the replacement rounds to the SAME + // Number as the original, so only the bigint comparison can see the swap. + const preOpenIno = 2n ** 53n; + const injectIno = (stat: Stats | BigIntStats, ino: bigint): void => { + stat.ino = typeof stat.ino === 'bigint' ? ino : Number(ino); + }; + const lstatSpy = spyOnLstatForwarding((entry, stat) => { + if (String(entry) === realTarget) { + injectIno(stat, preOpenIno); + } + }); + const originalOpen = fs.open.bind(fs); + const openSpy = vi.spyOn(fs, 'open').mockImplementation((async ( + entry: Parameters[0], + flags?: Parameters[1], + mode?: Parameters[2], + ) => { + const handle = await originalOpen( + entry, + flags as Parameters[1], + mode, + ); + if (String(entry) !== realTarget) { + return handle; + } + const originalStat = handle.stat.bind(handle); + handle.stat = (async (options?: Parameters[0]) => { + const stat = await originalStat(options); + injectIno(stat, preOpenIno + 1n); + return stat; + }) as typeof handle.stat; + return handle; + }) as typeof fs.open); + + vi.useFakeTimers(); + vi.setSystemTime(new Date(Date.now() + 6_000)); + + try { + const artifact = await store.get(artifactId); + expect(artifact).toMatchObject({ id: artifactId, status: 'missing' }); + } finally { + lstatSpy.mockRestore(); + openSpy.mockRestore(); + vi.useRealTimers(); + } + }); + + it('keeps an unswapped file available through an options-forwarding lstat spy', async () => { + const store = new SessionArtifactStore({ + sessionId: 's7-bigint-forwarding', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'keep.txt'), 'content'); + const created = await store.upsertMany([ + { title: 'Keep', workspacePath: 'keep.txt' }, + ]); + const artifactId = created.changes[0]!.artifactId; + + vi.useFakeTimers(); + vi.setSystemTime(new Date(Date.now() + 6_000)); + const lstatSpy = spyOnLstatForwarding(() => {}); + + try { + const artifact = await store.get(artifactId); + expect(artifact).toMatchObject({ id: artifactId, status: 'available' }); + } finally { + lstatSpy.mockRestore(); + vi.useRealTimers(); + } + }); + it('rejects workspace paths that become symlinks before open', async () => { const store = new SessionArtifactStore({ sessionId: 's7-open-nofollow', diff --git a/packages/acp-bridge/src/sessionArtifacts.ts b/packages/acp-bridge/src/sessionArtifacts.ts index 5085bd965f..14f7585128 100644 --- a/packages/acp-bridge/src/sessionArtifacts.ts +++ b/packages/acp-bridge/src/sessionArtifacts.ts @@ -5,7 +5,12 @@ */ import { createHash } from 'node:crypto'; -import { constants as fsConstants, promises as fs, type Stats } from 'node:fs'; +import { + constants as fsConstants, + promises as fs, + type BigIntStats, + type Stats, +} from 'node:fs'; import type { FileHandle } from 'node:fs/promises'; import path from 'node:path'; import { @@ -3139,16 +3144,21 @@ async function getWorkspaceStatus( if (!relative || isOutsidePath(relative)) { return { status: 'missing', escaped: true }; } - const preOpenStat = await fs.lstat(realPath); + // Identity is compared on bigint stats: NTFS file ids are 64-bit and the + // Number spelling loses precision above 2^53, so two files created close + // together can round to the SAME numeric ino and defeat the swap check. + const preOpenStat = await fs.lstat(realPath, { bigint: true }); const handle = await fs.open( realPath, fsConstants.O_RDONLY | fsConstants.O_NOFOLLOW, ); try { - const stat = await handle.stat(); - if (!isSameFile(preOpenStat, stat)) { + if (!isSameFile(preOpenStat, await handle.stat({ bigint: true }))) { return { status: 'missing', escaped: true }; } + // The identity fstat cannot be reused here: bigint stats return BigInt + // fields and truncate mtimeMs, breaking the Number comparisons below. + const stat = await handle.stat(); if (!stat.isFile()) { throw new Error('path is not a regular file'); } @@ -3219,7 +3229,7 @@ async function getWorkspaceStatus( } } -function isSameFile(before: Stats, after: Stats): boolean { +function isSameFile(before: BigIntStats, after: BigIntStats): boolean { return before.dev === after.dev && before.ino === after.ino; } diff --git a/packages/channels/dws/src/dws-event-stream.test.ts b/packages/channels/dws/src/dws-event-stream.test.ts index bd65299881..195b5ca7fe 100644 --- a/packages/channels/dws/src/dws-event-stream.test.ts +++ b/packages/channels/dws/src/dws-event-stream.test.ts @@ -81,6 +81,29 @@ describe('DWS event process', () => { }); }); + it('reports a clean exit instead of a stale stderr error', async () => { + const onLine = vi.fn(); + const onError = vi.fn(); + const fixture = fileURLToPath( + new URL('./fixtures/dws-event-clean-exit-source.mjs', import.meta.url), + ); + const subscription = await startDwsEventProcess( + process.execPath, + [fixture], + onLine, + onError, + ); + + await subscription.closed; + + expect(onLine).toHaveBeenCalledWith('{"type":"final"}'); + expect(onError).toHaveBeenCalledOnce(); + expect(onError.mock.calls[0]?.[0]).toMatchObject({ + message: 'DWS event consumer stopped (0).', + retryable: undefined, + }); + }); + it('preserves a terminal error across stdout buffered after exit', async () => { let releaseFirstLine!: () => void; const firstLineBlocked = new Promise((resolve) => { diff --git a/packages/channels/dws/src/dws-event-stream.ts b/packages/channels/dws/src/dws-event-stream.ts index 44e7c6d5ac..b730543767 100644 --- a/packages/channels/dws/src/dws-event-stream.ts +++ b/packages/channels/dws/src/dws-event-stream.ts @@ -216,7 +216,15 @@ export const startDwsEventProcess: DwsEventProcessStarter = ( if (state === 'pending') { settleStartupError(lastError ?? processError(code)); } else if (state === 'ready' && !stopping) { - reportError(lastError ?? processError(code)); + // A clean exit certifies any recorded stderr error as stale: the + // consumer chose to finish after writing it. The live-line clear in + // the stdout handler cannot be relied on for the final line — on + // Windows the pipe routinely drains only after `exitCode` is set, + // so its liveness gate (which protects errors from being cleared + // by lines buffered across a crash) also swallows that clear. + reportError( + code === 0 ? processError(code) : (lastError ?? processError(code)), + ); } }); }); diff --git a/packages/channels/dws/src/fixtures/dws-event-clean-exit-source.mjs b/packages/channels/dws/src/fixtures/dws-event-clean-exit-source.mjs new file mode 100644 index 0000000000..af4f32cb85 --- /dev/null +++ b/packages/channels/dws/src/fixtures/dws-event-clean-exit-source.mjs @@ -0,0 +1,15 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import process from 'node:process'; + +process.stderr.write('[event] ready\n'); +process.stdout.write('{"type":"final"}\n', () => { + process.stderr.write( + '{"message":"stale stderr error","retryable":false}\n', + () => process.exit(0), + ); +}); diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts index 94441ad1fd..946020fc8f 100644 --- a/packages/cli/src/commands/review/lib/worktree.test.ts +++ b/packages/cli/src/commands/review/lib/worktree.test.ts @@ -39,6 +39,15 @@ import { worktreeResidue, } from './worktree.js'; +// Replaces a gitfile that `git worktree add` created. On Windows git marks +// the linked worktree's `.git` hidden, and opening a hidden file for truncate +// (writeFileSync's CREATE_ALWAYS) fails with EPERM — so unlink first and let +// the rewrite create a fresh, unhidden file. +function overwriteGitfile(gitfilePath: string, content: string): void { + rmSync(gitfilePath, { force: true }); + writeFileSync(gitfilePath, content); +} + describe('worktreeResidue', () => { let repo: string; // The tree under measurement is a LINKED worktree — the production shape: @@ -245,7 +254,7 @@ describe('worktreeResidue', () => { ]); gitRepo('config', 'core.worktree', tree); - writeFileSync(join(tree, '.git'), `gitdir: ${join(repo, '.git')}\n`); + overwriteGitfile(join(tree, '.git'), `gitdir: ${join(repo, '.git')}\n`); const got = worktreeResidue(tree); @@ -296,7 +305,7 @@ describe('worktreeResidue', () => { writeFileSync(join(admin, 'commondir'), '../..\n'); writeFileSync(join(admin, 'HEAD'), `${forgedHead}\n`); copyFileSync(join(repo, '.git', 'index'), join(admin, 'index')); - writeFileSync(join(tree, '.git'), `gitdir: ${admin}\n`); + overwriteGitfile(join(tree, '.git'), `gitdir: ${admin}\n`); // Unpinned, the forge's index answers clean — and an unanchored clean // verdict is exactly the one the probe refuses (#9557). @@ -333,7 +342,7 @@ describe('worktreeResidue', () => { const admin = readFileSync(join(sibling, '.git'), 'utf8') .trim() .replace(/^gitdir:\s*/, ''); - writeFileSync(join(tree, '.git'), `gitdir: ${admin}\n`); + overwriteGitfile(join(tree, '.git'), `gitdir: ${admin}\n`); const got = worktreeResidue(tree); @@ -468,7 +477,7 @@ describe('worktreeResidue', () => { // absolutely, because its old relative spelling no longer resolves from // outside the repo. renameSync(tree, join(outside, 'review-wt')); - writeFileSync( + overwriteGitfile( join(outside, 'review-wt', '.git'), `gitdir: ${join(repo, '.git', 'worktrees', 'review-wt')}\n`, ); diff --git a/packages/cli/src/serve/daemon-git-worktree-guard.test.ts b/packages/cli/src/serve/daemon-git-worktree-guard.test.ts index a21d3df023..9df0e137c8 100644 --- a/packages/cli/src/serve/daemon-git-worktree-guard.test.ts +++ b/packages/cli/src/serve/daemon-git-worktree-guard.test.ts @@ -39,6 +39,16 @@ const outsideRepo = path.join(temporaryRoot, 'outside', 'repo'); // A second outside checkout whose path contains no Git word, so a test using // it cannot pass because `\bgit\b` happened to match inside the path. const plainOutsidePath = path.join(temporaryRoot, 'elsewhere', 'checkout'); +// Spelling for paths interpolated into COMMAND STRINGS on the lanes that +// execute through bash: an unquoted backslash is a POSIX escape, so a native +// Windows path pasted into a command reaches the tokenizer as `C:temprepo` +// and the test stops exercising what it names — resolvable targets become +// unresolvable ones, and cwd shifts silently fail in place. Forward slashes +// survive bash and resolve identically on Windows. Filesystem setup and +// denial-reason assertions keep the native spelling: the guard canonicalizes +// targets through realpath before printing them. +const cmdPath = (value: string): string => + process.platform === 'win32' ? value.replaceAll('\\', '/') : value; mkdirSync(path.join(outsideRepo, '.git'), { recursive: true }); mkdirSync(insideNested, { recursive: true }); @@ -68,17 +78,19 @@ afterAll(async () => { describe('createDaemonToolGuard', () => { it.each([ - () => `git -C ${outsideRepo} reset --hard`, - () => `git -C${outsideRepo} checkout -- .`, + () => `git -C ${cmdPath(outsideRepo)} reset --hard`, + () => `git -C${cmdPath(outsideRepo)} checkout -- .`, () => - `git --work-tree=${outsideRepo} --git-dir=${path.join(outsideRepo, '.git')} clean -fd`, - () => `git --git-dir ${path.join(outsideRepo, '.git')} commit -m x`, - () => `git --namespace foo -C ${outsideRepo} reset --hard`, - () => `git --super-prefix=foo --work-tree=${outsideRepo} clean -fd`, + `git --work-tree=${cmdPath(outsideRepo)} --git-dir=${cmdPath(path.join(outsideRepo, '.git'))} clean -fd`, + () => + `git --git-dir ${cmdPath(path.join(outsideRepo, '.git'))} commit -m x`, + () => `git --namespace foo -C ${cmdPath(outsideRepo)} reset --hard`, + () => + `git --super-prefix=foo --work-tree=${cmdPath(outsideRepo)} clean -fd`, // `grep` runs the target repo's diff..textconv programs and // `status` refreshes the target index + runs its core.fsmonitor. - () => `git -C ${outsideRepo} grep --textconv pattern`, - () => `git -C ${outsideRepo} status --porcelain`, + () => `git -C ${cmdPath(outsideRepo)} grep --textconv pattern`, + () => `git -C ${cmdPath(outsideRepo)} status --porcelain`, ])('denies relocated mutating Git command %#', async (buildCommand) => { const guard = createDaemonToolGuard(); @@ -171,7 +183,7 @@ describe('createDaemonToolGuard', () => { it.each([ // Git treats an empty `-C` as a no-op and applies the next relocation. - () => `git -C "" -C ${outsideRepo} reset --hard`, + () => `git -C "" -C ${cmdPath(outsideRepo)} reset --hard`, ])('denies relocations masked by token edge cases %#', async (command) => { const guard = createDaemonToolGuard(); @@ -185,7 +197,7 @@ describe('createDaemonToolGuard', () => { // is ordinary argv text and the divergent-syntax gate owns the shape. it .runIf(bashSemanticsLane) - .each([() => `git -C ${outsideRepo} reset --hard # note`])( + .each([() => `git -C ${cmdPath(outsideRepo)} reset --hard # note`])( 'denies relocations masked by token edge cases on the bash lanes %#', async (command) => { const guard = createDaemonToolGuard(); @@ -412,7 +424,9 @@ describe('createDaemonToolGuard', () => { const guard = createDaemonToolGuard(); await expect( - guard(request(`cd ${effectiveCwd} && sh -c 'git reset --hard'`)), + guard( + request(`cd ${cmdPath(effectiveCwd)} && sh -c 'git reset --hard'`), + ), ).resolves.toEqual({ allowed: true }); }, ); @@ -491,17 +505,17 @@ describe('createDaemonToolGuard', () => { guard(request(`sh -c 'cd ${outsideRepo}'; git reset --hard`)), ).resolves.toEqual({ allowed: true }); await expect( - guard(request(`cd ${effectiveCwd} && git reset --hard`)), + guard(request(`cd ${cmdPath(effectiveCwd)} && git reset --hard`)), ).resolves.toEqual({ allowed: true }); }, ); it.runIf(bashSemanticsLane).each([ () => `git -C \\ -${outsideRepo} reset --hard`, +${cmdPath(outsideRepo)} reset --hard`, () => `g\\ -it -C ${outsideRepo} reset --hard`, +it -C ${cmdPath(outsideRepo)} reset --hard`, ])( 'joins backslash continuations before parsing %#', async (buildCommand) => { @@ -608,12 +622,12 @@ it -C ${outsideRepo} reset --hard`, const guard = createDaemonToolGuard(); await expect( - guard(request(`git -C nested -C ${outsideRepo} reset --hard`)), + guard(request(`git -C nested -C ${cmdPath(outsideRepo)} reset --hard`)), ).resolves.toMatchObject({ allowed: false }); await expect( guard( request( - `git -C ${outsideRepo} -C ${path.relative(outsideRepo, effectiveCwd)} reset --hard`, + `git -C ${cmdPath(outsideRepo)} -C ${cmdPath(path.relative(outsideRepo, effectiveCwd))} reset --hard`, ), ), ).resolves.toEqual({ allowed: true }); @@ -1546,11 +1560,12 @@ it -C ${outsideRepo} reset --hard`, // An unrecognized program word hides what runs, so a git mention only // survives while the shell is provably still inside the boundary. it.each([ - () => `cd ${outsideRepo} && nice git reset --hard`, - () => `cd ${outsideRepo} && ionice -c3 git reset --hard`, - () => `cd ${outsideRepo} && echo x | xargs -I{} git reset --hard`, - () => `cd ${outsideRepo} && find . -maxdepth 0 -exec git reset --hard ;`, - () => `cd ${outsideRepo} && stdbuf -o0 git reset --hard`, + () => `cd ${cmdPath(outsideRepo)} && nice git reset --hard`, + () => `cd ${cmdPath(outsideRepo)} && ionice -c3 git reset --hard`, + () => `cd ${cmdPath(outsideRepo)} && echo x | xargs -I{} git reset --hard`, + () => + `cd ${cmdPath(outsideRepo)} && find . -maxdepth 0 -exec git reset --hard ;`, + () => `cd ${cmdPath(outsideRepo)} && stdbuf -o0 git reset --hard`, () => 'cd - && nice git reset --hard', ])( 'denies an unrecognized program running Git after a cwd shift %#', @@ -1605,7 +1620,11 @@ it -C ${outsideRepo} reset --hard`, guard(request('export FOO=bar && git commit -m x')), ).resolves.toEqual({ allowed: true }); await expect( - guard(request(`export GIT_WORK_TREE=${insideNested} && git commit -m x`)), + guard( + request( + `export GIT_WORK_TREE=${cmdPath(insideNested)} && git commit -m x`, + ), + ), ).resolves.toEqual({ allowed: true }); }); @@ -1666,11 +1685,14 @@ it -C ${outsideRepo} reset --hard`, // The relocated read-only allowance covers subcommands that neither write // files nor run target-repository programs — flags can revoke both. it.each([ - () => `git -C ${outsideRepo} cat-file --textconv --path=f.txt HEAD:f.txt`, - () => `git -C ${outsideRepo} cat-file --filters --path=f.txt HEAD:f.txt`, - () => `git -C ${outsideRepo} rev-parse --output=${outsideRepo}/o.txt HEAD`, () => - `git -C ${outsideRepo} cat-file --output ${outsideRepo}/o.txt -p HEAD`, + `git -C ${cmdPath(outsideRepo)} cat-file --textconv --path=f.txt HEAD:f.txt`, + () => + `git -C ${cmdPath(outsideRepo)} cat-file --filters --path=f.txt HEAD:f.txt`, + () => + `git -C ${cmdPath(outsideRepo)} rev-parse --output=${cmdPath(outsideRepo)}/o.txt HEAD`, + () => + `git -C ${cmdPath(outsideRepo)} cat-file --output ${cmdPath(outsideRepo)}/o.txt -p HEAD`, ])( 'denies a relocated read-only subcommand carrying a disqualifying flag %#', async (buildCommand) => { @@ -1697,8 +1719,8 @@ it -C ${outsideRepo} reset --hard`, // `ls-files` executes the target repository's core.fsmonitor hook — the // same property that excluded `status` (measured on git 2.47.3). it.each([ - () => `git -C ${outsideRepo} ls-files`, - () => `git -C ${outsideRepo} ls-files --others`, + () => `git -C ${cmdPath(outsideRepo)} ls-files`, + () => `git -C ${cmdPath(outsideRepo)} ls-files --others`, ])('denies a relocated ls-files %#', async (buildCommand) => { const guard = createDaemonToolGuard(); @@ -1714,11 +1736,11 @@ it -C ${outsideRepo} reset --hard`, // subcommand stays out of the read-only set because the flag is one token // away from any describe a model writes. it.each([ - () => `git -C ${outsideRepo} describe`, - () => `git -C ${outsideRepo} describe --tags`, - () => `git -C ${outsideRepo} describe --dirty`, - () => `git -C ${outsideRepo} describe --always --dirty`, - () => `git -C ${outsideRepo} describe --broken`, + () => `git -C ${cmdPath(outsideRepo)} describe`, + () => `git -C ${cmdPath(outsideRepo)} describe --tags`, + () => `git -C ${cmdPath(outsideRepo)} describe --dirty`, + () => `git -C ${cmdPath(outsideRepo)} describe --always --dirty`, + () => `git -C ${cmdPath(outsideRepo)} describe --broken`, ])('denies a relocated describe %#', async (buildCommand) => { const guard = createDaemonToolGuard(); @@ -1752,7 +1774,7 @@ it -C ${outsideRepo} reset --hard`, () => `xargs -I{} sh -c 'cd ${outsideRepo} && git reset --hard'`, // `executableBaseName` lowercases, so an uppercase program word resolves // to the same binary on a case-insensitive filesystem. - () => `cd ${outsideRepo} && nice GIT reset --hard`, + () => `cd ${cmdPath(outsideRepo)} && nice GIT reset --hard`, ])( 'denies a relocated mutation concealed in an unrecognized program %#', async (buildCommand) => { @@ -1766,8 +1788,8 @@ it -C ${outsideRepo} reset --hard`, // A program word the daemon cannot read is as opaque as an unrecognized one. it.each([ - () => `cd ${outsideRepo} && $CMD git reset --hard`, - () => `cd ${outsideRepo} && command $CMD git reset --hard`, + () => `cd ${cmdPath(outsideRepo)} && $CMD git reset --hard`, + () => `cd ${cmdPath(outsideRepo)} && command $CMD git reset --hard`, ])( 'denies a dynamic program running Git after a cwd shift %#', async (buildCommand) => { @@ -1840,8 +1862,9 @@ it -C ${outsideRepo} reset --hard`, // An unmodelled value-taking global option makes its value look like the // subcommand, which ends option parsing and hides the relocation after it. it.each([ - () => `git --shallow-file /tmp/shallow -C ${outsideRepo} reset --hard`, - () => `git --attr-source HEAD -C ${outsideRepo} reset --hard`, + () => + `git --shallow-file /tmp/shallow -C ${cmdPath(outsideRepo)} reset --hard`, + () => `git --attr-source HEAD -C ${cmdPath(outsideRepo)} reset --hard`, ])( 'parses relocations after value-taking global options %#', async (buildCommand) => { @@ -2030,11 +2053,12 @@ it -C ${outsideRepo} reset --hard`, // Values assigned earlier in the same command are visible to the guard. it.each([ - () => `X='git reset --hard'; cd ${outsideRepo}; $X`, + () => `X='git reset --hard'; cd ${cmdPath(outsideRepo)}; $X`, () => `X=git; Y='-C ${outsideRepo} reset --hard'; $X $Y`, () => - `eval 'GIT_WORK_TREE=${outsideRepo}'; export GIT_WORK_TREE; git reset --hard`, - () => `GIT_WORK_TREE=${outsideRepo}; export $NAME; git reset --hard`, + `eval 'GIT_WORK_TREE=${cmdPath(outsideRepo)}'; export GIT_WORK_TREE; git reset --hard`, + () => + `GIT_WORK_TREE=${cmdPath(outsideRepo)}; export $NAME; git reset --hard`, ])('resolves a relocation through shell variables %#', async (build) => { const guard = createDaemonToolGuard(); @@ -2402,7 +2426,7 @@ it -C ${outsideRepo} reset --hard`, for (const command of [ "env -iS 'git status'", // A `cd` target the guard already knows the value of. - `d=${insideNested}; cd $d; git status`, + `d=${cmdPath(insideNested)}; cd $d; git status`, // `set +a` turns allexport back off. `set -a; set +a; GIT_WORK_TREE=${outsideRepo}; echo done`, // Definitions used inside the boundary stay allowed. @@ -2431,9 +2455,10 @@ it -C ${outsideRepo} reset --hard`, () => `echo "$'$(GIT_DIR=${plainOutside}/.git git reset --hard HEAD~1)'"`, // The export attribute sticks to the name, so a LATER assignment to it // reaches the git subprocess. - () => `export GIT_DIR; GIT_DIR=${plainOutside}; git reset --hard`, () => - `export GIT_WORK_TREE; GIT_WORK_TREE=${plainOutside}; git reset --hard`, + `export GIT_DIR; GIT_DIR=${cmdPath(plainOutside)}; git reset --hard`, + () => + `export GIT_WORK_TREE; GIT_WORK_TREE=${cmdPath(plainOutside)}; git reset --hard`, // Both sides of a pipe run in subshells: the parent stays outside. () => `cd ${plainOutside}; echo x | cd ${effectiveCwd}; git commit -m x`, // A bare digit before a spaced redirect is a real argv word. diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 2e895300e8..f60d8ac862 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -1655,7 +1655,9 @@ describe('Server Config (config.ts)', () => { }); expect(child.getCwd()).toBe('/tmp/derived'); - expect(parent.getCwd()).toBe(baseParams.targetDir); + // The constructor resolves targetDir, so on win32 the POSIX fixture + // spelling comes back drive-qualified — compare the resolved form. + expect(parent.getCwd()).toBe(path.resolve(baseParams.targetDir)); expect(Object.getPrototypeOf(child)).toBe(parent); }); diff --git a/packages/core/src/extension/extension-git-client.ts b/packages/core/src/extension/extension-git-client.ts index 6cf31f537a..eef66b1b4c 100644 --- a/packages/core/src/extension/extension-git-client.ts +++ b/packages/core/src/extension/extension-git-client.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import * as os from 'node:os'; import type { SimpleGit, SimpleGitFactory, SimpleGitOptions } from 'simple-git'; import type { ExtensionInstallMetadata } from '../config/config.js'; import type { GitCredential } from './extension-git-credentials.js'; @@ -126,7 +125,11 @@ export function createExtensionGitClient( const environment: Record = { GIT_CONFIG_NOSYSTEM: '1', - GIT_CONFIG_GLOBAL: os.devNull, + // The literal '/dev/null' on every platform, NOT os.devNull: Git for + // Windows special-cases the POSIX spelling in its compat layer, while + // os.devNull's win32 value ('\\\\.\\nul') is rejected as + // "fatal: unable to access '\\.\nul': Invalid argument". + GIT_CONFIG_GLOBAL: '/dev/null', }; for (const key of [ 'PATH', diff --git a/packages/node-repl/src/node-repl.semantics.test.ts b/packages/node-repl/src/node-repl.semantics.test.ts index 2b414bb094..3c4ecbb151 100644 --- a/packages/node-repl/src/node-repl.semantics.test.ts +++ b/packages/node-repl/src/node-repl.semantics.test.ts @@ -370,8 +370,22 @@ describe('binding semantics through the real kernel', () => { expect(r.status).toBe('ok'); expect(textOf(r.events).trim()).toBe('via-symlink'); } finally { - fs.rmSync(store, { recursive: true, force: true }); - fs.rmSync(work, { recursive: true, force: true }); + // The kernel child's cwd is `work`; on Windows a directory that is any + // process's cwd cannot be removed (EBUSY). Dispose first, then retry + // the removal across the child's asynchronous teardown. + manager.dispose(); + fs.rmSync(store, { + recursive: true, + force: true, + maxRetries: 10, + retryDelay: 200, + }); + fs.rmSync(work, { + recursive: true, + force: true, + maxRetries: 10, + retryDelay: 200, + }); } }); diff --git a/packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.test.ts b/packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.test.ts index a47ec7383c..921d51d5d6 100644 --- a/packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.test.ts +++ b/packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.test.ts @@ -59,6 +59,13 @@ vi.mock('node:url', async () => { if (process.platform !== 'win32' && /^[a-zA-Z]:\\/.test(filePath)) { return actual.pathToFileURL(filePath, { windows: true }); } + // The mirror case: fixtures spell workspace paths POSIX-style, and on + // win32 the real pathToFileURL would drive-qualify them against the + // process cwd (file:///C:/workspace/…). Parse them as POSIX so the + // expected URLs read the same on every host. + if (process.platform === 'win32' && filePath.startsWith('/')) { + return actual.pathToFileURL(filePath, { windows: false }); + } return actual.pathToFileURL(filePath); }, };