Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions packages/acp-bridge/src/sessionArtifacts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3246,15 +3246,27 @@ describe('SessionArtifactStore', () => {
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);
if (!swapped && String(entry) === realTarget) {
swapped = true;
await fs.writeFile(replacement, 'after');
await fs.rename(replacement, target);
}
return stat;
});
const lstatSpy = vi
.spyOn(fs, 'lstat')
// 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.
.mockImplementation((async (
entry: Parameters<typeof fs.lstat>[0],
options?: Parameters<typeof fs.lstat>[1],
) => {
const stat = await originalLstat(
entry,
options as Parameters<typeof originalLstat>[1],
);
if (!swapped && String(entry) === realTarget) {
swapped = true;
await fs.writeFile(replacement, 'after');
await fs.rename(replacement, target);
}
return stat;
}) as typeof fs.lstat);

try {
const artifact = await store.get(artifactId);
Expand Down
18 changes: 13 additions & 5 deletions packages/acp-bridge/src/sessionArtifacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -3107,16 +3112,19 @@ 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 };
}
const stat = await handle.stat();
if (!stat.isFile()) {
throw new Error('path is not a regular file');
}
Expand Down Expand Up @@ -3187,7 +3195,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;
}

Expand Down
10 changes: 9 additions & 1 deletion packages/channels/dws/src/dws-event-stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
);
}
});
});
Expand Down
17 changes: 13 additions & 4 deletions packages/cli/src/commands/review/lib/worktree.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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`,
);
Expand Down
103 changes: 63 additions & 40 deletions packages/cli/src/serve/daemon-git-worktree-guard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });

Expand Down Expand Up @@ -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.<driver>.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();

Expand Down Expand Up @@ -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();

Expand All @@ -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();
Expand Down Expand Up @@ -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 });
},
);
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 %#',
Expand Down Expand Up @@ -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 });
});

Expand Down Expand Up @@ -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) => {
Expand All @@ -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();

Expand All @@ -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();

Expand Down Expand Up @@ -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) => {
Expand All @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -2030,7 +2053,7 @@ 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`,
Expand Down Expand Up @@ -2402,7 +2425,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.
Expand Down
Loading
Loading