mirror of
https://github.com/github/spec-kit.git
synced 2026-07-03 12:28:06 +08:00
* fix: stop check-prerequisites --paths-only from writing feature.json (#3025) check-prerequisites --paths-only / -PathsOnly is documented as pure, read-only path resolution, but when SPECIFY_FEATURE_DIRECTORY was set it called the persist routine and rewrote .specify/feature.json. That dirtied the working tree and overwrote a pinned feature directory during what should be a no-op. Add an explicit opt-out at the resolver boundary instead of a global env back-channel: - bash: get_feature_paths accepts a leading --no-persist flag that skips _persist_feature_json; check-prerequisites.sh passes it in --paths-only mode. - PowerShell: Get-FeaturePathsEnv gains a -NoPersist switch that skips Save-FeatureJson; check-prerequisites.ps1 passes it in -PathsOnly mode. Normal (non-paths-only) invocations are unchanged and still persist the override, so future sessions without the env var keep working. Add regression tests asserting --paths-only/-PathsOnly leaves a pinned feature.json untouched even when the env override differs, plus a guard that normal mode still persists. * fix: use ASCII hyphen in common.ps1 comment for PS 5.1 compatibility The em-dash in the persist comment introduced non-ASCII bytes, failing test_ps1_file_is_ascii_only which enforces ASCII-only PowerShell sources for Windows PowerShell 5.1 compatibility. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: add PowerShell normal-mode persistence guard (#3025) Addresses Copilot review feedback on #3190: the bash side had a `test_normal_mode_still_persists_feature_json` guard, but there was no symmetric PowerShell test asserting that running check-prerequisites.ps1 *without* -PathsOnly still persists the SPECIFY_FEATURE_DIRECTORY override into .specify/feature.json. Add test_ps_normal_mode_still_persists_feature_json, which guards against accidentally passing -NoPersist unconditionally (or flipping the default) in a future refactor. Verified it fails when -NoPersist is passed in the non -PathsOnly branch and passes with the current conditional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -78,8 +78,14 @@ done
|
||||
SCRIPT_DIR="$(CDPATH="" cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
source "$SCRIPT_DIR/common.sh"
|
||||
|
||||
# Get feature paths
|
||||
_paths_output=$(get_feature_paths) || { echo "ERROR: Failed to resolve feature paths" >&2; exit 1; }
|
||||
# Get feature paths.
|
||||
# In --paths-only mode this is pure resolution, so pass --no-persist to opt out
|
||||
# of the feature.json write side effect (issue #3025).
|
||||
if $PATHS_ONLY; then
|
||||
_paths_output=$(get_feature_paths --no-persist) || { echo "ERROR: Failed to resolve feature paths" >&2; exit 1; }
|
||||
else
|
||||
_paths_output=$(get_feature_paths) || { echo "ERROR: Failed to resolve feature paths" >&2; exit 1; }
|
||||
fi
|
||||
eval "$_paths_output"
|
||||
unset _paths_output
|
||||
|
||||
|
||||
@@ -152,6 +152,15 @@ _persist_feature_json() {
|
||||
}
|
||||
|
||||
get_feature_paths() {
|
||||
# Read-only callers (e.g. check-prerequisites.sh --paths-only) pass
|
||||
# --no-persist so pure path resolution never writes .specify/feature.json,
|
||||
# which would dirty the working tree or overwrite a pinned value (issue #3025).
|
||||
local no_persist=false
|
||||
if [[ "${1:-}" == "--no-persist" ]]; then
|
||||
no_persist=true
|
||||
shift
|
||||
fi
|
||||
|
||||
# Split decl/assignment so a SPECIFY_INIT_DIR validation failure in
|
||||
# get_repo_root propagates as a hard error instead of being masked by `local`.
|
||||
local repo_root
|
||||
@@ -168,8 +177,11 @@ get_feature_paths() {
|
||||
feature_dir="$SPECIFY_FEATURE_DIRECTORY"
|
||||
# Normalize relative paths to absolute under repo root
|
||||
[[ "$feature_dir" != /* ]] && feature_dir="$repo_root/$feature_dir"
|
||||
# Persist to feature.json so future sessions without the env var still work
|
||||
_persist_feature_json "$repo_root" "$SPECIFY_FEATURE_DIRECTORY"
|
||||
# Persist to feature.json so future sessions without the env var still
|
||||
# work — unless the caller opted out for read-only resolution (#3025).
|
||||
if [[ "$no_persist" != true ]]; then
|
||||
_persist_feature_json "$repo_root" "$SPECIFY_FEATURE_DIRECTORY"
|
||||
fi
|
||||
elif [[ -f "$repo_root/.specify/feature.json" ]]; then
|
||||
local _fd
|
||||
_fd=$(read_feature_json_feature_directory "$repo_root")
|
||||
|
||||
@@ -56,8 +56,14 @@ EXAMPLES:
|
||||
# Source common functions
|
||||
. "$PSScriptRoot/common.ps1"
|
||||
|
||||
# Get feature paths
|
||||
$paths = Get-FeaturePathsEnv
|
||||
# Get feature paths.
|
||||
# In -PathsOnly mode this is pure resolution, so pass -NoPersist to opt out of
|
||||
# the feature.json write side effect (issue #3025).
|
||||
if ($PathsOnly) {
|
||||
$paths = Get-FeaturePathsEnv -NoPersist
|
||||
} else {
|
||||
$paths = Get-FeaturePathsEnv
|
||||
}
|
||||
|
||||
# If paths-only mode, output paths and exit (no validation)
|
||||
if ($PathsOnly) {
|
||||
|
||||
@@ -143,6 +143,13 @@ function Save-FeatureJson {
|
||||
}
|
||||
|
||||
function Get-FeaturePathsEnv {
|
||||
# Read-only callers (e.g. check-prerequisites.ps1 -PathsOnly) pass -NoPersist
|
||||
# so pure path resolution never writes .specify/feature.json, which would
|
||||
# dirty the working tree or overwrite a pinned value (issue #3025).
|
||||
param(
|
||||
[switch]$NoPersist
|
||||
)
|
||||
|
||||
$repoRoot = Get-RepoRoot
|
||||
$currentBranch = Get-CurrentBranch
|
||||
|
||||
@@ -157,8 +164,11 @@ function Get-FeaturePathsEnv {
|
||||
if (-not [System.IO.Path]::IsPathRooted($featureDir)) {
|
||||
$featureDir = Join-Path $repoRoot $featureDir
|
||||
}
|
||||
# Persist to feature.json so future sessions without the env var still work
|
||||
Save-FeatureJson -RepoRoot $repoRoot -FeatureDirectory $env:SPECIFY_FEATURE_DIRECTORY
|
||||
# Persist to feature.json so future sessions without the env var still
|
||||
# work - unless the caller opted out for read-only resolution (#3025).
|
||||
if (-not $NoPersist) {
|
||||
Save-FeatureJson -RepoRoot $repoRoot -FeatureDirectory $env:SPECIFY_FEATURE_DIRECTORY
|
||||
}
|
||||
} elseif (Test-Path $featureJson) {
|
||||
$featureJsonRaw = Get-Content -LiteralPath $featureJson -Raw
|
||||
try {
|
||||
|
||||
@@ -163,6 +163,66 @@ def test_normal_mode_still_validates_branch(prereq_repo: Path) -> None:
|
||||
assert result.stdout.strip() == ""
|
||||
|
||||
|
||||
@requires_bash
|
||||
def test_paths_only_does_not_persist_feature_json(prereq_repo: Path) -> None:
|
||||
"""--paths-only must not rewrite feature.json even when the env override
|
||||
differs from the pinned value (#3025).
|
||||
|
||||
Path resolution is read-only, so it must never dirty the working tree or
|
||||
overwrite the persisted feature directory.
|
||||
"""
|
||||
pinned = "specs/001-my-feature"
|
||||
(prereq_repo / "specs" / "001-my-feature").mkdir(parents=True, exist_ok=True)
|
||||
(prereq_repo / "specs" / "002-other").mkdir(parents=True, exist_ok=True)
|
||||
_write_feature_json(prereq_repo, pinned)
|
||||
fj = prereq_repo / ".specify" / "feature.json"
|
||||
before = fj.read_text(encoding="utf-8")
|
||||
|
||||
script = prereq_repo / ".specify" / "scripts" / "bash" / "check-prerequisites.sh"
|
||||
env = _clean_env()
|
||||
env["SPECIFY_FEATURE_DIRECTORY"] = "specs/002-other"
|
||||
result = subprocess.run(
|
||||
["bash", str(script), "--json", "--paths-only"],
|
||||
cwd=prereq_repo,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
env=env,
|
||||
)
|
||||
assert result.returncode == 0, result.stderr
|
||||
# The override is honored in the output...
|
||||
data = json.loads(result.stdout)
|
||||
assert "002-other" in data["FEATURE_DIR"]
|
||||
# ...but the pinned file on disk is untouched.
|
||||
assert fj.read_text(encoding="utf-8") == before
|
||||
|
||||
|
||||
@requires_bash
|
||||
def test_normal_mode_still_persists_feature_json(prereq_repo: Path) -> None:
|
||||
"""Without --paths-only, the env override is still persisted to feature.json,
|
||||
so the --no-persist opt-out does not regress normal write behavior (#3025)."""
|
||||
(prereq_repo / "specs" / "001-my-feature").mkdir(parents=True, exist_ok=True)
|
||||
feat = prereq_repo / "specs" / "002-other"
|
||||
feat.mkdir(parents=True, exist_ok=True)
|
||||
(feat / "plan.md").write_text("# plan\n", encoding="utf-8")
|
||||
_write_feature_json(prereq_repo, "specs/001-my-feature")
|
||||
fj = prereq_repo / ".specify" / "feature.json"
|
||||
|
||||
script = prereq_repo / ".specify" / "scripts" / "bash" / "check-prerequisites.sh"
|
||||
env = _clean_env()
|
||||
env["SPECIFY_FEATURE_DIRECTORY"] = "specs/002-other"
|
||||
result = subprocess.run(
|
||||
["bash", str(script), "--json"],
|
||||
cwd=prereq_repo,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
env=env,
|
||||
)
|
||||
assert result.returncode == 0, result.stderr
|
||||
assert json.loads(fj.read_text(encoding="utf-8"))["feature_directory"] == "specs/002-other"
|
||||
|
||||
|
||||
# ── PowerShell tests ──────────────────────────────────────────────────────
|
||||
|
||||
|
||||
@@ -283,3 +343,64 @@ def test_ps_missing_tasks_error_goes_to_stderr(prereq_repo: Path) -> None:
|
||||
assert "tasks.md not found" in result.stderr
|
||||
assert "tasks.md not found" not in result.stdout
|
||||
assert result.stdout.strip() == ""
|
||||
|
||||
|
||||
@pytest.mark.skipif(not (HAS_PWSH or _WINDOWS_POWERSHELL), reason="no PowerShell available")
|
||||
def test_ps_paths_only_does_not_persist_feature_json(prereq_repo: Path) -> None:
|
||||
"""-PathsOnly must not rewrite feature.json even when the env override
|
||||
differs from the pinned value (#3025)."""
|
||||
pinned = "specs/001-my-feature"
|
||||
(prereq_repo / "specs" / "001-my-feature").mkdir(parents=True, exist_ok=True)
|
||||
(prereq_repo / "specs" / "002-other").mkdir(parents=True, exist_ok=True)
|
||||
_write_feature_json(prereq_repo, pinned)
|
||||
fj = prereq_repo / ".specify" / "feature.json"
|
||||
before = fj.read_text(encoding="utf-8")
|
||||
|
||||
script = prereq_repo / ".specify" / "scripts" / "powershell" / "check-prerequisites.ps1"
|
||||
exe = "pwsh" if HAS_PWSH else _WINDOWS_POWERSHELL
|
||||
env = _clean_env()
|
||||
env["SPECIFY_FEATURE_DIRECTORY"] = "specs/002-other"
|
||||
result = subprocess.run(
|
||||
[exe, "-NoProfile", "-File", str(script), "-Json", "-PathsOnly"],
|
||||
cwd=prereq_repo,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
env=env,
|
||||
)
|
||||
assert result.returncode == 0, result.stderr
|
||||
data = json.loads(result.stdout)
|
||||
assert "002-other" in data["FEATURE_DIR"]
|
||||
assert fj.read_text(encoding="utf-8") == before
|
||||
|
||||
|
||||
@pytest.mark.skipif(not (HAS_PWSH or _WINDOWS_POWERSHELL), reason="no PowerShell available")
|
||||
def test_ps_normal_mode_still_persists_feature_json(prereq_repo: Path) -> None:
|
||||
"""Without -PathsOnly, the env override is still persisted to feature.json,
|
||||
so the -NoPersist opt-out does not regress normal write behavior (#3025).
|
||||
|
||||
Symmetric to the bash test_normal_mode_still_persists_feature_json guard:
|
||||
asserts the default path still persists and that -NoPersist is not passed
|
||||
unconditionally.
|
||||
"""
|
||||
(prereq_repo / "specs" / "001-my-feature").mkdir(parents=True, exist_ok=True)
|
||||
feat = prereq_repo / "specs" / "002-other"
|
||||
feat.mkdir(parents=True, exist_ok=True)
|
||||
(feat / "plan.md").write_text("# plan\n", encoding="utf-8")
|
||||
_write_feature_json(prereq_repo, "specs/001-my-feature")
|
||||
fj = prereq_repo / ".specify" / "feature.json"
|
||||
|
||||
script = prereq_repo / ".specify" / "scripts" / "powershell" / "check-prerequisites.ps1"
|
||||
exe = "pwsh" if HAS_PWSH else _WINDOWS_POWERSHELL
|
||||
env = _clean_env()
|
||||
env["SPECIFY_FEATURE_DIRECTORY"] = "specs/002-other"
|
||||
result = subprocess.run(
|
||||
[exe, "-NoProfile", "-File", str(script), "-Json"],
|
||||
cwd=prereq_repo,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
env=env,
|
||||
)
|
||||
assert result.returncode == 0, result.stderr
|
||||
assert json.loads(fj.read_text(encoding="utf-8"))["feature_directory"] == "specs/002-other"
|
||||
|
||||
Reference in New Issue
Block a user