fix: add pre-flight check for existing files and normalize target path#14
fix: add pre-flight check for existing files and normalize target path#14amarsingh-me wants to merge 1 commit into
Conversation
Adds check_existing_files (bash) / Test-ExistingFiles (PowerShell) to abort early with a clear error if any destination files would be overwritten, rather than silently clobbering them. Also strips trailing slashes from the user-supplied target path so error messages never show double slashes in file paths.
There was a problem hiding this comment.
Pull request overview
Adds a pre-flight overwrite detection step to the init scripts and normalizes the user-provided target path to improve error output and prevent silent clobbering during scaffolding initialization.
Changes:
- Normalize Bash
TARGET_PATHby trimming a trailing slash. - Add a Bash
check_existing_filespre-flight check that aborts if destination files already exist. - Add a PowerShell
Test-ExistingFilespre-flight check that aborts if destination files already exist.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
scripts/init.sh |
Adds target-path normalization and a pre-flight scan intended to detect would-be overwrites before copying agents/instructions/guidelines and optional OpenCode/Claude assets. |
scripts/init.ps1 |
Adds a pre-flight scan intended to detect would-be overwrites before copying agents/instructions/guidelines and optional OpenCode/Claude assets. |
| for f in "${AGENTS_SRC_DIR}"/*.md; do | ||
| dst="${TARGET_AGENTS_DIR}/$(basename "$f")" | ||
| [[ -f "$dst" ]] && existing+=("$dst") | ||
| done | ||
|
|
||
| for f in "${INSTRUCTIONS_SRC_DIR}"/*.md; do | ||
| dst="${TARGET_INSTRUCTIONS_DIR}/$(basename "$f")" | ||
| [[ -f "$dst" ]] && existing+=("$dst") | ||
| done | ||
|
|
||
| for f in "${TARGET_GUIDELINES_DIR}"/*.md; do | ||
| [[ -f "$f" ]] && existing+=("$f") | ||
| done | ||
|
|
||
| if [[ "${DEPLOY_OPENCODE}" == "true" ]]; then | ||
| for f in "${AGENTS_SRC_DIR}"/*.md; do | ||
| agent_name=$(basename "$f" .md | sed 's/\.agent//') | ||
| dst="${TARGET_PATH}/.opencode/skills/${agent_name}/SKILL.md" | ||
| [[ -f "$dst" ]] && existing+=("$dst") |
There was a problem hiding this comment.
In check_existing_files, variables like dst and agent_name are assigned without local, which leaks them into the global scope and can lead to surprising interactions later in the script (especially with set -u). Declare these as local within the function (and within loops where appropriate) to keep the function side-effect free aside from existing.
| Get-ChildItem (Join-Path $TargetGuidelinesDir '*.md') -ErrorAction SilentlyContinue | ForEach-Object { | ||
| if (Test-Path $_.FullName -PathType Leaf) { $existing += $_.FullName } | ||
| } | ||
|
|
There was a problem hiding this comment.
This pre-flight check aborts if any *.md exists under $TargetGuidelinesDir, but the script overwrites only a fixed set of destination filenames (architecture-, review-, and optional documentation-* / conventions.md). This will incorrectly block runs when users have unrelated markdown files in that folder that wouldn’t be overwritten. Instead, test only the exact destination paths this run will write, based on $BackendDocFile/$FrontendDocFile and $includeConventions.
| Get-ChildItem (Join-Path $TargetGuidelinesDir '*.md') -ErrorAction SilentlyContinue | ForEach-Object { | |
| if (Test-Path $_.FullName -PathType Leaf) { $existing += $_.FullName } | |
| } | |
| $guidelineDestinations = @( | |
| (Join-Path $TargetGuidelinesDir $BackendArchFile), | |
| (Join-Path $TargetGuidelinesDir $BackendReviewFile), | |
| (Join-Path $TargetGuidelinesDir $FrontendArchFile), | |
| (Join-Path $TargetGuidelinesDir $FrontendReviewFile) | |
| ) | |
| if ($BackendDocFile) { | |
| $guidelineDestinations += Join-Path $TargetGuidelinesDir $BackendDocFile | |
| } | |
| if ($FrontendDocFile) { | |
| $guidelineDestinations += Join-Path $TargetGuidelinesDir $FrontendDocFile | |
| } | |
| if ($includeConventions) { | |
| $guidelineDestinations += Join-Path $TargetGuidelinesDir 'conventions.md' | |
| } | |
| $guidelineDestinations | ForEach-Object { | |
| if ($_ -and (Test-Path $_ -PathType Leaf)) { $existing += $_ } | |
| } |
| TARGET_PATH="${TARGET_PATH%/}" | ||
|
|
There was a problem hiding this comment.
TARGET_PATH="${TARGET_PATH%/}" only removes a single trailing slash and turns an input of / into an empty string, which then results in paths like /.github/... while printing an empty “Target path”. Consider trimming all trailing slashes while preserving / (and/or validating that TARGET_PATH is non-empty after normalization).
| TARGET_PATH="${TARGET_PATH%/}" | |
| while [[ "${TARGET_PATH}" != "/" && "${TARGET_PATH}" == */ ]]; do | |
| TARGET_PATH="${TARGET_PATH%/}" | |
| done | |
| if [[ -z "${TARGET_PATH}" ]]; then | |
| echo "Error: target path must not be empty." >&2 | |
| exit 1 | |
| fi |
| for f in "${TARGET_GUIDELINES_DIR}"/*.md; do | ||
| [[ -f "$f" ]] && existing+=("$f") | ||
| done |
There was a problem hiding this comment.
The pre-flight check treats any existing *.md under .github/guidelines/ as a blocker, but this script only overwrites a fixed set of filenames (architecture-, review-, and optional documentation-* / conventions.md). As written, users with unrelated/custom markdown files in that directory will be prevented from running even though nothing would be overwritten. Update the check to test only the specific destination files that this run will write (based on BACKEND_DOC_FILE/FRONTEND_DOC_FILE and INCLUDE_CONVENTIONS).
Adds check_existing_files (bash) / Test-ExistingFiles (PowerShell) to
abort early with a clear error if any destination files would be
overwritten, rather than silently clobbering them. Also strips trailing
slashes from the user-supplied target path so error messages never show
double slashes in file paths.