Skip to content

Commit 48d00de

Browse files
test(file-safety): kill the warn-text mutants the mutation run recorded
Adds one spec file, src/services/file-safety/__tests__/safeWriteText.daclNotices .spec.ts (261 lines, 10 cases), and changes no source file. The four best-effort DACL notices each carry the target, the error code and the consequence; every case asserts the whole message, so a mutant that empties a clause or swaps the error-code operator fails a named test instead of passing on a substring. The ENOENT pair at :563 is covered by both sides: an existing target whose access rights cannot be read is a notice, an absent target is the ordinary first write and must publish with no notice and no icacls run. Re-running the mutation on this tree killed eight of the nineteen recorded rows (package shape 338 mutants: 315 killed before, 323 after): :552 LogicalOperator, :553, :559 LogicalOperator, :563 ConditionalExpression, :563 StringLiteral, :570, :681 LogicalOperator and :682. Eleven rows are left with no test, no suppression, no directive and no exclusion, because no input reachable at this layer makes the two sides differ: - :552, :559, :569 and :681 StringLiteral replace the fallback operand of errorCode(...) ?? "no error code" with an empty string. errorCode returns String(error.code), which is never nullish, so the fallback is never evaluated; the report itself lists those two :559 and :569 mutants as covered by zero tests. - :440 and :441 drop a typeof bigint guard from the inode comparison. The guard only decides something when exactly one side is non-bigint and the values are still equal, and a bigint compared with a non-bigint is never ===, so the next operand in the same conjunction already rejects every such input. The case named "accepts a caller-staged file whose lstat reports a non-bigint inode" reaches the guard with the non-bigint pair and cannot flip it. - :455, :688 and :732 concern releaseBackupOnSuccess and backupPath. backupPath is written at :579, cleared at :622 only together with a throw, and the flag is set at :628 immediately after the same block succeeds, so at both read points a non-null backupPath means the flag is true and the guards' two sides agree. - core/tools/ApplyDiffTool.ts:91 and :106 replace "utf-8" with "". Node treats the empty encoding string as utf-8: Buffer.from and Buffer.byteLength were compared over nine sample classes (empty, ASCII, accented, CJK, emoji, BOM, replacement character, surrogate pair, NUL) and agreed on every one. These eleven are equivalence claims, so they are stated here for the maintainer to weigh rather than silenced with a directive. Checked with the new spec, the two existing safeWriteText specs and applyDiffTool.failureReporting.spec.ts, plus tsc --noEmit (no diagnostics) and eslint on the new file (no warnings, eslint-suppressions.json untouched).
1 parent cfbf79c commit 48d00de

1 file changed

Lines changed: 260 additions & 0 deletions

File tree

Lines changed: 260 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,260 @@
1+
import * as fs from "fs/promises"
2+
import * as fsSync from "fs"
3+
import * as path from "path"
4+
5+
// The win32 DACL steps are reachable on any runner through the platform override, but two of their
6+
// inputs are not reproducible on one portable filesystem: a target that exists while its access
7+
// rights cannot be read, and a saved DACL dump whose unlink keeps failing. Both sit behind fs calls,
8+
// so this file mocks fs/promises and fs the way safeWriteText.spec.ts does and drives them per case.
9+
vi.mock("fs/promises", () => ({
10+
copyFile: vi.fn(),
11+
chmod: vi.fn(),
12+
mkdir: vi.fn(),
13+
access: vi.fn(),
14+
rename: vi.fn(),
15+
unlink: vi.fn(),
16+
rmdir: vi.fn(),
17+
realpath: vi.fn(),
18+
lstat: vi.fn(),
19+
readlink: vi.fn(),
20+
}))
21+
22+
vi.mock("fs", () => ({
23+
openSync: vi.fn(),
24+
writeSync: vi.fn(),
25+
closeSync: vi.fn(),
26+
mkdirSync: vi.fn(),
27+
fsyncSync: vi.fn(),
28+
chmodSync: vi.fn(),
29+
fchmodSync: vi.fn(),
30+
statSync: vi.fn(),
31+
Stats: class Stats {},
32+
}))
33+
34+
import { safeWriteText, StagingPathError, type SafeWriteTextOptions } from "../safeWriteText"
35+
36+
const TARGET = path.join(path.resolve("/safeWriteText-notice"), "notice.txt")
37+
const DIR = path.dirname(TARGET)
38+
const CALLER_STAGED = path.join(DIR, "caller-staged.tmp")
39+
40+
function codeError(code: string, message: string): Error {
41+
return Object.assign(new Error(message), { code })
42+
}
43+
44+
function statsWithMode(mode: number): fsSync.Stats {
45+
const stats = Object.create(fsSync.Stats.prototype) as fsSync.Stats
46+
Object.assign(stats, { mode })
47+
return stats
48+
}
49+
50+
// The publish path reads only isFile/isSymbolicLink and the ino/dev pair out of a lstat result. The
51+
// identity pair is typed number|bigint here so one case can hand back a non-bigint ino, which
52+
// fs.lstat(path, { bigint: true }) never produces on a real filesystem.
53+
function fileStatsWithIdentity(ino: number | bigint, dev: number | bigint): fsSync.Stats {
54+
const stats = Object.create(fsSync.Stats.prototype) as fsSync.Stats
55+
Object.assign(stats, { ino, dev, isSymbolicLink: () => false, isFile: () => true })
56+
return stats
57+
}
58+
59+
interface IcaclsDouble {
60+
calls: string[][]
61+
runner: NonNullable<SafeWriteTextOptions["execFileRunner"]>
62+
}
63+
64+
// The SUT calls execFile in its (file, args, options, callback) form. The double assertion is needed
65+
// because the real callback also reports stdout and stderr, which this double has nothing of.
66+
function icaclsDouble(outcome: { save?: Error; restore?: Error }): IcaclsDouble {
67+
const calls: string[][] = []
68+
const runner = ((
69+
_cmd: string,
70+
args: readonly string[],
71+
_options: unknown,
72+
callback: (error: unknown) => void,
73+
) => {
74+
calls.push(Array.from(args))
75+
if (args[1] === "/save") {
76+
callback(outcome.save ?? null)
77+
} else {
78+
callback(outcome.restore ?? null)
79+
}
80+
return { kill: () => true }
81+
}) as unknown as NonNullable<SafeWriteTextOptions["execFileRunner"]>
82+
return { calls, runner }
83+
}
84+
85+
/** Run one publish against the mocks and collect every notice it delivers. */
86+
async function publish(options: SafeWriteTextOptions): Promise<{ leftoverPaths: string[]; warnings: string[] }> {
87+
const warnings: string[] = []
88+
const result = await safeWriteText(TARGET, "x", {
89+
platform: "win32",
90+
...options,
91+
onWarning: (message: string) => {
92+
warnings.push(message)
93+
},
94+
})
95+
return { leftoverPaths: result.leftoverPaths, warnings }
96+
}
97+
98+
/** Paths this publish tried to unlink, in call order. */
99+
function unlinkCalls(): string[] {
100+
return vi.mocked(fs.unlink).mock.calls.map((call) => String(call[0]))
101+
}
102+
103+
describe("safeWriteText win32 DACL notices", () => {
104+
beforeEach(() => {
105+
vi.resetAllMocks()
106+
vi.mocked(fs.mkdir).mockResolvedValue(undefined)
107+
vi.mocked(fs.access).mockResolvedValue(undefined)
108+
vi.mocked(fs.rename).mockResolvedValue(undefined)
109+
vi.mocked(fs.unlink).mockResolvedValue(undefined)
110+
vi.mocked(fs.rmdir).mockResolvedValue(undefined)
111+
vi.mocked(fs.realpath).mockResolvedValue(TARGET)
112+
vi.mocked(fs.lstat).mockResolvedValue(fileStatsWithIdentity(1n, 1n))
113+
vi.mocked(fsSync.statSync).mockReturnValue(statsWithMode(0o644))
114+
vi.mocked(fsSync.openSync).mockReturnValue(7)
115+
vi.mocked(fsSync.writeSync).mockImplementation(() => 1)
116+
})
117+
118+
it("names the target, the icacls error and the consequence when the DACL save fails", async () => {
119+
const { runner } = icaclsDouble({ save: codeError("EPERM", "icacls save failed") })
120+
121+
const { leftoverPaths, warnings } = await publish({ execFileRunner: runner })
122+
123+
// The code is printed, not swallowed: errorCode(saveDaclError) ?? "no error code" must print
124+
// the code when there is one, and the notice has to say what the publish risks by continuing.
125+
expect(warnings).toHaveLength(1)
126+
expect(warnings[0]).toBe(
127+
`safeWriteText: could not save the DACL of ${TARGET} (EPERM); ` +
128+
`publishing without it means the file at the target may answer to different access rights.`,
129+
)
130+
expect(leftoverPaths).toEqual([])
131+
})
132+
133+
it("names the dump path and the unlink error when the partial dump cannot be removed", async () => {
134+
const { calls, runner } = icaclsDouble({ save: codeError("EPERM", "icacls save failed") })
135+
vi.mocked(fs.unlink).mockImplementation(async (residuePath) => {
136+
if (String(residuePath).includes(".safeWriteText.acl_")) {
137+
throw codeError("EPERM", "dump still open")
138+
}
139+
})
140+
141+
const { leftoverPaths, warnings } = await publish({ execFileRunner: runner })
142+
143+
const dumpPath = String(calls[0]?.[2])
144+
expect(warnings).toHaveLength(2)
145+
expect(warnings[1]).toBe(
146+
`safeWriteText: the partial DACL dump could not be removed from ${dumpPath} (EPERM).`,
147+
)
148+
expect(leftoverPaths).toContain(dumpPath)
149+
})
150+
151+
it("names the target, the access error and the consequence when access rights cannot be read", async () => {
152+
vi.mocked(fs.access).mockImplementation(async (target) => {
153+
if (String(target) === TARGET) {
154+
throw codeError("EACCES", "access rights unreadable")
155+
}
156+
})
157+
158+
const { leftoverPaths, warnings } = await publish({ execFileRunner: icaclsDouble({}).runner })
159+
160+
// An existing target whose rights cannot be read is not an absent target: the publish continues
161+
// and the notice names the code, so it must not be confused with the ENOENT case below.
162+
expect(warnings).toHaveLength(1)
163+
expect(warnings[0]).toBe(
164+
`safeWriteText: could not read the access rights of ${TARGET} (EACCES); ` +
165+
`publishing without them means the file at the target may answer to different access rights.`,
166+
)
167+
expect(leftoverPaths).toEqual([])
168+
})
169+
170+
it("publishes an absent target with no notice and no DACL save", async () => {
171+
const { calls, runner } = icaclsDouble({})
172+
vi.mocked(fs.access).mockImplementation(async (target) => {
173+
if (String(target) === TARGET) {
174+
throw codeError("ENOENT", "no such file")
175+
}
176+
})
177+
178+
const { leftoverPaths, warnings } = await publish({ execFileRunner: runner })
179+
180+
// "could not read the access rights" is the notice for a target that is there; an absent target
181+
// is the ordinary first write and must produce no notice at all, and no icacls run.
182+
expect(warnings).toEqual([])
183+
expect(calls).toEqual([])
184+
expect(vi.mocked(fs.rename)).toHaveBeenCalledTimes(1)
185+
expect(String(vi.mocked(fs.rename).mock.calls[0]?.[1])).toBe(TARGET)
186+
expect(leftoverPaths).toEqual([])
187+
})
188+
189+
it("names the kept dump and the restore error when the restore fails after the commit", async () => {
190+
const { calls, runner } = icaclsDouble({ restore: codeError("EPERM", "icacls restore failed") })
191+
192+
const { leftoverPaths, warnings } = await publish({ execFileRunner: runner })
193+
194+
const dumpPath = String(calls[0]?.[2])
195+
expect(warnings).toHaveLength(1)
196+
expect(warnings[0]).toBe(
197+
`safeWriteText: committed ${TARGET} but could not restore its saved access rights ` +
198+
`(EPERM); the DACL dump is kept at ${dumpPath} ` +
199+
`so it can be restored manually.`,
200+
)
201+
// The dump is the operator's only way to put the rights back, so it is kept and reported.
202+
expect(leftoverPaths).toContain(dumpPath)
203+
expect(unlinkCalls().some((candidate) => candidate === dumpPath)).toBe(false)
204+
})
205+
206+
it("accepts a caller-staged file whose lstat reports a non-bigint inode", async () => {
207+
vi.mocked(fs.lstat).mockImplementation(async (stagedPath) => {
208+
const identity = String(stagedPath) === CALLER_STAGED ? 7 : 7
209+
return fileStatsWithIdentity(identity, 3)
210+
})
211+
212+
const { leftoverPaths } = await publish({ tempPath: CALLER_STAGED })
213+
214+
// A non-bigint ino pair is not an identity match: fs.lstat(path, { bigint: true }) answers
215+
// bigint, so treating a number pair as proof of the same file would refuse a staging file that
216+
// merely carries a number-shaped inode.
217+
expect(vi.mocked(fs.rename).mock.calls[0]?.[0]).toBe(CALLER_STAGED)
218+
expect(leftoverPaths).toEqual([])
219+
})
220+
221+
it("refuses a caller-staged file that is the target itself", async () => {
222+
vi.mocked(fs.lstat).mockResolvedValue(fileStatsWithIdentity(7n, 3n))
223+
224+
await expect(publish({ tempPath: TARGET })).rejects.toBeInstanceOf(StagingPathError)
225+
226+
expect(vi.mocked(fs.rename)).not.toHaveBeenCalled()
227+
})
228+
229+
it("removes the backup copy after a successful publish", async () => {
230+
const { leftoverPaths, warnings } = await publish({ backup: true, execFileRunner: icaclsDouble({}).runner })
231+
232+
expect(warnings).toEqual([])
233+
expect(unlinkCalls().some((candidate) => candidate.includes(".safeWriteText.bak_"))).toBe(true)
234+
expect(leftoverPaths).toEqual([])
235+
})
236+
237+
it("has no backup copy to remove when the target was absent", async () => {
238+
vi.mocked(fs.access).mockImplementation(async (target) => {
239+
if (String(target) === TARGET) {
240+
throw codeError("ENOENT", "no such file")
241+
}
242+
})
243+
244+
const { leftoverPaths, warnings } = await publish({ backup: true, execFileRunner: icaclsDouble({}).runner })
245+
246+
expect(warnings).toEqual([])
247+
expect(unlinkCalls().some((candidate) => candidate.includes(".safeWriteText.bak_"))).toBe(false)
248+
expect(leftoverPaths).toEqual([])
249+
})
250+
251+
it("removes the backup copy when the commit rename fails after the backup was copied", async () => {
252+
vi.mocked(fs.rename).mockRejectedValue(codeError("EPERM", "rename blocked"))
253+
254+
await expect(publish({ backup: true, execFileRunner: icaclsDouble({}).runner })).rejects.toThrow("rename blocked")
255+
256+
// The copy has served its purpose either way, so a failed commit must not leave it beside the
257+
// target where no caller would find it.
258+
expect(unlinkCalls().some((candidate) => candidate.includes(".safeWriteText.bak_"))).toBe(true)
259+
})
260+
})

0 commit comments

Comments
 (0)