diff --git a/scripts/bash/create-new-feature.sh b/scripts/bash/create-new-feature.sh index 54ba1dbf58..075cfc4c43 100644 --- a/scripts/bash/create-new-feature.sh +++ b/scripts/bash/create-new-feature.sh @@ -249,8 +249,17 @@ if [ "$USE_TIMESTAMP" = true ]; then FEATURE_NUM=$(date +%Y%m%d-%H%M%S) BRANCH_NAME="${FEATURE_NUM}-${BRANCH_SUFFIX}" else + # Validate --number input when provided + if [ -n "$BRANCH_NUMBER" ]; then + if ! echo "$BRANCH_NUMBER" | grep -Eq '^[0-9]+$' || [ "$((10#$BRANCH_NUMBER))" -lt 1 ]; then + >&2 echo "Error: --number requires a positive integer, got '$BRANCH_NUMBER'" + exit 1 + fi + fi + # Determine branch number if [ -z "$BRANCH_NUMBER" ]; then + # No manual number provided -- auto-detect if [ "$HAS_GIT" = true ]; then # Check existing branches on remotes BRANCH_NUMBER=$(check_existing_branches "$SPECS_DIR") @@ -259,6 +268,49 @@ else HIGHEST=$(get_highest_from_specs "$SPECS_DIR") BRANCH_NUMBER=$((HIGHEST + 1)) fi + elif [ "$ALLOW_EXISTING" = false ]; then + # Manual number provided -- validate it is not already in use + # (skip when --allow-existing-branch, as the caller intentionally targets an existing branch) + MANUAL_NUM=$((10#$BRANCH_NUMBER)) + MANUAL_NUM_PADDED=$(printf "%03d" "$MANUAL_NUM") + NUMBER_IN_USE=false + + # Check specs directory for collision + if [ -d "$SPECS_DIR" ]; then + for dir in "$SPECS_DIR"/*/; do + [ -d "$dir" ] || continue + dirname=$(basename "$dir") + if echo "$dirname" | grep -q "^${MANUAL_NUM_PADDED}-"; then + NUMBER_IN_USE=true + break + fi + done + fi + + # Check git branches for collision (fetch to catch remote-only branches) + if [ "$NUMBER_IN_USE" = false ] && [ "$HAS_GIT" = true ]; then + git fetch --all --prune 2>/dev/null || true + branches=$(git branch -a 2>/dev/null || echo "") + if [ -n "$branches" ]; then + while IFS= read -r branch; do + clean_branch=$(echo "$branch" | sed 's/^[* ]*//; s|^remotes/[^/]*/||') + if echo "$clean_branch" | grep -q "^${MANUAL_NUM_PADDED}-"; then + NUMBER_IN_USE=true + break + fi + done <<< "$branches" + fi + fi + + if [ "$NUMBER_IN_USE" = true ]; then + >&2 echo "Warning: --number $BRANCH_NUMBER conflicts with existing branch/spec (${MANUAL_NUM_PADDED}-*). Auto-detecting next available number." + if [ "$HAS_GIT" = true ]; then + BRANCH_NUMBER=$(check_existing_branches "$SPECS_DIR") + else + HIGHEST=$(get_highest_from_specs "$SPECS_DIR") + BRANCH_NUMBER=$((HIGHEST + 1)) + fi + fi fi # Force base-10 interpretation to prevent octal conversion (e.g., 010 → 8 in octal, but should be 10 in decimal) diff --git a/scripts/powershell/create-new-feature.ps1 b/scripts/powershell/create-new-feature.ps1 index 3708ea2db1..8dee67ef41 100644 --- a/scripts/powershell/create-new-feature.ps1 +++ b/scripts/powershell/create-new-feature.ps1 @@ -201,8 +201,15 @@ if ($Timestamp) { $featureNum = Get-Date -Format 'yyyyMMdd-HHmmss' $branchName = "$featureNum-$branchSuffix" } else { + # Validate -Number input when explicitly provided + if ($PSBoundParameters.ContainsKey('Number') -and $Number -lt 1) { + Write-Error "-Number requires a positive integer, got $Number" + exit 1 + } + # Determine branch number if ($Number -eq 0) { + # No manual number provided -- auto-detect if ($hasGit) { # Check existing branches on remotes $Number = Get-NextBranchNumber -SpecsDir $specsDir @@ -210,6 +217,49 @@ if ($Timestamp) { # Fall back to local directory check $Number = (Get-HighestNumberFromSpecs -SpecsDir $specsDir) + 1 } + } elseif (-not $AllowExistingBranch) { + # Manual number provided -- validate it is not already in use + # (skip when -AllowExistingBranch, as the caller intentionally targets an existing branch) + $manualNumPadded = '{0:000}' -f $Number + $numberInUse = $false + + # Check specs directory for collision + if (Test-Path $specsDir) { + foreach ($dir in (Get-ChildItem -Path $specsDir -Directory)) { + if ($dir.Name -match "^$manualNumPadded-") { + $numberInUse = $true + break + } + } + } + + # Check git branches for collision (fetch to catch remote-only branches) + if (-not $numberInUse -and $hasGit) { + try { + git fetch --all --prune 2>$null | Out-Null + $branches = git branch -a 2>$null + if ($LASTEXITCODE -eq 0 -and $branches) { + foreach ($branch in $branches) { + $cleanBranch = $branch.Trim() -replace '^\*?\s*', '' -replace '^remotes/[^/]+/', '' + if ($cleanBranch -match "^$manualNumPadded-") { + $numberInUse = $true + break + } + } + } + } catch { + Write-Verbose "Could not check Git branches: $_" + } + } + + if ($numberInUse) { + Write-Warning "-Number $Number conflicts with existing branch/spec ($manualNumPadded-*). Auto-detecting next available number." + if ($hasGit) { + $Number = Get-NextBranchNumber -SpecsDir $specsDir + } else { + $Number = (Get-HighestNumberFromSpecs -SpecsDir $specsDir) + 1 + } + } } $featureNum = ('{0:000}' -f $Number) diff --git a/tests/test_timestamp_branches.py b/tests/test_timestamp_branches.py index 0c9eb07b46..598a3b9bd4 100644 --- a/tests/test_timestamp_branches.py +++ b/tests/test_timestamp_branches.py @@ -326,8 +326,8 @@ def test_allow_existing_creates_spec_dir(self, git_repo: Path): assert (git_repo / "specs" / "006-spec-dir").is_dir() assert (git_repo / "specs" / "006-spec-dir" / "spec.md").exists() - def test_without_flag_still_errors(self, git_repo: Path): - """T009: Verify backwards compatibility (error without flag).""" + def test_without_flag_auto_detects_on_collision(self, git_repo: Path): + """T009: Without --allow-existing-branch, a conflicting --number auto-detects next available.""" subprocess.run( ["git", "checkout", "-b", "007-no-flag"], cwd=git_repo, check=True, capture_output=True, @@ -339,8 +339,9 @@ def test_without_flag_still_errors(self, git_repo: Path): result = run_script( git_repo, "--short-name", "no-flag", "--number", "7", "No flag feature", ) - assert result.returncode != 0, "should fail without --allow-existing-branch" - assert "already exists" in result.stderr + assert result.returncode == 0, result.stderr + assert "conflicts with existing branch/spec" in result.stderr + assert "008-no-flag" in result.stdout def test_allow_existing_no_overwrite_spec(self, git_repo: Path): """T010: Pre-create spec.md with content, verify it is preserved.""" @@ -405,6 +406,45 @@ def test_allow_existing_no_git(self, no_git_dir: Path): assert result.returncode == 0, result.stderr +class TestNumberInputValidation: + """Tests for --number input validation and spec-directory collision detection.""" + + def test_rejects_zero(self, git_repo: Path): + result = run_script(git_repo, "--short-name", "zero", "--number", "0", "Zero test") + assert result.returncode != 0 + assert "positive integer" in result.stderr + + def test_rejects_non_numeric(self, git_repo: Path): + result = run_script(git_repo, "--short-name", "abc", "--number", "abc", "Non-numeric test") + assert result.returncode != 0 + assert "positive integer" in result.stderr + + def test_rejects_negative(self, git_repo: Path): + result = run_script(git_repo, "--short-name", "neg", "--number", "-5", "Negative test") + assert result.returncode != 0 + assert "positive integer" in result.stderr + + def test_collision_detected_via_specs_directory(self, git_repo: Path): + """Collision detected from specs dir even when no matching branch exists.""" + spec_dir = git_repo / "specs" / "003-existing-spec" + spec_dir.mkdir(parents=True) + (spec_dir / "spec.md").write_text("# Existing\n") + result = run_script( + git_repo, "--short-name", "new-feature", "--number", "3", "Spec collision test", + ) + assert result.returncode == 0 + assert "conflicts with existing branch/spec" in result.stderr + assert "003-new-feature" not in result.stdout + + def test_unused_number_accepted(self, git_repo: Path): + """A manual --number that doesn't collide is accepted as-is.""" + result = run_script( + git_repo, "--short-name", "free", "--number", "50", "Free number test", + ) + assert result.returncode == 0, result.stderr + assert "050-free" in result.stdout + + class TestAllowExistingBranchPowerShell: def test_powershell_supports_allow_existing_branch_flag(self): """Static guard: PS script exposes and uses -AllowExistingBranch.""" @@ -412,3 +452,24 @@ def test_powershell_supports_allow_existing_branch_flag(self): assert "-AllowExistingBranch" in contents # Ensure the flag is referenced in script logic, not just declared assert "AllowExistingBranch" in contents.replace("-AllowExistingBranch", "") + + def test_powershell_rejects_invalid_number(self): + """Static guard: PS script validates -Number using PSBoundParameters and rejects < 1.""" + contents = CREATE_FEATURE_PS.read_text(encoding="utf-8") + assert "PSBoundParameters.ContainsKey" in contents + assert "Number -lt 1" in contents or re.search(r"\$Number\s+-lt\s+1", contents) + assert "positive integer" in contents + + def test_powershell_has_number_collision_validation(self): + """Static guard: PS script validates manual -Number against existing branches/specs.""" + contents = CREATE_FEATURE_PS.read_text(encoding="utf-8") + # Must check specs directory for collision + assert "manualNumPadded" in contents + # Must check git branches for collision + assert "git fetch --all --prune" in contents + # Must warn and auto-detect on conflict + assert "conflicts with existing branch/spec" in contents + # Must skip validation when -AllowExistingBranch is set; allow flexible whitespace + assert re.search(r"elseif\s*\(\s*-not\s+\$AllowExistingBranch\s*\)", contents), ( + "Expected an elseif guard that negates $AllowExistingBranch" + )