Skip to content

Commit 4e871e4

Browse files
committed
fix: address PR review feedback
Five issues addressed per review from @mnriem and @Copilot: 1. Scope guardrail to explicit --number/-Number only Previously the guardrail fired unconditionally. It now only activates when the caller explicitly passed --number (Bash) or -Number (PS). Auto-detected numbers are not affected, preserving the existing contract. 2. Reuse check_existing_branches / Get-NextBranchNumber in guardrail Previously the guardrail re-implemented get_highest_from_* without a git fetch, meaning the corrected number could still collide with an unfetched remote branch. Both scripts now delegate to the existing functions that already fetch remotes before computing the max. 3. Extend collision check to git branches (not just spec dirs) The guardrail trigger now checks both specs/NNN-* directories and existing local/remote git branches before deciding to auto-correct, providing a consistent experience. 4. Update help text --number / -Number help text updated from "overrides auto-detection" to "Preferred branch number (auto-corrected if prefix already exists in specs or branches)" to accurately reflect the new behavior. 5. Port guardrail to PowerShell (create-new-feature.ps1) Mirrors all Bash changes for cross-platform parity.
1 parent 172b285 commit 4e871e4

2 files changed

Lines changed: 71 additions & 21 deletions

File tree

scripts/bash/create-new-feature.sh

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ set -e
55
JSON_MODE=false
66
SHORT_NAME=""
77
BRANCH_NUMBER=""
8+
NUMBER_EXPLICIT=false # true only when --number was explicitly passed by the caller
89
ARGS=()
910
i=1
1011
while [ $i -le $# ]; do
@@ -39,14 +40,15 @@ while [ $i -le $# ]; do
3940
exit 1
4041
fi
4142
BRANCH_NUMBER="$next_arg"
43+
NUMBER_EXPLICIT=true
4244
;;
4345
--help|-h)
4446
echo "Usage: $0 [--json] [--short-name <name>] [--number N] <feature_description>"
4547
echo ""
4648
echo "Options:"
4749
echo " --json Output in JSON format"
4850
echo " --short-name <name> Provide a custom short name (2-4 words) for the branch"
49-
echo " --number N Specify branch number manually (overrides auto-detection)"
51+
echo " --number N Preferred branch number (auto-corrected if prefix already exists in specs or branches)"
5052
echo " --help, -h Show this help message"
5153
echo ""
5254
echo "Examples:"
@@ -256,27 +258,37 @@ fi
256258
# Force base-10 interpretation to prevent octal conversion (e.g., 010 → 8 in octal, but should be 10 in decimal)
257259
FEATURE_NUM=$(printf "%03d" "$((10#$BRANCH_NUMBER))")
258260

259-
# ── Guardrail: auto-correct if the chosen prefix already exists ──────────────
260-
# If a specs directory with this numeric prefix already exists, the requested
261-
# number is stale or wrong (e.g., an agent passed --number with a conflicting
262-
# value). Auto-correct to the global max + 1 so the workflow never fails due
263-
# to numbering collisions.
264-
if compgen -G "$SPECS_DIR/${FEATURE_NUM}-*" > /dev/null 2>&1; then
265-
REQUESTED_NUM="$FEATURE_NUM"
266-
# Recalculate from the true global max across specs + branches
267-
if [ "$HAS_GIT" = true ]; then
268-
GLOBAL_MAX_BRANCH=$(get_highest_from_branches)
269-
else
270-
GLOBAL_MAX_BRANCH=0
261+
# ── Guardrail: auto-correct if --number was explicitly passed and the prefix
262+
# already exists in specs/ or as a git branch. Only fires on explicit --number
263+
# to preserve the existing auto-detection contract.
264+
# Reuses check_existing_branches, which fetches remotes and checks both
265+
# specs directories and all local/remote branches.
266+
if [ "$NUMBER_EXPLICIT" = true ]; then
267+
# Check for conflict in spec directories
268+
SPEC_CONFLICT=false
269+
if compgen -G "$SPECS_DIR/${FEATURE_NUM}-*" > /dev/null 2>&1; then
270+
SPEC_CONFLICT=true
271+
fi
272+
273+
# Check for conflict in git branches (local and remote)
274+
BRANCH_CONFLICT=false
275+
if [ "$HAS_GIT" = true ] && git branch -a 2>/dev/null | grep -qE "(^|[[:space:]])(remotes/[^/]+/)?${FEATURE_NUM}-"; then
276+
BRANCH_CONFLICT=true
271277
fi
272-
GLOBAL_MAX_SPEC=$(get_highest_from_specs "$SPECS_DIR")
273-
GLOBAL_MAX=$GLOBAL_MAX_BRANCH
274-
if [ "$GLOBAL_MAX_SPEC" -gt "$GLOBAL_MAX" ]; then
275-
GLOBAL_MAX=$GLOBAL_MAX_SPEC
278+
279+
if [ "$SPEC_CONFLICT" = true ] || [ "$BRANCH_CONFLICT" = true ]; then
280+
REQUESTED_NUM="$FEATURE_NUM"
281+
# Delegate to check_existing_branches, which fetches and computes
282+
# max(all specs, all branches) + 1 — same logic used by auto-detection.
283+
if [ "$HAS_GIT" = true ]; then
284+
BRANCH_NUMBER=$(check_existing_branches "$SPECS_DIR")
285+
else
286+
HIGHEST=$(get_highest_from_specs "$SPECS_DIR")
287+
BRANCH_NUMBER=$((HIGHEST + 1))
288+
fi
289+
FEATURE_NUM=$(printf "%03d" "$((10#$BRANCH_NUMBER))")
290+
>&2 echo "⚠️ [specify] --number $REQUESTED_NUM conflicts with an existing spec dir or branch. Auto-corrected to $FEATURE_NUM."
276291
fi
277-
BRANCH_NUMBER=$((GLOBAL_MAX + 1))
278-
FEATURE_NUM=$(printf "%03d" "$((10#$BRANCH_NUMBER))")
279-
>&2 echo "⚠️ Requested number $REQUESTED_NUM conflicts with existing spec directory. Auto-corrected to $FEATURE_NUM."
280292
fi
281293

282294
BRANCH_NAME="${FEATURE_NUM}-${BRANCH_SUFFIX}"

scripts/powershell/create-new-feature.ps1

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ if ($Help) {
1818
Write-Host "Options:"
1919
Write-Host " -Json Output in JSON format"
2020
Write-Host " -ShortName <name> Provide a custom short name (2-4 words) for the branch"
21-
Write-Host " -Number N Specify branch number manually (overrides auto-detection)"
21+
Write-Host " -Number N Preferred branch number (auto-corrected if prefix already exists in specs or branches)"
2222
Write-Host " -Help Show this help message"
2323
Write-Host ""
2424
Write-Host "Examples:"
@@ -213,6 +213,9 @@ if ($ShortName) {
213213
}
214214

215215
# Determine branch number
216+
# Track whether the caller explicitly passed -Number so the guardrail below
217+
# only fires for explicit overrides, not for auto-detected numbers.
218+
$numberExplicit = ($Number -ne 0)
216219
if ($Number -eq 0) {
217220
if ($hasGit) {
218221
# Check existing branches on remotes
@@ -224,6 +227,41 @@ if ($Number -eq 0) {
224227
}
225228

226229
$featureNum = ('{0:000}' -f $Number)
230+
231+
# ── Guardrail: auto-correct if -Number was explicitly passed and the prefix
232+
# already exists in specs/ or as a git branch. Only fires on explicit -Number
233+
# to preserve the existing auto-detection contract.
234+
# Reuses Get-NextBranchNumber, which fetches remotes and checks both
235+
# specs directories and all local/remote branches.
236+
if ($numberExplicit) {
237+
$requestedNum = $featureNum
238+
239+
# Check for conflict in spec directories
240+
$specConflict = (Get-ChildItem -Path $specsDir -Directory -ErrorAction SilentlyContinue |`
241+
Where-Object { $_.Name -match "^$featureNum-" }).Count -gt 0
242+
243+
# Check for conflict in git branches (local and remote)
244+
$branchConflict = $false
245+
if ($hasGit) {
246+
$allBranches = git branch -a 2>$null
247+
if ($LASTEXITCODE -eq 0) {
248+
$branchConflict = ($allBranches | Where-Object { $_ -match "(^|\s)(remotes/[^/]+/)?$featureNum-" }).Count -gt 0
249+
}
250+
}
251+
252+
if ($specConflict -or $branchConflict) {
253+
# Delegate to Get-NextBranchNumber, which fetches and computes
254+
# max(all specs, all branches) + 1 — same logic used by auto-detection.
255+
if ($hasGit) {
256+
$Number = Get-NextBranchNumber -SpecsDir $specsDir
257+
} else {
258+
$Number = (Get-HighestNumberFromSpecs -SpecsDir $specsDir) + 1
259+
}
260+
$featureNum = ('{0:000}' -f $Number)
261+
Write-Warning "[specify] -Number $requestedNum conflicts with an existing spec dir or branch. Auto-corrected to $featureNum."
262+
}
263+
}
264+
227265
$branchName = "$featureNum-$branchSuffix"
228266

229267
# GitHub enforces a 244-byte limit on branch names

0 commit comments

Comments
 (0)