Skip to content

Commit 1f4eae7

Browse files
t3dotggclaude
t3dotgg
and
claude
authored
fix(scripts): check:changed no longer skips knip after a removed import (#22784)
Co-authored-by: Claude Opus 5.5 <[email protected]>
1 parent 74541ba commit 1f4eae7

3 files changed

Lines changed: 49 additions & 37 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ An empty database is a bad test. Seed your worktree's `.t3` with a copy of real
9898

9999
## Verifying
100100

101-
- Before you push, run `vp run check:changed`. It runs CI checks for changed files: format, lint, tests beside changes, typecheck of affected packages and dependents, plus knip when files, exports, or dependencies change.
101+
- Before you push, run `vp run check:changed`. It runs CI's fast checks for changed files: format, lint, knip, typecheck of affected packages and dependents, and tests beside changes. CI still runs the full test suites.
102102
- While you iterate, run `vp test run <files>`. For big files, add `-t "<name>"`.
103103
- Do not use `npx tsc`. Use `vp run typecheck`; it uses tsc-rs, which is 3x faster.
104104
- Run `node -e` snippets from `apps/server` or the owning package. `effect` does not resolve from the repo root.

‎docs/operations/development.md‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -102,12 +102,12 @@ for the security model.
102102

103103
## Checks
104104

105-
`vp run check:changed` runs what CI runs for your branch's changes: format and lint of the
106-
changed files, `knip:check` when files, exports, or dependencies change, typecheck of the affected
107-
packages and their dependents, and the test files beside the changed files. It maps files to
108-
packages with [ci-changes.cjs](../../.github/scripts/ci-changes.cjs), as CI does. It compares
109-
against the merge base with `origin/main`, or a ref you pass. Tests that reach a changed file only
110-
through imports run in CI.
105+
`vp run check:changed` runs CI's fast checks for your branch's changes: format and lint of the
106+
changed files, `knip:check` when code or a manifest changes, the generated GraphQL checks, typecheck
107+
of the affected packages and their dependents, and the test files beside the changed files. It maps
108+
files to packages with [ci-changes.cjs](../../.github/scripts/ci-changes.cjs), as CI does. It
109+
compares against the merge base with `origin/main`, or with a ref you pass. CI still runs the full
110+
test suites and the jobs the summary lists, so a local pass does not promise a green CI run.
111111

112112
While you iterate, run single checks:
113113

‎scripts/check-changed.ts‎

Lines changed: 42 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
// @effect-diagnostics nodeBuiltinImport:off globalConsole:off
2-
// Runs locally what CI runs for this branch's changes: format and lint of the
3-
// changed files, knip, typecheck of the affected packages and their dependents,
4-
// and the test files beside the changed files.
2+
// Runs CI's fast checks for this branch's changes: format and lint of the
3+
// changed files, knip, generated GraphQL operations, typecheck of the affected
4+
// packages and their dependents, and the test files beside the changed files.
5+
// CI still runs the full test suites and the jobs listed in the summary.
56
//
67
// vp run check:changed compare against the merge base with origin/main
7-
// vp run check:changed <ref> compare against <ref>
8+
// vp run check:changed <ref> compare against the merge base with <ref>
89
//
9-
// Uncommitted and untracked files count. The package mapping comes from
10-
// .github/scripts/ci-changes.cjs, so this typechecks what CI would typecheck.
10+
// Uncommitted and untracked files count. Affected packages come from
11+
// .github/scripts/ci-changes.cjs, the same mapping CI uses to gate its jobs.
1112
import * as NodeChildProcess from "node:child_process";
1213
import * as NodeFS from "node:fs";
1314
import * as NodePath from "node:path";
@@ -21,7 +22,18 @@ const git = (...args: Array<string>) =>
2122
.split("\n")
2223
.filter(Boolean);
2324

24-
const base = process.argv[2] ?? git("merge-base", "HEAD", "origin/main")[0] ?? "origin/main";
25+
const ref = process.argv[2] ?? "origin/main";
26+
const mergeBase = NodeChildProcess.spawnSync("git", ["merge-base", "HEAD", ref], {
27+
cwd: root,
28+
encoding: "utf8",
29+
});
30+
if (mergeBase.status !== 0) {
31+
console.error(
32+
`check:changed: no merge base with ${ref}. Run \`git fetch origin\` or pass a ref.`,
33+
);
34+
process.exit(1);
35+
}
36+
const base = mergeBase.stdout.trim();
2537
// --no-renames lists a rename as its old and new path, as CI does.
2638
const untracked = git("ls-files", "--others", "--exclude-standard");
2739
const files = [
@@ -52,25 +64,17 @@ for (const file of code) {
5264
}
5365
}
5466

55-
// knip:check takes about 35 s and cannot be scoped to a package, so it runs
56-
// only when the change can leave a file, export, or dependency unused.
57-
const addedOrDeleted = new Set([
58-
...git("diff", "--name-only", "--no-renames", "--diff-filter=AD", base, "--"),
59-
...untracked,
60-
]);
61-
const changedExport =
62-
code.length > 0 &&
63-
git("diff", "--unified=0", base, "--", ...code).some((line) =>
64-
/^[+-](?!\+\+ |-- ).*\bexport\b/.test(line),
65-
);
66-
const runKnip =
67-
changedExport ||
68-
files.some(
69-
(file) =>
70-
(CODE.test(file) && addedOrDeleted.has(file)) ||
71-
NodePath.basename(file) === "package.json" ||
72-
file === "knip.jsonc",
73-
);
67+
// knip:check takes about 35 s and cannot be scoped to a package. Any code
68+
// change can leave a file, export, or dependency unused (removing the last
69+
// import of a helper does), so it runs whenever code or a manifest changes.
70+
const runKnip = files.some(
71+
(file) => CODE.test(file) || NodePath.basename(file) === "package.json" || file === "knip.jsonc",
72+
);
73+
74+
// CI's Lint job checks that generated GraphQL operations match their sources.
75+
const graphql = ["apps/server", "packages/source-control-github"].filter((dir) =>
76+
files.some((file) => file.startsWith(`${dir}/`)),
77+
);
7478

7579
const filters = (names: ReadonlyArray<string>) => names.flatMap((name) => ["--filter", name]);
7680
const steps = [
@@ -80,6 +84,10 @@ const steps = [
8084
},
8185
code.length > 0 && { name: "lint", args: ["lint", ...code] },
8286
runKnip && { name: "knip", args: ["run", "knip:check"] },
87+
...graphql.map((dir) => ({
88+
name: `graphql (${dir})`,
89+
args: ["run", "--filter", `./${dir}`, "generate", "--check"],
90+
})),
8391
affected.length > 0 && {
8492
name: `typecheck (${affected.length} packages)`,
8593
args: ["run", "--cache", "--concurrency-limit", "4", ...filters(affected), "typecheck"],
@@ -104,9 +112,13 @@ console.log("\ncheck:changed summary");
104112
for (const result of results) {
105113
console.log(` ${result.ok ? "pass" : "FAIL"} ${result.name} ${result.seconds}s`);
106114
}
107-
const ciOnly = (["build", "rust", "mobile_native", "release_smoke"] as const).filter(
108-
(job) => outputs[job],
109-
);
115+
const ciOnly = Object.entries({
116+
"full test suites": outputs.test || outputs.test_web || outputs.test_server,
117+
build: outputs.build,
118+
rust: outputs.rust,
119+
"mobile native lint": outputs.mobile_native,
120+
"release smoke": outputs.release_smoke,
121+
}).flatMap(([job, runs]) => (runs ? [job] : []));
110122
if (ciOnly.length > 0) console.log(` CI also runs: ${ciOnly.join(", ")}`);
111-
if (!runKnip) console.log(" knip skipped: no files, exports, or dependencies changed");
123+
if (!runKnip) console.log(" knip skipped: no code or manifest changed");
112124
if (results.some((result) => !result.ok)) process.exit(1);

0 commit comments

Comments
 (0)