diff --git a/scripts/bash/create-new-feature.sh b/scripts/bash/create-new-feature.sh index 06681d9b6e..6a4ad083a2 100644 --- a/scripts/bash/create-new-feature.sh +++ b/scripts/bash/create-new-feature.sh @@ -176,6 +176,21 @@ fit_branch_name() { printf '%s' "$branch_name" } +# After a lost exclusive mkdir, pick the next sequential number. +rescan_sequential_feature() { + local highest + highest=$(get_highest_from_specs "$SPECS_DIR") + if [ "$highest" -eq "$MAX_FEATURE_NUMBER" ]; then + echo "Error: feature number must be between 0 and $MAX_FEATURE_NUMBER, got '9223372036854775808'" >&2 + exit 1 + fi + BRANCH_NUMBER=$((highest + 1)) + FEATURE_NUM=$(printf "%03d" "$((10#$BRANCH_NUMBER))") + BRANCH_NAME=$(fit_branch_name "$FEATURE_NUM" "$BRANCH_SUFFIX") + FEATURE_DIR="$SPECS_DIR/$BRANCH_NAME" + SPEC_FILE="$FEATURE_DIR/spec.md" +} + # Quote a value for POSIX shell reuse, byte-identical to Python's shlex.quote # so the persistence hints match the Python variant exactly (printf %q output # differs between bash versions and from shlex.quote for spaces/metachars). @@ -343,14 +358,32 @@ FEATURE_DIR="$SPECS_DIR/$BRANCH_NAME" SPEC_FILE="$FEATURE_DIR/spec.md" if [ "$DRY_RUN" != true ]; then - if [ -d "$FEATURE_DIR" ] && [ "$ALLOW_EXISTING" != true ]; then + # Exclusive create first so a lost race rescans instead of exiting on the + # pre-mkdir exists check, and so NEEDS_SPEC is derived for the final + # FEATURE_DIR (not a path abandoned after retry). + RESERVED_NEW_DIR=false + while true; do + if [ "$ALLOW_EXISTING" = true ] && [ -d "$FEATURE_DIR" ]; then + RESERVED_NEW_DIR=false + break + fi + if mkdir "$FEATURE_DIR" 2>/dev/null; then + RESERVED_NEW_DIR=true + break + fi + if [ ! -d "$FEATURE_DIR" ]; then + echo "Error: could not create feature directory '$FEATURE_DIR'" >&2 + exit 1 + fi + if [ "$ALLOW_EXISTING" = true ]; then + break + fi if [ "$USE_TIMESTAMP" = true ]; then >&2 echo "Error: Feature directory '$FEATURE_DIR' already exists. Rerun to get a new timestamp or use a different --short-name." - else - >&2 echo "Error: Feature directory '$FEATURE_DIR' already exists. Please use a different feature name or specify a different number with --number." + exit 1 fi - exit 1 - fi + rescan_sequential_feature + done NEEDS_SPEC=false SPEC_TEMPLATE_FOUND=false @@ -363,13 +396,14 @@ if [ "$DRY_RUN" != true ]; then else resolve_status=$? if [ "$resolve_status" -ne 1 ]; then + if [ "$RESERVED_NEW_DIR" = true ]; then + rmdir "$FEATURE_DIR" 2>/dev/null || true + fi exit "$resolve_status" fi fi fi - mkdir -p "$FEATURE_DIR" - if [ "$NEEDS_SPEC" = true ]; then if [ "$SPEC_TEMPLATE_FOUND" = true ]; then printf '%s' "$SPEC_TEMPLATE_CONTENT" > "$SPEC_FILE" diff --git a/scripts/powershell/create-new-feature.ps1 b/scripts/powershell/create-new-feature.ps1 index 9ce2c678a4..cb45999d91 100644 --- a/scripts/powershell/create-new-feature.ps1 +++ b/scripts/powershell/create-new-feature.ps1 @@ -262,23 +262,82 @@ $featureDir = Join-Path $specsDir $branchName $specFile = Join-Path $featureDir 'spec.md' if (-not $DryRun) { - if ((Test-Path -LiteralPath $featureDir -PathType Container) -and -not $AllowExistingBranch) { - if ($Timestamp) { - Write-Error "Error: Feature directory '$featureDir' already exists. Rerun to get a new timestamp or use a different -ShortName." - } else { - Write-Error "Error: Feature directory '$featureDir' already exists. Please use a different feature name or specify a different number with -Number." + # Exclusive create first so a lost race rescans instead of exiting on the + # pre-create exists check, and so $needsSpec is derived for the final + # FEATURE_DIR (not a path abandoned after retry). + $reservedNewDir = $false + while ($true) { + if ($AllowExistingBranch -and (Test-Path -LiteralPath $featureDir -PathType Container)) { + $reservedNewDir = $false + break + } + # Test-only barrier: pause before New-Item when SPECKIT_MKDIR_BARRIER is + # set so concurrent reservations can be forced (mirrors the bash mkdir + # PATH wrapper / Python sitecustomize gate). + if ($env:SPECKIT_MKDIR_BARRIER) { + $barrierRoot = $env:SPECKIT_MKDIR_BARRIER + $released = Join-Path $barrierRoot 'released' + $nWaiters = 2 + if ($env:SPECKIT_MKDIR_BARRIER_N) { + [void][int]::TryParse($env:SPECKIT_MKDIR_BARRIER_N, [ref]$nWaiters) + } + $leaf = Split-Path -Leaf $featureDir + $parentLeaf = Split-Path -Leaf (Split-Path -Parent $featureDir) + if ($parentLeaf -eq 'specs' -and $leaf -match '^\d' -and -not (Test-Path -LiteralPath $released)) { + $waiter = Join-Path $barrierRoot ("w-ps-{0}-{1}" -f $PID, [DateTime]::UtcNow.Ticks) + [void][System.IO.Directory]::CreateDirectory($waiter) + for ($i = 0; $i -lt 200; $i++) { + if (Test-Path -LiteralPath $released) { break } + $count = @(Get-ChildItem -LiteralPath $barrierRoot -Directory -Filter 'w-*' -ErrorAction SilentlyContinue).Count + if ($count -ge $nWaiters) { + [void][System.IO.File]::WriteAllText($released, '') + break + } + Start-Sleep -Milliseconds 50 + } + } + } + try { + New-Item -ItemType Directory -Path $featureDir | Out-Null + $reservedNewDir = $true + break + } catch { + if (-not (Test-Path -LiteralPath $featureDir -PathType Container)) { + throw + } + if ($AllowExistingBranch) { + break + } + if ($Timestamp) { + Write-Error "Error: Feature directory '$featureDir' already exists. Rerun to get a new timestamp or use a different -ShortName." + exit 1 + } + $highestNumber = Get-HighestNumberFromSpecs -SpecsDir $specsDir + if ($highestNumber -eq [long]::MaxValue) { + Write-Error "Error: feature number must be between 0 and $([long]::MaxValue), got '9223372036854775808'" + exit 1 + } + $resolvedNumber = $highestNumber + 1 + $featureNum = ('{0:000}' -f $resolvedNumber) + $branchName = Get-FittedBranchName -FeatureNum $featureNum -BranchSuffix $branchSuffix + $featureDir = Join-Path $specsDir $branchName + $specFile = Join-Path $featureDir 'spec.md' } - exit 1 } $needsSpec = -not (Test-Path -PathType Leaf $specFile) $content = $null if ($needsSpec) { - $content = Resolve-TemplateContent -TemplateName 'spec-template' -RepoRoot $repoRoot + try { + $content = Resolve-TemplateContent -TemplateName 'spec-template' -RepoRoot $repoRoot + } catch { + if ($reservedNewDir -and (Test-Path -LiteralPath $featureDir -PathType Container)) { + Remove-Item -LiteralPath $featureDir -Force -ErrorAction SilentlyContinue + } + throw + } } - New-Item -ItemType Directory -Path $featureDir -Force | Out-Null - if ($needsSpec) { if ($null -ne $content) { $utf8NoBom = New-Object System.Text.UTF8Encoding($false) diff --git a/scripts/python/create_new_feature.py b/scripts/python/create_new_feature.py index f36064afbb..2b13032b58 100644 --- a/scripts/python/create_new_feature.py +++ b/scripts/python/create_new_feature.py @@ -367,21 +367,51 @@ def main(argv: list[str] | None = None) -> int: spec_file = feature_dir / "spec.md" if not args.dry_run: - if feature_dir.is_dir() and not args.allow_existing: - if args.use_timestamp: - print( - f"Error: Feature directory '{feature_dir}' already exists. " - "Rerun to get a new timestamp or use a different --short-name.", - file=sys.stderr, - ) - else: - print( - f"Error: Feature directory '{feature_dir}' already exists. " - "Please use a different feature name or specify a different " - "number with --number.", - file=sys.stderr, - ) - return 1 + # Exclusive create first so a lost race rescans instead of exiting on the + # pre-mkdir exists check, and so needs_spec is derived for the final + # FEATURE_DIR (not a path abandoned after retry). + reserved_new_dir = False + while True: + if args.allow_existing and feature_dir.is_dir(): + reserved_new_dir = False + break + try: + feature_dir.mkdir(parents=True) + reserved_new_dir = True + break + except FileExistsError: + # FileExistsError also means a regular file (or non-dir) occupies + # the path. Reject that before treat-as-reusable / rescan — a + # non-dir is never a feature directory, and rescan would loop + # forever because _get_highest_from_specs only counts dirs. + if not feature_dir.is_dir(): + print( + f"Error: could not create feature directory " + f"'{feature_dir}'", + file=sys.stderr, + ) + return 1 + if args.allow_existing: + break + if args.use_timestamp: + print( + f"Error: Feature directory '{feature_dir}' already exists. " + "Rerun to get a new timestamp or use a different --short-name.", + file=sys.stderr, + ) + return 1 + number = _get_highest_from_specs(specs_dir) + 1 + if number > _MAX_FEATURE_NUMBER: + print( + f"Error: feature number must be between 0 and " + f"{_MAX_FEATURE_NUMBER}, got '{number}'", + file=sys.stderr, + ) + return 1 + feature_num = f"{number:03d}" + branch_name = _fit_branch_name(feature_num, branch_suffix) + feature_dir = specs_dir / branch_name + spec_file = feature_dir / "spec.md" template_content = None needs_spec = not spec_file.is_file() @@ -392,10 +422,13 @@ def main(argv: list[str] | None = None) -> int: ) except TemplateResolutionError as exc: print(f"Error: {exc}", file=sys.stderr) + if reserved_new_dir: + try: + feature_dir.rmdir() + except OSError: + pass return 1 - feature_dir.mkdir(parents=True, exist_ok=True) - if needs_spec: if template_content is not None: spec_file.write_bytes(template_content.encode("utf-8")) diff --git a/tests/test_create_new_feature_python_parity.py b/tests/test_create_new_feature_python_parity.py index 6cc50d80eb..28d0d546e8 100644 --- a/tests/test_create_new_feature_python_parity.py +++ b/tests/test_create_new_feature_python_parity.py @@ -2,7 +2,10 @@ from __future__ import annotations +import os import re +import shutil +import subprocess from pathlib import Path import pytest @@ -1255,3 +1258,274 @@ def test_no_ascii_word_description_matches_across_twins(tmp_path: Path): json_stdout(ps)["BRANCH_NAME"], } assert names == {"001-"}, names + + +def _shared_specs_repos(tmp_path: Path) -> tuple[Path, Path, Path]: + """Two project roots that share one specs/ directory via symlink.""" + repo_a = _setup_repo(tmp_path, "proj-a") + repo_b = _setup_repo(tmp_path, "proj-b") + shared = tmp_path / "shared-specs" + shared.mkdir() + try: + (repo_a / "specs").symlink_to(shared, target_is_directory=True) + (repo_b / "specs").symlink_to(shared, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("Symlinks are not available in this environment") + (repo_a / ".specify" / "templates" / "spec-template.md").write_text( + "ALPHA-SPEC\n", encoding="utf-8" + ) + (repo_b / ".specify" / "templates" / "spec-template.md").write_text( + "BETA-SPEC\n", encoding="utf-8" + ) + return repo_a, repo_b, shared + + +def _mkdir_barrier_env(tmp_path: Path, n_waiters: int = 2) -> dict[str, str]: + """Pause FEATURE_DIR mkdir until n_waiters arrive so the TOCTOU window is forced. + + Two processes that have already scanned the same max spec number then both + call mkdir / New-Item on the same specs/NNN-name path. Non-exclusive create + lets both succeed and the second write overwrites spec.md. + + Bash is gated via a PATH mkdir wrapper; Python via sitecustomize on + os.mkdir; PowerShell via the SPECKIT_MKDIR_BARRIER hook in + create-new-feature.ps1. + """ + env = clean_env() + barrier = tmp_path / "mkdir-barrier" + barrier.mkdir() + released = barrier / "released" + real_mkdir = shutil.which("mkdir") + if real_mkdir is None: + pytest.skip("mkdir not available") + bin_dir = tmp_path / "mkdir-bin" + bin_dir.mkdir() + wrapper = bin_dir / "mkdir" + wrapper.write_text( + f"""#!/bin/sh +path_last="" +for arg in "$@"; do + path_last="$arg" +done +base=$(basename -- "$path_last") +parent=$(basename -- "$(dirname -- "$path_last")") +case "$parent/$base" in + specs/[0-9]*-*) + if [ ! -f "{released}" ]; then + i=0 + while ! mkdir "{barrier}/w-$i" 2>/dev/null; do + i=$((i + 1)) + if [ "$i" -gt 100 ]; then + break + fi + done + n=0 + while [ "$n" -lt 200 ]; do + count=$(ls -d "{barrier}"/w-* 2>/dev/null | wc -l) + if [ "$count" -ge {n_waiters} ]; then + touch "{released}" + break + fi + if [ -f "{released}" ]; then + break + fi + sleep 0.05 + n=$((n + 1)) + done + fi + ;; +esac +exec "{real_mkdir}" "$@" +""", + encoding="utf-8", + ) + wrapper.chmod(0o755) + env["PATH"] = f"{bin_dir}{os.pathsep}{env.get('PATH', '')}" + env["SPECKIT_MKDIR_BARRIER"] = str(barrier) + env["SPECKIT_MKDIR_BARRIER_N"] = str(n_waiters) + site = tmp_path / "mkdir-site" + site.mkdir() + (site / "sitecustomize.py").write_text( + """ +import os +import time +from pathlib import Path + +_orig_mkdir = os.mkdir +_barrier = Path(os.environ["SPECKIT_MKDIR_BARRIER"]) +_released = _barrier / "released" +_n_waiters = int(os.environ.get("SPECKIT_MKDIR_BARRIER_N", "2")) + + +def _is_feature_dir(path): + p = Path(path) + return p.parent.name == "specs" and "-" in p.name and p.name[0:1].isdigit() + + +def _gated_mkdir(path, mode=0o777, *args, **kwargs): + if _is_feature_dir(path) and not _released.exists(): + (_barrier / f"w-py-{os.getpid()}-{time.time_ns()}").mkdir(exist_ok=True) + for _ in range(200): + if _released.exists(): + break + waiters = list(_barrier.glob("w-*")) + if len(waiters) >= _n_waiters: + _released.touch() + break + time.sleep(0.05) + return _orig_mkdir(path, mode, *args, **kwargs) + + +os.mkdir = _gated_mkdir +""", + encoding="utf-8", + ) + env["PYTHONPATH"] = f"{site}{os.pathsep}{env.get('PYTHONPATH', '')}" + return env + + +def _popen( + cmd: list[str], repo: Path, env: dict[str, str] +) -> subprocess.Popen[str]: + return subprocess.Popen( + cmd, + cwd=repo, + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + + +def _wait( + proc: subprocess.Popen[str], cmd: list[str] +) -> subprocess.CompletedProcess[str]: + stdout, stderr = proc.communicate(timeout=20) + return subprocess.CompletedProcess(cmd, proc.returncode, stdout, stderr) + + +@requires_bash +@pytest.mark.parametrize( + "variant", + [ + "bash", + "python", + pytest.param( + "powershell", + marks=pytest.mark.skipif( + not HAS_POWERSHELL, reason="no PowerShell available" + ), + ), + ], +) +def test_concurrent_reservations_do_not_share_spec_directory( + tmp_path: Path, variant: str +) -> None: + """Two concurrent reservations must not land in the same FEATURE_DIR. + + create-new-feature scans max(specs)+1 then reserves FEATURE_DIR with an + exclusive mkdir / New-Item (no -Force) / Path.mkdir (no exist_ok). A lost + race must rescan to the next number so the second writer cannot overwrite + the first spec.md — including the PowerShell New-Item/catch path. + """ + repo_a, repo_b, shared = _shared_specs_repos(tmp_path) + env = _mkdir_barrier_env(tmp_path) + args = ("--json", "--short-name", "user-auth", "Add user authentication") + if variant == "bash": + cmd_a = bash_cmd(repo_a, SCRIPT, *args) + cmd_b = bash_cmd(repo_b, SCRIPT, *args) + elif variant == "powershell": + cmd_a = ps_cmd(repo_a, SCRIPT, *args) + cmd_b = ps_cmd(repo_b, SCRIPT, *args) + else: + cmd_a = py_cmd(repo_a, SCRIPT, *args) + cmd_b = py_cmd(repo_b, SCRIPT, *args) + + proc_a = _popen(cmd_a, repo_a, env) + proc_b = _popen(cmd_b, repo_b, env) + result_a = _wait(proc_a, cmd_a) + result_b = _wait(proc_b, cmd_b) + + assert result_a.returncode == 0, result_a.stderr + assert result_b.returncode == 0, result_b.stderr + + data_a = json_stdout(result_a) + data_b = json_stdout(result_b) + assert data_a["BRANCH_NAME"] != data_b["BRANCH_NAME"] + assert {data_a["FEATURE_NUM"], data_b["FEATURE_NUM"]} == {"001", "002"} + + names = sorted(path.name for path in shared.iterdir() if path.is_dir()) + assert names == ["001-user-auth", "002-user-auth"] + contents = { + (shared / name / "spec.md").read_text(encoding="utf-8") for name in names + } + assert contents == {"ALPHA-SPEC\n", "BETA-SPEC\n"} + + +@pytest.mark.parametrize("variant", ["bash", "python"]) +def test_nondirectory_occupies_feature_path_is_rejected( + repo: Path, variant: str +) -> None: + """A regular file at the feature path must not be treated as reusable.""" + specs = repo / "specs" + specs.mkdir(parents=True, exist_ok=True) + (specs / "001-user-auth").write_text("not-a-directory\n", encoding="utf-8") + + args = ("--json", "--short-name", "user-auth", "Add user authentication") + if variant == "bash": + result = run(bash_cmd(repo, SCRIPT, *args), repo) + else: + result = run(py_cmd(repo, SCRIPT, *args), repo) + + assert result.returncode != 0, result.stdout + err = _normalized_error_text(result.stderr, repo) + assert "could not create feature directory" in err + assert not (specs / "002-user-auth").exists() + + +@pytest.mark.skipif(not HAS_POWERSHELL, reason="no PowerShell available") +def test_nondirectory_occupies_feature_path_is_rejected_powershell(repo: Path) -> None: + """PowerShell twin rejects a regular file at the feature path.""" + specs = repo / "specs" + specs.mkdir(parents=True, exist_ok=True) + (specs / "001-user-auth").write_text("not-a-directory\n", encoding="utf-8") + + result = run( + ps_cmd( + repo, + SCRIPT, + "--json", + "--short-name", + "user-auth", + "Add user authentication", + ), + repo, + ) + assert result.returncode != 0, result.stdout + assert not (specs / "002-user-auth").exists() + + +@pytest.mark.parametrize("variant", ["bash", "python"]) +def test_allow_existing_rejects_nondirectory_feature_path( + repo: Path, variant: str +) -> None: + """--allow-existing-branch must not treat a regular file as a feature dir.""" + specs = repo / "specs" + specs.mkdir(parents=True, exist_ok=True) + (specs / "001-user-auth").write_text("not-a-directory\n", encoding="utf-8") + + args = ( + "--json", + "--allow-existing-branch", + "--short-name", + "user-auth", + "Add user authentication", + ) + if variant == "bash": + result = run(bash_cmd(repo, SCRIPT, *args), repo) + else: + result = run(py_cmd(repo, SCRIPT, *args), repo) + + assert result.returncode != 0, result.stdout + err = _normalized_error_text(result.stderr, repo) + assert "could not create feature directory" in err