fix(build-tools): normalize paths and reject backslash globs

[Alex Villarreal (alexvy86)]

Description

AB#45159

Build-tools task globs are configuration values and must use POSIX separators. Previously, backslash-containing globs were accepted and could be interpreted inconsistently across operating systems. This change validates declarative input/output globs with an actionable error while preserving repo-root-only normalization for token substitution.

The Dockerfile package policy resolver now normalizes relative paths before generating COPY text, matching the validator and preventing flub check policy --fix from writing OS-specific paths. It also reverts the incorrect full-string normalization introduced by PR #28214.

Validation includes real build-graph evidence: the pre-fix build accepted src\\**\\*.ts, while the fixed build rejects it during graph construction. A real resolver run against a temporary Dockerfile wrote the expected COPY packages/foo/package*.json packages/foo/ line after the fix.

Reviewer Guidance

The review process is outlined in the pull request guidelines.

Please review the scope of the path audit, especially the decision to validate configured globs rather than normalize them silently.

Validated with the build-tools compile flow, targeted build-tools tests (34 passing), and the isolated Dockerfile policy regression (1 passing).

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (227 lines, 8 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

How this works

• Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

• Tick Start review below to dispatch the review fleet.

• After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

• This comment updates as new commits land; your reviewer selections are preserved.

• Start review

[Copilot]

🟡 Changes recommended

The shared normalization can miss additionalConfigFiles paths containing backslashes and needs a path-specific fix or explicit validation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request updates build-tools path handling to reject backslash globs and produce normalized POSIX Dockerfile paths.

Changes:

• Validate POSIX separators in task globs.
• Limit ${repoRoot} normalization.
• Normalize Dockerfile package paths and add regression tests.

File summaries

File	Summary
build-tools/packages/build-tools/src/test/taskDefinitions.test.ts	Tests rejection of invalid glob separators.
build-tools/packages/build-tools/src/test/repoRootToken.test.ts	Updates repository-root token expectations.
build-tools/packages/build-tools/src/fluidBuild/fluidTaskDefinitions.ts	Adds input/output glob separator validation.
build-tools/packages/build-tools/src/fluidBuild/fluidBuildConfig.ts	Narrows token normalization; moderate issue remains for additionalConfigFiles paths containing backslashes (2 votes).
build-tools/packages/build-cli/src/test/library/repoPolicyCheck/dockerfilePackages.test.ts	Tests Dockerfile path normalization; nit requests coverage through the policy resolver (1 vote).
build-tools/packages/build-cli/src/library/repoPolicyCheck/dockerfilePackages.ts	Normalizes relative paths for POSIX Dockerfile COPY output.

Review details

Suppressed comments (1)

build-tools/packages/build-cli/src/library/repoPolicyCheck/dockerfilePackages.ts:56

• This regression test exercises only getDockerfileCopyText; it never invokes the policy resolver or verifies the Dockerfile write. The caller-side normalization added here could therefore regress without this test failing. Add the temporary-Dockerfile resolver case described in the PR so the actual flub check policy --fix path is covered.

			TscUtils.normalizeSlashes(path.relative(gitRoot, file)).replace(serverPath, ""),

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ⚠️ Approve with Suggestions

0 Spicy, 1 Pungent, 2 Smelly

Findings

Sev	#	Area	File	What	Fix
🧄 Pungent	H1	Testing	build-tools/packages/build-cli/src/test/library/repoPolicyCheck/dockerfilePackages.test.ts:27	The only regression test for the TscUtils.normalizeSlashes fix in dockerfilePackages.ts is gated behind if (path.sep !== "\\") { this.skip(); }, so it only runs on Windows machines. normalizeSlashes (build-tools/packages/build-tools/src/fluidBuild/tscUtils.ts:393) is a pure string .replace(/\\/g, "/"), not OS-dependent, and CI for this repo runs on Linux/macOS, so this test is skipped on every real CI run and provides zero actual coverage of the bug fix. A regression (e.g. someone reverting the normalizeSlashes call) would ship undetected.	Remove the platform gate and construct the backslash-containing path directly (e.g. path.win32.join(...) or a literal string with \\) instead of relying on process.chdir plus a real Windows-style relative path, so the test exercises getDockerfileCopyText/resolver on any OS. Assert the Dockerfile is rewritten with forward slashes regardless of the platform running the test.
🧅 Smelly	M1	Testing	build-tools/packages/build-tools/src/fluidBuild/tasks/taskFactory.ts:105	getLeafTaskForCommand now calls validateDeclarativeTaskGlobSeparators(taskMatch) when a task is resolved via node.pkg.packageJson.fluidBuild?.declarativeTasks or context.fluidBuildConfig?.declarativeTasks (executable-keyed declarative tasks), but there is no test file for taskFactory.ts at all. This is a distinct validation entry point from the files-based path validated in fluidTaskDefinitions.ts, and from normalizeGlobalTaskDefinitions/getTaskDefinitions already covered by tests. If this call were removed or the wrong object passed, a backslash glob configured via declarativeTasks would silently proceed unvalidated and no test would catch it.	Add a test that builds a BuildPackage/BuildContext with fluidBuildConfig.declarativeTasks (or packageJson.fluidBuild.declarativeTasks) containing an executable mapped to inputGlobs: ["src\\**\\*.ts"], calls TaskFactory.Create/getLeafTaskForCommand with a matching command, and asserts it throws the 'contains backslashes' error.
🧅 Smelly	M2	Testing	build-tools/packages/build-tools/src/fluidBuild/tasks/leaf/leafTask.ts:601-603	getAdditionalConfigFilesFullPaths now wraps replaceRepoRootToken(configPath, repoRoot) with the new toPosixPath before resolving the full path, compensating for replaceRepoRootToken no longer normalizing the whole path (only the repo root prefix). toPosixPath itself is unit-tested in globPatterns.test.ts and replaceRepoRootToken's narrower behavior is tested in repoRootToken.test.ts, but no test exercises this specific integration point (LeafWithDoneFileTask's config-file path resolution). If the toPosixPath wrap were dropped or misapplied here, a windows-style additionalConfigFiles entry combined with ${repoRoot} would produce a mixed-separator path that fast-glob/fs APIs could mis-resolve, and no existing test would catch it.	Add a test constructing a LeafWithDoneFileTask (or a package with additionalConfigFiles containing a backslash segment, e.g. ${repoRoot}\\config\\tsconfig.json) and assert the resolved full path returned by the config-files accessor uses forward slashes only.

---

View workflow run

[Alex Villarreal (alexvy86)]

[Agent-generated]

Added an unreleased Bug Fixes section to build-tools/CHANGELOG.md documenting both path-separator changes:

• fluid-build now rejects backslashes in declarative task globs instead of relying on OS-specific behavior.
• check:policy --fix now normalizes Dockerfile package paths consistently with validation.

This follows the build-tools release-group changelog process; no client changeset is required.

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (2)

build-tools/packages/build-tools/src/fluidBuild/fluidBuildConfig.ts:32

• The new separator checks cover only normalizeGlobalTaskDefinitions and package task overrides. fluidBuild.declarativeTasks is a second supported source of DeclarativeTask objects (taskFactory.ts:101-107), and package-local declarations are passed directly there, so ${repoRoot}\\src\\**\\*.ts still reaches replaceRepoRootTokens unchanged and is handed to fast-glob as an escaped path. Validate both repo-level and package-level declarativeTasks before task creation, with regression coverage, so every declarative glob gets the new invariant.

	return pathOrGlob.replace(REPO_ROOT_REGEX, normalizedRepoRoot);

build-tools/packages/build-tools/src/fluidBuild/fluidBuildConfig.ts:32

• The @remarks above still says the result is safe for fast-glob, but this return now deliberately preserves backslashes in pathOrGlob. A direct caller can therefore receive an unsafe glob; update the contract to say that only repoRoot is normalized and callers must provide or validate POSIX globs.

	return pathOrGlob.replace(REPO_ROOT_REGEX, normalizedRepoRoot);

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Alex Villarreal (alexvy86)]

[Agent-generated]

Addressed the latest review findings in commit 24176470cbf:

• Added shared declarative-glob validation at TaskFactory construction, covering both repo-level and package-local fluidBuild.declarativeTasks paths that bypass task-definition expansion.
• Updated the replaceRepoRootToken documentation to state that only the dynamic repository root is normalized and callers must provide or validate POSIX globs.
• Reused the same validation for global and package task definitions and added regression coverage.

Validation: the build-tools release-group build passed, and the targeted task-definition and repo-root token tests passed (35 tests total across the two runs). The changelog entry already documents the intentional runtime behavior change.

[Copilot]

🔵 Needs a closer look

A moderate review comment requests replacement regression coverage for repo-root-only normalization; a helper export also has a minor nit.

Review details

Suppressed comments (2)

build-tools/packages/build-cli/src/library/repoPolicyCheck/dockerfilePackages.ts:13

• The new regression test exercises handler.resolver and does not import this helper, and there are no other callers that require it to be exported. Keeping export unnecessarily widens the module's API and can make this implementation detail part of the contract; keep the helper private.

export function getDockerfileCopyText(packageFilePath: string): string {

build-tools/packages/build-tools/src/fluidBuild/fluidBuildConfig.ts:33

• This change intentionally removes full-string normalization, but the direct regression test for ${repoRoot}\\... suffixes was deleted. Without a replacement assertion that the repo root is normalized while a backslash in the configured suffix remains unchanged, a future refactor could silently restore glob normalization and bypass the new validation contract.

	return pathOrGlob.replace(REPO_ROOT_REGEX, normalizedRepoRoot);

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Alex Villarreal (alexvy86)]

[Agent-generated]

Addressed both follow-up review comments in d1eb2ddced9:

• Added a regression test proving ${repoRoot} is normalized while a backslash in the configured suffix remains unchanged.
• Made the internal getDockerfileCopyText helper private now that the resolver integration test covers the actual policy-fix path.

Validation: Biome passed, the build-tools test project compiled, and the updated repo-root token suite passed with 18 tests.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

🔵 Needs a closer look

The Dockerfile regression test is always skipped by the Linux build-tools CI pipeline.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

build-tools/packages/build-cli/src/test/library/repoPolicyCheck/dockerfilePackages.test.ts:29

• This regression test never runs in the build-tools CI: build-build-tools.yml:94-109 uses build-npm-package.yml, whose build/test pool is Linux (build-npm-package.yml:148-150). Consequently, removing the resolver normalization would still leave CI green. Please add a platform-independent seam that feeds a backslash-containing relative path into the COPY-text generation (while retaining this Windows integration test) so the regression is exercised on Linux CI.

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Alex Villarreal (alexvy86)]

[Agent-generated]

Addressed the CI coverage gap in e1027e15a50:

• Kept the Windows integration test.
• Added a platform-independent resolver regression test that feeds a backslash-containing package path (packages\bar) into the Dockerfile policy fixer, so Linux CI exercises the normalizeSlashes behavior instead of skipping all coverage for this fix.

Validation: Biome passed for the changed test file, the build-cli test project compiled, and dockerfilePackages.test.js passed with 2 tests.

[Copilot]

🟢 Approval recommended

The changes consistently enforce POSIX globs and preserve normalization for non-glob paths with appropriate regression coverage.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[github-actions]

Bundle size comparison

Base commit: could not be determined; will be reported when the comparison runs
Head commit: 840ce14fe8638af8d256279c2f0101bedf50c2e1

Pending — Build - client packages is running. Results will appear here when the build completes.