Skip to content

Commit d05cf1f

Browse files
cliffhallclaude
andcommitted
fix: retry flags on continuation lines; workspaces missing from the lockfile
Copilot round 4 on #4920: - verify:no-test-retries reads a backslash-continued command as one command, so a retry flag on a continuation line is found. - verify:install-fresh fails on a workspace manifest the lockfile has no entry for. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Signed-off-by: cliffhall <[email protected]>
1 parent b916f29 commit d05cf1f

4 files changed

Lines changed: 68 additions & 2 deletions

File tree

‎scripts/verify-install-fresh.mjs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import { existsSync, readFileSync } from "node:fs";
4848
import path from "node:path";
4949
import { fileURLToPath, pathToFileURL } from "node:url";
50+
import { workspaceDirs } from "./lib/workspaces.mjs";
5051

5152
const repoRoot = path.resolve(
5253
path.dirname(fileURLToPath(import.meta.url)),
@@ -164,6 +165,21 @@ export function main(root = repoRoot) {
164165
? JSON.parse(readFileSync(manifest, "utf8"))
165166
: undefined;
166167
});
168+
// A workspace added since the last install has a manifest and no lockfile
169+
// entry at all, so the comparison above (which walks the lockfile) never
170+
// reaches it. Found from the root manifest's `workspaces`, not the lockfile.
171+
const rootManifestPath = path.join(root, "package.json");
172+
const workspaces = existsSync(rootManifestPath)
173+
? JSON.parse(readFileSync(rootManifestPath, "utf8")).workspaces
174+
: undefined;
175+
if (Array.isArray(workspaces) && workspaces.length > 0) {
176+
for (const { dir, hasManifest } of workspaceDirs(root, workspaces)) {
177+
if (hasManifest && lock.packages?.[dir] === undefined)
178+
problems.push(
179+
` ${dir}/package.json: a workspace the lockfile has no entry for`,
180+
);
181+
}
182+
}
167183
for (const d of drift)
168184
problems.push(
169185
` ${path.posix.join(d.dir || ".", "package.json")} ${d.section}.${d.name}: manifest ${d.manifest ?? "(absent)"}, lockfile ${d.lock ?? "(absent)"}`,

‎scripts/verify-install-fresh.test.mjs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,20 @@ test("main fails when a manifest declares what the lockfile does not", (t) => {
172172
);
173173
});
174174

175+
test("main fails on a workspace the lockfile has never seen", (t) => {
176+
const { root, write } = fixture(t);
177+
t.mock.method(console, "log", () => {});
178+
const error = t.mock.method(console, "error", () => {});
179+
write("node_modules/sdk/package.json", { version: "2.1.0" });
180+
write("package.json", { name: "root", workspaces: ["src/*"] });
181+
write("src/newserver/package.json", { name: "newserver" });
182+
assert.equal(main(root), 1);
183+
assert.match(
184+
error.mock.calls[0].arguments[0],
185+
/src\/newserver\/package\.json: a workspace the lockfile has no entry for/,
186+
);
187+
});
188+
175189
function fixture(t) {
176190
const root = mkdtempSync(path.join(tmpdir(), "install-fresh-"));
177191
t.after(() => rmSync(root, { recursive: true, force: true }));

‎scripts/verify-no-test-retries.mjs‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,15 +102,34 @@ export function findRetries(file, text) {
102102
const rules = RULES.filter((r) => r.files.test(file));
103103
if (rules.length === 0) return [];
104104
const findings = [];
105-
text.split("\n").forEach((lineText, index) => {
105+
const lines = text.split("\n");
106+
lines.forEach((physical, index) => {
107+
// A shell command continued with a trailing backslash is one command: the
108+
// runner can be on one line and its retry flag on the next. Each line is
109+
// therefore read together with its continuation lines, and a finding is
110+
// reported at the line the command starts on. Only a line that does not
111+
// itself continue a previous one starts a command, so a continued command
112+
// is reported once.
113+
let lineText = physical;
114+
if (/\\\s*$/.test(physical)) {
115+
if (index > 0 && /\\\s*$/.test(lines[index - 1])) return;
116+
let end = index;
117+
while (end < lines.length - 1 && /\\\s*$/.test(lines[end])) end += 1;
118+
lineText = lines
119+
.slice(index, end + 1)
120+
.map((l) => l.replace(/\\\s*$/, " "))
121+
.join("");
122+
} else if (index > 0 && /\\\s*$/.test(lines[index - 1])) {
123+
return;
124+
}
106125
for (const rule of rules) {
107126
if (rule.pattern.test(lineText))
108127
findings.push({
109128
file,
110129
line: index + 1,
111130
rule: rule.id,
112131
why: rule.why,
113-
text: lineText.trim(),
132+
text: lineText.trim().replace(/\s+/g, " "),
114133
});
115134
}
116135
});

‎scripts/verify-no-test-retries.test.mjs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,23 @@ for (const [file, text, rules] of cases) {
100100
});
101101
}
102102

103+
test("a flag on a continuation line is found, at the line the command starts", () => {
104+
const found = findRetries(
105+
".github/workflows/python.yml",
106+
[
107+
" - run: |",
108+
" uv run pytest \\",
109+
" -q \\",
110+
" --reruns 3",
111+
" echo done",
112+
].join("\n"),
113+
);
114+
assert.deepEqual(
115+
found.map((f) => [f.rule, f.line, f.text]),
116+
[["workflow-retry-flag", 2, "uv run pytest -q --reruns 3"]],
117+
);
118+
});
119+
103120
test("a finding carries its file, line and the offending text", () => {
104121
const [finding] = findRetries(
105122
"src/a/vitest.config.ts",

0 commit comments

Comments
 (0)