From c80f728ccd76822dd2cf10100d96646b2ee10c03 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 12 Oct 2026 14:20:22 -0700 Subject: [PATCH 1/2] fix(check): refuse paths outside the project, and name the missing paths it skips check reported success with zero findings for a path outside the project or one that does not exist, so a check that never looked read as a clean codebase. A path outside the project is now refused with INVALID_INPUT and exit 1. A missing path is still skipped with exit 0, since raw git diff output names deleted files, but the skipped paths are named in a notice. --- .changeset/check-paths-outside-project.md | 5 + packages/cli/src/agent/check.md | 30 +++-- packages/cli/src/agent/ci.md | 8 +- packages/cli/src/commands/check.ts | 130 +++++++++++++++++---- packages/cli/test/check.test.ts | 136 +++++++++++++++++++++- 5 files changed, 268 insertions(+), 41 deletions(-) create mode 100644 .changeset/check-paths-outside-project.md diff --git a/.changeset/check-paths-outside-project.md b/.changeset/check-paths-outside-project.md new file mode 100644 index 00000000..6bdb905b --- /dev/null +++ b/.changeset/check-paths-outside-project.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`check` no longer reports a clean result for paths it never scanned. A path outside the project, existing or not, is refused with `INVALID_INPUT` and exit 1. A path that does not exist is still skipped with exit 0, so raw `git diff --name-only` output can still be piped in, but the skipped paths are now named in a notice (`notices` under `--json`). diff --git a/packages/cli/src/agent/check.md b/packages/cli/src/agent/check.md index 0fddf32e..c2353b08 100644 --- a/packages/cli/src/agent/check.md +++ b/packages/cli/src/agent/check.md @@ -1,4 +1,4 @@ -# Topic: check (CLI v%(CLI_VERSION)s / topic v7) +# Topic: check (CLI v%(CLI_VERSION)s / topic v8) ## Goal Run the applicable rules against the codebase and report matches. Two @@ -209,8 +209,15 @@ delete the rules to make `check` pass, and do not suggest ``` %(TASKLESS_CLI)s check --json $(git diff --name-only main...HEAD) ``` - Paths that don't exist on disk are silently filtered, so you can - pipe raw `git diff` output directly without pre-filtering. + Paths that don't exist on disk are skipped, so you can pipe raw + `git diff` output directly without pre-filtering. The skipped paths + are named in a notice (`notices` under `--json`), so a mistyped + path is visible rather than reading as clean. + + Paths must be inside the project (the working directory, or + `--dir`). A path outside it, existing or not, is refused with + `INVALID_INPUT` and exit 1, and nothing is scanned. Run `check` + from the project that holds the files instead. When runtime rules were present but not run (e.g. logged out), the JSON also carries `"skipped": [{ "rule": "", "reason": "…" }]` @@ -253,7 +260,9 @@ delete the rules to make `check` pass, and do not suggest (exit code 1); `success: true` with a non-empty `results` array means there are only warning/info/hint findings (exit code 0); `success: true` with an - empty `results` array means the codebase is clean. Findings are + empty `results` array means the codebase is clean, unless a notice + says the supplied paths were skipped, in which case nothing was + scanned. Findings are never reported via the `{ ok: false, code, message }` envelope: the envelope only appears when the scan itself fails (e.g. `SCAN_FAILED`) and the normal results payload is absent. @@ -261,19 +270,20 @@ delete the rules to make `check` pass, and do not suggest ## Exit codes - `0`: All checks passed, no rules configured, or all supplied - paths missing + paths missing (named in a notice) - `1`: Errors detected, scan failed, runtime rules withheld because the plan does not include them, an issued ast-grep or Vale rule was - edited or copied under a new id, a rule was unaccounted for, or two - engines share a rule id + edited or copied under a new id, a rule was unaccounted for, two + engines share a rule id, or a supplied path is outside the project ## Errors When `--json` is set, failures emit `{ ok: false, code, message }`: -| code | meaning | fix | -|---------------|-----------------------|----------------------------------| -| `SCAN_FAILED` | ast-grep scan errored | Report; check rule YAML validity | +| code | meaning | fix | +|-----------------|-------------------------------|-------------------------------------------| +| `SCAN_FAILED` | ast-grep scan errored | Report; check rule YAML validity | +| `INVALID_INPUT` | a path is outside the project | Run from the project that holds the files | ## See Also diff --git a/packages/cli/src/agent/ci.md b/packages/cli/src/agent/ci.md index 42580e4d..4d675ab0 100644 --- a/packages/cli/src/agent/ci.md +++ b/packages/cli/src/agent/ci.md @@ -1,4 +1,4 @@ -# Topic: ci (CLI v%(CLI_VERSION)s / topic v4) +# Topic: ci (CLI v%(CLI_VERSION)s / topic v5) ## Goal Wire `%(TASKLESS_CLI)s check` into the user's existing CI so rules run @@ -59,8 +59,10 @@ which CI they use. If multiple match, ask which should run Taskless. | Azure Pipelines | `System.PullRequest.TargetBranch` | | Bitbucket Pipelines | `BITBUCKET_PR_DESTINATION_BRANCH` | - `%(TASKLESS_CLI)s check` silently filters paths that don't exist, so raw - `git diff --name-only` output can pipe in directly. + `%(TASKLESS_CLI)s check` skips paths that don't exist (naming them in a + notice), so raw `git diff --name-only` output can pipe in directly. + Run it from the repository root: `git diff` paths are relative to the + root, and a path outside the project is refused. **Recommended default:** diff scan on PRs, full scan on pushes to main. diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 48d0240b..ee91c247 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -1,5 +1,5 @@ import { resolve, isAbsolute, relative } from "node:path"; -import { stat } from "node:fs/promises"; +import { realpath, stat } from "node:fs/promises"; import process from "node:process"; import { defineCommand } from "citty"; @@ -30,34 +30,82 @@ async function pathExists(absolutePath: string): Promise { } } +/** `target` relative to `root`, or undefined when it lies outside `root`. */ +function relativeInside(root: string, target: string): string | undefined { + const relativePath = relative(root, target); + const escapes = + relativePath === ".." || + relativePath.startsWith(`..${"/"}`) || + relativePath.startsWith(`..${"\\"}`) || + isAbsolute(relativePath); + return escapes ? undefined : relativePath; +} + +interface ClassifiedPaths { + /** In the project and on disk, relative to cwd so `sg scan` can use them. */ + kept: string[]; + /** In the project and not on disk, as the caller wrote them. */ + missing: string[]; + /** Outside the project, as the caller wrote them. */ + outside: string[]; +} + /** - * Resolve each positional path against cwd and filter out any that don't - * exist on disk. Returns a list of paths relative to cwd so `sg scan` can - * be spawned from cwd and use those paths directly. + * Sort each positional path into the three cases `check` answers differently. + * + * All three used to collapse into "drop it silently", so a path outside the + * project, or one that does not exist, produced `success: true` with nothing + * scanned and nothing said. That is a clean result for a check that did not + * look, and a caller cannot tell it from a clean codebase (#475). + * + * Containment is decided from the path as written, before asking whether it + * exists, so `/no/such/file.py` is reported as outside rather than as missing. + * It is checked against both cwd as given and its real path: on macOS `/tmp` + * is `/private/tmp`, and a hook handing over absolute paths in one spelling + * while cwd is in the other must not read as escaping the project. */ -async function filterExistingPaths( +async function classifyPaths( cwd: string, rawPaths: string[] -): Promise { +): Promise { const resolvedCwd = resolve(cwd); - const kept: string[] = []; + const roots = [resolvedCwd]; + try { + const realCwd = await realpath(resolvedCwd); + if (realCwd !== resolvedCwd) roots.push(realCwd); + } catch { + // cwd not existing is reported later by whatever reads the project. + } + + const classified: ClassifiedPaths = { kept: [], missing: [], outside: [] }; for (const rawPath of rawPaths) { const absolutePath = isAbsolute(rawPath) ? resolve(rawPath) : resolve(resolvedCwd, rawPath); - if (!(await pathExists(absolutePath))) continue; - const relativePath = relative(resolvedCwd, absolutePath); - // Reject paths that escape cwd (e.g. `../outside-project`) so `sg scan` - // never traverses outside the project directory. - const escapesCwd = - relativePath === ".." || - relativePath.startsWith(`..${"/"}`) || - relativePath.startsWith(`..${"\\"}`) || - isAbsolute(relativePath); - if (escapesCwd) continue; - kept.push(relativePath === "" ? "." : relativePath); + const relativePath = roots + .map((root) => relativeInside(root, absolutePath)) + .find((candidate) => candidate !== undefined); + if (relativePath === undefined) { + classified.outside.push(rawPath); + } else if (await pathExists(absolutePath)) { + classified.kept.push(relativePath === "" ? "." : relativePath); + } else { + classified.missing.push(rawPath); + } } - return kept; + return classified; +} + +/** + * The notice for paths that were skipped because they do not exist. + * + * Skipped rather than refused: raw `git diff --name-only` output names + * deleted files, and the CI recipe pipes it straight in, so a missing path is + * an ordinary input. It is still named, so a mistyped path is visible. + */ +function missingPathsNotice(missing: string[]): string { + const noun = missing.length === 1 ? "path that does" : "paths that do"; + return `Skipped ${missing.length} ${noun} not exist: ${missing.join(", ")}`; } /** @@ -205,22 +253,49 @@ export const checkCommand = defineCommand({ try { const positionalPaths = extractPositionalPaths(rawArgs); const hadExplicitPaths = positionalPaths.length > 0; - const existingPaths = hadExplicitPaths - ? await filterExistingPaths(cwd, positionalPaths) - : []; + const { + kept: existingPaths, + missing, + outside, + } = await classifyPaths(cwd, positionalPaths); + + // Refused, not skipped. `check` only scans inside the project, and a + // path outside it is a caller pointing at the wrong place: reporting it + // clean is the false negative #475 describes. + if (outside.length > 0) { + const message = `${outside.length === 1 ? "Path is" : "Paths are"} outside the project (${resolve(cwd)}): ${outside.join(", ")}. \`check\` only scans inside the project; run it from that project, or pass \`--dir\`.`; + if (args.json) { + writeJsonError("INVALID_INPUT", message); + } else { + console.error(`Error: ${message}`); + } + process.exitCode = 1; + return; + } + + const pathNotices = + missing.length > 0 ? [missingPathsNotice(missing)] : []; // If the user passed paths but none exist (e.g. all-deleted diff), - // exit cleanly with empty results rather than falling back to a full scan. + // exit cleanly with empty results rather than falling back to a full + // scan, saying which paths were skipped. if (hadExplicitPaths && existingPaths.length === 0) { if (args.json) { console.log( JSON.stringify( - checkOutputSchema.parse({ success: true, results: [] }) + checkOutputSchema.parse({ + success: true, + results: [], + ...(pathNotices.length > 0 ? { notices: pathNotices } : {}), + }) ) ); + } else { + for (const notice of pathNotices) warnNotice(notice); } return; } + for (const notice of pathNotices) warnNotice(notice); // REFUSES rather than migrates. This used to call // `ensureTasklessDirectory`, so a command whose entire job is to report @@ -330,6 +405,7 @@ export const checkCommand = defineCommand({ checkOutputSchema.parse({ success: true, results: [], + ...(pathNotices.length > 0 ? { notices: pathNotices } : {}), }) ) ); @@ -410,7 +486,11 @@ export const checkCommand = defineCommand({ const results = dispatched.results; for (const notice of dispatched.notices) warnNotice(notice); - const runNotices = [...plan.notices, ...dispatched.notices]; + const runNotices = [ + ...pathNotices, + ...plan.notices, + ...dispatched.notices, + ]; for (const failure of dispatched.failures) warn(`Error: ${failure}`); let errorCount = 0; diff --git a/packages/cli/test/check.test.ts b/packages/cli/test/check.test.ts index 84c48f7c..f8124f17 100644 --- a/packages/cli/test/check.test.ts +++ b/packages/cli/test/check.test.ts @@ -1,6 +1,14 @@ import { execFile } from "node:child_process"; -import { mkdtemp, rm, mkdir, writeFile, cp, stat } from "node:fs/promises"; -import { resolve, join } from "node:path"; +import { + mkdtemp, + rm, + mkdir, + writeFile, + cp, + stat, + realpath, +} from "node:fs/promises"; +import { resolve, join, relative } from "node:path"; import { tmpdir } from "node:os"; import { promisify } from "node:util"; import { describe, expect, it, beforeEach, afterEach } from "vitest"; @@ -455,7 +463,7 @@ describe("check", () => { } }); - it("silently filters paths that do not exist", async () => { + it("skips paths that do not exist, and names them in a notice", async () => { await cp(fixturesDirectory, temporaryDirectory, { recursive: true }); const { stdout, exitCode } = await runCli([ @@ -471,11 +479,15 @@ describe("check", () => { expect(exitCode).toBe(1); const parsed = JSON.parse(stdout.trim()) as { results: Array<{ file: string }>; + notices?: string[]; }; expect(parsed.results.length).toBeGreaterThan(0); for (const result of parsed.results) { expect(result.file).toContain("sample.js"); } + expect(parsed.notices).toContainEqual( + expect.stringContaining("deleted-in-diff.js") + ); }); it("exits 0 with empty results when every passed path is missing", async () => { @@ -494,9 +506,127 @@ describe("check", () => { const parsed = JSON.parse(stdout.trim()) as { success: boolean; results: unknown[]; + notices?: string[]; }; expect(parsed.success).toBe(true); expect(parsed.results).toEqual([]); + // Clean, but not silent: "nothing to scan" must read differently from + // "scanned and found nothing" (#475). + expect(parsed.notices).toEqual([ + expect.stringMatching(/missing-a\.js.*missing-b\.js/), + ]); + }); + + it("names skipped missing paths on stderr in text mode", async () => { + await cp(fixturesDirectory, temporaryDirectory, { recursive: true }); + + const { stderr, exitCode } = await runCli([ + "check", + "-d", + temporaryDirectory, + "missing-a.js", + ]); + + expect(exitCode).toBe(0); + expect(stderr).toContain("missing-a.js"); + }); + + describe("refuses a path outside the project", () => { + let outsideDirectory: string; + + beforeEach(async () => { + await cp(fixturesDirectory, temporaryDirectory, { recursive: true }); + outsideDirectory = await mkdtemp(join(tmpdir(), "taskless-outside-")); + await writeFile(join(outsideDirectory, "bug.js"), 'eval("outside");\n'); + }); + + afterEach(async () => { + await rm(outsideDirectory, { recursive: true, force: true }); + }); + + const cases: Array<[string, () => string]> = [ + ["an absolute file", () => join(outsideDirectory, "bug.js")], + ["an absolute directory", () => outsideDirectory], + [ + "a relative file", + () => relative(temporaryDirectory, join(outsideDirectory, "bug.js")), + ], + ["an absolute path that does not exist", () => "/no/such/file.js"], + ]; + + for (const [label, pathOf] of cases) { + it(`exits 1 with INVALID_INPUT for ${label}`, async () => { + const outsidePath = pathOf(); + const { stdout, exitCode } = await runCli([ + "check", + "-d", + temporaryDirectory, + "--json", + outsidePath, + ]); + + expect(exitCode).toBe(1); + const parsed = JSON.parse(stdout.trim()) as { + ok: boolean; + code: string; + message: string; + }; + expect(parsed.ok).toBe(false); + expect(parsed.code).toBe("INVALID_INPUT"); + expect(parsed.message).toContain(outsidePath); + }); + } + + it("refuses even when another path is inside", async () => { + const outsidePath = join(outsideDirectory, "bug.js"); + const { stdout, exitCode } = await runCli([ + "check", + "-d", + temporaryDirectory, + "--json", + "sample.js", + outsidePath, + ]); + + expect(exitCode).toBe(1); + const parsed = JSON.parse(stdout.trim()) as { code: string }; + expect(parsed.code).toBe("INVALID_INPUT"); + }); + + it("prints the error on stderr in text mode", async () => { + const outsidePath = join(outsideDirectory, "bug.js"); + const { stderr, exitCode } = await runCli([ + "check", + "-d", + temporaryDirectory, + outsidePath, + ]); + + expect(exitCode).toBe(1); + expect(stderr).toContain(`Error: Path is outside the project`); + expect(stderr).toContain(outsidePath); + }); + }); + + it("accepts an absolute path spelled through the project's real path", async () => { + // On macOS tmpdir() is under /var, a symlink to /private/var. A hook + // that hands over real paths must not read as escaping the project. + await cp(fixturesDirectory, temporaryDirectory, { recursive: true }); + const realSample = join(await realpath(temporaryDirectory), "sample.js"); + + const { stdout, exitCode } = await runCli([ + "check", + "-d", + temporaryDirectory, + "--json", + realSample, + ]); + + expect(exitCode).toBe(1); + const parsed = JSON.parse(stdout.trim()) as { + results: Array<{ file: string }>; + }; + expect(parsed.results.length).toBeGreaterThan(0); }); it("zero paths keeps full-project scan behavior", async () => { From e1dfdb788ccb424258c5b5a6460439527bbe0fd9 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 12 Oct 2026 15:10:11 -0700 Subject: [PATCH 2/2] fix(check): compare a path's real spelling too, so a symlinked path is not read as outside Only cwd was resolved to its real path. On macOS process.cwd() already returns the real form, so a path typed through /tmp still read as outside the project. The path's nearest existing ancestor is now resolved as well, which keeps a missing path comparable and judges an in-project symlink by where it sits. --- packages/cli/src/commands/check.ts | 45 ++++++++++++++++++++++++++---- packages/cli/test/check.test.ts | 21 ++++++++++++++ 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index ee91c247..7928322d 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -1,4 +1,11 @@ -import { resolve, isAbsolute, relative } from "node:path"; +import { + basename, + dirname, + isAbsolute, + join, + relative, + resolve, +} from "node:path"; import { realpath, stat } from "node:fs/promises"; import process from "node:process"; import { defineCommand } from "citty"; @@ -41,6 +48,30 @@ function relativeInside(root: string, target: string): string | undefined { return escapes ? undefined : relativePath; } +/** + * `absolutePath` with its nearest existing ancestor directory replaced by that + * directory's real path. + * + * The ancestor rather than the path itself, for two reasons: a missing path + * (a file deleted in the diff, even its whole directory) still has a real + * spelling to compare, and a symlink inside the project pointing out of it is + * judged by where it sits, not by where it leads, exactly as it was before. + */ +async function realParentSpelling(absolutePath: string): Promise { + let ancestor = dirname(absolutePath); + const rest = [basename(absolutePath)]; + for (;;) { + try { + return join(await realpath(ancestor), ...rest); + } catch { + const parent = dirname(ancestor); + if (parent === ancestor) return absolutePath; + rest.unshift(basename(ancestor)); + ancestor = parent; + } + } +} + interface ClassifiedPaths { /** In the project and on disk, relative to cwd so `sg scan` can use them. */ kept: string[]; @@ -60,9 +91,10 @@ interface ClassifiedPaths { * * Containment is decided from the path as written, before asking whether it * exists, so `/no/such/file.py` is reported as outside rather than as missing. - * It is checked against both cwd as given and its real path: on macOS `/tmp` - * is `/private/tmp`, and a hook handing over absolute paths in one spelling - * while cwd is in the other must not read as escaping the project. + * Both sides are compared as written AND through their real paths: on macOS + * `/tmp` is `/private/tmp`, and `process.cwd()` returns the real spelling, so + * a path handed over in either spelling must not read as escaping the project + * while cwd is in the other. */ async function classifyPaths( cwd: string, @@ -82,8 +114,11 @@ async function classifyPaths( const absolutePath = isAbsolute(rawPath) ? resolve(rawPath) : resolve(resolvedCwd, rawPath); + const spellings = [absolutePath, await realParentSpelling(absolutePath)]; const relativePath = roots - .map((root) => relativeInside(root, absolutePath)) + .flatMap((root) => + spellings.map((spelling) => relativeInside(root, spelling)) + ) .find((candidate) => candidate !== undefined); if (relativePath === undefined) { classified.outside.push(rawPath); diff --git a/packages/cli/test/check.test.ts b/packages/cli/test/check.test.ts index f8124f17..73cf6014 100644 --- a/packages/cli/test/check.test.ts +++ b/packages/cli/test/check.test.ts @@ -608,6 +608,27 @@ describe("check", () => { }); }); + it("accepts an absolute path spelled through a symlink when cwd is the real path", async () => { + // The reverse of the case below, and the common one: process.cwd() + // returns the real path, while a caller may type the symlinked form. + await cp(fixturesDirectory, temporaryDirectory, { recursive: true }); + const realDirectory = await realpath(temporaryDirectory); + + const { stdout, exitCode } = await runCli([ + "check", + "-d", + realDirectory, + "--json", + join(temporaryDirectory, "sample.js"), + ]); + + expect(exitCode).toBe(1); + const parsed = JSON.parse(stdout.trim()) as { + results: Array<{ file: string }>; + }; + expect(parsed.results.length).toBeGreaterThan(0); + }); + it("accepts an absolute path spelled through the project's real path", async () => { // On macOS tmpdir() is under /var, a symlink to /private/var. A hook // that hands over real paths must not read as escaping the project.