From 7812e1182b4eb019f5f654e8c4dfc5e88ecbe3b9 Mon Sep 17 00:00:00 2001 From: Yashasvi Date: Sat, 19 Sep 2026 12:11:44 +0530 Subject: [PATCH] fix(drift): resolve MISSING_PATH false positives from #143 Three defects reported in #143 still reproduced on 0.8.x. Each one drops a claim that was never a path, rather than demoting the issue it produced, so a healthy scaffold stops losing points instead of losing them more slowly. - Negation was heading-scoped only. It is now also scoped to the sentence that contains a reference. The sentence, not the line, is the unit that governs: markdown wraps prose freely, so a cue and the reference it applies to routinely sit on different lines of one bullet. Scoping to the whole paragraph was tried first and proved too coarse -- on this repository it silenced four real directories because an unrelated clause elsewhere in the same block mentioned a removal. - A bare trailing directory was excluded from the unrooted-reference guard by its own separator. The separator now marks a directory reference instead of disqualifying the value, so `screenshots/` is prose when nothing by that name exists while `.mex/local/` still roots at a real directory and remains a claim the checker tests. - The fallback filename globs lacked `dot: true`, so a file documented by its bare name was never found inside a hidden directory such as `.github/workflows/`. The numeric-delta, `overall/overall` and glob cases from the same report already pass through the unrooted-reference guard added since, and needed no change. All are covered by tests so they stay fixed. Adds the first tests under src/drift, which had no coverage across its 19 source files. A version-shaped reference such as `release/2.1.0` is still reported, because a trailing version number reads as a file extension to the same guard; that case is covered by a skipped test and tracked separately. --- .../__tests__/path-false-positives.test.ts | 109 ++++++++++++++++++ src/drift/checkers/path.ts | 27 +++-- src/drift/claims.ts | 70 ++++++++++- src/markdown.ts | 20 ++++ 4 files changed, 216 insertions(+), 10 deletions(-) create mode 100644 src/drift/__tests__/path-false-positives.test.ts diff --git a/src/drift/__tests__/path-false-positives.test.ts b/src/drift/__tests__/path-false-positives.test.ts new file mode 100644 index 00000000..6dd8713d --- /dev/null +++ b/src/drift/__tests__/path-false-positives.test.ts @@ -0,0 +1,109 @@ +import { describe, it, expect } from "vitest"; +import { execFileSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { extractClaims } from "../claims.js"; +import { checkPaths } from "../checkers/path.js"; + +/** + * Cases from issue #143 (MISSING_PATH false positives) plus the + * version-shaped-ref case found on 0.8.x. Each scaffold is its own git + * repository so `git check-ignore` resolves against the fixture rather than + * whatever repository the temp directory happens to sit inside. + */ +function runScaffold(markdown: string, extraFiles: Record = {}) { + const projectRoot = mkdtempSync(join(tmpdir(), "mex-drift-")); + execFileSync("git", ["init", "-q"], { cwd: projectRoot }); + + const scaffoldRoot = join(projectRoot, ".mex"); + mkdirSync(scaffoldRoot, { recursive: true }); + + for (const [relative, contents] of Object.entries(extraFiles)) { + const target = join(projectRoot, relative); + mkdirSync(join(target, ".."), { recursive: true }); + writeFileSync(target, contents); + } + + const docPath = join(scaffoldRoot, "ROUTER.md"); + writeFileSync(docPath, markdown); + + const claims = extractClaims(docPath, ".mex/ROUTER.md"); + return checkPaths(claims, projectRoot, scaffoldRoot); +} + +const missingPaths = (markdown: string, extraFiles?: Record) => + runScaffold(markdown, extraFiles) + .filter((issue) => issue.code === "MISSING_PATH") + .map((issue) => issue.claim?.value); + +describe("MISSING_PATH false positives", () => { + it("does not claim a file the document says was deleted (#143 defect 1)", () => { + expect( + missingPaths("# Changelog\n\n- Deleted orphaned files: `check_commands.js`\n") + ).toEqual([]); + }); + + it("does not claim a bare trailing directory (#143 defect 2)", () => { + expect(missingPaths("# Layout\n\n- Screenshots live in `screenshots/`\n")).toEqual([]); + }); + + it("does not claim a numeric delta (#143 defect 2)", () => { + expect(missingPaths("# Rating\n\n- Net movement was `+12/-5` this week\n")).toEqual([]); + }); + + it("finds a file inside a dot-directory (#143 defect 3)", () => { + expect( + missingPaths("# CI\n\n- The workflow is `deploy.yml`\n", { + ".github/workflows/deploy.yml": "name: deploy\n", + }) + ).toEqual([]); + }); + + // Not part of #143. A trailing version number reads as a file extension to + // the guard above, so `release/2.1.0` never reaches the unrooted check and is + // reported as a missing path. Tracked separately; unskip with the fix. + it.skip("does not claim a version-shaped branch name", () => { + expect( + missingPaths("# Release\n\n- Work landed on `release/2.1.0` and `python/3.11`\n") + ).toEqual([]); + }); + + it("does not claim a pseudo-path pair such as overall/overall", () => { + expect(missingPaths("# Stats\n\n- Rating shown as `overall/overall`\n")).toEqual([]); + }); + + it("does not claim middot-separated pseudo-paths", () => { + expect(missingPaths("# Modes\n\n- Queues: `8ball/pro` · `9ball/pro`\n")).toEqual([]); + }); + + it("does not claim a glob", () => { + expect( + missingPaths("# Routes\n\n- Admin routes live in `web/routes/admin/*.js`\n") + ).toEqual([]); + }); + + // Markdown wraps prose freely, so negation has to be scoped to the enclosing + // paragraph. Scoping it to the raw line missed both of these. + it("honours negation when the reference is on a continuation line", () => { + expect( + missingPaths( + "# Cleanup\n\n- Deleted orphaned files during the sweep:\n `check_commands.js`\n" + ) + ).toEqual([]); + }); + + it("honours negation across a wrapped sentence", () => { + expect( + missingPaths("# Cleanup\n\n- The helper was removed, so\n `legacy_client.py` is gone\n") + ).toEqual([]); + }); + + it("still checks a directory reference rooted at a directory that exists", () => { + // The trailing-separator change must not turn every documented directory + // into prose: `.mex/local/` roots at a real directory and stays a claim. + expect(missingPaths("# State\n\n- Local state lives in `.mex/local/`\n")).toEqual([ + ".mex/local/", + ]); + }); +}); diff --git a/src/drift/checkers/path.ts b/src/drift/checkers/path.ts index 9112be33..1d862daf 100644 --- a/src/drift/checkers/path.ts +++ b/src/drift/checkers/path.ts @@ -75,20 +75,28 @@ export function checkPaths( } /** - * True when a slash-separated value names no file type, does not end in a - * directory separator, and its first segment does not exist at either root. - * API routes and placeholders take this shape; a real relative path almost - * always starts from a directory that is actually there. + * True when a value names no file type and its first segment does not exist at + * either root. API routes and placeholders take this shape; a real relative + * path almost always starts from a directory that is actually there. + * + * A trailing separator marks a directory reference rather than disqualifying + * the value. `screenshots/` in a layout note is prose when nothing by that name + * is there, while `.mex/local/` still roots at a directory that exists and so + * remains a claim the checker is entitled to test. */ function isUnrootedReference( value: string, projectRoot: string, scaffoldRoot: string ): boolean { - if (!value.includes("/") || value.startsWith("/") || value.endsWith("/")) return false; - if (/\.[A-Za-z0-9]+$/.test(value)) return false; + if (value.startsWith("/")) return false; - const first = value.split("/")[0]; + const trimmed = value.replace(/\/+$/, ""); + const isDirectoryRef = trimmed !== value; + if (!trimmed.includes("/") && !isDirectoryRef) return false; + if (/\.[A-Za-z0-9]+$/.test(trimmed)) return false; + + const first = trimmed.split("/")[0]; if (!first || first.startsWith("@") || first === "." || first === "..") return false; if (existsSync(resolve(projectRoot, first))) return false; @@ -234,10 +242,13 @@ function pathExists( // Bare filenames: search recursively — the file may exist in a subdirectory if (!value.includes("/")) { + // `dot: true` so a file that lives in a hidden directory is found: + // a backticked `deploy.yml` normally sits in `.github/workflows/`. const matches = globSync(`**/${value}`, { cwd: projectRoot, ignore: ["node_modules/**", ".mex/**", "dist/**", ".git/**"], maxDepth: 5, + dot: true, }); if (matches.length > 0) return true; @@ -249,6 +260,7 @@ function pathExists( cwd: scaffoldRoot, ignore: ["node_modules/**"], maxDepth: 5, + dot: true, }); if (inScaffold.length > 0) return true; } @@ -265,6 +277,7 @@ function pathExists( cwd: projectRoot, ignore: ["node_modules/**", "dist/**", ".git/**"], maxDepth: 6, + dot: true, }); if (matches.length > 0) return true; } diff --git a/src/drift/claims.ts b/src/drift/claims.ts index 8ad2312e..f63b064a 100644 --- a/src/drift/claims.ts +++ b/src/drift/claims.ts @@ -1,8 +1,13 @@ import { readFileSync } from "node:fs"; import { visit } from "unist-util-visit"; -import { parseMarkdown, getHeadingAtLine, isNegatedSection } from "../markdown.js"; +import { + parseMarkdown, + getHeadingAtLine, + isNegatedSection, + isNegatedText, +} from "../markdown.js"; import type { Claim } from "../types.js"; -import type { Root, Code, InlineCode, ListItem, Strong, Text } from "mdast"; +import type { Root, Code, Content, InlineCode, ListItem, Strong, Text } from "mdast"; const KNOWN_EXTENSIONS = /\.(ts|js|tsx|jsx|py|go|rs|rb|java|json|yaml|yml|toml|md|css|scss|html|vue|svelte|sh)$/; const COMMAND_PREFIXES = /^(npm|yarn|pnpm|bun|make|cargo|python|pip|go|node|npx|tsx)\s/; @@ -95,6 +100,14 @@ export function extractClaims(filePath: string, source: string): Claim[] { const tree = parseMarkdown(content); const claims: Claim[] = []; + // Negation is not always heading-scoped: a bullet that records a deletion + // sits under an ordinary heading. Nor is a raw line the right unit, because + // markdown wraps prose freely and the word and the reference it governs + // routinely land on different lines. The sentence is the unit that actually + // governs: a paragraph describing an unrelated removal elsewhere must not + // silence every path it happens to mention. + const negatedByContext = collectNegatedReferences(tree); + // A stack doc declares a dependency as `- **name** — description`, so only // bold that opens a list item is a declaration. Collected up front because // both the inline-code pass and the bold pass need to know which nodes are @@ -121,7 +134,7 @@ export function extractClaims(filePath: string, source: string): Claim[] { visit(tree, "inlineCode", (node: InlineCode) => { const line = node.position?.start.line ?? 0; const heading = getHeadingAtLine(tree, line); - const negated = isNegatedSection(heading); + const negated = isNegatedSection(heading) || negatedByContext.has(node); // A package named inside a dependency entry is not a file. `youtubei.js` // ends in a known extension, so without this it was reported as a @@ -249,6 +262,57 @@ export function extractClaims(filePath: string, source: string): Claim[] { return claims; } +/** + * Inline-code nodes whose own sentence describes them as deleted or absent. + * + * The paragraph text is rebuilt in reading order while recording where each + * reference sits inside it, so a cue can be attributed to the sentence that + * contains it. A sentence ends at `.`, `!` or `?` followed by whitespace, + * which leaves version numbers such as `0.8.1` intact and keeps a colon inside + * its sentence -- "Deleted orphaned files: `check_commands.js`" is one clause. + */ +function collectNegatedReferences(tree: Root): Set { + const negated = new Set(); + + visit(tree, "paragraph", (paragraph) => { + let text = ""; + const positions: Array<{ node: InlineCode; at: number }> = []; + + const walk = (node: Content): void => { + if (node.type === "inlineCode") { + positions.push({ node, at: text.length }); + text += node.value; + return; + } + if ("value" in node && typeof node.value === "string") { + text += node.value; + return; + } + if ("children" in node) (node.children as Content[]).forEach(walk); + }; + (paragraph.children as Content[]).forEach(walk); + if (positions.length === 0) return; + + const spans: Array<[number, number]> = []; + let start = 0; + const boundary = /[.!?](?=\s|$)/g; + let match: RegExpExecArray | null; + while ((match = boundary.exec(text)) !== null) { + spans.push([start, match.index + 1]); + start = match.index + 1; + } + spans.push([start, text.length]); + + for (const { node, at } of positions) { + const span = spans.find(([from, to]) => at >= from && at < to); + const sentence = span ? text.slice(span[0], span[1]) : text; + if (isNegatedText(sentence)) negated.add(node); + } + }); + + return negated; +} + function getStrongText(node: Strong): string | null { const text = node.children .filter((c): c is Text => c.type === "text") diff --git a/src/markdown.ts b/src/markdown.ts index cfbf91dd..684ccf20 100644 --- a/src/markdown.ts +++ b/src/markdown.ts @@ -179,6 +179,26 @@ export function getTextContent(node: Content | Root): string { return ""; } +/** + * Words that mark surrounding prose as describing something that is gone or + * deliberately absent. A heading is not always the right scope: a changelog + * bullet naming a deleted file sits under an ordinary heading yet still refers + * to something that should not exist on disk. + */ +const NEGATED_TEXT = + /\b(?:deleted|removed|dropped|retired|orphaned|unreferenced|absent|no longer|does not exist|never created)\b/i; + +/** + * True when this passage describes a path as deleted or deliberately absent. + * Callers pass a whole block rather than one line: markdown wraps prose freely, + * so the word and the reference it governs routinely sit on different lines of + * the same sentence. + */ +export function isNegatedText(text: string | undefined): boolean { + if (!text) return false; + return NEGATED_TEXT.test(text); +} + /** Check if a heading or its ancestors suggest negation */ export function isNegatedSection(heading: string | null): boolean { if (!heading) return false;