Skip to content

refactor(cli): unify path normalization, add tests + Windows install docs#2863

Merged
bpamiri merged 2 commits into
developfrom
refactor/cli-unify-path-normalization
Jun 5, 2026
Merged

refactor(cli): unify path normalization, add tests + Windows install docs#2863
bpamiri merged 2 commits into
developfrom
refactor/cli-unify-path-normalization

Conversation

@bpamiri
Copy link
Copy Markdown
Collaborator

@bpamiri bpamiri commented Jun 5, 2026

Summary

Follow-up to #2835, which fixed the Windows there is no Resource provider available with the name [c] crash (#2841) by normalising backslash paths before they reach Lucee's Resource API. This PR adds the parts #2835 didn't carry — without changing the runtime fix:

  1. Consolidates the normalizer (refactor, no behavior change). fix(cli): normalize Windows drive-letter paths in wheels new #2835 added a private $normalizePath() inside Module.cfc. The canonical implementation now lives in Helpers.normalizePath() — its natural home alongside the other pure string utilities (capitalize, pluralize, stripSpecialChars) — and Module.$normalizePath() is a one-line delegating wrapper. Helpers is a dependency-free leaf utility, so it is safe to instantiate during init(). All 6 call sites and the $safeDirExists() java.io.File fallback from fix(cli): normalize Windows drive-letter paths in wheels new #2835 are untouched.
  2. Adds regression coverage. 7 normalizePath() specs in HelpersSpec.cfc. Because Module.$normalizePath() delegates to Helpers.normalizePath(), these exercise the actual bootstrap path — not a parallel copy.
  3. Documents the failure mode. A Windows troubleshooting section in the CLI installation guide so users on stale installs can self-diagnose.

This is CLI-only code (cli/lucli/**), so it runs on LuCLI's bundled Lucee — the cross-engine invariants don't apply.

Why a follow-up instead of #2843

#2843 proposed a competing, more comprehensive fix for #2841 before #2835 landed. Once #2835 merged, #2843 became largely redundant and conflict-heavy, and a naïve conflict resolution would have reverted #2835's $safeDirExists defense layer. Rather than untangle that, its durable value (tests + docs) is salvaged here against a clean base. #2843 is closed as superseded.

Verification

667/667 cli/lucli specs pass locally (Lucee 7 + SQLite via LuCLI), including the 7 new normalizePath() cases — 0 failures, 0 errors.

Pull Request Checklist

  • TestsHelpersSpec.cfc gains a normalizePath() describe block: backslash→forward, POSIX no-op, empty input, doubled-slash collapse, drive-letter prefix round-trip, UNC preservation, UNC doubled-slash collapse.
  • Framework Docs — n/a (no framework behavior change).
  • AI Reference Docs — n/a (refactor + coverage + user-guide doc).
  • CLAUDE.md — n/a (no convention change).
  • CHANGELOG.md[Unreleased] → Changed entry added.
  • Test runner passes667/667 CLI specs locally; CI exercises the full matrix + verb smoke.

🤖 Generated with Claude Code

…docs

Follow-up to #2835, which fixed the Windows "no Resource provider [c]"
crash (#2841). This does not change the runtime fix #2835 shipped:

- Move the canonical normalizePath() into Helpers.cfc (its natural home
  beside capitalize/pluralize/stripSpecialChars) and reduce
  Module.$normalizePath() to a one-line delegating wrapper. Helpers is a
  dependency-free leaf utility, so it is safe to construct during init().
  The 6 call sites and the $safeDirExists() fallback are unchanged.
- Add 7 normalizePath() regression specs to HelpersSpec.cfc. Because
  Module delegates to Helpers now, these exercise the real bootstrap path
  rather than a parallel copy.
- Document the Windows "there is no Resource provider available with the
  name [c]" failure in the CLI installation guide.

Supersedes #2843. Verified locally: 667/667 cli/lucli specs pass.

Signed-off-by: Peter Amiri <peter@alurium.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

This PR consolidates the Windows path-normalisation fix from a private inline copy in Module.cfc into Helpers.normalizePath(), adds 7 regression specs that exercise the real bootstrap code path, and documents the Lucee Resource-API crash for users on stale Windows installs. The refactor is minimal, correctness-preserving, and well-scoped. I have one minor correctness nit about a mid() call with a potentially zero-length count argument, and a small docs observation. No blocking issues.

Correctness

mid(rv, 3, len(rv) - 2) with count 0 when rv is exactly "//"

cli/lucli/services/Helpers.cfc, line 131:

var body = len(leading) ? mid(rv, 3, len(rv) - 2) : rv;

When rv = "//" (length 2) and leading = "//", this evaluates to mid("//", 3, 0). CLAUDE.md Cross-Engine Invariant #8 specifically flags Left(str, 0) as a Lucee 7 crash; mid with a count of 0 and a start position past the string end may trigger the same underlying Java string-range error. The path "//" alone is not a legal filesystem path that would reach this function in practice, but a small guard would make the defensive intent explicit:

var body = len(leading) ? (len(rv) > 2 ? mid(rv, 3, len(rv) - 2) : "") : rv;

The test suite covers "//server/share/path" and "//server//share" but not the degenerate "//" input. Adding that case would confirm the branch is safe across engines.

Docs

Troubleshooting section mentions only Scoop

web/sites/guides/src/content/docs/v4-0-0/command-line-tools/installation.mdx, new Windows section:

Update to the latest version (`scoop update wheels` for Scoop installs)

The parenthetical qualifier is good — it prevents confusion for non-Scoop users. If the installation guide covers other Windows package managers or a manual binary download, it would be worth noting the equivalent update path (or pointing to the relevant install section) so those users are not left guessing. Low-priority nit; the current wording is accurate.

Commits

Single commit refactor(cli): unify path normalization, add tests + Windows install docs — type refactor, scope cli, 73-char header, sentence-case subject. Conforms to commitlint.config.js. ✓

Tests

Seven specs in HelpersSpec.cfc cover backslash conversion, POSIX no-op, empty input, doubled-slash collapse, drive-letter round-trip, UNC preservation, and UNC doubled-slash collapse. Because Module.$normalizePath() delegates to Helpers.normalizePath(), these specs exercise the actual bootstrap path as claimed. The only gap is the "//" edge case noted above. ✓

@wheels-bot
Copy link
Copy Markdown
Contributor

wheels-bot Bot commented Jun 5, 2026

Wheels Bot -- Reviewer B (round 1)

A's review is solid. No sycophancy, no meaningful false positives, no missed issues on re-scan. The one precision issue is that A invokes Cross-Engine Invariant #8 (which documents Left(str, 0) crashing Lucee 7) as an analogy for the mid() concern -- but mid(str, pos, 0) maps to Java's String.substring(n, n) which returns "" rather than throwing. A hedges with "may trigger" rather than asserting a definite crash, so this is a minor imprecision in reasoning, not a false positive. The defensive guard suggestion stands regardless. The docs nit is accurate.

Sycophancy

None detected. A cited specific files, lines, and engine-specific invariants rather than issuing a blanket approval.

False positives

None. A's mid() concern is grounded in a real edge case (rv = "//" producing a count-0 argument). The analogy to Invariant #8 is slightly imprecise -- that invariant is Left(str, 0)-specific and mid(str, pos, 0) has different Java semantics -- but A hedged appropriately. The guard suggestion is cheap and correct regardless.

Missed issues

None detected. The PR is CLI-only (cli/lucli/**) running on LuCLI's bundled Lucee, so the broad cross-engine matrix does not apply. Commit header is 73 chars and satisfies commitlint.config.js. The seven test cases exercise the real bootstrap path via delegation. CHANGELOG entry is accurate and placed correctly.

Verdict alignment

A's COMMENTED state is consistent with the findings: two minor nits, neither blocking. The code is correct for all real-world inputs -- no legal filesystem path produces a bare // or \\ with nothing following.

Convergence

Aligned. A's findings are accurate and appropriately weighted. The PR is a clean, well-tested refactor with limited blast radius. No blocking issues remain. Joint recommendation: approve.

…ate docs

Address Reviewer A on #2863:

- Guard the mid(rv, 3, len(rv) - 2) call in Helpers.normalizePath()
  against a count of 0 when rv is exactly "//" — cf. CLAUDE.md
  cross-engine invariant #8 (Left(str, 0) crashes Lucee 7; mid() with a
  zero count and out-of-range start may do the same). This PR introduced
  the mid() into the bootstrap path via delegation, so the guard sits on
  the path that actually runs. Output is unchanged for every real input.
- Add a regression spec for the degenerate "//" root.
- Broaden the Windows troubleshooting note so manual JAR installs get an
  update path too, not just Scoop.

Verified locally: 668/668 cli/lucli specs pass.

Signed-off-by: Peter Amiri <peter@alurium.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

The new commit (881ec428) directly addresses both findings from the prior round: the mid() guard is in place, the degenerate "//" spec was added, and the Windows troubleshooting note now covers both Scoop and manual JAR installs. No new issues found.

Correctness

Helpers.normalizePath() now reads:

var body = len(leading) ? (len(rv) > 2 ? mid(rv, 3, len(rv) - 2) : "") : rv;

Traced through all exercised cases:

  • "//server/share" — leading "//", body mid(rv,3,12) = "server/share", collapse no-op → "//server/share"
  • "//" — leading "//", len(rv) > 2 is false, body "", collapse no-op → "//"
  • "C:/Users/tim" — leading "", body = rv, collapse no-op → "C:/Users/tim"
  • "/a/b//c" — leading "", body = rv, reReplace("/{2,}", "/", "all")"/a/b/c"

Cross-engine invariant #8 is satisfied — the mid() with count 0 cannot be reached. ✓

One minor observation for the record: normalizePath() does more than the original replace() that $normalizePath() carried privately — it also collapses doubled slashes. This is intentional (the CHANGELOG entry calls it out), but the PR description's "no behavior change" phrasing is a slight simplification. In practice the six call sites in Module.cfc only receive real filesystem paths that won't carry doubled slashes, so the delta is a safe no-op for all real inputs.

Tests

668/668 specs pass. The new "handles a bare double-slash root without a mid() range error" case in HelpersSpec.cfc pins the exact edge that was guarded. ✓

Docs

The troubleshooting paragraph now covers both update paths:

scoop update wheels for Scoop installs, or re-fetch the latest Wheels Module (see Manual JAR install above)

Docs nit resolved. ✓

Commits

Both commits conform to commitlint.config.js: type refactor, scope cli, headers 73 and 77 chars, sentence-case subjects. Both carry Signed-off-by trailers. ✓

@wheels-bot
Copy link
Copy Markdown
Contributor

wheels-bot Bot commented Jun 5, 2026

Wheels Bot — Reviewer B (round 1)

A's review on the new SHA is clean. No sycophancy, no false positives, no missed issues. I re-traced the same code paths independently and A's logic holds.

Sycophancy

None detected. A traced specific code paths, ran the guard through four concrete cases, and noted the subtle "no behavior change" overstatement in the PR description rather than accepting the author's framing uncritically.

False positives

None. I traced all four cases A listed:

  • "//server/share"len(rv)=14 > 2, body = mid(rv,3,12) = "server/share", no-op collapse, result "//server/share"
  • "//"len(rv) > 2 is false, body = "", no-op collapse, result "//"
  • "C:/Users/tim" — leading is "", body = rv, no-op collapse, result unchanged ✓
  • "/a/b//c" — leading is "", body = rv, reReplace("/{2,}","/","all")"/a/b/c"

The left(rv, 2) call on a 1-char string is safe — CFML returns all available chars without throwing, and the == "//" comparison is false. The empty-string early return guards it anyway.

Missed issues

None detected. The PR is CLI-only code running on LuCLI's bundled Lucee — the cross-engine matrix does not apply. Tests exercise the real bootstrap path via delegation. Both commits carry Signed-off-by trailers. Commit headers at 73 and 77 chars are within the 100-char limit. PR title (73 chars, the squash-merge target) is a valid conventional-commit header.

A's observation that doubled-slash collapse is technically new behavior relative to the private replace() in #2835 is accurate, and A correctly concluded it is a safe no-op for all real filesystem paths reaching the six call sites.

Verdict alignment

A's APPROVED state is consistent with the findings. No blocking issues remain; the two minor prior-round nits are addressed.

Convergence

Aligned with A. The PR is a clean, well-tested refactor with limited blast radius. Joint recommendation: approve.

@bpamiri bpamiri merged commit 7e988fc into develop Jun 5, 2026
14 checks passed
@bpamiri bpamiri deleted the refactor/cli-unify-path-normalization branch June 5, 2026 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant