fix: install.cmd bug fixes, test suite gap closure, and UX improvements#3
fix: install.cmd bug fixes, test suite gap closure, and UX improvements#3
Conversation
…k (Pro Git audit)
…ipts, pre-push hook)
…branch_cleanup exit code
…tings on reconfigure
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Several hook files (.githooks/*) violate the repository’s shell standards (not bash, missing set -uo pipefail, using [ ... ]), and pre-commit contains a portability/correctness bug by using \s in an ERE grep. changelog_generate.sh currently produces incorrect output for --to (and effectively ignores merge-inclusion settings) due to _build_changelog() rebuilding the range as ..HEAD. branch_cleanup.sh can mis-handle detached-HEAD output due to parsing git branch formatting instead of using plumbing-friendly refs. The modified-only lint argument handling is configuration-fragile and the new ShellCheck suppressions don’t meet the repo’s suppression-justification rule.
Additional notes (2)
-
Maintainability |
scripts/git/check_lint.sh:105-105
--modified-onlynow strips the last token fromCGW_LINT_CHECK_ARGS/CGW_FORMAT_CHECK_ARGSvia${var% *}under the assumption that the last token is always a path (like.). That’s fragile: -
If the configured args don’t end in a path token, you’ll silently drop a real option.
-
If the args are a single word,
% *returns the whole string (fine), but the behavior becomes configuration-dependent and hard to reason about.
Also: the # shellcheck disable=SC2086 suppressions don’t include the required one-line justification (per .charlie/instructions/code-style.md [R6]).
- Readability |
scripts/git/commit_enhanced.sh:215-228
Behavior/prompt mismatch: the interactive prompt says “Stage all changes?” but the implementation is nowgit add -u(tracked-only). That will not stage new files, which can easily lead to incomplete commits and confusion.
If the intent is “avoid accidentally adding local-only files”, you already call unstage_local_only_files, so you can keep the UX of “all changes” while still protecting local-only paths.
Summary of changes
Summary
This PR significantly expands claude-git-workflow with new scripts, hook support, docs, and tests.
Hooking & installation
- Adds pre-push support alongside pre-commit:
- New hook templates under
hooks/and tracked hooks under.githooks/. configure.shnow generates.githooks/pre-pushandinstall_hooks.shinstalls both hooks.install.cmdupdated to copy/validatehooks/pre-pushand back up existing hook templates.
- New hook templates under
New git utility scripts
- Adds several new workflow scripts:
- History/safety:
rebase_safe.sh,bisect_helper.sh,undo_last.sh - Maintenance:
branch_cleanup.sh,repo_health.sh,clean_build.sh,setup_attributes.sh - Release/changelog:
create_release.sh,changelog_generate.sh - Convenience:
stash_work.sh
- History/safety:
Config + behavior changes
_common.shlogging now uses${PROJECT_ROOT}/logsso logs go to the repo root regardless of CWD._config.shintroduces new knobs:CGW_LINT_EXTENSIONS,CGW_MERGE_CONFLICT_STYLE,CGW_MERGE_IGNORE_WHITESPACE.check_lint.sh/fix_lint.shnow support modified-only linting viaCGW_LINT_EXTENSIONS.merge_with_validation.shnow applies optional merge strategy flags.commit_enhanced.shstages tracked-only changes viagit add -uand adds a non-blocking whitespace check.
Docs + tests
- README and skill docs updated to include new scripts/features.
- Adds a broad set of new Bats integration tests covering new and previously untested scripts.
| #!/bin/sh | ||
| # Pre-commit hook — blocks local-only files from being committed | ||
| # Generated by claude-git-workflow configure.sh | ||
| # | ||
| # Blocks ADDITIONS (A) and MODIFICATIONS (M) of local-only files. | ||
| # DELETIONS (D) are allowed (removing from git tracking is fine). | ||
| # | ||
| # The pattern below is populated by configure.sh from CGW_LOCAL_FILES. | ||
| # To regenerate after changing .cgw.conf, run: ./scripts/git/configure.sh --skip-skill | ||
|
|
||
| echo "Checking for local-only files..." | ||
|
|
||
| PROBLEMATIC_FILES=$(git diff --cached --name-status | grep -E "^[AM]\s+(CLAUDE\.md|SESSION_LOG\.md|\.claude|logs)") | ||
|
|
||
| if [ -n "$PROBLEMATIC_FILES" ]; then | ||
| echo "ERROR: Attempting to add or modify local-only files!" | ||
| echo "" | ||
| echo "The following files must remain local only:" | ||
| echo "$PROBLEMATIC_FILES" | ||
| echo "" | ||
| echo "DELETIONS are allowed (removing from git tracking)" | ||
| echo "ADDITIONS/MODIFICATIONS are blocked" | ||
| echo "" | ||
| echo "To fix: git reset HEAD <file>" | ||
| echo "" | ||
| exit 1 | ||
| fi | ||
|
|
||
| DELETED_FILES=$(git diff --cached --name-status | grep -E "^D\s+(CLAUDE\.md|SESSION_LOG\.md|\.claude|logs)") | ||
| if [ -n "$DELETED_FILES" ]; then | ||
| echo " Local-only files being removed from git tracking (allowed)" | ||
| fi | ||
|
|
||
| echo " No local-only files detected" | ||
|
|
There was a problem hiding this comment.
This hook violates the repo’s shell scripting instructions and also has a correctness bug in its regex.
- Instruction violations:
.charlie/instructions/code-style.md[R1]: missingset -uo pipefail- [R3]: uses
[ ... ]instead of[[ ... ]] - [R5]: indentation uses 4 spaces (and overall inconsistent with the 2-space rule)
- Scope says hooks are bash; this is
#!/bin/sh
- Correctness:
grep -Edoes not support\sportably in ERE, so patterns like"^[AM]\s+..."are likely to never match (or to behave inconsistently across platforms).
Suggestion
Convert this to the project-standard bash hook style and fix the whitespace matching. For example:
#!/usr/bin/env bash
set -uo pipefail
problematic_files="$(
git diff --cached --name-status \
| grep -E "^[AM][[:space:]]+(CLAUDE\.md|MEMORY\.md|\.claude/|logs/)" || true
)"
if [[ -n "${problematic_files}" ]]; then
echo "ERROR: Attempting to add or modify local-only files!" >&2
echo "${problematic_files}" >&2
exit 1
fiAlso consider anchoring directory matches to avoid false positives (e.g. (^|/)(\.claude/|logs/)). Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| #!/bin/sh | ||
| # Pre-push hook — validates commits before they leave local repo | ||
| # Generated by claude-git-workflow configure.sh | ||
| # | ||
| # Checks all commits about to be pushed: | ||
| # 1. No local-only files tracked in any unpushed commit | ||
| # 2. All commit messages follow conventional format | ||
| # | ||
| # This complements push_validated.sh (CGW wrapper) by catching issues | ||
| # even when users bypass the wrapper and run git push directly. | ||
| # See Pro Git Ch8 p.362 — pre-push hook. | ||
| # | ||
| # The patterns below are populated by configure.sh from CGW_LOCAL_FILES | ||
| # and CGW_ALL_PREFIXES. To regenerate, run: ./scripts/git/configure.sh --skip-skill | ||
| # | ||
| # CONVENTIONAL PREFIXES: __CGW_ALL_PREFIXES__ | ||
| # LOCAL FILES PATTERN: __CGW_LOCAL_FILES_PATTERN__ | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Read stdin: remote <name>, url <url>, <local-ref> <local-sha> <remote-ref> <remote-sha> | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| LOCAL_FILES_PATTERN="__CGW_LOCAL_FILES_PATTERN__" | ||
| ALL_PREFIXES="__CGW_ALL_PREFIXES__" | ||
|
|
||
| # Collect push info from stdin (git passes it to pre-push) | ||
| while read -r LOCAL_REF LOCAL_SHA REMOTE_REF REMOTE_SHA; do | ||
| # Skip deletions (empty sha = 0000...0) | ||
| if [ "${LOCAL_SHA}" = "0000000000000000000000000000000000000000" ]; then | ||
| continue | ||
| fi | ||
|
|
||
| # Determine commit range to check | ||
| if [ "${REMOTE_SHA}" = "0000000000000000000000000000000000000000" ]; then | ||
| # New branch being pushed — check all commits not in any remote branch | ||
| RANGE="${LOCAL_SHA}" | ||
| COMMITS=$(git rev-list "${RANGE}" --not --remotes 2>/dev/null || true) | ||
| else | ||
| RANGE="${REMOTE_SHA}..${LOCAL_SHA}" | ||
| COMMITS=$(git rev-list "${RANGE}" 2>/dev/null || true) | ||
| fi | ||
|
|
||
| [ -z "${COMMITS}" ] && continue | ||
|
|
||
| echo "pre-push: checking $(echo "${COMMITS}" | wc -l | tr -d ' ') commit(s) in ${LOCAL_REF}..." | ||
|
|
||
| # ── [1] Local-only file check ────────────────────────────────────────── | ||
| if [ -n "${LOCAL_FILES_PATTERN}" ]; then | ||
| for COMMIT in ${COMMITS}; do | ||
| FILES_IN_COMMIT=$(git diff-tree --no-commit-id -r --name-only "${COMMIT}" 2>/dev/null || true) | ||
| PROBLEM=$(echo "${FILES_IN_COMMIT}" | grep -E "${LOCAL_FILES_PATTERN}" || true) | ||
| if [ -n "${PROBLEM}" ]; then | ||
| echo "" | ||
| echo "ERROR [pre-push]: Commit ${COMMIT} contains local-only files:" | ||
| echo "${PROBLEM}" | sed 's/^/ /' | ||
| echo "" | ||
| echo "These files must not be pushed: ${LOCAL_FILES_PATTERN}" | ||
| echo "To fix: git rebase -i HEAD~N and drop/edit the offending commit" | ||
| echo "" | ||
| exit 1 | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| # ── [2] Conventional commit format check ────────────────────────────── | ||
| if [ -n "${ALL_PREFIXES}" ]; then | ||
| for COMMIT in ${COMMITS}; do | ||
| MSG=$(git log -1 --format="%s" "${COMMIT}" 2>/dev/null || true) | ||
|
|
||
| # Skip merge commits (they often have non-conventional messages) | ||
| PARENT_COUNT=$(git cat-file -p "${COMMIT}" 2>/dev/null | grep -c "^parent " || true) | ||
| [ "${PARENT_COUNT}" -ge 2 ] && continue | ||
|
|
||
| if ! echo "${MSG}" | grep -qE "^(${ALL_PREFIXES}):"; then | ||
| echo "" | ||
| echo "WARNING [pre-push]: Commit ${COMMIT} has non-conventional message:" | ||
| echo " ${MSG}" | ||
| echo "" | ||
| echo "Expected format: <type>: <description>" | ||
| echo "Types: ${ALL_PREFIXES}" | ||
| echo "" | ||
| echo "Push blocked. Fix with: ./scripts/git/undo_last.sh amend-message '<type>: <msg>'" | ||
| echo "Or bypass (not recommended): git push --no-verify" | ||
| echo "" | ||
| exit 1 | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| done |
There was a problem hiding this comment.
This hook has the same baseline issues as the pre-commit hook (scope/rules mismatch), plus it currently uses tabs for indentation.
- Instruction violations (per
.charlie/instructions/code-style.md): [R1] (missingset -uo pipefail), [R3] ([ ... ]), [R5] (tabs), and the hook is#!/bin/sheven though the hook/shell standards in this repo are bash-centric. - Portability: relying on
/bin/shwhile using bash-leaning conventions elsewhere increases platform variability (especially on macOS where/bin/shisdash-like behavior differs).
Suggestion
Standardize the hook to the same bash+strict-mode baseline used everywhere else in this repo, and replace tab indentation with 2 spaces. Mechanically:
- Change shebang to
#!/usr/bin/env bash - Add
set -uo pipefail - Replace
[ ... ]with[[ ... ]] - Re-indent with 2 spaces
Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| # Determine git log range | ||
| local log_range | ||
| if [[ -n "${from_ref}" ]]; then | ||
| log_range="${from_ref}..${to_ref}" | ||
| else | ||
| log_range="${to_ref}" | ||
| fi | ||
|
|
||
| # Get to_ref description for header | ||
| local to_desc | ||
| to_desc=$(git describe --tags --exact-match "${to_ref}" 2>/dev/null || \ | ||
| git log -1 --format="%h" "${to_ref}" 2>/dev/null || echo "${to_ref}") | ||
| local to_date | ||
| to_date=$(git log -1 --format="%ad" --date=short "${to_ref}" 2>/dev/null || date +%Y-%m-%d) | ||
|
|
||
| # Collect commits in range | ||
| local merge_flag="--no-merges" | ||
| [[ ${include_merges} -eq 1 ]] && merge_flag="" | ||
|
|
||
| # shellcheck disable=SC2086 # merge_flag intentionally word-splits when empty | ||
| local commits | ||
| commits=$(git log ${merge_flag} --format="%H|%s|%b" "${log_range}" 2>/dev/null || true) | ||
|
|
||
| if [[ -z "${commits}" ]]; then | ||
| echo "No commits found in range: ${log_range}" >&2 | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Categorize commits by conventional type | ||
| # Categories: feat, fix, docs, perf, refactor, style, test, chore, other | ||
| local -a cat_feat=() cat_fix=() cat_docs=() cat_perf=() | ||
| local -a cat_refactor=() cat_style=() cat_test=() cat_chore=() cat_other=() | ||
|
|
||
| while IFS='|' read -r hash subject body; do | ||
| [[ -z "${hash}" ]] && continue | ||
|
|
||
| local prefix rest | ||
| if echo "${subject}" | grep -qE "^[a-zA-Z]+:"; then | ||
| prefix=$(echo "${subject}" | sed 's/:.*//') | ||
| rest=$(echo "${subject}" | sed 's/^[^:]*: *//') | ||
| else | ||
| prefix="other" | ||
| rest="${subject}" | ||
| fi | ||
|
|
||
| # Get short hash and PR reference if any | ||
| local short_hash | ||
| short_hash=$(git log -1 --format="%h" "${hash}" 2>/dev/null || echo "${hash:0:7}") | ||
|
|
||
| local entry="${rest} (${short_hash})" | ||
|
|
||
| case "${prefix}" in | ||
| feat) cat_feat+=("${entry}") ;; | ||
| fix) cat_fix+=("${entry}") ;; | ||
| docs) cat_docs+=("${entry}") ;; | ||
| perf) cat_perf+=("${entry}") ;; | ||
| refactor) cat_refactor+=("${entry}") ;; | ||
| style) cat_style+=("${entry}") ;; | ||
| test) cat_test+=("${entry}") ;; | ||
| chore) cat_chore+=("${entry}") ;; | ||
| *) cat_other+=("${entry}") ;; | ||
| esac | ||
| done <<< "${commits}" | ||
|
|
||
| # Build output | ||
| local output="" | ||
| output=$(_build_changelog \ | ||
| "${output_format}" "${to_desc}" "${to_date}" "${from_ref}" \ | ||
| "${cat_feat[@]+"${cat_feat[@]}"}" \ | ||
| "BREAK_FEAT" \ | ||
| "${cat_fix[@]+"${cat_fix[@]}"}" \ | ||
| "BREAK_FIX" \ | ||
| "${cat_docs[@]+"${cat_docs[@]}"}" \ | ||
| "BREAK_DOCS" \ | ||
| "${cat_perf[@]+"${cat_perf[@]}"}" \ | ||
| "BREAK_PERF" \ | ||
| "${cat_refactor[@]+"${cat_refactor[@]}"}" \ | ||
| "BREAK_REFACTOR" \ | ||
| "${cat_style[@]+"${cat_style[@]}"}" \ | ||
| "BREAK_STYLE" \ | ||
| "${cat_test[@]+"${cat_test[@]}"}" \ | ||
| "BREAK_TEST" \ | ||
| "${cat_chore[@]+"${cat_chore[@]}"}" \ | ||
| "BREAK_CHORE" \ | ||
| "${cat_other[@]+"${cat_other[@]}"}" \ | ||
| 2>/dev/null || true) | ||
|
|
||
| # Write output | ||
| if [[ -n "${output_file}" ]]; then | ||
| echo "${output}" >"${output_file}" | ||
| echo "✓ Changelog written to: ${output_file}" >&2 | ||
| else | ||
| echo "${output}" | ||
| fi | ||
| } | ||
|
|
||
| _build_changelog() { | ||
| local fmt="$1" version="$2" date_str="$3" from_ref="$4" | ||
| shift 4 | ||
|
|
||
| # Parse the positional args back into categories using BREAK_ sentinel tokens | ||
| local current_cat="feat" | ||
| declare -A cats | ||
| cats[feat]="" cats[fix]="" cats[docs]="" cats[perf]="" | ||
| cats[refactor]="" cats[style]="" cats[test]="" cats[chore]="" cats[other]="" | ||
|
|
||
| # Re-collect — simpler: re-run git log with format per category | ||
| # (The sentinel approach doesn't work cleanly with arrays passed through positional args) | ||
| # Instead, recollect directly here. | ||
| local merge_flag="--no-merges" | ||
|
|
||
| local log_range | ||
| if [[ -n "${from_ref}" ]]; then | ||
| log_range="${from_ref}..HEAD" | ||
| else | ||
| log_range="HEAD" | ||
| fi | ||
|
|
||
| # Read commits again (simpler than passing arrays through function args) | ||
| while IFS='|' read -r hash subject; do | ||
| [[ -z "${hash}" ]] && continue | ||
| local prefix rest short_hash | ||
| prefix=$(echo "${subject}" | grep -oE "^[a-zA-Z]+" || echo "other") | ||
| rest=$(echo "${subject}" | sed 's/^[^:]*: *//') | ||
| short_hash=$(git log -1 --format="%h" "${hash}" 2>/dev/null || echo "${hash:0:7}") | ||
| local entry=" - ${rest} (${short_hash})" | ||
| case "${prefix}" in | ||
| feat) cats[feat]+="${entry}"$'\n' ;; | ||
| fix) cats[fix]+="${entry}"$'\n' ;; | ||
| docs) cats[docs]+="${entry}"$'\n' ;; | ||
| perf) cats[perf]+="${entry}"$'\n' ;; | ||
| refactor) cats[refactor]+="${entry}"$'\n' ;; | ||
| style) cats[style]+="${entry}"$'\n' ;; | ||
| test) cats[test]+="${entry}"$'\n' ;; | ||
| chore) cats[chore]+="${entry}"$'\n' ;; | ||
| *) cats[other]+="${entry}"$'\n' ;; | ||
| esac | ||
| done < <(git log --no-merges --format="%H|%s" "${log_range}" 2>/dev/null || true) | ||
|
|
There was a problem hiding this comment.
changelog_generate.sh computes log_range using --from/--to, but _build_changelog() ignores --to entirely and re-reads commits using from_ref..HEAD. That makes --to <ref> incorrect and also makes --include-merges ineffective (the rebuild hardcodes --no-merges).
This isn’t just cleanup debt—the output can be wrong for releases generated against non-HEAD refs.
Suggestion
Fix _build_changelog() to use the already computed range and options instead of re-reading HEAD. Two viable approaches:
- Preferred: build the final output directly in
main()from the categorized arrays you already collected (no secondgit log). - Or: pass
to_ref+include_mergesinto_build_changelog()and uselog_range="${from_ref}..${to_ref}"(not..HEAD), and respect--include-merges.
Reply with "@CharlieHelps yes please" if you want me to add a commit that refactors this to a single-pass implementation.
| # Also install pre-push hook if template exists alongside pre-commit | ||
| local pre_push_template="${hooks_template_dir}/pre-push" | ||
| if [[ -f "${pre_push_template}" ]]; then | ||
| # Build CGW_ALL_PREFIXES for substitution into pre-push template. | ||
| # Can't source _config.sh here (see top-of-file comment), so compute locally | ||
| # by reading CGW_EXTRA_PREFIXES from the just-written .cgw.conf. | ||
| local _base_prefixes="feat|fix|docs|chore|test|refactor|style|perf" | ||
| local _extra_prefixes | ||
| _extra_prefixes=$(grep -m1 '^CGW_EXTRA_PREFIXES=' "${PROJECT_ROOT}/.cgw.conf" \ | ||
| | sed 's/CGW_EXTRA_PREFIXES=//;s/"//g' || true) | ||
| local _all_prefixes | ||
| if [[ -n "${_extra_prefixes}" ]]; then | ||
| _all_prefixes="${_base_prefixes}|${_extra_prefixes}" | ||
| else | ||
| _all_prefixes="${_base_prefixes}" | ||
| fi | ||
| local all_prefixes_escaped="${_all_prefixes//|/\\|}" | ||
| sed -e "s|__CGW_LOCAL_FILES_PATTERN__|${sed_files_pattern}|g" \ | ||
| -e "s|__CGW_ALL_PREFIXES__|${all_prefixes_escaped}|g" \ | ||
| "${pre_push_template}" >"${PROJECT_ROOT}/.githooks/pre-push" | ||
| chmod +x "${PROJECT_ROOT}/.githooks/pre-push" | ||
| fi |
There was a problem hiding this comment.
The pre-push hook installation builds CGW_ALL_PREFIXES via parsing .cgw.conf and then substitutes into a sed replacement, but the escaping is incomplete.
If CGW_EXTRA_PREFIXES contains & or backslashes, sed replacement can corrupt the generated hook. (Pipes are handled, but & is special in sed replacements and should be escaped, and \\ handling should be consistent with the local-files pattern handling.)
Suggestion
Escape _all_prefixes for sed replacement the same way you already do for files_pattern:
local sed_prefixes="${_all_prefixes//\\/\\\\}"
sed_prefixes="${sed_prefixes//&/\\&}"
# (then escape delimiter '|')
sed_prefixes="${sed_prefixes//|/\\|}"Then substitute with sed_prefixes instead of all_prefixes_escaped. Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this change.
| # ── [1] Merged local branches ───────────────────────────────────────────── | ||
| echo "--- [1] Merged Local Branches (merged into ${CGW_TARGET_BRANCH}) ---" | ||
|
|
||
| local -a merged_branches=() | ||
| while IFS= read -r branch; do | ||
| branch="${branch# }" # strip leading spaces from git branch output | ||
| branch="${branch#* }" # strip leading asterisk+space for current branch | ||
|
|
||
| # Skip empty | ||
| [[ -z "${branch}" ]] && continue | ||
|
|
||
| # Skip protected branches | ||
| local is_protected=0 | ||
| for pb in "${protected[@]}"; do | ||
| [[ "${branch}" == "${pb}" ]] && is_protected=1 && break | ||
| done | ||
| [[ ${is_protected} -eq 1 ]] && continue | ||
|
|
||
| # Skip current branch | ||
| [[ "${branch}" == "${current_branch}" ]] && continue | ||
|
|
||
| merged_branches+=("${branch}") | ||
| done < <(git branch --merged "${CGW_TARGET_BRANCH}" 2>/dev/null) | ||
|
|
There was a problem hiding this comment.
Branch enumeration via git branch --merged ... is brittle:
- In detached-HEAD states,
git branchcan emit lines like* (HEAD detached at ...), and after the current stripping logic (${branch#* }) you may end up with a non-branch token that then gets scheduled for deletion (and will fail noisily). - The parsing approach is also sensitive to formatting changes in
git branchoutput.
Suggestion
Use plumbing-friendly output instead of parsing git branch formatting. For example:
while IFS= read -r branch; do
[[ -z "${branch}" ]] && continue
# ... protected/current checks ...
merged_branches+=("${branch}")
done < <(git for-each-ref --format='%(refname:short)' refs/heads --merged "${CGW_TARGET_BRANCH}")This avoids detached-HEAD pseudo-lines entirely. Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| set -uo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| source "${SCRIPT_DIR}/_common.sh" | ||
|
|
||
| init_logging "bisect_helper" | ||
|
|
||
| _bisect_original_branch="" | ||
|
|
||
| _cleanup_bisect() { | ||
| # If bisect is active and we were interrupted, abort it and return to original branch | ||
| if git rev-parse --git-dir >/dev/null 2>&1; then | ||
| if git bisect log >/dev/null 2>&1; then | ||
| echo "" >&2 | ||
| echo "⚠ Interrupted — aborting bisect session" >&2 | ||
| git bisect reset 2>/dev/null || true | ||
| fi | ||
| fi | ||
| if [[ -n "${_bisect_original_branch}" ]]; then | ||
| local current | ||
| current=$(git branch --show-current 2>/dev/null || true) | ||
| if [[ -n "${current}" ]] && [[ "${current}" != "${_bisect_original_branch}" ]]; then | ||
| git checkout "${_bisect_original_branch}" 2>/dev/null || true | ||
| fi | ||
| fi | ||
| } | ||
| trap _cleanup_bisect EXIT INT TERM |
There was a problem hiding this comment.
New scripts are missing the required ShellCheck source directive on the _common.sh import, violating .charlie/instructions/code-style.md [R7].
This also uses tabs for indentation throughout, violating [R5] (2-space indentation).
Suggestion
Add the ShellCheck source directive and reindent to 2 spaces:
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
# shellcheck source=scripts/git/_common.sh
source "${SCRIPT_DIR}/_common.sh"Then apply the same change across the other new scripts that source _common.sh. Reply with "@CharlieHelps yes please" if you’d like me to add a commit that normalizes all new scripts for [R5]/[R7] compliance.
| #!/bin/sh | ||
| # Pre-push hook — validates commits before they leave local repo | ||
| # Generated by claude-git-workflow configure.sh | ||
| # | ||
| # Checks all commits about to be pushed: | ||
| # 1. No local-only files tracked in any unpushed commit | ||
| # 2. All commit messages follow conventional format | ||
| # | ||
| # This complements push_validated.sh (CGW wrapper) by catching issues | ||
| # even when users bypass the wrapper and run git push directly. | ||
| # See Pro Git Ch8 p.362 — pre-push hook. | ||
| # | ||
| # The patterns below are populated by configure.sh from CGW_LOCAL_FILES | ||
| # and CGW_ALL_PREFIXES. To regenerate, run: ./scripts/git/configure.sh --skip-skill | ||
| # | ||
| # CONVENTIONAL PREFIXES: __CGW_ALL_PREFIXES__ | ||
| # LOCAL FILES PATTERN: __CGW_LOCAL_FILES_PATTERN__ | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Read stdin: remote <name>, url <url>, <local-ref> <local-sha> <remote-ref> <remote-sha> | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| LOCAL_FILES_PATTERN="__CGW_LOCAL_FILES_PATTERN__" | ||
| ALL_PREFIXES="__CGW_ALL_PREFIXES__" | ||
|
|
||
| # Collect push info from stdin (git passes it to pre-push) | ||
| while read -r LOCAL_REF LOCAL_SHA REMOTE_REF REMOTE_SHA; do | ||
| # Skip deletions (empty sha = 0000...0) | ||
| if [ "${LOCAL_SHA}" = "0000000000000000000000000000000000000000" ]; then | ||
| continue | ||
| fi | ||
|
|
||
| # Determine commit range to check | ||
| if [ "${REMOTE_SHA}" = "0000000000000000000000000000000000000000" ]; then | ||
| # New branch being pushed — check all commits not in any remote branch | ||
| RANGE="${LOCAL_SHA}" | ||
| COMMITS=$(git rev-list "${RANGE}" --not --remotes 2>/dev/null || true) | ||
| else | ||
| RANGE="${REMOTE_SHA}..${LOCAL_SHA}" | ||
| COMMITS=$(git rev-list "${RANGE}" 2>/dev/null || true) | ||
| fi | ||
|
|
||
| [ -z "${COMMITS}" ] && continue | ||
|
|
||
| echo "pre-push: checking $(echo "${COMMITS}" | wc -l | tr -d ' ') commit(s) in ${LOCAL_REF}..." | ||
|
|
||
| # ── [1] Local-only file check ────────────────────────────────────────── | ||
| if [ -n "${LOCAL_FILES_PATTERN}" ]; then | ||
| for COMMIT in ${COMMITS}; do | ||
| FILES_IN_COMMIT=$(git diff-tree --no-commit-id -r --name-only "${COMMIT}" 2>/dev/null || true) | ||
| PROBLEM=$(echo "${FILES_IN_COMMIT}" | grep -E "${LOCAL_FILES_PATTERN}" || true) | ||
| if [ -n "${PROBLEM}" ]; then | ||
| echo "" | ||
| echo "ERROR [pre-push]: Commit ${COMMIT} contains local-only files:" | ||
| echo "${PROBLEM}" | sed 's/^/ /' | ||
| echo "" | ||
| echo "These files must not be pushed: ${LOCAL_FILES_PATTERN}" | ||
| echo "To fix: git rebase -i HEAD~N and drop/edit the offending commit" | ||
| echo "" | ||
| exit 1 | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| # ── [2] Conventional commit format check ────────────────────────────── | ||
| if [ -n "${ALL_PREFIXES}" ]; then | ||
| for COMMIT in ${COMMITS}; do | ||
| MSG=$(git log -1 --format="%s" "${COMMIT}" 2>/dev/null || true) | ||
|
|
||
| # Skip merge commits (they often have non-conventional messages) | ||
| PARENT_COUNT=$(git cat-file -p "${COMMIT}" 2>/dev/null | grep -c "^parent " || true) | ||
| [ "${PARENT_COUNT}" -ge 2 ] && continue | ||
|
|
||
| if ! echo "${MSG}" | grep -qE "^(${ALL_PREFIXES}):"; then | ||
| echo "" | ||
| echo "WARNING [pre-push]: Commit ${COMMIT} has non-conventional message:" | ||
| echo " ${MSG}" | ||
| echo "" | ||
| echo "Expected format: <type>: <description>" | ||
| echo "Types: ${ALL_PREFIXES}" | ||
| echo "" | ||
| echo "Push blocked. Fix with: ./scripts/git/undo_last.sh amend-message '<type>: <msg>'" | ||
| echo "Or bypass (not recommended): git push --no-verify" | ||
| echo "" | ||
| exit 1 | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| done |
There was a problem hiding this comment.
Same as .githooks/pre-commit: this hook is sh, lacks set -uo pipefail, uses [ ... ], and has tab indentation. Since this file is a template intended to be copied into user repos, it should follow the same bash conventions as the rest of the project ([R1], [R3], [R5]).
Suggestion
Convert the template to bash + style compliance:
#!/usr/bin/env bash+set -uo pipefail- Replace
[ ... ]with[[ ... ]] - Replace tabs with 2-space indentation
Also consider factoring the duplicated content so hooks/pre-push is the single source of truth and .githooks/pre-push is generated/installed from it.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| # CGW_LINT_EXTENSIONS controls which files are considered (default: *.py) | ||
| # shellcheck disable=SC2086 | ||
| modified_files=$(git diff --name-only --diff-filter=ACMR HEAD -- ${CGW_LINT_EXTENSIONS:-*.py}) |
There was a problem hiding this comment.
Same glob-expansion bug as check_lint.sh: unquoted ${CGW_LINT_EXTENSIONS} can be expanded by the shell, producing incorrect pathspecs and missing files.
Suggestion
Use the same quoted-array approach as in check_lint.sh:
local -a lint_exts=()
read -r -a lint_exts <<< "${CGW_LINT_EXTENSIONS:-*.py}"
modified_files=$(git diff --name-only --diff-filter=ACMR HEAD -- "${lint_exts[@]}")Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| init_logging() { | ||
| local script_name="$1" | ||
| # Use PROJECT_ROOT for an absolute path so logs land in the right place | ||
| # even when the script is invoked from a subdirectory. PROJECT_ROOT is set | ||
| # by _config.sh before any script calls init_logging. | ||
| local log_dir="${PROJECT_ROOT}/logs" | ||
|
|
||
| if [[ ! -d "logs" ]]; then | ||
| mkdir -p "logs" | ||
| if [[ ! -d "${log_dir}" ]]; then | ||
| mkdir -p "${log_dir}" | ||
| fi | ||
|
|
||
| get_timestamp | ||
|
|
||
| # shellcheck disable=SC2034 | ||
| logfile="logs/${script_name}_${timestamp}.log" | ||
| logfile="${log_dir}/${script_name}_${timestamp}.log" | ||
| # shellcheck disable=SC2034 | ||
| reportfile="logs/${script_name}_analysis_${timestamp}.log" | ||
| reportfile="${log_dir}/${script_name}_analysis_${timestamp}.log" |
There was a problem hiding this comment.
init_logging() now assumes PROJECT_ROOT is set and non-empty. If any script calls init_logging before _config.sh has established PROJECT_ROOT (or if PROJECT_ROOT somehow becomes empty), this will write into /logs or an unintended path.
Suggestion
Add a defensive fallback to the previous relative behavior when PROJECT_ROOT is empty/unset:
local log_dir="logs"
if [[ -n "${PROJECT_ROOT:-}" ]]; then
log_dir="${PROJECT_ROOT}/logs"
fiReply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| fi | ||
| } | ||
| trap _cleanup_cherry_pick INT TERM | ||
| trap _cleanup_cherry_pick EXIT INT TERM |
There was a problem hiding this comment.
Switching traps from INT TERM to EXIT INT TERM can change successful-script behavior: the cleanup now runs on normal exit too, which may unintentionally reset state (e.g., checking out the original branch even after a successful operation). Ensure the cleanup functions are explicitly gated (e.g., only run on interrupt/error) and don’t override intended final state.
Suggestion
Gate EXIT cleanup with an explicit flag, or trap ERR (where appropriate) instead of unconditional EXIT. Example pattern:
_cleanup_needed=0
trap '_cleanup_needed=1' INT TERM
trap '[[ ${_cleanup_needed} -eq 1 ]] && _cleanup_cherry_pick' EXIT(or set _cleanup_needed=1 only after the script actually changes branches).
Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
1 participant
Changes
Branch
development→main