From 5c93a22927fd7c62fcb8b6931152e596201b3ef4 Mon Sep 17 00:00:00 2001 From: Aldrich_CC <109075336+Chen17-sq@users.noreply.github.com> Date: Sat, 16 May 2026 04:57:49 +0800 Subject: [PATCH 1/2] refactor(copilot-rule): reuse toPosixPath and switch sameRelativePath to object params Addresses the review findings tracked in #1603: - Replace the local backslash regex with the shared toPosixPath utility, keeping the consecutive-slash collapse and explaining the WSL edge case it covers - Rename the parameter from path to p to avoid shadowing the node:path import - Move sameRelativePath from four positional strings to {dir, file} objects per the coding guidelines and update both call sites - Add forDeletion tests that pin down root detection with a trailing backslash separator and confirm non-root paths with mid-path backslashes stay non-root Refs #1603. --- src/features/rules/copilot-rule.test.ts | 22 ++++++++++++++++++ src/features/rules/copilot-rule.ts | 30 +++++++++++++------------ 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/features/rules/copilot-rule.test.ts b/src/features/rules/copilot-rule.test.ts index 29445309d..ea1769dd6 100644 --- a/src/features/rules/copilot-rule.test.ts +++ b/src/features/rules/copilot-rule.test.ts @@ -796,6 +796,28 @@ description: "Test trimming" }); }); + describe("forDeletion", () => { + it("should mark root deletion target when separators are mixed", () => { + const copilotRule = CopilotRule.forDeletion({ + outputRoot: testDir, + relativeDirPath: ".github\\", + relativeFilePath: "copilot-instructions.md", + }); + + expect(copilotRule.isRoot()).toBe(true); + }); + + it("should treat non-root paths as non-root even with mixed separators", () => { + const copilotRule = CopilotRule.forDeletion({ + outputRoot: testDir, + relativeDirPath: ".github\\instructions", + relativeFilePath: "feature.instructions.md", + }); + + expect(copilotRule.isRoot()).toBe(false); + }); + }); + describe("validate", () => { it("should return success for valid frontmatter", () => { const copilotRule = new CopilotRule({ diff --git a/src/features/rules/copilot-rule.ts b/src/features/rules/copilot-rule.ts index eadbb2401..a687387a8 100644 --- a/src/features/rules/copilot-rule.ts +++ b/src/features/rules/copilot-rule.ts @@ -5,7 +5,7 @@ import { z } from "zod/mini"; import { RULESYNC_RULES_RELATIVE_DIR_PATH } from "../../constants/rulesync-paths.js"; import { ValidationResult } from "../../types/ai-file.js"; import { formatError } from "../../utils/error.js"; -import { readFileContent } from "../../utils/file.js"; +import { readFileContent, toPosixPath } from "../../utils/file.js"; import { parseFrontmatter, stringifyFrontmatter } from "../../utils/frontmatter.js"; import { RulesyncRule, RulesyncRuleFrontmatter } from "./rulesync-rule.js"; import { @@ -48,12 +48,18 @@ export type CopilotRuleSettablePathsGlobal = ToolRuleSettablePathsGlobal & { }; }; -const normalizeRelativePath = (path: string): string => - path.replace(/\\/g, "/").replace(/\/+/g, "/"); +// toPosixPath converts backslashes to forward slashes so paths compare equally +// on Windows and POSIX. The extra slash collapse covers a WSL/mixed-separator +// edge case where `node:path/posix.join` keeps a literal backslash inside an +// input segment (e.g. ".github\\") and produces ".github//instructions/x.md" +// after the backslash is rewritten. +const normalizeRelativePath = (p: string): string => toPosixPath(p).replace(/\/+/g, "/"); -const sameRelativePath = (leftDir: string, leftFile: string, rightDir: string, rightFile: string) => - normalizeRelativePath(join(leftDir, leftFile)) === - normalizeRelativePath(join(rightDir, rightFile)); +type RelativePathParts = { dir: string; file: string }; + +const sameRelativePath = (left: RelativePathParts, right: RelativePathParts): boolean => + normalizeRelativePath(join(left.dir, left.file)) === + normalizeRelativePath(join(right.dir, right.file)); /** * Rule generator for GitHub Copilot @@ -217,10 +223,8 @@ export class CopilotRule extends ToolRule { const paths = this.getSettablePaths({ global }); const isRoot = relativeDirPath ? sameRelativePath( - relativeDirPath, - relativeFilePath, - paths.root.relativeDirPath, - paths.root.relativeFilePath, + { dir: relativeDirPath, file: relativeFilePath }, + { dir: paths.root.relativeDirPath, file: paths.root.relativeFilePath }, ) : relativeFilePath === paths.root.relativeFilePath; const resolvedRelativeDirPath = @@ -282,10 +286,8 @@ export class CopilotRule extends ToolRule { }: ToolRuleForDeletionParams): CopilotRule { const paths = this.getSettablePaths({ global }); const isRoot = sameRelativePath( - relativeDirPath, - relativeFilePath, - paths.root.relativeDirPath, - paths.root.relativeFilePath, + { dir: relativeDirPath, file: relativeFilePath }, + { dir: paths.root.relativeDirPath, file: paths.root.relativeFilePath }, ); return new CopilotRule({ From 69cde2fb1556be561f59ab9da486c80a69a9f6f5 Mon Sep 17 00:00:00 2001 From: Aldrich_CC <109075336+Chen17-sq@users.noreply.github.com> Date: Wed, 20 May 2026 03:06:45 +0800 Subject: [PATCH 2/2] review-fix(copilot-rule): tighten comment + test names + add fromFile mid-path test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses dyoshikawa's review on #1644: - Comment block on normalizeRelativePath previously said 'node:path/posix.join' and showed a multi-segment example, but the file imports node:path and sameRelativePath joins dir + single filename. Rewrote the comment so the module reference matches the import line and the example matches the actual call shape ('.github\\' + 'copilot-instructions.md'). - forDeletion tests renamed: 'when separators are mixed' → 'when relativeDirPath has a trailing backslash' / 'when relativeDirPath has a mid-path backslash'. They never actually mixed / and \\; the new labels describe what's really exercised. - Same rename applied to the existing fromFile 'should normalize separators…' test. - Added the optional symmetric fromFile mid-path backslash test the review flagged as closing the coverage gap #1603 originally identified. Materializes the fixture directory at the literal '.github\\instructions' name since fromFile reads from the raw (un-normalized) relativeDirPath after the root check. --- src/features/rules/copilot-rule.test.ts | 42 ++++++++++++++++++++++--- src/features/rules/copilot-rule.ts | 14 ++++++--- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/features/rules/copilot-rule.test.ts b/src/features/rules/copilot-rule.test.ts index ea1769dd6..343329f7e 100644 --- a/src/features/rules/copilot-rule.test.ts +++ b/src/features/rules/copilot-rule.test.ts @@ -620,11 +620,11 @@ This should be treated as a non-root rule.`; expect(copilotRule.getBody()).toBe("This should be treated as a non-root rule."); }); - it("should normalize separators when detecting explicit root paths", async () => { + it("should normalize a trailing-backslash relativeDirPath when detecting explicit root paths", async () => { const githubDir = join(testDir, ".github"); await ensureDir(githubDir); - const rootContent = "Root detected with mixed separators."; + const rootContent = "Root detected with a trailing backslash on relativeDirPath."; await writeFileContent(join(githubDir, "copilot-instructions.md"), rootContent); const copilotRule = await CopilotRule.fromFile({ @@ -638,6 +638,40 @@ This should be treated as a non-root rule.`; expect(copilotRule.getBody()).toBe(rootContent); }); + it("should treat non-root paths as non-root when relativeDirPath has a mid-path backslash", async () => { + // Symmetric coverage to the forDeletion equivalent — exercises the + // sameRelativePath check on the fromFile call site so the original + // coverage gap flagged in #1603 is closed on both call sites. + // + // Note on the fixture path: fromFile reads the file from the *raw* + // relativeDirPath after the root check (only the comparison goes + // through normalizeRelativePath). On POSIX `\` is a regular filename + // character, so we materialize the directory at the literal + // ".github\instructions" name so the read inside fromFile succeeds. + const literalDir = join(testDir, ".github\\instructions"); + await ensureDir(literalDir); + const fileContent = `--- +description: "Mid-path backslash on relativeDirPath" +applyTo: "**/*.ts" +--- + +This should resolve to a non-root rule.`; + await writeFileContent(join(literalDir, "feature.instructions.md"), fileContent); + + const copilotRule = await CopilotRule.fromFile({ + outputRoot: testDir, + // Mid-path backslash (".github\\instructions" → ".github\instructions"). + // sameRelativePath normalizes both sides before comparing against + // paths.root, so the comparison correctly returns false here. + relativeDirPath: ".github\\instructions", + relativeFilePath: "feature.instructions.md", + validate: true, + }); + + expect(copilotRule.isRoot()).toBe(false); + expect(copilotRule.getBody()).toBe("This should resolve to a non-root rule."); + }); + it("should detect root only when both relativeDirPath and filename match", async () => { const githubDir = join(testDir, ".github"); await ensureDir(githubDir); @@ -797,7 +831,7 @@ description: "Test trimming" }); describe("forDeletion", () => { - it("should mark root deletion target when separators are mixed", () => { + it("should mark root deletion target when relativeDirPath has a trailing backslash", () => { const copilotRule = CopilotRule.forDeletion({ outputRoot: testDir, relativeDirPath: ".github\\", @@ -807,7 +841,7 @@ description: "Test trimming" expect(copilotRule.isRoot()).toBe(true); }); - it("should treat non-root paths as non-root even with mixed separators", () => { + it("should treat non-root paths as non-root when relativeDirPath has a mid-path backslash", () => { const copilotRule = CopilotRule.forDeletion({ outputRoot: testDir, relativeDirPath: ".github\\instructions", diff --git a/src/features/rules/copilot-rule.ts b/src/features/rules/copilot-rule.ts index a687387a8..6b53a43f4 100644 --- a/src/features/rules/copilot-rule.ts +++ b/src/features/rules/copilot-rule.ts @@ -48,11 +48,15 @@ export type CopilotRuleSettablePathsGlobal = ToolRuleSettablePathsGlobal & { }; }; -// toPosixPath converts backslashes to forward slashes so paths compare equally -// on Windows and POSIX. The extra slash collapse covers a WSL/mixed-separator -// edge case where `node:path/posix.join` keeps a literal backslash inside an -// input segment (e.g. ".github\\") and produces ".github//instructions/x.md" -// after the backslash is rewritten. +// toPosixPath converts backslashes to forward slashes so paths compare +// equally on Windows and POSIX. The extra slash collapse covers a +// mixed-separator edge case where `node:path.join` on POSIX treats a +// trailing backslash as a literal character: joining `.github\\` (one +// literal backslash) with `copilot-instructions.md` produces +// `.github\\/copilot-instructions.md`, which becomes +// `.github//copilot-instructions.md` after `toPosixPath`. The slash +// collapse below normalizes that back to a single `/` so the result +// compares equal to the canonical `.github/copilot-instructions.md`. const normalizeRelativePath = (p: string): string => toPosixPath(p).replace(/\/+/g, "/"); type RelativePathParts = { dir: string; file: string };