From e531975312399056a38e7eb2752a25eb8ae0a3d6 Mon Sep 17 00:00:00 2001 From: wallstop Date: Fri, 2 Oct 2026 16:17:10 +0000 Subject: [PATCH 1/4] Fix the line-ending drift that broke the CSharpier gate on non-Windows checkouts Issue #129. `.editorconfig` required CRLF for every file while `.gitattributes` set `* text=auto` with no `eol`, so git stored LF and checked out the platform default. CSharpier follows `.editorconfig`, so `csharpier -- check` reported all 110 checked files on Linux and macOS and passed only on Windows, where the checkout is CRLF. The pre-commit hook rewrote staged files to an ending git normalized straight back, so the change never landed. LF is already the stored form of all 112 tracked `*.cs` blobs, so pinning it is a configuration change and rewrites no blob. `eol=lf` overrides `core.autocrlf`, so the checkout ending is LF on every platform and the gate is platform independent. `scripts/lint-line-endings.js` keeps the two configurations from drifting apart again. Every path either file names is resolved on both sides and the endings must match, so a narrowed `.editorconfig` section cannot disagree with a broad `.gitattributes` rule. A `text` rule without `eol`, an `eol` without `text`, a binary rule that pins an ending, a missing base rule, and a missing `[*]` section all fail closed. Wired into `lint:llm:full`, the llm-lint workflow (whose path filters now include both files), and pre-commit. Validation: the guard reports three drift sites against the pre-fix configuration and passes after it. `dotnet tool run csharpier -- check Editor Runtime Tests` reports `Checked 110 files` and exits 0, where it previously exited 1 on every file. A no-op format of a tracked file leaves its blob hash unchanged. `npm run lint:llm:full` green with 19 self-test files and 20 new line-ending cases. --- .editorconfig | 2 +- .gitattributes | 7 +- .github/workflows/llm-lint.yml | 7 + .pre-commit-config.yaml | 7 + package.json | 3 +- scripts/lint-line-endings.js | 352 ++++++++++++++++++ scripts/lint-line-endings.js.meta | 7 + scripts/tests/test-lint-line-endings.ps1 | 271 ++++++++++++++ scripts/tests/test-lint-line-endings.ps1.meta | 7 + 9 files changed, 659 insertions(+), 4 deletions(-) create mode 100644 scripts/lint-line-endings.js create mode 100644 scripts/lint-line-endings.js.meta create mode 100644 scripts/tests/test-lint-line-endings.ps1 create mode 100644 scripts/tests/test-lint-line-endings.ps1.meta diff --git a/.editorconfig b/.editorconfig index 4745e6b..414305d 100644 --- a/.editorconfig +++ b/.editorconfig @@ -1,7 +1,7 @@ [*] charset = utf-8-bom -end_of_line = crlf +end_of_line = lf trim_trailing_whitespace = false insert_final_newline = false indent_style = space diff --git a/.gitattributes b/.gitattributes index c0e37ba..11368ae 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,7 +1,10 @@ ############################################################################### -# Set default behavior to automatically normalize line endings. +# LF is the one canonical line ending: it is what this repository stores and +# what .editorconfig asks editors and CSharpier to write. Pinning eol keeps the +# checkout ending on every platform, so `csharpier -- check` and the pre-commit +# hook agree with git instead of only working on Windows. ############################################################################### -* text=auto +* text=auto eol=lf ############################################################################### # Shell scripts must stay LF so shebangs and bash parsing survive Windows diff --git a/.github/workflows/llm-lint.yml b/.github/workflows/llm-lint.yml index 22674c4..16d602f 100644 --- a/.github/workflows/llm-lint.yml +++ b/.github/workflows/llm-lint.yml @@ -17,6 +17,8 @@ on: - ".github/copilot-instructions.md" - ".github/workflows/llm-lint.yml" - ".pre-commit-config.yaml" + - ".editorconfig" + - ".gitattributes" - ".devcontainer/**" - "Editor/**/*.cs" - "Runtime/**/*.cs" @@ -40,6 +42,8 @@ on: - ".github/copilot-instructions.md" - ".github/workflows/llm-lint.yml" - ".pre-commit-config.yaml" + - ".editorconfig" + - ".gitattributes" - ".devcontainer/**" - "Editor/**/*.cs" - "Runtime/**/*.cs" @@ -88,6 +92,9 @@ jobs: - name: Enforce the release credential contract run: node scripts/lint-release-secrets.js --verbose + - name: Enforce the line-ending contract + run: node scripts/lint-line-endings.js --verbose + - name: Enforce assembly warning policy run: pwsh -NoProfile -File scripts/lint-assembly-warnings.ps1 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 969619b..d49282e 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -48,6 +48,13 @@ repos: pass_filenames: false files: ^(\.llm/references/RELEASING\.md|\.github/workflows/(release-prep|release-tag|npm-publish)\.yml)$ description: Fails when a release workflow reads an undocumented secret, or a documented secret is read by no release workflow. + - id: lint-line-endings + name: Enforce the line-ending contract + entry: node scripts/lint-line-endings.js + language: system + pass_filenames: false + files: ^(\.editorconfig|\.gitattributes)$ + description: Fails when .editorconfig and .gitattributes disagree about the line ending CSharpier writes and git checks out. - id: lint-assembly-warnings name: Enforce assembly warning policy entry: pwsh -NoProfile -File scripts/lint-assembly-warnings.ps1 diff --git a/package.json b/package.json index 9e285da..7096bb8 100644 --- a/package.json +++ b/package.json @@ -47,7 +47,8 @@ "lint:csharp-usings": "node scripts/lint-csharp-usings.js", "lint:csharp-usings:fix": "node scripts/lint-csharp-usings.js --fix", "lint:release-secrets": "node scripts/lint-release-secrets.js", - "lint:llm:full": "pwsh -NoProfile -File scripts/generate-skills-index.ps1 && pwsh -NoProfile -File scripts/lint-llm-instructions.ps1 -VerboseOutput && pwsh -NoProfile -File scripts/lint-file-lengths.ps1 -VerboseOutput && node scripts/lint-csharp-member-order.js --verbose && node scripts/lint-release-secrets.js --verbose && pwsh -NoProfile -File scripts/tests/run-all.ps1", + "lint:line-endings": "node scripts/lint-line-endings.js", + "lint:llm:full": "pwsh -NoProfile -File scripts/generate-skills-index.ps1 && pwsh -NoProfile -File scripts/lint-llm-instructions.ps1 -VerboseOutput && pwsh -NoProfile -File scripts/lint-file-lengths.ps1 -VerboseOutput && node scripts/lint-csharp-member-order.js --verbose && node scripts/lint-release-secrets.js --verbose && node scripts/lint-line-endings.js --verbose && pwsh -NoProfile -File scripts/tests/run-all.ps1", "mcp:sync": "bash .llm/mcp/sync-mcp.sh", "unity:mcp:host": "node .llm/mcp/unity-mcp-host.mjs", "tools:install": "bash .devcontainer/install-npm-tools.sh", diff --git a/scripts/lint-line-endings.js b/scripts/lint-line-endings.js new file mode 100644 index 0000000..7b4c34b --- /dev/null +++ b/scripts/lint-line-endings.js @@ -0,0 +1,352 @@ +#!/usr/bin/env node +/** + * Line-ending contract: the ending CSharpier writes must be the ending git + * checks out, on every platform. + * + * `.editorconfig` and `.gitattributes` each describe the working tree, and they + * disagreed. `.editorconfig` required CRLF for every file, `.gitattributes` set + * `* text=auto` with no `eol`, so git stored LF and checked out the platform + * default. CSharpier follows `.editorconfig`, so `csharpier -- check` passed + * only on Windows and reported every tracked C# file on Linux and macOS, and + * the pre-commit hook rewrote staged files to an ending git normalized straight + * back so the change never landed. No CI job ran CSharpier, so the drift was + * silent. This check fails closed on that class of drift. + * + * Every path either configuration names is resolved on both sides and the two + * endings must be equal, so a narrowed `.editorconfig` section cannot disagree + * with a broad `.gitattributes` rule any more than a broad section can disagree + * with a narrow rule. Resolving both directions needs one representative path + * per `.editorconfig` section that declares an ending and one per + * `.gitattributes` rule. + * + * Resolution follows git's own attribute model: each attribute takes the value + * of the last matching rule that sets it, so a later `text` and an earlier + * `eol` combine. A path git treats as binary (`-text`) needs no ending. A rule + * that sets `eol` without `text` is rejected because git ignores it. + * + * Only the glob subset the two files use is supported: `*`, `?`, and `{a,b}` + * alternation, where a pattern without a slash matches a basename at any depth. + * + * `--verbose` prints the resolved canonical ending and the compared paths. Exit + * codes: 0 = in sync, 1 = drift, a malformed rule, or an unreadable input. + * + * `LINE_ENDINGS_EDITORCONFIG` and `LINE_ENDINGS_GITATTRIBUTES` override the + * inputs so the self-test can point at a fixture tree. Nothing in CI sets them. + */ + +"use strict"; + +const fs = require("fs"); +const path = require("path"); + +const REPO_ROOT = path.resolve(__dirname, ".."); + +const EDITORCONFIG = process.env.LINE_ENDINGS_EDITORCONFIG + ? path.resolve(REPO_ROOT, process.env.LINE_ENDINGS_EDITORCONFIG) + : path.join(REPO_ROOT, ".editorconfig"); +const GITATTRIBUTES = process.env.LINE_ENDINGS_GITATTRIBUTES + ? path.resolve(REPO_ROOT, process.env.LINE_ENDINGS_GITATTRIBUTES) + : path.join(REPO_ROOT, ".gitattributes"); + +const ENDING_NAMES = new Set(["lf", "crlf"]); +const DEFAULT_GLOB = "*"; +const SAMPLE_NAME = "sample"; + +function readFileOrThrow(filePath, purpose) { + try { + return fs.readFileSync(filePath, "utf8"); + } catch { + throw new Error(`Cannot ${purpose}: '${filePath}' is missing or unreadable.`); + } +} + +/* + A repo-relative path when the file lives inside the repository, otherwise the + absolute path, so a message never shows a '../../..' chain. +*/ +function displayPath(filePath) { + const relative = path.relative(REPO_ROOT, filePath).split(path.sep).join("/"); + return relative.startsWith("../") ? filePath : relative; +} + +/* + Splits one `{a,b,c}` glob into its alternatives. Nesting is not used by + either file, so an inner brace stays literal instead of being expanded. +*/ +function expandBraces(glob) { + const open = glob.indexOf("{"); + const close = glob.indexOf("}", open + 1); + if (open < 0 || close < 0) { + return [glob]; + } + const prefix = glob.slice(0, open); + const suffix = glob.slice(close + 1); + return glob + .slice(open + 1, close) + .split(",") + .flatMap((alternative) => expandBraces(prefix + alternative + suffix)); +} + +function compileGlob(glob) { + let source = "^"; + for (const character of glob) { + if (character === "*") { + source += "[^/]*"; + } else if (character === "?") { + source += "[^/]"; + } else { + source += character.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + } + } + return new RegExp(`${source}$`); +} + +function globMatches(glob, filePath) { + for (const alternative of expandBraces(glob)) { + const expression = compileGlob(alternative); + if (expression.test(filePath)) { + return true; + } + if (!alternative.includes("/") && expression.test(path.posix.basename(filePath))) { + return true; + } + } + return false; +} + +/* + A representative path for a glob, so each rule contributes the kind of file + it governs to the comparison: `*.cs` becomes `sample.cs`, a literal path + stays itself, and a bare `*` becomes an extensionless sample. +*/ +function samplePath(glob) { + const widest = expandBraces(glob)[0]; + return widest.includes("*") ? widest.replace(/\*/g, SAMPLE_NAME) : widest; +} + +function normalizeEnding(value, origin) { + const ending = value.trim().toLowerCase(); + if (!ENDING_NAMES.has(ending)) { + throw new Error(`${origin} names the unknown line ending '${value}'; use lf or crlf.`); + } + return ending; +} + +function parseEditorConfig(filePath) { + const sections = []; + let current = null; + const lines = readFileOrThrow(filePath, "read the editor configuration").split("\n"); + for (const [index, raw] of lines.entries()) { + const line = raw.trim(); + if (line.length === 0 || line.startsWith("#") || line.startsWith(";")) { + continue; + } + const lineNumber = index + 1; + if (line.startsWith("[") && line.endsWith("]")) { + current = { glob: line.slice(1, -1).trim(), lineNumber, properties: new Map() }; + sections.push(current); + continue; + } + const separator = line.indexOf("="); + if (separator < 0) { + throw new Error(`${displayPath(filePath)}:${lineNumber} is not a 'key = value' line.`); + } + // A property before the first section header is a preamble such as + // `root = true`; it configures the file rather than any path. + if (current !== null) { + current.properties.set( + line.slice(0, separator).trim().toLowerCase(), + line.slice(separator + 1), + ); + } + } + if (!sections.some((section) => section.glob === DEFAULT_GLOB)) { + throw new Error( + `${displayPath(filePath)} must carry a '[${DEFAULT_GLOB}]' section so every file has a ` + + "declared line ending.", + ); + } + return sections; +} + +/* + The last matching section that sets `end_of_line` wins, which is how + `.editorconfig` resolves precedence. +*/ +function resolveEditorEnding(sections, filePath, origin) { + let resolved = null; + for (const section of sections) { + const value = section.properties.get("end_of_line"); + if (value === undefined || !globMatches(section.glob, filePath)) { + continue; + } + resolved = { + ending: normalizeEnding(value, `${displayPath(origin)}:${section.lineNumber}`), + lineNumber: section.lineNumber, + }; + } + return resolved; +} + +function parseGitAttributes(filePath) { + const rules = []; + const lines = readFileOrThrow(filePath, "read the git attributes").split("\n"); + for (const [index, raw] of lines.entries()) { + const line = raw.trim(); + if (line.length === 0 || line.startsWith("#")) { + continue; + } + const fields = line.split(/\s+/); + const attributes = new Map(); + for (const field of fields.slice(1)) { + const separator = field.indexOf("="); + if (separator < 0) { + // `-text` unsets an attribute and `!text` resets it to git's + // default; neither is a boolean `false` written after '='. + attributes.set(field.replace(/^[-!]/, ""), field.startsWith("-") ? false : field.startsWith("!") ? "unset" : true); + } else { + attributes.set(field.slice(0, separator), field.slice(separator + 1)); + } + } + rules.push({ pattern: fields[0], lineNumber: index + 1, attributes }); + } + return rules; +} + +/* + Git resolves each attribute from the last matching rule that sets it, so the + `text` and `eol` answers can come from different rules. A binary path needs + no ending, which is reported as no ending at all. +*/ +function resolveGitEnding(rules, filePath, origin) { + let text = { value: undefined, lineNumber: 0 }; + let ending = { value: undefined, lineNumber: 0 }; + let matched = { lineNumber: 0 }; + for (const rule of rules) { + if (!globMatches(rule.pattern, filePath)) { + continue; + } + matched = rule; + if (rule.attributes.has("text")) { + text = { value: rule.attributes.get("text"), lineNumber: rule.lineNumber }; + } + if (rule.attributes.has("eol")) { + ending = { value: rule.attributes.get("eol"), lineNumber: rule.lineNumber }; + } + } + if (text.value === false || text.value === "unset") { + return { binary: true, ending: null, lineNumber: matched.lineNumber }; + } + if (ending.value === undefined) { + return { binary: false, ending: null, lineNumber: matched.lineNumber }; + } + return { + binary: false, + ending: normalizeEnding(ending.value, `${displayPath(origin)}:${matched.lineNumber}`), + lineNumber: matched.lineNumber, + }; +} + +function main() { + const verbose = process.argv.includes("--verbose"); + const sections = parseEditorConfig(EDITORCONFIG); + const rules = parseGitAttributes(GITATTRIBUTES); + const editorName = displayPath(EDITORCONFIG); + const gitName = displayPath(GITATTRIBUTES); + const failures = []; + + const baseRule = rules.find((rule) => rule.pattern === DEFAULT_GLOB); + if (baseRule === undefined) { + failures.push( + `'${gitName}' has no '${DEFAULT_GLOB}' rule, so no path has a declared checkout ending.`, + ); + } else if (baseRule.attributes.get("text") === false || baseRule.attributes.get("text") === "unset") { + failures.push(`'${gitName}:${baseRule.lineNumber}' marks every path binary.`); + } + + for (const rule of rules) { + const origin = `${gitName}:${rule.lineNumber}`; + const text = rule.attributes.get("text"); + if (text === false || text === "unset") { + if (rule.attributes.has("eol")) { + failures.push( + `${origin} marks '${rule.pattern}' binary and also sets an ending; drop the ending.`, + ); + } + continue; + } + if (rule.attributes.has("eol") && !rule.attributes.has("text")) { + failures.push( + `${origin} sets an ending for '${rule.pattern}' without 'text'; git ignores it.`, + ); + } + } + + const canonical = resolveEditorEnding(sections, SAMPLE_NAME, EDITORCONFIG); + if (canonical === null) { + throw new Error( + `${editorName} sets no 'end_of_line' for '[${DEFAULT_GLOB}]'; CSharpier and the git ` + + "checkout would each pick their own default.", + ); + } + + const candidates = []; + for (const section of sections) { + if (section.properties.has("end_of_line")) { + candidates.push(samplePath(section.glob)); + } + } + for (const rule of rules) { + candidates.push(samplePath(rule.pattern)); + } + + for (const candidate of [...new Set(candidates)]) { + const editor = resolveEditorEnding(sections, candidate, EDITORCONFIG); + const git = resolveGitEnding(rules, candidate, GITATTRIBUTES); + if (git.binary || editor === null) { + continue; + } + if (git.ending === null) { + failures.push( + `'${candidate}': git leaves the checkout ending to the platform ` + + `('${gitName}:${git.lineNumber}' sets no ending) but ` + + `'${editorName}:${editor.lineNumber}' requires ${editor.ending}.`, + ); + continue; + } + if (git.ending !== editor.ending) { + failures.push( + `'${candidate}': git checks out ${git.ending} ('${gitName}:${git.lineNumber}') but ` + + `'${editorName}:${editor.lineNumber}' requires ${editor.ending}.`, + ); + } + } + + if (failures.length > 0) { + console.error( + `The line-ending contract is broken. CSharpier follows '${editorName}'; git follows ` + + `'${gitName}'.`, + ); + for (const failure of failures) { + console.error(` ${failure}`); + } + process.exit(1); + } + + if (verbose) { + const textRules = rules.filter((rule) => rule.attributes.has("text") || rule.attributes.has("eol")) + .length; + console.log( + `Line-ending contract in sync: ${canonical.ending} across ` + + `${new Set(candidates).size} path(s) and ${textRules} rule(s) in '${gitName}'.`, + ); + } + process.exit(0); +} + +try { + main(); +} catch (error) { + console.error(`error: ${error instanceof Error ? error.message : String(error)}`); + process.exit(1); +} \ No newline at end of file diff --git a/scripts/lint-line-endings.js.meta b/scripts/lint-line-endings.js.meta new file mode 100644 index 0000000..dacc881 --- /dev/null +++ b/scripts/lint-line-endings.js.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: a4084f1274ee68ff794a097f7dcdd1ac +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: \ No newline at end of file diff --git a/scripts/tests/test-lint-line-endings.ps1 b/scripts/tests/test-lint-line-endings.ps1 new file mode 100644 index 0000000..c7e9b71 --- /dev/null +++ b/scripts/tests/test-lint-line-endings.ps1 @@ -0,0 +1,271 @@ +Set-StrictMode -Version 2.0 + +$script:TestFailureCount = 0 +. (Join-Path $PSScriptRoot 'TestHelpers.ps1') + +$lintScript = Join-Path (Split-Path -Parent $PSScriptRoot) 'lint-line-endings.js' +$repoRoot = Split-Path -Parent (Split-Path -Parent $PSScriptRoot) +Write-Host '== Line-ending contract self-tests ==' + +function Invoke-LineEndingLint { + param( + [string]$EditorConfig, + [string]$GitAttributes, + [string[]]$LintArguments = @() + ) + + $hadEditorConfig = Test-Path Env:LINE_ENDINGS_EDITORCONFIG + $previousEditorConfig = $env:LINE_ENDINGS_EDITORCONFIG + $hadGitAttributes = Test-Path Env:LINE_ENDINGS_GITATTRIBUTES + $previousGitAttributes = $env:LINE_ENDINGS_GITATTRIBUTES + try { + $env:LINE_ENDINGS_EDITORCONFIG = $EditorConfig + $env:LINE_ENDINGS_GITATTRIBUTES = $GitAttributes + $output = & node $lintScript @LintArguments 2>&1 | Out-String + return [pscustomobject]@{ ExitCode = $LASTEXITCODE; Output = $output } + } finally { + if ($hadEditorConfig) { + $env:LINE_ENDINGS_EDITORCONFIG = $previousEditorConfig + } else { + Remove-Item Env:LINE_ENDINGS_EDITORCONFIG -ErrorAction SilentlyContinue + } + if ($hadGitAttributes) { + $env:LINE_ENDINGS_GITATTRIBUTES = $previousGitAttributes + } else { + Remove-Item Env:LINE_ENDINGS_GITATTRIBUTES -ErrorAction SilentlyContinue + } + } +} + +# The lint resolves its overrides against the repository root, so fixtures pass +# absolute paths. +function Invoke-FixtureLint { + param([string]$Root, [string[]]$LintArguments = @()) + + return Invoke-LineEndingLint ` + -EditorConfig (Join-Path $Root '.editorconfig') ` + -GitAttributes (Join-Path $Root '.gitattributes') ` + -LintArguments $LintArguments +} + +$editorConfigLf = @' +root = true + +[*] +end_of_line = lf +'@ + +# The configuration that shipped the failure: the editor asks for CRLF for every +# file and git checks out the platform default. +$editorConfigCrlf = @' +root = true + +[*] +end_of_line = crlf +'@ + +$gitAttributesPlatformDefault = @' +* text=auto +'@ + +$gitAttributesLf = @' +* text=auto eol=lf +'@ + +$gitAttributesCrlf = @' +* text=auto eol=crlf +'@ + +# Each row is one configuration pair and the outcome the guard owes it. The +# drifted rows reproduce the shapes this repository has shipped or could ship. +$cases = @( + [pscustomobject]@{ + Name = 'Passes_WhenBothConfigurationsUseLf' + EditorConfig = $editorConfigLf + GitAttributes = $gitAttributesLf + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + Name = 'Passes_WhenBothConfigurationsUseCrlf' + EditorConfig = $editorConfigCrlf + GitAttributes = $gitAttributesCrlf + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheEditorAsksForCrlfAndGitChecksOutLf' + EditorConfig = $editorConfigCrlf + GitAttributes = $gitAttributesLf + ExpectPass = $false + Expect = "git checks out lf.+requires crlf" + } + [pscustomobject]@{ + Name = 'Fails_WhenGitAttributesLeavesTheEndingToThePlatform' + EditorConfig = $editorConfigLf + GitAttributes = $gitAttributesPlatformDefault + ExpectPass = $false + Expect = "git leaves the checkout ending to the platform.+requires lf" + } + [pscustomobject]@{ + # The same defect narrowed to one extension: a guard that only compared + # each rule with the `[*]` section would read this as in sync, because the + # broad rule's sample path carries no extension. + Name = 'Fails_WhenOnlyOneExtensionAsksForCrlf' + EditorConfig = "$editorConfigLf`n`n[*.cs]`nend_of_line = crlf`n" + GitAttributes = $gitAttributesLf + ExpectPass = $false + Expect = "'sample\.cs': git checks out lf.+requires crlf" + } + [pscustomobject]@{ + Name = 'Fails_WhenTheDefaultRuleIsMissing' + EditorConfig = $editorConfigLf + GitAttributes = "*.cs text eol=lf`n" + ExpectPass = $false + Expect = "has no '\*' rule" + } + [pscustomobject]@{ + Name = 'Fails_WhenTheDefaultRuleMarksEveryPathBinary' + EditorConfig = $editorConfigLf + GitAttributes = "* -text`n" + ExpectPass = $false + Expect = "marks every path binary" + } + [pscustomobject]@{ + Name = 'Fails_WhenASpecificRuleDisagreesWithTheEditor' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`n*.ps1 text eol=crlf`n" + ExpectPass = $false + Expect = "'sample\.ps1': git checks out crlf.+requires lf" + } + [pscustomobject]@{ + Name = 'Fails_WhenAGitAttributesEndingHasNoTextAttribute' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`n*.unitypackage eol=lf`n" + ExpectPass = $false + Expect = "sets an ending for '\*\.unitypackage' without 'text'" + } + [pscustomobject]@{ + Name = 'Fails_WhenABinaryRuleAlsoPinsAnEnding' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`n*.unitypackage -text eol=lf`n" + ExpectPass = $false + Expect = "marks '\*\.unitypackage' binary and also sets an ending" + } + [pscustomobject]@{ + Name = 'Fails_WhenTheEndingNameIsUnknown' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=cr`n" + ExpectPass = $false + Expect = "unknown line ending 'cr'" + } + [pscustomobject]@{ + Name = 'Fails_WhenNoEditorConfigSectionDeclaresTheDefault' + EditorConfig = "root = true`n`n[*.cs]`nindent_size = 4`n" + GitAttributes = $gitAttributesLf + ExpectPass = $false + Expect = "must carry a '\[\*\]' section" + } + [pscustomobject]@{ + Name = 'Fails_WhenTheDefaultSectionOmitsEndOfLine' + EditorConfig = "root = true`n`n[*]`nindent_size = 4`n" + GitAttributes = $gitAttributesLf + ExpectPass = $false + Expect = "sets no 'end_of_line' for '\[\*\]'" + } + [pscustomobject]@{ + Name = 'Fails_WhenAnEditorConfigLineIsNotAProperty' + EditorConfig = "root = true`n`n[*]`nend_of_line`n" + GitAttributes = $gitAttributesLf + ExpectPass = $false + Expect = "is not a 'key = value' line" + } + [pscustomobject]@{ + Name = 'IgnoresBinaryRulesThatPinNoEnding' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`n*.png binary`n*.unitypackage -text`n" + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + # A later section wins, so a path-specific ending must be honoured on both + # sides instead of compared against the default alone. + Name = 'Passes_WhenALaterEditorConfigSectionNarrowsTheEnding' + EditorConfig = "$editorConfigLf`n`n[*.ps1]`nend_of_line = crlf`n" + GitAttributes = "* text=auto eol=lf`n*.ps1 text eol=crlf`n" + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + # The repository groups extensions in one brace list; a matcher that + # ignored the braces would resolve those sections as unmatched and report + # a matching ending as a mismatch. + Name = 'Passes_WhenAnEditorConfigSectionGroupsExtensionsInBraces' + EditorConfig = "$editorConfigLf`n`n[{*.cs,*.ps1}]`nindent_size = 4`nend_of_line = crlf`n" + GitAttributes = "* text=auto eol=lf`n*.cs text eol=crlf`n" + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + # Git takes each attribute from the last matching rule, so a binary rule + # after the text rule must win even though the text rule pins an ending. + Name = 'Passes_WhenALaterBinaryRuleOverridesTheTextRule' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`ndocs/images/*.png -text`n" + ExpectPass = $true + Expect = '' + } +) + +foreach ($script:case in $cases) { + Invoke-TestCase $script:case.Name { + $root = New-TempRoot -Prefix 'line-endings-' + try { + Write-FixtureFile -Root $root -RelativePath '.editorconfig' -Content $script:case.EditorConfig + Write-FixtureFile -Root $root -RelativePath '.gitattributes' -Content $script:case.GitAttributes + $result = Invoke-FixtureLint -Root $root + if ($script:case.ExpectPass) { + Assert-True ($result.ExitCode -eq 0) "expected a clean contract: $($result.Output)" + } else { + Assert-True ($result.ExitCode -eq 1) "expected drift to fail: $($result.Output)" + Assert-True ( + $result.Output -match $script:case.Expect + ) "expected '$($script:case.Expect)' in: $($result.Output)" + } + } finally { + Remove-TempRoot $root + } + } +} + +Invoke-TestCase 'Fails_WhenAGitAttributesFileIsMissing' { + $root = New-TempRoot -Prefix 'line-endings-' + try { + Write-FixtureFile -Root $root -RelativePath '.editorconfig' -Content $editorConfigLf + $result = Invoke-FixtureLint -Root $root + Assert-True ($result.ExitCode -eq 1) "a missing file must fail closed: $($result.Output)" + Assert-True ( + $result.Output -match 'git attributes' + ) "must explain the unreadable input: $($result.Output)" + } finally { + Remove-TempRoot $root + } +} + +Invoke-TestCase 'Passes_AgainstTheRepositoryItself' { + $result = Invoke-LineEndingLint ` + -EditorConfig (Join-Path $repoRoot '.editorconfig') ` + -GitAttributes (Join-Path $repoRoot '.gitattributes') ` + -LintArguments @('--verbose') + Assert-True ($result.ExitCode -eq 0) "the repository must satisfy its own contract: $($result.Output)" + Assert-True ( + $result.Output -match 'Line-ending contract in sync: lf across \d+ path' + ) "verbose output must report the resolved ending: $($result.Output)" +} + +if ($script:TestFailureCount -gt 0) { + Write-Host "line-ending self-tests: $($script:TestFailureCount) failed" + exit 1 +} +Write-Host 'line-ending self-tests: all passed' +exit 0 \ No newline at end of file diff --git a/scripts/tests/test-lint-line-endings.ps1.meta b/scripts/tests/test-lint-line-endings.ps1.meta new file mode 100644 index 0000000..7928d6a --- /dev/null +++ b/scripts/tests/test-lint-line-endings.ps1.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: ae94ea4f26985f3bae342aedcaf2f197 +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: \ No newline at end of file From 9cd77e1d00125cba240c5577ede59744fc803b30 Mon Sep 17 00:00:00 2001 From: wallstop Date: Fri, 2 Oct 2026 16:32:55 +0000 Subject: [PATCH 2/4] Harden the line-ending guard after mutation testing A seeded-regression pass over the new guard found two problems. - `-text` was parsed as an attribute named `-text` with the value `true`, so a binary rule looked like text and the `binary` macro was ignored. Both now resolve to a binary path, which is what git does. - A recursive `**` glob was compiled as two single stars, which silently stops matching deeper paths and would hide the drift behind the rule. The pattern is now refused with the file and line that uses it, because neither file needs it. A byte-order-mark strip and the self-test that covered it were removed as redundant: parsing trims every line, and trimming already drops the mark. The drift report gained two fixtures that a matcher cannot satisfy by accident. A narrowed `.editorconfig` section now fails against a broad `.gitattributes` rule, which the first version read as in sync because the broad rule's representative path carries no extension. Validation: 25 self-test cases pass. Five seeded regressions (ignore `binary`, guess `**`, accept any ending name, skip the `[*]` requirement, accept a binary rule that pins an ending) are each killed by a distinct case. --- scripts/lint-line-endings.js | 116 +++++++++++++++-------- scripts/tests/test-lint-line-endings.ps1 | 25 +++++ 2 files changed, 100 insertions(+), 41 deletions(-) diff --git a/scripts/lint-line-endings.js b/scripts/lint-line-endings.js index 7b4c34b..fd01ea7 100644 --- a/scripts/lint-line-endings.js +++ b/scripts/lint-line-endings.js @@ -25,7 +25,9 @@ * that sets `eol` without `text` is rejected because git ignores it. * * Only the glob subset the two files use is supported: `*`, `?`, and `{a,b}` - * alternation, where a pattern without a slash matches a basename at any depth. + * alternation, where a pattern without a slash matches a basename at any depth. A + * recursive `**` is refused instead of guessed, because a matcher that reads it + * as two `*` silently stops matching and would hide the drift behind it. * * `--verbose` prints the resolved canonical ending and the compared paths. Exit * codes: 0 = in sync, 1 = drift, a malformed rule, or an unreadable input. @@ -54,6 +56,8 @@ const SAMPLE_NAME = "sample"; function readFileOrThrow(filePath, purpose) { try { + // Every line is trimmed before it is parsed, and trimming already drops + // the byte-order mark some editors put on line one. return fs.readFileSync(filePath, "utf8"); } catch { throw new Error(`Cannot ${purpose}: '${filePath}' is missing or unreadable.`); @@ -73,7 +77,7 @@ function displayPath(filePath) { Splits one `{a,b,c}` glob into its alternatives. Nesting is not used by either file, so an inner brace stays literal instead of being expanded. */ -function expandBraces(glob) { +function expandBraces(glob, origin) { const open = glob.indexOf("{"); const close = glob.indexOf("}", open + 1); if (open < 0 || close < 0) { @@ -84,10 +88,18 @@ function expandBraces(glob) { return glob .slice(open + 1, close) .split(",") - .flatMap((alternative) => expandBraces(prefix + alternative + suffix)); + .flatMap((alternative) => expandBraces(prefix + alternative + suffix, origin)); } -function compileGlob(glob) { +function compileGlob(glob, origin) { + // A recursive `**` needs git's depth rules matched exactly, and a matcher + // that reads it as two `*` silently stops matching. The two files use plain + // `*`, so refuse the pattern rather than guess. + if (glob.includes("**")) { + throw new Error( + `${origin} uses the recursive glob '${glob}', which this check cannot match.`, + ); + } let source = "^"; for (const character of glob) { if (character === "*") { @@ -101,9 +113,9 @@ function compileGlob(glob) { return new RegExp(`${source}$`); } -function globMatches(glob, filePath) { - for (const alternative of expandBraces(glob)) { - const expression = compileGlob(alternative); +function globMatches(glob, filePath, origin) { + for (const alternative of expandBraces(glob, origin)) { + const expression = compileGlob(alternative, origin); if (expression.test(filePath)) { return true; } @@ -119,8 +131,8 @@ function globMatches(glob, filePath) { it governs to the comparison: `*.cs` becomes `sample.cs`, a literal path stays itself, and a bare `*` becomes an extensionless sample. */ -function samplePath(glob) { - const widest = expandBraces(glob)[0]; +function samplePath(glob, origin) { + const widest = expandBraces(glob, origin)[0]; return widest.includes("*") ? widest.replace(/\*/g, SAMPLE_NAME) : widest; } @@ -173,21 +185,41 @@ function parseEditorConfig(filePath) { The last matching section that sets `end_of_line` wins, which is how `.editorconfig` resolves precedence. */ -function resolveEditorEnding(sections, filePath, origin) { +function resolveEditorEnding(sections, filePath, source) { let resolved = null; for (const section of sections) { const value = section.properties.get("end_of_line"); - if (value === undefined || !globMatches(section.glob, filePath)) { + const origin = `${displayPath(source)}:${section.lineNumber}`; + if (value === undefined || !globMatches(section.glob, filePath, origin)) { continue; } resolved = { - ending: normalizeEnding(value, `${displayPath(origin)}:${section.lineNumber}`), + ending: normalizeEnding(value, origin), lineNumber: section.lineNumber, }; } return resolved; } +/* + One `.gitattributes` attribute token. `-name` unsets an attribute, `!name` + resets it to git's default, and a bare `name` sets it; neither leading mark is + a boolean `false` written after '='. +*/ +function parseAttribute(field) { + const separator = field.indexOf("="); + if (separator >= 0) { + return [field.slice(0, separator), field.slice(separator + 1)]; + } + if (field.startsWith("-")) { + return [field.slice(1), false]; + } + if (field.startsWith("!")) { + return [field.slice(1), "unset"]; + } + return [field, true]; +} + function parseGitAttributes(filePath) { const rules = []; const lines = readFileOrThrow(filePath, "read the git attributes").split("\n"); @@ -199,14 +231,13 @@ function parseGitAttributes(filePath) { const fields = line.split(/\s+/); const attributes = new Map(); for (const field of fields.slice(1)) { - const separator = field.indexOf("="); - if (separator < 0) { - // `-text` unsets an attribute and `!text` resets it to git's - // default; neither is a boolean `false` written after '='. - attributes.set(field.replace(/^[-!]/, ""), field.startsWith("-") ? false : field.startsWith("!") ? "unset" : true); - } else { - attributes.set(field.slice(0, separator), field.slice(separator + 1)); - } + const [name, value] = parseAttribute(field); + attributes.set(name, value); + } + // `binary` is git's macro for `-diff -merge -text`, so it decides the + // ending the same way an explicit `-text` does. + if (attributes.has("binary")) { + attributes.set("text", false); } rules.push({ pattern: fields[0], lineNumber: index + 1, attributes }); } @@ -216,34 +247,35 @@ function parseGitAttributes(filePath) { /* Git resolves each attribute from the last matching rule that sets it, so the `text` and `eol` answers can come from different rules. A binary path needs - no ending, which is reported as no ending at all. + no ending at all and is reported as such. */ -function resolveGitEnding(rules, filePath, origin) { - let text = { value: undefined, lineNumber: 0 }; - let ending = { value: undefined, lineNumber: 0 }; - let matched = { lineNumber: 0 }; +function resolveGitEnding(rules, filePath, source) { + let isBinary = false; + let ending; + let matched = 0; for (const rule of rules) { - if (!globMatches(rule.pattern, filePath)) { + if (!globMatches(rule.pattern, filePath, `${displayPath(source)}:${rule.lineNumber}`)) { continue; } - matched = rule; + matched = rule.lineNumber; if (rule.attributes.has("text")) { - text = { value: rule.attributes.get("text"), lineNumber: rule.lineNumber }; + const value = rule.attributes.get("text"); + isBinary = value === false || value === "unset"; } if (rule.attributes.has("eol")) { ending = { value: rule.attributes.get("eol"), lineNumber: rule.lineNumber }; } } - if (text.value === false || text.value === "unset") { - return { binary: true, ending: null, lineNumber: matched.lineNumber }; + if (isBinary) { + return { binary: true, ending: null, lineNumber: matched }; } - if (ending.value === undefined) { - return { binary: false, ending: null, lineNumber: matched.lineNumber }; + if (ending === undefined) { + return { binary: false, ending: null, lineNumber: matched }; } return { binary: false, - ending: normalizeEnding(ending.value, `${displayPath(origin)}:${matched.lineNumber}`), - lineNumber: matched.lineNumber, + ending: normalizeEnding(ending.value, `${displayPath(source)}:${ending.lineNumber}`), + lineNumber: matched, }; } @@ -293,14 +325,15 @@ function main() { const candidates = []; for (const section of sections) { if (section.properties.has("end_of_line")) { - candidates.push(samplePath(section.glob)); + candidates.push(samplePath(section.glob, editorName)); } } for (const rule of rules) { - candidates.push(samplePath(rule.pattern)); + candidates.push(samplePath(rule.pattern, `${gitName}:${rule.lineNumber}`)); } + const compared = [...new Set(candidates)]; - for (const candidate of [...new Set(candidates)]) { + for (const candidate of compared) { const editor = resolveEditorEnding(sections, candidate, EDITORCONFIG); const git = resolveGitEnding(rules, candidate, GITATTRIBUTES); if (git.binary || editor === null) { @@ -334,11 +367,12 @@ function main() { } if (verbose) { - const textRules = rules.filter((rule) => rule.attributes.has("text") || rule.attributes.has("eol")) - .length; + const endingRules = rules.filter( + (rule) => rule.attributes.has("text") || rule.attributes.has("eol"), + ).length; console.log( - `Line-ending contract in sync: ${canonical.ending} across ` + - `${new Set(candidates).size} path(s) and ${textRules} rule(s) in '${gitName}'.`, + `Line-ending contract in sync: ${canonical.ending} across ${compared.length} path(s) ` + + `and ${endingRules} rule(s) in '${gitName}'.`, ); } process.exit(0); diff --git a/scripts/tests/test-lint-line-endings.ps1 b/scripts/tests/test-lint-line-endings.ps1 index c7e9b71..a47a902 100644 --- a/scripts/tests/test-lint-line-endings.ps1 +++ b/scripts/tests/test-lint-line-endings.ps1 @@ -215,6 +215,31 @@ $cases = @( ExpectPass = $true Expect = '' } + [pscustomobject]@{ + # `binary` is git's macro for `-text`, so a path it covers carries no + # ending and a narrower editor section must not be reported against it. + Name = 'Passes_WhenTheBinaryMacroCoversAPathTheEditorNarrows' + EditorConfig = "$editorConfigLf`n`n[*.png]`nend_of_line = crlf`n" + GitAttributes = "* text=auto eol=lf`n*.png binary`n" + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + # A `**` pattern is refused rather than read as two `*`: a matcher that + # guessed would stop matching deeper paths and hide the drift behind them. + Name = 'Fails_WhenAGitAttributesRuleUsesARecursiveGlob' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`nEditor/** text eol=crlf`n" + ExpectPass = $false + Expect = "recursive glob 'Editor/\*\*'" + } + [pscustomobject]@{ + Name = 'Fails_WhenAnEditorConfigSectionUsesARecursiveGlob' + EditorConfig = "$editorConfigLf`n`n[Editor/**]`nend_of_line = crlf`n" + GitAttributes = $gitAttributesLf + ExpectPass = $false + Expect = "recursive glob 'Editor/\*\*'" + } ) foreach ($script:case in $cases) { From e2d4cbc96043f595987c243a3ec03aba3c6965b8 Mon Sep 17 00:00:00 2001 From: wallstop Date: Fri, 2 Oct 2026 17:24:31 +0000 Subject: [PATCH 3/4] Compare every brace alternative in the line-ending guard Bugbot review finding on PR #131. `samplePath` kept only the first `{a,b}` alternative, so later extensions in a brace group were never compared. With git pinning the first extension back to the default and leaving the rest on the broad rule, the guard reported the contract as in sync while the second extension still checked out LF against an editor that asked for CRLF. The same miss existed on the editor side when a narrowed section carried the brace group. Both directions were silent false passes, reproduced before the fix: - git side: `.editorconfig` `[ * ]` CRLF, `.gitattributes` `* eol=crlf` plus `{*.md,*.ps1} eol=lf` plus `*.md eol=crlf`. `sample.ps1` drifted and the guard exited 0. - editor side: `.editorconfig` `[ * ]` LF plus `[{*.md,*.ps1}]` CRLF, `.gitattributes` `* eol=lf` plus `*.md eol=crlf`. `sample.ps1` drifted and the guard exited 0. `samplePaths` now contributes one representative path per alternative, so the later ones reach the comparison. Brace expansion also became depth aware: it stopped at the first `}`, which left an inner group as literal text that matched nothing. Swept the same class in this matcher. A construct it cannot resolve exactly was read as literal text and silently stopped matching, which is how the finding arose. An anchored or directory-only `/`, a character class, and a backslash escape join the recursive `**` in a refusal that names the file and line, so a future pattern cannot be half-read. `.llm/references/forbidden-patterns.md` records the rule for every guard: resolve every alternative and nesting level, or refuse the construct. Validation: 33 self-test cases pass. Seven seeded regressions are each killed by a distinct case, including the two this finding describes and the two nested brace expansions. One earlier seeded regression turned out to be a no-op that the suite could not kill, so the nested-brace fixtures were rewritten to compare `sample.ps1` from the inner group. --- .llm/references/forbidden-patterns.md | 1 + scripts/lint-line-endings.js | 108 +++++++++++++++++------ scripts/tests/test-lint-line-endings.ps1 | 96 ++++++++++++++++++-- 3 files changed, 171 insertions(+), 34 deletions(-) diff --git a/.llm/references/forbidden-patterns.md b/.llm/references/forbidden-patterns.md index 379ee76..3833e4c 100644 --- a/.llm/references/forbidden-patterns.md +++ b/.llm/references/forbidden-patterns.md @@ -35,3 +35,4 @@ Patterns that must not appear in this codebase, with the compliant alternative. | `Assert.IsNull` / `Assert.IsNotNull`, `Is.Null` / `Is.Not.Null` constraints on any value, `?.`, `??`, or implicit bool tests on `UnityEngine.Object` | `Assert.That(value == null)` / `Assert.That(value != null)`; explicit `== null` / `!= null` comparisons | Unity overloads the equality operators so a destroyed-object wrapper compares null; NUnit's IsNull, Is.Null constraints, and C#'s `?.`/`??`/bool forms use reference equality and cannot see that lifetime (`scripts/lint-csharp-null-assertions.js` enforces the assertion and constraint forms) | | Static mutable state with no documented reason or teardown reset | document why it is static, weak-key by lifecycle owner, unregister on every removal path, and reset it in the owning window's `Cleanup()` (`AssetGuidTypeIndex.Shared` suspension reset and the theme-selection ownership registry are the precedents) | static editor state outlives windows and strands cross-window state (PR #122 review finding) | | `using` directives above the `namespace` declaration | place them inside the namespace block; file-level usings are sanctioned only for namespaceless assembly-attribute files and `[assembly: ...]` preambles like `InternalsVisibleTo` (`npm run lint:csharp-usings` enforces this, `:fix` moves the simple cases) | one namespace-scoped convention keeps the file header stable and the rule mechanically checkable (#124) | +| A guard that reads a machine-readable config resolves only part of a construct, such as the first `{a,b}` alternative, a nested group, or one level of a nested mapping | resolve every alternative and nesting level, or refuse the pattern with the file and line that uses it | a partially-read rule still matches its own representative path, so the guard passes while the real configuration disagrees; refusing beats guessing (PR #131 review finding in `scripts/lint-line-endings.js`) | diff --git a/scripts/lint-line-endings.js b/scripts/lint-line-endings.js index fd01ea7..16e2697 100644 --- a/scripts/lint-line-endings.js +++ b/scripts/lint-line-endings.js @@ -17,7 +17,9 @@ * with a broad `.gitattributes` rule any more than a broad section can disagree * with a narrow rule. Resolving both directions needs one representative path * per `.editorconfig` section that declares an ending and one per - * `.gitattributes` rule. + * `.gitattributes` rule, and one per `{a,b}` alternative of each, because a + * pattern whose later alternatives are never compared hides the drift behind + * them. * * Resolution follows git's own attribute model: each attribute takes the value * of the last matching rule that sets it, so a later `text` and an earlier @@ -25,9 +27,11 @@ * that sets `eol` without `text` is rejected because git ignores it. * * Only the glob subset the two files use is supported: `*`, `?`, and `{a,b}` - * alternation, where a pattern without a slash matches a basename at any depth. A - * recursive `**` is refused instead of guessed, because a matcher that reads it - * as two `*` silently stops matching and would hide the drift behind it. + * alternation, where a pattern without a slash matches a basename at any depth. + * A recursive `**`, an anchored or directory-only `/`, a character class, and a + * backslash escape are refused instead of guessed, because a matcher that reads + * one as literal text silently stops matching and would hide the drift behind + * the rule. * * `--verbose` prints the resolved canonical ending and the compared paths. Exit * codes: 0 = in sync, 1 = drift, a malformed rule, or an unreadable input. @@ -74,32 +78,76 @@ function displayPath(filePath) { } /* - Splits one `{a,b,c}` glob into its alternatives. Nesting is not used by - either file, so an inner brace stays literal instead of being expanded. + Splits one `{a,b,c}` glob into every alternative, including nested groups. + Splitting on the first `}` would leave a nested group as literal text and + then match nothing, so the closing brace is found by depth. */ -function expandBraces(glob, origin) { +function expandBraces(glob) { const open = glob.indexOf("{"); - const close = glob.indexOf("}", open + 1); - if (open < 0 || close < 0) { + if (open < 0) { + return [glob]; + } + let depth = 0; + let close = -1; + for (let index = open; index < glob.length; index++) { + if (glob[index] === "{") { + depth++; + } else if (glob[index] === "}") { + depth--; + if (depth === 0) { + close = index; + break; + } + } + } + if (close < 0) { return [glob]; } const prefix = glob.slice(0, open); const suffix = glob.slice(close + 1); - return glob - .slice(open + 1, close) - .split(",") - .flatMap((alternative) => expandBraces(prefix + alternative + suffix, origin)); + const alternatives = []; + let start = open + 1; + depth = 0; + for (let index = open + 1; index <= close; index++) { + const character = glob[index]; + if (character === "{") { + depth++; + } else if (character === "}") { + depth--; + } else if (character === "," && depth === 0) { + alternatives.push(glob.slice(start, index)); + start = index + 1; + } + } + alternatives.push(glob.slice(start, close)); + return alternatives.flatMap((alternative) => + expandBraces(`${prefix}${alternative}${suffix}`), + ); } -function compileGlob(glob, origin) { - // A recursive `**` needs git's depth rules matched exactly, and a matcher - // that reads it as two `*` silently stops matching. The two files use plain - // `*`, so refuse the pattern rather than guess. - if (glob.includes("**")) { +/* + Glob constructs this matcher cannot resolve exactly. A pattern that uses one + would be read as literal text and would silently stop matching the files git + or `.editorconfig` will actually apply it to, so it is refused instead. +*/ +const UNSUPPORTED_GLOBS = [ + { label: "a recursive '**' segment", matches: (glob) => glob.includes("**") }, + { label: "an anchored or directory-only '/'", matches: (glob) => glob.startsWith("/") || glob.endsWith("/") }, + { label: "a character class", matches: (glob) => glob.includes("[") }, + { label: "a backslash escape", matches: (glob) => glob.includes("\\") }, +]; + +function refuseUnsupportedGlob(glob, origin) { + const unsupported = UNSUPPORTED_GLOBS.find((candidate) => candidate.matches(glob)); + if (unsupported !== undefined) { throw new Error( - `${origin} uses the recursive glob '${glob}', which this check cannot match.`, + `${origin} uses ${unsupported.label} in '${glob}', which this check cannot match.`, ); } +} + +function compileGlob(glob, origin) { + refuseUnsupportedGlob(glob, origin); let source = "^"; for (const character of glob) { if (character === "*") { @@ -114,7 +162,7 @@ function compileGlob(glob, origin) { } function globMatches(glob, filePath, origin) { - for (const alternative of expandBraces(glob, origin)) { + for (const alternative of expandBraces(glob)) { const expression = compileGlob(alternative, origin); if (expression.test(filePath)) { return true; @@ -127,13 +175,17 @@ function globMatches(glob, filePath, origin) { } /* - A representative path for a glob, so each rule contributes the kind of file - it governs to the comparison: `*.cs` becomes `sample.cs`, a literal path - stays itself, and a bare `*` becomes an extensionless sample. + One representative path per brace alternative, so every file a glob governs + reaches the comparison: `*.cs` becomes `sample.cs`, `{*.md,*.ps1}` becomes + both `sample.md` and `sample.ps1`, a literal path stays itself, and a bare `*` + becomes an extensionless sample. Taking only the first alternative would leave + the rest uncompared and hide the drift behind them. */ -function samplePath(glob, origin) { - const widest = expandBraces(glob, origin)[0]; - return widest.includes("*") ? widest.replace(/\*/g, SAMPLE_NAME) : widest; +function samplePaths(glob, origin) { + return expandBraces(glob).map((alternative) => { + refuseUnsupportedGlob(alternative, origin); + return alternative.includes("*") ? alternative.replace(/\*/g, SAMPLE_NAME) : alternative; + }); } function normalizeEnding(value, origin) { @@ -325,11 +377,11 @@ function main() { const candidates = []; for (const section of sections) { if (section.properties.has("end_of_line")) { - candidates.push(samplePath(section.glob, editorName)); + candidates.push(...samplePaths(section.glob, editorName)); } } for (const rule of rules) { - candidates.push(samplePath(rule.pattern, `${gitName}:${rule.lineNumber}`)); + candidates.push(...samplePaths(rule.pattern, `${gitName}:${rule.lineNumber}`)); } const compared = [...new Set(candidates)]; diff --git a/scripts/tests/test-lint-line-endings.ps1 b/scripts/tests/test-lint-line-endings.ps1 index a47a902..b5ae6ea 100644 --- a/scripts/tests/test-lint-line-endings.ps1 +++ b/scripts/tests/test-lint-line-endings.ps1 @@ -197,12 +197,13 @@ $cases = @( Expect = '' } [pscustomobject]@{ - # The repository groups extensions in one brace list; a matcher that - # ignored the braces would resolve those sections as unmatched and report - # a matching ending as a mismatch. + # The repository groups extensions in one brace list, so every alternative + # has to be compared on both sides: a matcher that ignored the braces would + # resolve those sections as unmatched and report a matching ending as a + # mismatch, and one that compared only the first would leave `*.ps1` out. Name = 'Passes_WhenAnEditorConfigSectionGroupsExtensionsInBraces' EditorConfig = "$editorConfigLf`n`n[{*.cs,*.ps1}]`nindent_size = 4`nend_of_line = crlf`n" - GitAttributes = "* text=auto eol=lf`n*.cs text eol=crlf`n" + GitAttributes = "* text=auto eol=lf`n*.cs text eol=crlf`n*.ps1 text eol=crlf`n" ExpectPass = $true Expect = '' } @@ -231,14 +232,97 @@ $cases = @( EditorConfig = $editorConfigLf GitAttributes = "* text=auto eol=lf`nEditor/** text eol=crlf`n" ExpectPass = $false - Expect = "recursive glob 'Editor/\*\*'" + Expect = "recursive '\*\*' segment in 'Editor/\*\*'" } [pscustomobject]@{ Name = 'Fails_WhenAnEditorConfigSectionUsesARecursiveGlob' EditorConfig = "$editorConfigLf`n`n[Editor/**]`nend_of_line = crlf`n" GitAttributes = $gitAttributesLf ExpectPass = $false - Expect = "recursive glob 'Editor/\*\*'" + Expect = "recursive '\*\*' segment in 'Editor/\*\*'" + } + [pscustomobject]@{ + # Comparing only the first alternative left `*.ps1` unchecked, so a real + # disagreement passed. Git pins `*.md` back to the default; `*.ps1` still + # checks out LF while the editor asks for CRLF. + Name = 'Fails_WhenALaterBraceAlternativeDriftsOnTheGitSide' + EditorConfig = $editorConfigCrlf + GitAttributes = "* text=auto eol=crlf`n{*.md,*.ps1} text eol=lf`n*.md text eol=crlf`n" + ExpectPass = $false + Expect = "'sample\.ps1': git checks out lf.+requires crlf" + } + [pscustomobject]@{ + # The mirror image: the narrowed section carries the brace group and git + # leaves the second extension on the broad rule. + Name = 'Fails_WhenALaterBraceAlternativeDriftsOnTheEditorSide' + EditorConfig = "$editorConfigLf`n`n[{*.md,*.ps1}]`nend_of_line = crlf`n" + GitAttributes = "* text=auto eol=lf`n*.md text eol=crlf`n" + ExpectPass = $false + Expect = "'sample\.ps1': git checks out lf.+requires crlf" + } + [pscustomobject]@{ + # Every alternative of a brace group compared on both sides and every one + # in sync, so comparing the later ones must not start reporting the + # alternatives that agree. + Name = 'Passes_WhenEveryBraceAlternativeAgrees' + EditorConfig = "$editorConfigLf`n`n[{*.md,*.ps1}]`nend_of_line = crlf`n" + GitAttributes = "* text=auto eol=lf`n{*.md,*.ps1} text eol=crlf`n" + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + # `*.ps1` comes from the inner group, so a split that stops at the first + # `}` or that ignores nesting depth never compares it and its drift goes + # unreported. + Name = 'Fails_WhenBraceGroupsNest' + EditorConfig = "$editorConfigCrlf`n`n[*.{cs,{ps1,bat}}]`nend_of_line = lf`n" + GitAttributes = "* text=auto eol=crlf`n*.ps1 text eol=crlf`n" + ExpectPass = $false + Expect = "'sample\.ps1': git checks out crlf.+requires lf" + } + [pscustomobject]@{ + Name = 'Passes_WhenNestedBraceGroupsAgree' + EditorConfig = "$editorConfigLf`n`n[*.{cs,{ps1,bat}}]`nend_of_line = crlf`n" + GitAttributes = "* text=auto eol=lf`n*.cs text eol=crlf`n*.ps1 text eol=crlf`n*.bat text eol=crlf`n" + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + # The other constructs the matcher cannot resolve exactly. Each would + # otherwise be read as literal text and silently stop matching. + Name = 'Fails_WhenAGitAttributesRuleIsAnchored' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`n/docs/*.md text eol=crlf`n" + ExpectPass = $false + Expect = "anchored or directory-only '/' in '/docs/\*\.md'" + } + [pscustomobject]@{ + Name = 'Fails_WhenAGitAttributesRuleIsDirectoryOnly' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`nbuild/ text eol=crlf`n" + ExpectPass = $false + Expect = "anchored or directory-only '/' in 'build/'" + } + [pscustomobject]@{ + Name = 'Fails_WhenAGitAttributesRuleUsesACharacterClass' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`n*[0-9].cs text eol=crlf`n" + ExpectPass = $false + Expect = "character class in '\*\[0-9\]\.cs'" + } + [pscustomobject]@{ + Name = 'Fails_WhenAGitAttributesRuleUsesABackslashEscape' + EditorConfig = $editorConfigLf + GitAttributes = "* text=auto eol=lf`ndocs/\*.md text eol=crlf`n" + ExpectPass = $false + Expect = "backslash escape in 'docs/\\\*\.md'" + } + [pscustomobject]@{ + Name = 'Fails_WhenAnEditorConfigSectionIsAnchored' + EditorConfig = "$editorConfigLf`n`n[/docs/*.md]`nend_of_line = crlf`n" + GitAttributes = $gitAttributesLf + ExpectPass = $false + Expect = "anchored or directory-only '/' in '/docs/\*\.md'" } ) From 6c7f07e7e98e3d114f84f28729223e18240f8f66 Mon Sep 17 00:00:00 2001 From: wallstop Date: Fri, 2 Oct 2026 17:24:32 +0000 Subject: [PATCH 4/4] Warn agents that frontmatter tolerance hides a misspelled category Found while sweeping the same class of defect as the #131 review finding: the SKILL.md frontmatter reader tolerates unrecognized keys by design, which `manage-skills` documents. That tolerance has no teeth for a key that is recognizable but misspelled. A `catgeory:` under `metadata`, or `category:` written at the top level instead of under `metadata`, is ignored and the category falls back to `Feature`. The skill is filed under the wrong heading in the generated index and `generate-skills-index.ps1` plus `lint-llm-instructions.ps1` both report success. The reader cannot refuse unknown keys without contradicting its documented policy, so the preventive step goes in the skill: check the category heading in `.llm/skills/index.md` after regenerating. --- .llm/skills/manage-skills/SKILL.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.llm/skills/manage-skills/SKILL.md b/.llm/skills/manage-skills/SKILL.md index 3373e09..3f15eb0 100644 --- a/.llm/skills/manage-skills/SKILL.md +++ b/.llm/skills/manage-skills/SKILL.md @@ -30,6 +30,11 @@ metadata: domain-specific how-to. - Optional spec keys (`license`, `compatibility`, `allowed-tools`, `metadata.*`) are tolerated but rarely needed. +- Tolerance has no teeth: an unrecognized key is ignored rather than rejected. A + misspelled `metadata.category`, or `category` written at the top level instead of + under `metadata`, silently falls back to `Feature`, so the skill is filed under the + wrong heading in the generated index with a passing lint. Check the category in + `.llm/skills/index.md` after regenerating. ## Body Contract