diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7adf6dddac6..f1b47d01c30 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1288,6 +1288,7 @@ jobs: env: OPENCLAW_LOCAL_CHECK: "0" TASK: ${{ matrix.task }} + PR_BASE_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || '' }} shell: bash run: | set -euo pipefail @@ -1297,6 +1298,10 @@ jobs: pnpm tool-display:check pnpm check:host-env-policy:swift pnpm dup:check:coverage + if [ -n "$PR_BASE_SHA" ]; then + git fetch --no-tags --depth=1 origin "+${PR_BASE_SHA}:refs/remotes/origin/pr-base" + node scripts/report-test-temp-creations.mjs --base refs/remotes/origin/pr-base --head HEAD --no-merge-base + fi pnpm deps:patches:check pnpm lint:webhook:no-low-level-body-read pnpm lint:auth:no-pairing-store-group diff --git a/docs/help/testing.md b/docs/help/testing.md index 7ea1f83f23a..231486b52be 100644 --- a/docs/help/testing.md +++ b/docs/help/testing.md @@ -42,6 +42,45 @@ When you touch tests or want extra confidence: - Coverage gate: `pnpm test:coverage` - E2E suite: `pnpm test:e2e` +## Test Temp Directories + +Prefer the shared helpers in `test/helpers/temp-dir.ts` for test-owned +temporary directories. They make ownership explicit and keep cleanup in the same +test lifecycle: + +```ts +import { afterEach } from "vitest"; +import { createTempDirTracker } from "../helpers/temp-dir.js"; + +const tempDirs = createTempDirTracker(); + +afterEach(tempDirs.cleanup); + +it("uses a temp workspace", () => { + const workspace = tempDirs.make("openclaw-example-"); + // use workspace +}); +``` + +Use `makeTempDir(tempDirs, prefix)` and `cleanupTempDirs(tempDirs)` when a test +already owns an array or set of paths. Avoid new bare `fs.mkdtemp*` calls in +tests unless a case is explicitly verifying raw temp-dir behavior. Add an +auditable allow comment with a concrete reason when a test intentionally needs a +bare temp directory: + +```ts +// openclaw-temp-dir: allow verifies raw fs cleanup behavior +const workspace = fs.mkdtempSync(prefix); +``` + +For migration visibility, `node scripts/report-test-temp-creations.mjs` reports +new bare temp-dir creation in added diff lines without blocking existing cleanup +styles. Its file scope intentionally follows the same test-path classification +used by `scripts/changed-lanes.mjs` instead of maintaining a separate test-helper +filename heuristic, while skipping the shared helper implementation itself. +`check:changed` runs this report for changed test paths as a warning-only CI +signal; findings are GitHub warning annotations, not failures. + When debugging real providers/models (requires real creds): - Live suite (models + gateway tool/image probes): `pnpm test:live` diff --git a/scripts/changed-lanes.mjs b/scripts/changed-lanes.mjs index d3ed38184a1..67e51dcd410 100644 --- a/scripts/changed-lanes.mjs +++ b/scripts/changed-lanes.mjs @@ -81,6 +81,10 @@ export function createEmptyChangedLanes() { }; } +export function isChangedLaneTestPath(changedPath) { + return TEST_PATH_RE.test(normalizeChangedPath(changedPath)); +} + /** * @param {string[]} changedPaths * @param {{ packageJsonChangeKind?: "liveDockerTooling" | "tooling" | null }} [options] @@ -165,7 +169,7 @@ export function detectChangedLanes(changedPaths, options = {}) { } if (EXTENSION_PATH_RE.test(changedPath)) { - if (TEST_PATH_RE.test(changedPath)) { + if (isChangedLaneTestPath(changedPath)) { lanes.extensionTests = true; reasons.push(`${changedPath}: extension test`); } else { @@ -177,7 +181,7 @@ export function detectChangedLanes(changedPaths, options = {}) { } if (CORE_PATH_RE.test(changedPath)) { - if (TEST_PATH_RE.test(changedPath)) { + if (isChangedLaneTestPath(changedPath)) { lanes.coreTests = true; reasons.push(`${changedPath}: core test`); } else { diff --git a/scripts/check-changed.mjs b/scripts/check-changed.mjs index e8529beac43..7db39d1903f 100644 --- a/scripts/check-changed.mjs +++ b/scripts/check-changed.mjs @@ -13,6 +13,7 @@ import path from "node:path"; import { performance } from "node:perf_hooks"; import { detectChangedLanesForPaths, + isChangedLaneTestPath, listChangedPathsFromGit, listStagedChangedPaths, normalizeChangedPath, @@ -160,6 +161,10 @@ export function shouldRunShrinkwrapGuard(paths) { return paths.some((changedPath) => SHRINKWRAP_POLICY_PATH_RE.test(changedPath)); } +export function shouldRunTestTempCreationReport(paths) { + return paths.some((changedPath) => isChangedLaneTestPath(changedPath)); +} + export function createShrinkwrapGuardCommand(paths) { if (!shouldRunShrinkwrapGuard(paths)) { return null; @@ -210,6 +215,22 @@ export function createChangedCheckPlan(result, options = {}) { }; const addTypecheck = (name, args) => add(name, args, createSparseTsgoSkipEnv(baseEnv)); const addLint = (name, args) => add(name, args, baseEnv); + const addTestTempCreationReport = () => { + if (!shouldRunTestTempCreationReport(result.paths)) { + return; + } + addCommand( + "test temp creation report (warning-only)", + "node", + [ + "scripts/report-test-temp-creations.mjs", + ...(options.staged + ? ["--staged"] + : ["--base", options.base ?? "origin/main", "--head", options.head ?? "HEAD"]), + ], + baseEnv, + ); + }; add("conflict markers", ["check:no-conflict-markers"]); add("changelog attributions", ["check:changelog-attributions"]); @@ -235,6 +256,8 @@ export function createChangedCheckPlan(result, options = {}) { }; } + addTestTempCreationReport(); + const lanes = result.lanes; const runAll = lanes.all; diff --git a/scripts/report-test-temp-creations.mjs b/scripts/report-test-temp-creations.mjs new file mode 100644 index 00000000000..86b2af00338 --- /dev/null +++ b/scripts/report-test-temp-creations.mjs @@ -0,0 +1,234 @@ +#!/usr/bin/env node + +import { execFileSync } from "node:child_process"; +import { isChangedLaneTestPath } from "./changed-lanes.mjs"; +import { booleanFlag, parseFlagArgs, stringFlag } from "./lib/arg-utils.mjs"; +import { runAsScript } from "./lib/ts-guard-utils.mjs"; + +const DEFAULT_BASE_REF = "origin/main"; +const DEFAULT_HEAD_REF = "HEAD"; +const TEMP_DIR_HELPER_PATH = "test/helpers/temp-dir.ts"; +const FINDING_PATTERNS = [ + { + pattern: /\bmkdtemp(?:Sync)?\s*\(/u, + reason: "new mkdtemp temp directory creation", + }, + { + pattern: /\btmp\s*\.\s*dir(?:Sync)?\s*\(/u, + reason: "new tmp.dir temp directory creation", + }, +]; +const TEMP_DIR_ALLOW_COMMENT_RE = + /(?:^|\s)(?:\/\/|\/\*|\*|#)\s*openclaw-temp-dir:\s*allow\s+(.+)$/u; + +function usage() { + return `Usage: node scripts/report-test-temp-creations.mjs [options] + +Description: + Reports new bare test temp-directory creation patterns in added diff lines. + This is a low-noise migration aid, not a cleanup data-flow checker. It does + not scan existing lines and does not decide whether cleanup is sufficient. + Add "openclaw-temp-dir: allow " in a same-line or immediately + preceding added comment when a test intentionally needs bare temp creation. + File scope intentionally reuses scripts/changed-lanes.mjs test-path + classification instead of maintaining a separate test-helper heuristic. + +Options: + --base Base ref for branch diffs. Default: ${DEFAULT_BASE_REF} + --head Head ref for branch diffs. Default: ${DEFAULT_HEAD_REF} + --no-merge-base Use a two-dot base..head diff for shallow CI checkouts. + --staged Inspect staged changes instead of a branch diff. + --json Print JSON findings to stdout. + --fail-on-findings Exit 1 when findings are present. Default is report-only. + -h, --help Show this help. + +Outputs: + Human mode prints findings to stderr and exits 0 unless --fail-on-findings is set. + GitHub Actions mode prints warning annotations and exits 0 unless --fail-on-findings is set. + JSON mode prints an array of { file, line, reason, source } to stdout. + +Examples: + node scripts/report-test-temp-creations.mjs --base origin/main --head HEAD + node scripts/report-test-temp-creations.mjs --staged --json +`; +} + +function normalizePath(filePath) { + return String(filePath ?? "") + .replaceAll("\\", "/") + .replace(/^\.\/+/u, ""); +} + +function shouldInspectFile(filePath) { + const normalizedPath = normalizePath(filePath); + return normalizedPath !== TEMP_DIR_HELPER_PATH && isChangedLaneTestPath(normalizedPath); +} + +function isTruthyEnvFlag(value) { + const normalized = String(value ?? "") + .trim() + .toLowerCase(); + return normalized !== "" && normalized !== "0" && normalized !== "false" && normalized !== "no"; +} + +function escapeGithubCommandValue(value) { + return String(value).replaceAll("%", "%25").replaceAll("\r", "%0D").replaceAll("\n", "%0A"); +} + +function escapeGithubCommandProperty(value) { + return escapeGithubCommandValue(value).replaceAll(":", "%3A").replaceAll(",", "%2C"); +} + +function hasTempDirAllowMarker(source) { + const reason = source.match(TEMP_DIR_ALLOW_COMMENT_RE)?.[1]?.trim() ?? ""; + return reason.length > 0; +} + +function isTempDirAllowComment(source) { + const trimmed = source.trim(); + return /^(?:\/\/|\/\*|\*|#)/u.test(trimmed) && hasTempDirAllowMarker(trimmed); +} + +export function formatGithubWarning(finding) { + const file = escapeGithubCommandProperty(finding.file); + const line = escapeGithubCommandProperty(finding.line); + const message = escapeGithubCommandValue( + `${finding.reason}: prefer test/helpers/temp-dir.ts for new test-owned temp directories.`, + ); + return `::warning file=${file},line=${line}::${message}`; +} + +function parseArgs(argv) { + const args = { + base: DEFAULT_BASE_REF, + failOnFindings: false, + head: DEFAULT_HEAD_REF, + help: false, + json: false, + noMergeBase: false, + staged: false, + }; + return parseFlagArgs(argv, args, [ + stringFlag("--base", "base"), + booleanFlag("--fail-on-findings", "failOnFindings"), + stringFlag("--head", "head"), + booleanFlag("-h", "help"), + booleanFlag("--help", "help"), + booleanFlag("--json", "json"), + booleanFlag("--no-merge-base", "noMergeBase"), + booleanFlag("--staged", "staged"), + ]); +} + +function readDiff(args, cwd = process.cwd()) { + const range = args.noMergeBase ? `${args.base}..${args.head}` : `${args.base}...${args.head}`; + const diffArgs = args.staged + ? ["diff", "--cached", "--unified=0", "--diff-filter=ACMR", "--"] + : ["diff", "--unified=0", "--diff-filter=ACMR", range, "--"]; + return execFileSync("git", diffArgs, { + cwd, + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + stdio: ["ignore", "pipe", "pipe"], + }); +} + +export function collectTempCreationFindingsFromDiff(diffText) { + const findings = []; + let currentFile = null; + let currentLine = 0; + let allowNextLine = null; + + for (const line of diffText.split(/\r?\n/u)) { + const fileMatch = line.match(/^\+\+\+ b\/(.+)$/u); + if (fileMatch) { + currentFile = normalizePath(fileMatch[1]); + allowNextLine = null; + continue; + } + if (line === "+++ /dev/null") { + currentFile = null; + allowNextLine = null; + continue; + } + + const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/u); + if (hunkMatch) { + currentLine = Number.parseInt(hunkMatch[1], 10); + allowNextLine = null; + continue; + } + + if (line.startsWith("+") && !line.startsWith("+++")) { + if (currentFile && shouldInspectFile(currentFile)) { + const source = line.slice(1); + const allowed = + hasTempDirAllowMarker(source) || + (allowNextLine?.file === currentFile && allowNextLine.line === currentLine); + for (const { pattern, reason } of FINDING_PATTERNS) { + if (pattern.test(source)) { + if (!allowed) { + findings.push({ + file: currentFile, + line: currentLine, + reason, + source: source.trim(), + }); + } + break; + } + } + allowNextLine = isTempDirAllowComment(source) + ? { file: currentFile, line: currentLine + 1 } + : null; + } + currentLine += 1; + continue; + } + + if (line.startsWith(" ") || line === "") { + allowNextLine = null; + currentLine += 1; + } + } + + return findings; +} + +export async function main(argv, io) { + const args = parseArgs(argv ?? process.argv.slice(2)); + const stdout = io?.stdout ?? process.stdout; + const stderr = io?.stderr ?? process.stderr; + const env = io?.env ?? process.env; + if (args.help) { + stdout.write(usage()); + return 0; + } + + const findings = collectTempCreationFindingsFromDiff(readDiff(args)); + if (args.json) { + stdout.write(`${JSON.stringify(findings, null, 2)}\n`); + } else if (findings.length === 0) { + stderr.write("No new bare test temp-directory creation patterns found.\n"); + } else if (isTruthyEnvFlag(env.GITHUB_ACTIONS)) { + for (const finding of findings) { + stderr.write(`${formatGithubWarning(finding)}\n`); + } + } else { + stderr.write("New bare test temp-directory creation patterns:\n"); + for (const finding of findings) { + stderr.write(`- ${finding.file}:${finding.line} ${finding.reason}: ${finding.source}\n`); + } + stderr.write("Prefer test/helpers/temp-dir.ts for new test-owned temp directories.\n"); + } + + return args.failOnFindings && findings.length > 0 ? 1 : 0; +} + +runAsScript(import.meta.url, async (argv, io) => { + const exitCode = await main(argv, io); + if (!io) { + process.exitCode = exitCode; + } + return exitCode; +}); diff --git a/scripts/test-projects.test-support.mjs b/scripts/test-projects.test-support.mjs index cbd758c1708..5cc1abed284 100644 --- a/scripts/test-projects.test-support.mjs +++ b/scripts/test-projects.test-support.mjs @@ -1760,6 +1760,10 @@ function resolveSiblingTestTarget(changedPath, cwd) { return fs.existsSync(path.join(cwd, sibling)) ? sibling : null; } +function shouldCombineSiblingTestWithImportGraph(changedPath) { + return changedPath.startsWith("test/helpers/"); +} + function shouldRouteChangedTargetWithoutImportGraph(changedPath) { return ( changedPath.endsWith(".live.test.ts") || @@ -1778,7 +1782,7 @@ function resolvePreciseChangedTestTargets(changedPath, options) { return [changedPath]; } const siblingTest = resolveSiblingTestTarget(changedPath, cwd); - if (siblingTest) { + if (siblingTest && !shouldCombineSiblingTestWithImportGraph(changedPath)) { return [siblingTest]; } if (BROAD_ONLY_TEST_HELPERS.has(changedPath)) { @@ -1795,10 +1799,10 @@ function resolvePreciseChangedTestTargets(changedPath, options) { forceFull: options.forceFullImportGraph === true, }); if (affectedTests.length > 0) { - return affectedTests; + return siblingTest ? uniqueOrdered([siblingTest, ...affectedTests]) : affectedTests; } } - return null; + return siblingTest ? [siblingTest] : null; } function isDeletedChangedTestTarget(changedPath, cwd) { diff --git a/src/scripts/test-projects.test.ts b/src/scripts/test-projects.test.ts index 2a50bd14ea9..a5e6d942cce 100644 --- a/src/scripts/test-projects.test.ts +++ b/src/scripts/test-projects.test.ts @@ -873,9 +873,11 @@ describe("test-projects args", () => { includePatterns: [ "src/scripts/docs-link-audit.test.ts", "src/scripts/sync-plugin-versions.test.ts", + "test/helpers/temp-dir.test.ts", "test/scripts/ios-pin-version.test.ts", "test/scripts/ios-team-id.test.ts", "test/scripts/ios-version.test.ts", + "test/scripts/report-test-temp-creations.test.ts", "test/test-env.test.ts", "test/vitest-scoped-config.test.ts", ], diff --git a/test/helpers/temp-dir.test.ts b/test/helpers/temp-dir.test.ts new file mode 100644 index 00000000000..9077b7ea2ff --- /dev/null +++ b/test/helpers/temp-dir.test.ts @@ -0,0 +1,42 @@ +import fs from "node:fs"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { cleanupTempDirs, createTempDirTracker, makeTempDir } from "./temp-dir.js"; + +const tempDirs = new Set(); + +afterEach(() => { + cleanupTempDirs(tempDirs); +}); + +describe("temp-dir test helpers", () => { + it("keeps a non-executed temp warning fixture for CI proof", () => { + // openclaw-temp-dir: allow test fixture for the temp warning report + const warningFixture = 'tmp.dirSync({ prefix: "openclaw-warning-fixture-" })'; + + expect(warningFixture).toContain("tmp.dirSync"); + }); + + it("tracks created temp dirs and removes populated dirs", () => { + const tracker = createTempDirTracker(); + const dir = tracker.make("openclaw-temp-dir-helper-"); + tempDirs.add(dir); + fs.writeFileSync(path.join(dir, "artifact.txt"), "artifact\n", "utf8"); + + tracker.cleanup(); + tempDirs.delete(dir); + + expect(fs.existsSync(dir)).toBe(false); + expect([...tracker.dirs]).toEqual([]); + }); + + it("supports existing caller-owned temp dir collections", () => { + const dir = makeTempDir(tempDirs, "openclaw-temp-dir-existing-"); + fs.mkdirSync(path.join(dir, "nested"), { recursive: true }); + + cleanupTempDirs(tempDirs); + + expect(fs.existsSync(dir)).toBe(false); + expect([...tempDirs]).toEqual([]); + }); +}); diff --git a/test/helpers/temp-dir.ts b/test/helpers/temp-dir.ts index 67577fbedfb..7db37f4587e 100644 --- a/test/helpers/temp-dir.ts +++ b/test/helpers/temp-dir.ts @@ -5,8 +5,16 @@ import path from "node:path"; // Synchronous temporary directory helpers for tests. +export type TempDirCollection = string[] | Set; + +export interface TestTempDirTracker { + readonly dirs: ReadonlySet; + make(prefix: string): string; + cleanup(): void; +} + /** Create a temp dir and register it in an array or set for cleanup. */ -export function makeTempDir(tempDirs: string[] | Set, prefix: string): string { +export function makeTempDir(tempDirs: TempDirCollection, prefix: string): string { const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); if (Array.isArray(tempDirs)) { tempDirs.push(dir); @@ -17,12 +25,25 @@ export function makeTempDir(tempDirs: string[] | Set, prefix: string): s } /** Remove all tracked temporary directories and clear the tracker. */ -export function cleanupTempDirs(tempDirs: string[] | Set): void { +export function cleanupTempDirs(tempDirs: TempDirCollection): void { const dirs = Array.isArray(tempDirs) ? tempDirs.splice(0) : [...tempDirs]; for (const dir of dirs) { - fs.rmSync(dir, { recursive: true, force: true }); + fs.rmSync(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 20 }); } if (!Array.isArray(tempDirs)) { tempDirs.clear(); } } + +export function createTempDirTracker(): TestTempDirTracker { + const dirs = new Set(); + return { + dirs, + make(prefix: string): string { + return makeTempDir(dirs, prefix); + }, + cleanup(): void { + cleanupTempDirs(dirs); + }, + }; +} diff --git a/test/scripts/changed-lanes.test.ts b/test/scripts/changed-lanes.test.ts index 5df48bc4020..aa8855e387c 100644 --- a/test/scripts/changed-lanes.test.ts +++ b/test/scripts/changed-lanes.test.ts @@ -6,6 +6,7 @@ import { afterEach, describe, expect, it } from "vitest"; import { createEmptyChangedLanes, detectChangedLanes, + isChangedLaneTestPath, isLiveDockerPackageScriptOnlyChange, isPackageScriptOnlyChange, listChangedPathsFromGit, @@ -19,6 +20,7 @@ import { createTargetedCoreLintCommand, shouldDelegateChangedCheckToCrabbox, shouldRunShrinkwrapGuard, + shouldRunTestTempCreationReport, createShrinkwrapGuardCommand, } from "../../scripts/check-changed.mjs"; import { isDirectRunPath } from "../../scripts/lib/direct-run.mjs"; @@ -143,6 +145,13 @@ afterEach(() => { }); describe("scripts/changed-lanes", () => { + it("keeps a non-executed changed-gate warning fixture", () => { + // openclaw-temp-dir: allow test fixture for the temp warning report + const warningFixture = 'fs.mkdtemp("openclaw-warning-fixture-", () => {})'; + + expect(warningFixture).toContain("mkdtemp"); + }); + it("detects direct script execution from Windows argv paths", () => { expect( isDirectRunPath( @@ -458,6 +467,13 @@ describe("scripts/changed-lanes", () => { expect(result.lanes.all).toBe(false); }); + it("exposes the shared changed-lane test path classifier", () => { + expect(isChangedLaneTestPath("src/shared/string-normalization.test.ts")).toBe(true); + expect(isChangedLaneTestPath("packages/foo/__tests__/helper.ts")).toBe(true); + expect(isChangedLaneTestPath("src/example.ts")).toBe(false); + expect(isChangedLaneTestPath("src/latest.ts")).toBe(false); + }); + it("routes core production changes to core prod and core test lanes", () => { const result = detectChangedLanes(["packages/normalization-core/src/string-normalization.ts"]); const plan = createChangedCheckPlan(result, { env: { PATH: "/usr/bin" } }); @@ -896,6 +912,7 @@ describe("scripts/changed-lanes", () => { "duplicate scan target coverage", "dependency pin guard", "package patch guard", + "test temp creation report (warning-only)", "typecheck core tests", "lint core", "lint scripts", @@ -1388,6 +1405,30 @@ describe("scripts/changed-lanes", () => { expect(plan.commands.map((command) => command.args[0])).not.toContain("test"); }); + it("adds the warning-only temp creation report for changed test paths", () => { + const result = detectChangedLanes(["test/helpers/temp-fixture.ts"]); + const plan = createChangedCheckPlan(result, { base: "main", head: "feature" }); + const command = plan.commands.find( + (candidate) => candidate.name === "test temp creation report (warning-only)", + ); + + expect(shouldRunTestTempCreationReport(result.paths)).toBe(true); + expect(command).toMatchObject({ + bin: "node", + args: ["scripts/report-test-temp-creations.mjs", "--base", "main", "--head", "feature"], + }); + }); + + it("keeps the temp creation report out of non-test changed paths", () => { + const result = detectChangedLanes(["scripts/check-changed.mjs"]); + const plan = createChangedCheckPlan(result); + + expect(shouldRunTestTempCreationReport(result.paths)).toBe(false); + expect(plan.commands.map((command) => command.name)).not.toContain( + "test temp creation report (warning-only)", + ); + }); + it("does not route generated plugin bundle artifacts as direct Vitest targets", () => { const result = detectChangedLanes([ "extensions/demo/src/host/assets/.bundle.hash", diff --git a/test/scripts/report-test-temp-creations.test.ts b/test/scripts/report-test-temp-creations.test.ts new file mode 100644 index 00000000000..55c91d87207 --- /dev/null +++ b/test/scripts/report-test-temp-creations.test.ts @@ -0,0 +1,246 @@ +import { execFileSync, spawnSync } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { + collectTempCreationFindingsFromDiff, + formatGithubWarning, +} from "../../scripts/report-test-temp-creations.mjs"; +import { createTempDirTracker } from "../helpers/temp-dir.js"; + +const repoRoot = process.cwd(); +const tempDirs = createTempDirTracker(); +const nestedGitEnvKeys = [ + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_DIR", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_QUARANTINE_PATH", + "GIT_WORK_TREE", +] as const; + +function createNestedGitEnv(): NodeJS.ProcessEnv { + const env = { + ...process.env, + GIT_CONFIG_NOSYSTEM: "1", + GIT_TERMINAL_PROMPT: "0", + }; + for (const key of nestedGitEnvKeys) { + delete env[key]; + } + return env; +} + +afterEach(() => { + tempDirs.cleanup(); +}); + +describe("report-test-temp-creations", () => { + it("keeps a non-executed warning fixture for changed-gate proof", () => { + // openclaw-temp-dir: allow test fixture for the temp warning report + const warningFixture = 'fs.mkdtempSync("openclaw-warning-fixture-")'; + + expect(warningFixture).toContain("mkdtempSync"); + }); + + it("reports added bare temp creation lines using changed-lane test path scope", () => { + const bareTempSource = [ + "const tempRoot = fs.", + "mkdtemp", + 'Sync(path.join(os.tmpdir(), "case-"));', + ].join(""); + const mkdtempSource = ["const tempRoot = fs.", "mkdtemp", 'Sync("case-");'].join(""); + const diff = [ + "diff --git a/src/example.test.ts b/src/example.test.ts", + "--- a/src/example.test.ts", + "+++ b/src/example.test.ts", + "@@ -10,0 +11,3 @@", + `+${bareTempSource}`, + '+const helperRoot = makeTempDir(tempDirs, "case-");', + "+console.log(tempRoot, helperRoot);", + "diff --git a/src/example.ts b/src/example.ts", + "--- a/src/example.ts", + "+++ b/src/example.ts", + "@@ -4,0 +5,1 @@", + `+${["const productionTemp = fs.", "mkdtemp", 'Sync("case-");'].join("")}`, + "diff --git a/test/helper.test-support.mjs b/test/helper.test-support.mjs", + "--- a/test/helper.test-support.mjs", + "+++ b/test/helper.test-support.mjs", + "@@ -1,0 +2,1 @@", + `+${mkdtempSource}`, + "diff --git a/test/helpers/temp-fixture.ts b/test/helpers/temp-fixture.ts", + "--- a/test/helpers/temp-fixture.ts", + "+++ b/test/helpers/temp-fixture.ts", + "@@ -1,0 +2,1 @@", + `+${mkdtempSource}`, + "diff --git a/test/helpers/temp-dir.ts b/test/helpers/temp-dir.ts", + "--- a/test/helpers/temp-dir.ts", + "+++ b/test/helpers/temp-dir.ts", + "@@ -1,0 +2,1 @@", + `+${mkdtempSource}`, + "diff --git a/packages/foo/__tests__/helper.ts b/packages/foo/__tests__/helper.ts", + "--- a/packages/foo/__tests__/helper.ts", + "+++ b/packages/foo/__tests__/helper.ts", + "@@ -1,0 +2,1 @@", + `+${mkdtempSource}`, + "diff --git a/extensions/discord/src/monitor/message-handler.test-helpers.ts b/extensions/discord/src/monitor/message-handler.test-helpers.ts", + "--- a/extensions/discord/src/monitor/message-handler.test-helpers.ts", + "+++ b/extensions/discord/src/monitor/message-handler.test-helpers.ts", + "@@ -1,0 +2,1 @@", + `+${mkdtempSource}`, + ].join("\n"); + + expect(collectTempCreationFindingsFromDiff(diff)).toEqual([ + { + file: "src/example.test.ts", + line: 11, + reason: "new mkdtemp temp directory creation", + source: bareTempSource, + }, + { + file: "test/helper.test-support.mjs", + line: 2, + reason: "new mkdtemp temp directory creation", + source: mkdtempSource, + }, + { + file: "test/helpers/temp-fixture.ts", + line: 2, + reason: "new mkdtemp temp directory creation", + source: mkdtempSource, + }, + { + file: "packages/foo/__tests__/helper.ts", + line: 2, + reason: "new mkdtemp temp directory creation", + source: mkdtempSource, + }, + ]); + }); + + it("honors explicit allow comments with reasons", () => { + const mkdtempCall = ["fs.", "mkdtemp", 'Sync("case-")'].join(""); + const tmpDirCall = ["tmp.", "dir", 'Sync({ prefix: "case-" })'].join(""); + const allowedSource = `const allowed = ${mkdtempCall};`; + const inlineAllowedSource = `const inlineAllowed = ${tmpDirCall}; // openclaw-temp-dir: allow verifies tmp API behavior`; + const blockedSource = `const blocked = ${mkdtempCall};`; + const stringMarkerSource = `const stringMarker = ${mkdtempCall}; const note = "openclaw-temp-dir: allow quoted text";`; + const emptyReasonSource = `const emptyReason = ${mkdtempCall};`; + const diff = [ + "diff --git a/test/helpers/raw-temp.test.ts b/test/helpers/raw-temp.test.ts", + "--- a/test/helpers/raw-temp.test.ts", + "+++ b/test/helpers/raw-temp.test.ts", + "@@ -1,0 +2,5 @@", + "+// openclaw-temp-dir: allow verifies raw fs cleanup behavior", + `+${allowedSource}`, + `+${inlineAllowedSource}`, + `+${blockedSource}`, + `+${stringMarkerSource}`, + "diff --git a/test/helpers/empty-allow.test.ts b/test/helpers/empty-allow.test.ts", + "--- a/test/helpers/empty-allow.test.ts", + "+++ b/test/helpers/empty-allow.test.ts", + "@@ -1,0 +2,2 @@", + "+// openclaw-temp-dir: allow", + `+${emptyReasonSource}`, + ].join("\n"); + + expect(collectTempCreationFindingsFromDiff(diff)).toEqual([ + { + file: "test/helpers/raw-temp.test.ts", + line: 5, + reason: "new mkdtemp temp directory creation", + source: blockedSource, + }, + { + file: "test/helpers/raw-temp.test.ts", + line: 6, + reason: "new mkdtemp temp directory creation", + source: stringMarkerSource, + }, + { + file: "test/helpers/empty-allow.test.ts", + line: 3, + reason: "new mkdtemp temp directory creation", + source: emptyReasonSource, + }, + ]); + }); + + it("prints help with usage, outputs, and examples", () => { + const output = execFileSync( + process.execPath, + [path.join(repoRoot, "scripts", "report-test-temp-creations.mjs"), "--help"], + { + cwd: repoRoot, + encoding: "utf8", + }, + ); + + expect(output).toContain("Usage: node scripts/report-test-temp-creations.mjs"); + expect(output).toContain("Outputs:"); + expect(output).toContain("--no-merge-base"); + expect(output).toContain("Examples:"); + }); + + it("formats GitHub warning annotations for CI report mode", () => { + expect( + formatGithubWarning({ + file: "test/helpers/temp,fixture.ts", + line: 12, + reason: "new mkdtemp temp directory creation", + // openclaw-temp-dir: allow test fixture for GitHub warning formatting + source: "const tempRoot = fs.mkdtempSync();", + }), + ).toBe( + "::warning file=test/helpers/temp%2Cfixture.ts,line=12::new mkdtemp temp directory creation: prefer test/helpers/temp-dir.ts for new test-owned temp directories.", + ); + }); + + it("exits non-zero for staged findings when requested", () => { + const root = tempDirs.make("openclaw-temp-report-"); + const env = createNestedGitEnv(); + execFileSync("git", ["init", "-q", "--initial-branch=main"], { cwd: root, env }); + fs.mkdirSync(path.join(root, "test", "helpers"), { recursive: true }); + fs.writeFileSync(path.join(root, "test", "helpers", "case.ts"), "const value = 1;\n", "utf8"); + execFileSync("git", ["add", "test/helpers/case.ts"], { cwd: root, env }); + execFileSync( + "git", + [ + "-c", + "user.email=test@example.com", + "-c", + "user.name=Test User", + "commit", + "-q", + "-m", + "initial", + ], + { cwd: root, env }, + ); + + const source = [ + "const tempRoot = fs.", + "mkdtemp", + 'Sync(path.join(os.tmpdir(), "case-"));\n', + ].join(""); + fs.appendFileSync(path.join(root, "test", "helpers", "case.ts"), source, "utf8"); + execFileSync("git", ["add", "test/helpers/case.ts"], { cwd: root, env }); + + const result = spawnSync( + process.execPath, + [ + path.join(repoRoot, "scripts", "report-test-temp-creations.mjs"), + "--staged", + "--fail-on-findings", + ], + { + cwd: root, + encoding: "utf8", + env, + }, + ); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("test/helpers/case.ts"); + }); +}); diff --git a/test/scripts/test-projects.test.ts b/test/scripts/test-projects.test.ts index 29730a225a8..921bfaf3426 100644 --- a/test/scripts/test-projects.test.ts +++ b/test/scripts/test-projects.test.ts @@ -1123,6 +1123,8 @@ describe("scripts/test-projects changed-target routing", () => { withTinyGitRepo( { "test/helpers/temp-dir.ts": "export const tempDir = 'x';\n", + "test/helpers/temp-dir.test.ts": + "import { tempDir } from './temp-dir.js';\nvoid tempDir;\n", "src/foo.test.ts": "import { tempDir } from '../test/helpers/temp-dir.js';\nvoid tempDir;\n", }, @@ -1131,7 +1133,7 @@ describe("scripts/test-projects changed-target routing", () => { }, ); - expect(targets).toEqual(["src/foo.test.ts"]); + expect(targets).toEqual(["test/helpers/temp-dir.test.ts", "src/foo.test.ts"]); }); it("keeps the broad changed run available for shared test helpers", () => {