Skip to content

refactor(copilot-rule): reuse toPosixPath and switch sameRelativePath to object params #1644

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dyoshikawa merged 2 commits into from
May 20, 2026
Merged

refactor(copilot-rule): reuse toPosixPath and switch sameRelativePath to object params #1644

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c93a22
refactor(copilot-rule): reuse toPosixPath and switch sameRelativePath…
Chen17-sq May 15, 2026
69cde2f
review-fix(copilot-rule): tighten comment + test names + add fromFile…
Chen17-sq May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/features/rules/copilot-rule.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,28 @@ description: "Test trimming"
});
});

describe("forDeletion", () => {

@dyoshikawa dyoshikawa May 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that forDeletion now has coverage for the backslash path. A symmetric test for fromFile with relativeDirPath: ".github\\instructions" would close the other half of the issue's coverage ask, since fromFile also routes through sameRelativePath. Not blocking — the helper is shared — but adding it would make the safety net explicit at both call sites.

it("should mark root deletion target when separators are mixed", () => {

@dyoshikawa dyoshikawa May 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small naming nit: both new cases say "separators are mixed" / "mixed separators", but neither input actually mixes / and \ in the same string — they use backslashes only (trailing here, mid-path below). Maybe rename to something like "should mark root deletion target when the dir uses a trailing backslash" and "should treat non-root paths with backslash separators as non-root"? Or, if you'd like to keep the phrasing, add one truly mixed case like relativeDirPath: ".github\\instructions/sub" — that matches what #1603 explicitly asked for.

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({
Expand Down
30 changes: 16 additions & 14 deletions src/features/rules/copilot-rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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

@dyoshikawa dyoshikawa May 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this references node:path/posix.join, but the actual import at the top of the file is node:path (import { join } from "node:path"). On POSIX, node:path.join(".github\\", "x.md") keeps the backslash literal, which is what the slash-collapse handles. The PR description has the accurate phrasing — consider updating the comment to say node:path.join on POSIX so future readers do not get confused.

@dyoshikawa dyoshikawa May 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment refers to node:path/posix.join, but the import at the top of this file is node:path (the platform-aware variant), not node:path/posix. The behavior it describes — node:path.join(".github\\", "x.md") leaving the backslash literal — happens on POSIX where \ isn't a separator, and that's actually what we rely on here. Also the example output mentions .github//instructions/x.md, but the join at the call sites in this file produces .github//copilot-instructions.md for the root case. Mind tightening the comment so it matches the real import and the real string under test? Otherwise the next reader may go hunting for a path/posix import that doesn't exist.

// 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, "/");

@dyoshikawa dyoshikawa May 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: p works for avoiding the node:path shadow, but relativePath would communicate intent a bit better. Feel free to ignore — toPosixPath in utils/file.ts already uses p, so this is at least consistent with the existing helper.


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
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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({
Expand Down
Loading