Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
60 changes: 58 additions & 2 deletions src/features/rules/copilot-rule.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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);
Expand Down Expand Up @@ -796,6 +830,28 @@ description: "Test trimming"
});
});

describe("forDeletion", () => {

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 relativeDirPath has a trailing backslash", () => {
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 when relativeDirPath has a mid-path backslash", () => {
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
34 changes: 20 additions & 14 deletions src/features/rules/copilot-rule.ts
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,22 @@ 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
// 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, "/");

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 +227,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 +290,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