mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(scripts): use a .NET Framework-safe trim in the PowerShell init-dir resolver (#3872)
`Resolve-SpecifyInitDir` normalized the resolved path with
`[System.IO.Path]::TrimEndingDirectorySeparator`, which is .NET Core only.
Windows PowerShell 5.1 runs on .NET Framework, so on every 5.1 host the call
throws at that line and root resolution fails before the requested command runs:
$ $env:SPECIFY_INIT_DIR = "C:\repo\web"
$ .specify\scripts\powershell\check-prerequisites.ps1 -Json
check-prerequisites.ps1 : Method invocation failed because
[System.IO.Path] does not contain a method named
'TrimEndingDirectorySeparator'.
The same file already documents this exact incompatibility and avoids it
correctly in `Get-FeaturePathsEnv` (~150 lines below), which uses `TrimEnd`
with a comment naming `TrimEndingDirectorySeparator` as .NET Core only.
Worse than a clean failure when the resolver is called directly: the throw is
non-terminating, so `$initRoot` stays `$null` and the very next `Join-Path`
throws too, `Get-RepoRoot` returns `$null`, and the shell exits **0**. A caller
that checks the exit code sees success with an empty root.
Switched to the `TrimEnd('/', '\')` the file already endorses. Note the obvious
swap is not quite enough on its own: a bare `TrimEnd` turns `C:\` into `C:`,
which is not the drive root but a drive-relative reference that later path APIs
re-resolve against the *current directory* — so validation would probe the wrong
tree and, from a cwd that happens to contain `.specify/`, could silently accept
`C:` as the project root. A `GetPathRoot` length check keeps a path that is its
own root intact. Both `GetPathRoot` and `TrimEnd` exist on .NET Framework.
Trailing-separator trimming (the reason the call was there — bash's `cd && pwd`
never yields one, so the two resolvers must agree) is unchanged, as are all
error paths and messages.
Tests in `tests/test_init_dir.py`:
- A static check that no shipped `.ps1` calls a .NET Core-only
`[System.IO.Path]` member (`TrimEndingDirectorySeparator`,
`EndsInDirectorySeparator`, `GetRelativePath`, `Join`). This one runs on all
platforms and is what actually guards CI: the matrix runs the PowerShell
tests under `pwsh`, which is .NET Core, so a .NET Framework-only regression
is otherwise invisible to it. Anchored to the `Path` type so `[string]::Join`
is not flagged.
- Two runtime tests under `powershell.exe` specifically (never `pwsh`),
covering resolution and trailing-separator parity.
- A drive-root test asserting the reported root survives the trim intact.
Test-the-test: reverting the source change fails all four (the runtime pair
with the `does not contain a method named` throw, the static check by locating
the call). Applying only the naive `TrimEnd` fails the drive-root test, which
reports `C:` instead of `C:\`. Verified on Windows PowerShell 5.1.19041.6456,
including the previously-crashing `check-prerequisites.ps1 -Json` end to end.
Also fixes six pre-existing `test_ps_*` failures on 5.1-only hosts, which were
this bug rather than test-harness issues.
Fixes #3749
Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -11,6 +11,7 @@ See proposals/monorepo-support and github/spec-kit discussion #2834.
|
||||
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
@@ -30,6 +31,11 @@ HAS_PWSH = shutil.which("pwsh") is not None
|
||||
_POWERSHELL = shutil.which("powershell.exe") or shutil.which("powershell")
|
||||
_PS_EXE = "pwsh" if HAS_PWSH else _POWERSHELL
|
||||
|
||||
# Windows PowerShell 5.1 (.NET Framework) specifically, never pwsh: the only
|
||||
# host that lacks the .NET Core-only [System.IO.Path] members, so it is the only
|
||||
# one that can pin a 5.1 compatibility regression (issue #3749).
|
||||
_WINDOWS_POWERSHELL = _POWERSHELL if os.name == "nt" else None
|
||||
|
||||
|
||||
def _clean_env() -> dict[str, str]:
|
||||
"""Inherited env minus all SPECIFY_* vars, so a developer/CI override
|
||||
@@ -101,6 +107,10 @@ requires_pwsh = pytest.mark.skipif(
|
||||
not (HAS_PWSH or _POWERSHELL), reason="no PowerShell available"
|
||||
)
|
||||
|
||||
requires_windows_powershell = pytest.mark.skipif(
|
||||
_WINDOWS_POWERSHELL is None, reason="Windows PowerShell 5.1 not available"
|
||||
)
|
||||
|
||||
|
||||
# ── Bash: positive cases ────────────────────────────────────────────────────
|
||||
|
||||
@@ -465,3 +475,131 @@ def test_ps_file_path_errors_no_fallback(tmp_path: Path) -> None:
|
||||
result = _ps("Get-RepoRoot", cwd=web, env=env)
|
||||
assert result.returncode != 0
|
||||
assert "does not point to an existing directory" in result.stderr
|
||||
|
||||
|
||||
# ── Windows PowerShell 5.1 compatibility (issue #3749) ──────────────────────
|
||||
#
|
||||
# The CI matrix runs these PowerShell tests under `pwsh` on every OS, and pwsh
|
||||
# is .NET Core, so a .NET Framework-only regression is invisible to it. The
|
||||
# static test below therefore runs everywhere and is the one that actually
|
||||
# guards CI; the runtime tests pin the real behavior where a 5.1 host exists.
|
||||
|
||||
# .NET Core-only [System.IO.Path] members. Absent on .NET Framework, so calling
|
||||
# one throws "does not contain a method named ..." on Windows PowerShell 5.1.
|
||||
_DOTNET_CORE_ONLY_PATH_MEMBERS = (
|
||||
"TrimEndingDirectorySeparator",
|
||||
"EndsInDirectorySeparator",
|
||||
"GetRelativePath",
|
||||
"Join",
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("member", _DOTNET_CORE_ONLY_PATH_MEMBERS)
|
||||
def test_shipped_ps1_avoids_dotnet_core_only_path_members(member: str) -> None:
|
||||
"""No shipped .ps1 may call a .NET Core-only [System.IO.Path] member.
|
||||
|
||||
Windows PowerShell 5.1 ships on every Windows box and runs on .NET
|
||||
Framework, where these members do not exist. A call is not a graceful
|
||||
degradation but a hard "Method invocation failed" at the call site, which
|
||||
for a root resolver aborts the command before it starts.
|
||||
|
||||
Runs on all platforms because the CI matrix only has pwsh (.NET Core),
|
||||
where such a call works fine -- so this static check is what keeps CI able
|
||||
to catch the regression at all.
|
||||
"""
|
||||
# Anchored to the Path type: [string]::Join and other same-named members on
|
||||
# .NET Framework types are unaffected and must not be flagged.
|
||||
pattern = re.compile(
|
||||
r"\[(?:System\.IO\.)?Path\]::" + re.escape(member) + r"\s*\(",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
offenders = []
|
||||
for ps1 in sorted(PROJECT_ROOT.glob("scripts/powershell/*.ps1")) + sorted(
|
||||
PROJECT_ROOT.glob("extensions/*/scripts/powershell/*.ps1")
|
||||
):
|
||||
for lineno, line in enumerate(
|
||||
ps1.read_text(encoding="utf-8").splitlines(), start=1
|
||||
):
|
||||
code = line.split("#", 1)[0]
|
||||
if pattern.search(code):
|
||||
offenders.append(f"{ps1.relative_to(PROJECT_ROOT)}:{lineno}")
|
||||
assert not offenders, (
|
||||
f"[System.IO.Path]::{member}() is .NET Core only and throws on Windows "
|
||||
f"PowerShell 5.1; found at {offenders}. Use a .NET Framework-safe "
|
||||
f"equivalent (e.g. TrimEnd('/', '\\') for a trailing separator)."
|
||||
)
|
||||
|
||||
|
||||
@requires_windows_powershell
|
||||
def test_ps51_init_dir_resolves(tmp_path: Path) -> None:
|
||||
"""SPECIFY_INIT_DIR must resolve under Windows PowerShell 5.1 (issue #3749).
|
||||
|
||||
Before the fix, Resolve-SpecifyInitDir called the .NET Core-only
|
||||
[System.IO.Path]::TrimEndingDirectorySeparator, so every 5.1 invocation
|
||||
threw at that line -- root resolution failed before the requested command
|
||||
ran, and $initRoot stayed $null so the very next Join-Path threw too.
|
||||
"""
|
||||
web = _make_project(tmp_path, "web")
|
||||
env = {**_clean_env(), "SPECIFY_INIT_DIR": str(web)}
|
||||
result = subprocess.run(
|
||||
[_WINDOWS_POWERSHELL, "-NoProfile", "-Command", f'. "{COMMON_PS}"; Get-RepoRoot'],
|
||||
cwd=tmp_path,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
env=env,
|
||||
)
|
||||
assert "does not contain a method named" not in result.stderr, result.stderr
|
||||
assert result.returncode == 0, result.stderr
|
||||
assert result.stdout.strip() == str(web)
|
||||
|
||||
|
||||
@requires_windows_powershell
|
||||
def test_ps51_init_dir_trailing_separator_trimmed(tmp_path: Path) -> None:
|
||||
"""The 5.1-safe trim must still strip a trailing separator, for bash parity.
|
||||
|
||||
Resolve-Path echoes back the input's trailing separator; the bash resolver's
|
||||
`cd && pwd` never yields one, so the two must agree.
|
||||
"""
|
||||
web = _make_project(tmp_path, "web")
|
||||
for suffix in ("/", "\\"):
|
||||
env = {**_clean_env(), "SPECIFY_INIT_DIR": f"{web}{suffix}"}
|
||||
result = subprocess.run(
|
||||
[
|
||||
_WINDOWS_POWERSHELL,
|
||||
"-NoProfile",
|
||||
"-Command",
|
||||
f'. "{COMMON_PS}"; Get-RepoRoot',
|
||||
],
|
||||
cwd=tmp_path,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
env=env,
|
||||
)
|
||||
assert result.returncode == 0, result.stderr
|
||||
assert result.stdout.strip() == str(web)
|
||||
|
||||
|
||||
@requires_pwsh
|
||||
def test_ps_drive_root_reports_root_not_bare_drive(tmp_path: Path) -> None:
|
||||
"""A path that IS its own root must survive the trim intact.
|
||||
|
||||
A bare TrimEnd('/', '\\') -- the obvious 5.1-safe swap -- turns 'C:\\' into
|
||||
'C:', which is not the drive root but a drive-relative reference that every
|
||||
later path API re-resolves against the *current directory*. Validation would
|
||||
then probe the wrong tree entirely and, on a cwd that happens to contain
|
||||
.specify/, silently accept 'C:' as the project root. The drive root
|
||||
normally has no .specify/, so assert on the error naming the intact root.
|
||||
"""
|
||||
root = Path(tmp_path.anchor or "/")
|
||||
if (root / ".specify").exists():
|
||||
pytest.skip("filesystem root is itself a Spec Kit project")
|
||||
env = {**_clean_env(), "SPECIFY_INIT_DIR": str(root)}
|
||||
result = _ps("Get-RepoRoot", cwd=tmp_path, env=env)
|
||||
assert result.returncode != 0
|
||||
assert "not a Spec Kit project" in result.stderr
|
||||
# The error echoes the resolved root, so it pins what the trim produced:
|
||||
# 'C:\' (or '/') intact, never the bare 'C:' (or '') a naive TrimEnd leaves.
|
||||
reported = result.stderr.replace("\r", "").rstrip("\n").split("directory): ", 1)[-1]
|
||||
assert reported == str(root)
|
||||
|
||||
Reference in New Issue
Block a user