fix: unit test isolation, CI feedback, and config priority#2
Conversation
- New create_pr.sh: GitHub PR creation via gh CLI (triggers Charlie CI) - New CGW_MERGE_MODE config: "direct" (local merge) or "pr" (GitHub PR) - Add --skip-lint / --skip-md-lint to commit_enhanced.sh, check_lint.sh, push_validated.sh - Add CGW_MARKDOWNLINT_CMD support for dedicated markdown linting - Fix ShellCheck warnings: export CGW_ALL_PREFIXES, disable SC2034 for reserved params - Fix SC2126 in merge_docs.sh: grep -vc instead of grep | wc -l - Update auto-git-workflow command with Phase 4A/4B (direct vs PR merge) - Update README with branch setup, PR workflow, CI section - Update cgw.conf.example with CGW_MERGE_MODE docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CLAUDE.md, SESSION_LOG.md, .claude/commands/, .claude/skills/ are local-only development artifacts and should not be committed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two test tiers covering 13 shell scripts: - Unit tests (tests/unit/): pure function tests for _common.sh and _config.sh - Integration tests (tests/integration/): full script tests against temp git repos Helpers: tests/helpers/setup.bash (temp repo creation) and mocks.bash (mock lint/gh CLI). CI: add test job to branch-protection.yml (bats-core installed via git clone). gitattributes: add *.bats and *.bash → LF enforcement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…apture, pipe quoting, test repo root
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
check_lint.sh has a real regression: formatting checks are now effectively disabled unless CGW_LINT_CMD is set, and the “no tools configured” fast-path ignores CGW_FORMAT_CMD. The .cgw.conf env-priority implementation can still yield inconsistent derived values because env vars are restored only after the config is evaluated. create_pr.sh masks branch-compare errors as “0 commits ahead” and should validate origin/${CGW_TARGET_BRANCH} (and ideally local-vs-remote sync) to avoid misleading failures. Test helpers have at least one portability issue (mktemp) and one integration test that doesn’t hit its intended code path.
Additional notes (4)
- Maintainability |
scripts/git/check_lint.sh:63-63
check_lint.shnow treats “no lint tool configured” as “no checks configured”, but it ignoresCGW_FORMAT_CMD. This means a repo that disables code lint (CGW_LINT_CMD="") but still wants formatting enforcement (CGW_FORMAT_CMDset) will incorrectly skip all checks and exit0.
Additionally, the format check is currently nested under if [[ -n "${CGW_LINT_CMD}" ]], so format will never run unless a lint command is also configured—this is a functional regression from the prior behavior.
- Maintainability |
scripts/git/push_validated.sh:177-185
push_validated.shdecides whether to run the pre-push lint step by checking onlyCGW_LINT_CMDandCGW_MARKDOWNLINT_CMD. This again ignoresCGW_FORMAT_CMD, so “format-only” setups won’t be validated on push even thoughcheck_lint.shis intended to cover formatting too.
Also, since check_lint.sh already knows how to self-skip, the extra gating here is easy to get out of sync (as happened with CGW_FORMAT_CMD).
-
Maintainability |
tests/integration/create_pr.bats:99-114
This test claims to cover the “no commits ahead of target” case, but it setsCGW_SOURCE_BRANCH=mainandCGW_TARGET_BRANCH=main, which will always hit the earlier “source == target” validation and never exercise the “ahead count is 0” path. That leaves the intended branch-comparison behavior untested. -
Compatibility |
scripts/git/configure.sh:239-246
Thesedreplacement for__CGW_LOCAL_FILES_PATTERN__is still not safe/correct for patterns containing backslashes (and&).
You’re building regex fragments like CLAUDE\.md (good), but when interpolated into sed "s|...|${...}|", sed treats backslashes in the replacement specially and can consume them, resulting in the generated hook containing CLAUDE.md (dot becomes wildcard) rather than CLAUDE\.md.
Escaping only the | delimiter is not sufficient here.
Summary of changes
Overview
This PR significantly expands CGW’s automation and quality gates:
- Config precedence: updates
scripts/git/_config.shsoCGW_*environment variables are restored after sourcing.cgw.conf(intended: env > config > defaults). - Lint flow updates: adds
--skip-lint/--skip-md-lintsupport and introduces an optional dedicated markdown lint step viaCGW_MARKDOWNLINT_CMDacross:scripts/git/check_lint.shscripts/git/commit_enhanced.shscripts/git/push_validated.sh
- New PR workflow: adds
scripts/git/create_pr.shto open GitHub PRs viagh(supports--draft,--dry-run, non-interactive mode) and documents PR-based promotion viaCGW_MERGE_MODE. - Docs + examples: updates
README.md,command/auto-git-workflow.md, andcgw.conf.exampleto describe branch setup, CI, PR mode, and new flags. - CI + tests: adds a GitHub Actions test job and introduces a comprehensive
bats-coretest suite (unit + integration), plus test fixtures/helpers. - Misc: enforces LF endings for
*.bats/*.bash, expands.gitignorefor local dev artifacts, and includes minor ShellCheck-driven cleanups (e.g.,merge_docs.sh,_common.sh).
| if [[ -f "${_CGW_CONF}" ]]; then | ||
| # Save CGW_* env vars so they take priority over .cgw.conf | ||
| _cgw_saved_env=$(env | grep "^CGW_" || true) | ||
| # shellcheck source=/dev/null | ||
| source "${_CGW_CONF}" | ||
| # Restore: env vars take priority over .cgw.conf values | ||
| while IFS= read -r _line; do | ||
| # shellcheck disable=SC2163 # export VAR=VALUE form: _line contains "KEY=VALUE" literal string | ||
| [[ -n "${_line}" ]] && export "${_line?}" | ||
| done <<< "${_cgw_saved_env}" | ||
| unset _cgw_saved_env _line |
There was a problem hiding this comment.
The .cgw.conf “env wins” implementation restores CGW_* variables after sourcing the config. That does make final values come from the environment, but it can still produce internally inconsistent config when .cgw.conf computes one variable from another.
Example failure mode:
- Env sets
CGW_TARGET_BRANCH=release .cgw.confsetsCGW_TARGET_BRANCH=mainand then setsCGW_PROTECTED_BRANCHES="${CGW_TARGET_BRANCH} staging"- After restore,
CGW_TARGET_BRANCHbecomesrelease, butCGW_PROTECTED_BRANCHESremainsmain staging
This contradicts the stated “three-tier resolution” behavior (env > config > defaults) in practice for derived values.
Suggestion
Instead of sourcing .cgw.conf verbatim and restoring afterward, load it in a way that skips assignments to variables that were already set in the environment, so env values remain visible throughout config evaluation.
One pragmatic approach (given .cgw.conf is generated and should mostly be assignments/comments) is:
# record exported CGW_* keys present before reading config
declare -A _cgw_env_keys=()
while IFS= read -r k; do _cgw_env_keys["${k}"]=1; done < <(compgen -e | grep '^CGW_' || true)
if [[ -f "${_CGW_CONF}" ]]; then
while IFS= read -r line; do
# pass through blank lines/comments
[[ -z "${line}" || "${line}" =~ ^[[:space:]]*# ]] && continue
if [[ "${line}" =~ ^[[:space:]]*(CGW_[A-Z0-9_]+)= ]]; then
key="${BASH_REMATCH[1]}"
[[ -n "${_cgw_env_keys[${key}]+x}" ]] && continue
fi
eval "${line}"
done < "${_CGW_CONF}"
fi
unset _cgw_env_keys key lineReply with "@CharlieHelps yes please" if you want me to put together a commit (and adjust/extend tests) for this safer precedence behavior.
| # Verify source branch exists locally and remotely | ||
| if ! git show-ref --verify "refs/heads/${CGW_SOURCE_BRANCH}" > /dev/null 2>&1; then | ||
| err "Source branch '${CGW_SOURCE_BRANCH}' does not exist locally" | ||
| echo " Create it with: git checkout -b ${CGW_SOURCE_BRANCH}" >&2 | ||
| log_section_end "BRANCH VALIDATION" "$logfile" "1" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! git ls-remote --exit-code origin "refs/heads/${CGW_SOURCE_BRANCH}" > /dev/null 2>&1; then | ||
| err "Source branch '${CGW_SOURCE_BRANCH}' not pushed to origin" | ||
| echo " Push it with: ./scripts/git/push_validated.sh" >&2 | ||
| log_section_end "BRANCH VALIDATION" "$logfile" "1" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check for commits ahead of target | ||
| local commits_ahead | ||
| commits_ahead=$(git rev-list --count "origin/${CGW_TARGET_BRANCH}..origin/${CGW_SOURCE_BRANCH}" 2>/dev/null || echo "0") | ||
|
|
||
| if [[ "${commits_ahead}" == "0" ]]; then | ||
| echo "[!] No commits ahead of ${CGW_TARGET_BRANCH} — nothing to PR" | tee -a "$logfile" | ||
| log_section_end "BRANCH VALIDATION" "$logfile" "1" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
create_pr.sh doesn’t verify that origin/${CGW_TARGET_BRANCH} exists and it masks git rev-list failures by coercing errors to 0. In those cases, the script will report “nothing to PR” even when the real problem is “target branch missing/unfetched” or another comparison error.
It also doesn’t check that the local source branch is actually in sync with origin/${CGW_SOURCE_BRANCH} (a common cause of surprise when the PR doesn’t include local commits).
Suggestion
Tighten branch validation and don’t coerce comparison errors into “0 commits”. For example:
# verify target exists remotely too
if ! git ls-remote --exit-code origin "refs/heads/${CGW_TARGET_BRANCH}" >/dev/null 2>&1; then
err "Target branch '${CGW_TARGET_BRANCH}' not found on origin"
log_section_end "BRANCH VALIDATION" "$logfile" "1"
exit 1
fi
# ensure local HEAD matches origin (optional but very useful)
if [[ "$(git rev-parse "${CGW_SOURCE_BRANCH}")" != "$(git rev-parse "origin/${CGW_SOURCE_BRANCH}")" ]]; then
err "Local '${CGW_SOURCE_BRANCH}' is not pushed to origin (run push_validated.sh)"
log_section_end "BRANCH VALIDATION" "$logfile" "1"
exit 1
fi
# fail loudly if rev-list cannot compare
if ! commits_ahead=$(git rev-list --count "origin/${CGW_TARGET_BRANCH}..origin/${CGW_SOURCE_BRANCH}" 2>/dev/null); then
err "Unable to compare origin/${CGW_TARGET_BRANCH}..origin/${CGW_SOURCE_BRANCH} (fetch? missing branch?)"
log_section_end "BRANCH VALIDATION" "$logfile" "1"
exit 1
fiReply with "@CharlieHelps yes please" if you’d like me to add a commit implementing these validation improvements.
| # create_temp_dir — set BATS_TEST_TMPDIR to an isolated temp dir | ||
| create_temp_dir() { | ||
| TEST_TMPDIR="$(mktemp -d)" | ||
| export TEST_TMPDIR | ||
| } |
There was a problem hiding this comment.
mktemp -d without a template is not portable across macOS/BSD mktemp (commonly errors with “too few X’s in template”). Since the repo targets macOS as well, the test helper should be portable to avoid local test runs failing.
Suggestion
Make temp dir creation portable across GNU/BSD mktemp implementations:
create_temp_dir() {
TEST_TMPDIR="$(mktemp -d 2>/dev/null || mktemp -d -t cgw.XXXXXX)"
export TEST_TMPDIR
}Reply with "@CharlieHelps yes please" if you want me to add a commit updating the helper accordingly.
| # hide_gh | ||
| # Removes `gh` from PATH by creating a directory-of-blocked-cmds | ||
| # that the shell searches before real PATH entries. `gh` is absent → command -v fails. | ||
| hide_gh() { | ||
| # setup_mock_bin already prepended MOCK_BIN_DIR; just ensure no gh there | ||
| rm -f "${MOCK_BIN_DIR}/gh" | ||
| } |
There was a problem hiding this comment.
hide_gh() doesn’t actually “hide” the system gh binary. It only removes ${MOCK_BIN_DIR}/gh, but PATH still contains the runner’s real gh, so command -v gh will succeed on environments where gh is preinstalled (notably GitHub-hosted runners). This makes the “no gh CLI in PATH” test non-deterministic and likely to fail in CI.
Suggestion
Make hide_gh() deterministic by running the script under a PATH that excludes the real gh while still allowing git.
One pragmatic approach for tests:
- Add a helper to install a
gitshim into${MOCK_BIN_DIR}:
install_git_shim() {
local real_git
real_git="$(command -v git)"
cat > "${MOCK_BIN_DIR}/git" <<EOF
#!/usr/bin/env bash
exec "${real_git}" "\$@"
EOF
chmod +x "${MOCK_BIN_DIR}/git"
}- Then in
hide_gh(), set:
install_git_shim
export PATH="${MOCK_BIN_DIR}:/bin"
rm -f "${MOCK_BIN_DIR}/gh"Reply with "@CharlieHelps yes please" if you want me to add a commit that implements this (and adjusts any tests that rely on the current PATH behavior).
| - name: Install bats-core and helpers | ||
| run: | | ||
| git clone --depth 1 https://github.com/bats-core/bats-core.git /tmp/bats | ||
| sudo /tmp/bats/install.sh /usr/local | ||
| git clone --depth 1 https://github.com/bats-core/bats-support.git /tmp/bats-support | ||
| git clone --depth 1 https://github.com/bats-core/bats-assert.git /tmp/bats-assert | ||
|
|
There was a problem hiding this comment.
The workflow installs bats via git clone --depth 1 from default branches. That’s brittle (upstream changes can break CI unexpectedly) and increases supply-chain risk compared to pinning versions.
This is especially relevant since branch protection will depend on these checks.
Suggestion
Pin bats-core/bats-support/bats-assert to tags or commit SHAs, or switch to a maintained action (e.g. bats-core/bats-action) with version pinning.
Reply with "@CharlieHelps yes please" if you want me to add a commit that pins these dependencies (either via tags/SHAs or an action).
Scripts: - check_lint.sh: Fix early-exit condition to include CGW_FORMAT_CMD; un-nest format check from lint check block so it runs independently (regression fix) - push_validated.sh: Add CGW_FORMAT_CMD to pre-push lint gate - _config.sh: Replace source+restore pattern with line-by-line parsing that skips vars already in env, fixing derived-value inconsistency; allow PROJECT_ROOT env var override to skip auto-detection - create_pr.sh: Add target branch remote validation; replace error-masking '|| echo 0' with proper exit-code check; add local/remote sync warning - configure.sh: Escape backslashes in sed replacement before escaping pipes Tests: - create_pr.bats: Fix 'no commits ahead' test to actually exercise that code path; fix _run_create_pr helper (PATH single-quote bug, stderr capture, empty-arg passthrough, PROJECT_ROOT isolation); fix inline bash -c blocks - mocks.bash: hide_gh() installs a shim that shadows system gh - setup.bash: Portable mktemp fallback for macOS/BSD CI: - branch-protection.yml: Pin bats installs to v1.13.0/v0.3.0/v2.2.4 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix five categories of pre-existing test isolation bugs: - configure.sh: guard PROJECT_ROOT detection with env var check so tests can pin the root to their temp repo (was always overwriting with real CGW root) - commit_enhanced.sh: treat --unknown-flag args as errors (exit 1) instead of silently capturing them as the commit message - validate_branches.sh: exit non-zero when CGW_SOURCE_BRANCH does not exist in the repo (was only warning about branch relationship) - merge_validation.bats: checkout development before calling _run_merge; merge_with_validation.sh refuses to run from the target branch - All integration _run_* helpers: remove bad export PATH= lines and add export PROJECT_ROOT= so scripts target the test repo, not the real project Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@CharlieHelps review latest commits |
There was a problem hiding this comment.
scripts/git/_config.sh’s.cgw.confloader is still too permissive/fragile: it can crash on non-assignment lines andevals anything not filtered, so it should be restricted to supportedCGW_*assignments.scripts/git/create_pr.shrelies onorigin/<branch>refs without ensuring they exist/are current; adding a targetedgit fetchwould prevent avoidable comparison failures.scripts/git/configure.sh’ssedreplacement escaping still misses&, which can corrupt generated hook patterns.tests/helpers/mocks.bashhide_gh()doesn’t actually test the “gh missing” branch becausecommand -v ghwill still succeed.
Additional notes (1)
- Maintainability |
README.md:110-117
This documentation recommends a rawgit pushduring branch setup. That conflicts with Git Workflow Rules [R1] (“Always use CGW wrapper scripts — never run rawgit push… directly”). If the intent is that initial branch setup is an exception, it should be stated; otherwise, readers will bypass safety checks by following the README.
Summary of changes
Summary
This PR expands CGW’s automation, configuration, CI, and test coverage.
CI / repo config
- Enforced LF endings for
*.bats/*.bashvia.gitattributes. - Added a test job to
.github/workflows/branch-protection.ymlrunning unit + integration Bats suites (with version-pinned Bats deps). - Ignored additional local-only dev artifacts in
.gitignore(CLAUDE.md,SESSION_LOG.md,.claude/...).
Config & workflow
- Updated
scripts/git/_config.shto:- Allow
PROJECT_ROOTto be pinned via env (test/CI isolation). - Load
.cgw.confline-by-line so env vars take priority during evaluation. - Introduce
CGW_MARKDOWNLINT_CMD/CGW_MARKDOWNLINT_ARGSandCGW_MERGE_MODE.
- Allow
- Improved hook generation in
configure.shby escaping backslashes beforesedsubstitution.
Lint/format gates
- Extended
check_lint.sh,commit_enhanced.sh, andpush_validated.shwith--skip-lint/--skip-md-lint(and env equivalents). - Fixed
check_lint.shto run format checks independently of code lint.
New functionality
- Added
scripts/git/create_pr.shto open PRs viagh(supports--draft,--dry-run,--non-interactive) and added docs for PR-based promotion.
Tests
- Added comprehensive Bats unit and integration test suites plus helpers/mocks and fixtures.
| # Read .cgw.conf line-by-line, only applying variables not already set in the environment. | ||
| # This ensures env vars take priority AND derived values (e.g. CGW_PROTECTED_BRANCHES | ||
| # referencing CGW_TARGET_BRANCH) stay consistent with the values actually used. | ||
| while IFS= read -r _line; do | ||
| [[ "${_line}" =~ ^[[:space:]]*# ]] && continue # skip comments | ||
| [[ "${_line}" =~ ^[[:space:]]*$ ]] && continue # skip blank lines | ||
| # Strip leading 'export ' if present, isolate the variable name | ||
| _cgw_var="${_line#export }" | ||
| _cgw_var="${_cgw_var%%=*}" | ||
| _cgw_var="${_cgw_var## }" | ||
| # Only set if not already in environment (preserves env var priority) | ||
| if [[ -z "${!_cgw_var+x}" ]]; then | ||
| # shellcheck disable=SC2163 # eval required: _line contains "KEY=VALUE" or "export KEY=VALUE" | ||
| eval "${_line}" | ||
| fi | ||
| done < "${_CGW_CONF}" |
There was a problem hiding this comment.
_config.sh now avoids a full source of .cgw.conf, but it still evals any non-comment/non-blank line and also assumes the left-hand side can be safely turned into a variable name.
That’s fragile:
- A malformed line (or even a legitimate non-assignment like
unset CGW_FOO) will produce an invalid_cgw_varand can hard-fail due to indirect expansion (${!_cgw_var+x}) underset -u. - It also expands the blast radius of
.cgw.confbeyond “assignments only” (even though the comment implies it’s assignment-focused).
Given .cgw.conf is user-editable, this should be hardened to only evaluate assignment lines you explicitly support (preferably only CGW_* keys).
Suggestion
Tighten parsing to only accept CGW_...=... (optionally prefixed by export) and derive the key via regex capture instead of string slicing. This avoids invalid indirect expansions and avoids executing unexpected shell statements.
Example:
if [[ -f "${_CGW_CONF}" ]]; then
while IFS= read -r _line; do
[[ "${_line}" =~ ^[[:space:]]*# ]] && continue
[[ "${_line}" =~ ^[[:space:]]*$ ]] && continue
if [[ "${_line}" =~ ^[[:space:]]*(export[[:space:]]+)?(CGW_[A-Z0-9_]+)= ]]; then
_cgw_var="${BASH_REMATCH[2]}"
if [[ -z "${!_cgw_var+x}" ]]; then
# shellcheck disable=SC2163 # eval required: config lines are assignments
eval "${_line}"
fi
else
err "Ignoring unsupported line in .cgw.conf: ${_line}"
fi
done < "${_CGW_CONF}"
unset _line _cgw_var
fiReply with "@CharlieHelps yes please" if you’d like me to add a commit implementing this hardened parser and adjusting tests accordingly.
| # Escape backslashes first (e.g. \. from dot-escaping), then | (sed delimiter) | ||
| local sed_files_pattern="${files_pattern//\\/\\\\}" | ||
| sed_files_pattern="${sed_files_pattern//|/\\|}" | ||
| mkdir -p "${PROJECT_ROOT}/.githooks" | ||
| sed "s|__CGW_LOCAL_FILES_PATTERN__|${files_pattern}|g" \ | ||
| sed "s|__CGW_LOCAL_FILES_PATTERN__|${sed_files_pattern}|g" \ |
There was a problem hiding this comment.
sed replacement escaping improved by handling backslashes and the | delimiter, but it still doesn’t escape &, which sed treats specially in the replacement (it expands to the entire match).
If a user adds a local-only file containing & to CGW_LOCAL_FILES, the generated hook pattern can be corrupted in a way that’s hard to debug.
Suggestion
Escape & for the sed replacement string in addition to backslashes and delimiter characters.
Example:
local sed_files_pattern="${files_pattern//\\/\\\\}"
sed_files_pattern="${sed_files_pattern//&/\\&}"
sed_files_pattern="${sed_files_pattern//|/\\|}"Reply with "@CharlieHelps yes please" if you want me to add a small commit covering this sed replacement edge case.
…tch, escape sed &, fix mock
Addressed Charlie CI Round-2 FeedbackAll 5 review items resolved in commit b7a525a:
Bonus: Added |
|
@CharlieHelps review latest commits |
There was a problem hiding this comment.
The .cgw.conf parser in _config.sh has broken comment/blank-line skip regexes, which will generate noisy “unsupported line” warnings for normal configs and should be fixed. create_pr.sh still has avoidable reliability and policy issues: it silences git fetch failures (risking stale comparisons) and prints guidance that violates Git Workflow Rules [R1] by recommending raw git push. push_validated.sh doesn’t honor CGW_SKIP_LINT, creating inconsistent env-driven behavior across scripts. commit_enhanced.sh introduces ShellCheck suppressions without the required [R6] justifications.
Additional notes (1)
- Maintainability |
scripts/git/push_validated.sh:71-71
push_validated.shhonorsCGW_SKIP_MD_LINT, but it does not honorCGW_SKIP_LINTeven thoughcheck_lint.shandcommit_enhanced.shdo. This inconsistency makes automation harder (env-based skipping works for commit/check, but not push).
Summary of changes
Overview
This PR expands CGW’s workflow automation, CI coverage, and configuration behavior.
Key changes
-
Repo / CI
- Enforce LF endings for
*.bats/*.bashvia.gitattributes. - Add a test job to
.github/workflows/branch-protection.ymlrunning Bats unit + integration suites (version-pinned deps). - Add
.shellcheckrcto align localshellcheckruns with CI. - Extend
.gitignorewith local dev artifacts (e.g.CLAUDE.md,.claude/...).
- Enforce LF endings for
-
Configuration + precedence
- Allow
PROJECT_ROOTto be provided via env to support test isolation. - Change
.cgw.confloading to a line-by-line parser that preserves env-var priority during evaluation. - Add config for
CGW_MARKDOWNLINT_CMD/CGW_MARKDOWNLINT_ARGSandCGW_MERGE_MODE.
- Allow
-
Workflow scripts
check_lint.sh: add--skip-lint/--skip-md-lint, independent format checks, and optional markdown lint step.commit_enhanced.sh: add--skip-lint/--skip-md-lintand markdown lint integration.push_validated.sh: pass through markdown-lint skipping tocheck_lint.shand include format/md-lint tools in gating.configure.sh: hardensedreplacement escaping (handle\,&, and|).- New
scripts/git/create_pr.shto open PRs viagh(supports--draft,--dry-run,--non-interactive).
-
Tests
- Add Bats helpers (
tests/helpers/setup.bash,tests/helpers/mocks.bash), a config fixture, and new unit/integration suites covering core scripts.
- Add Bats helpers (
Docs
- Update
README.md,command/auto-git-workflow.md, andcgw.conf.exampleto document PR workflow, branch setup, CI, and new config flags.
| local pr_url | ||
| if pr_url=$(gh pr create "${gh_flags[@]}" 2>&1 | tee -a "$logfile"); then | ||
| log_section_end "CREATE PR" "$logfile" "0" | ||
| echo "" | tee -a "$logfile" | ||
| echo "✓ PR created: ${pr_url}" | tee -a "$logfile" | ||
| echo "" | tee -a "$logfile" | ||
| if [[ ${draft} -eq 0 ]]; then | ||
| echo "Charlie CI will auto-review this PR." | tee -a "$logfile" | ||
| else | ||
| echo "Draft PR created. Mark as Ready for Review to trigger Charlie CI." | tee -a "$logfile" | ||
| fi | ||
| echo "Full log: $logfile" | ||
| else |
There was a problem hiding this comment.
pr_url currently captures all gh pr create output (including any non-URL text), and then you print it as PR created: ${pr_url}. If gh emits multi-line output, this will produce messy logs and potentially confuse callers that expect a single URL.
Suggestion
Prefer making gh return a single URL and log the full output separately. For example:
local pr_url
if pr_url=$(gh pr create "${gh_flags[@]}" --json url -q .url 2>&1 | tee -a "${logfile}"); then
echo "PR created: ${pr_url}" | tee -a "${logfile}"
else
...
fiReply with "@CharlieHelps yes please" if you’d like me to add a commit implementing this and adjusting the integration test assertions accordingly.
|
|
||
| # shellcheck disable=SC2086 | ||
| lint_output=$("${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$lint_output" ]] && [[ "$lint_output" != *"All checks passed"* ]]; then | ||
| echo "[LINT ERRORS]" | tee -a "$logfile" | ||
| echo "$lint_output" | tee -a "$logfile" | ||
| fi | ||
| local lint_output format_output | ||
|
|
||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| format_output=$("${CGW_FORMAT_CMD}" ${CGW_FORMAT_CHECK_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$format_output" ]] && [[ "$format_output" == *"would reformat"* ]]; then | ||
| echo "[FORMAT ERRORS]" | tee -a "$logfile" | ||
| echo "$format_output" | tee -a "$logfile" | ||
| lint_output=$("${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$lint_output" ]] && [[ "$lint_output" != *"All checks passed"* ]]; then | ||
| echo "[LINT ERRORS]" | tee -a "$logfile" | ||
| echo "$lint_output" | tee -a "$logfile" | ||
| fi | ||
| fi | ||
|
|
||
| log_section_end "LINT CHECK" "$logfile" "$python_lint_error" | ||
|
|
||
| if [[ ${python_lint_error} -eq 1 ]]; then | ||
| echo "[!] Lint errors detected" | ||
| if [[ ${non_interactive} -eq 1 ]]; then | ||
| echo "[Non-interactive] Auto-fixing lint issues..." | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_FIX_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| "${CGW_FORMAT_CMD}" ${CGW_FORMAT_FIX_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| fi | ||
|
|
||
| if [[ ${staged_only} -eq 0 ]]; then | ||
| git add . | ||
| unstage_local_only_files | ||
| format_output=$("${CGW_FORMAT_CMD}" ${CGW_FORMAT_CHECK_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$format_output" ]] && [[ "$format_output" == *"would reformat"* ]]; then | ||
| echo "[FORMAT ERRORS]" | tee -a "$logfile" | ||
| echo "$format_output" | tee -a "$logfile" | ||
| fi | ||
| fi | ||
|
|
||
| # Re-check | ||
| python_lint_error=0 | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" || python_lint_error=1 | ||
| log_section_end "LINT CHECK" "$logfile" "$python_lint_error" | ||
|
|
||
| if [[ ${python_lint_error} -eq 1 ]]; then | ||
| err "Lint errors remain after auto-fix" | ||
| exit 1 | ||
| fi | ||
| else | ||
| read -rp "Auto-fix lint issues? (yes/no/skip): " fix_lint | ||
| case "$fix_lint" in | ||
| yes|y) | ||
| if [[ ${python_lint_error} -eq 1 ]]; then | ||
| echo "[!] Lint errors detected" | ||
| if [[ ${non_interactive} -eq 1 ]]; then | ||
| echo "[Non-interactive] Auto-fixing lint issues..." | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_FIX_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_FIX_ARGS} ${CGW_LINT_EXCLUDES} | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| "${CGW_FORMAT_CMD}" ${CGW_FORMAT_FIX_ARGS} ${CGW_FORMAT_EXCLUDES} | ||
| fi | ||
| "${CGW_FORMAT_CMD}" ${CGW_FORMAT_FIX_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| fi | ||
|
|
||
| if [[ ${staged_only} -eq 0 ]]; then | ||
| git add . | ||
| unstage_local_only_files | ||
| ;; | ||
| skip|s) | ||
| echo "[!] Proceeding with lint warnings (CI may flag these)" ;; | ||
| *) | ||
| echo "Commit cancelled — fix lint errors first" | ||
| exit 1 ;; | ||
| esac | ||
| fi | ||
|
|
||
| # Re-check | ||
| python_lint_error=0 | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" || python_lint_error=1 | ||
|
|
||
| if [[ ${python_lint_error} -eq 1 ]]; then | ||
| err "Lint errors remain after auto-fix" | ||
| exit 1 | ||
| fi | ||
| else | ||
| read -rp "Auto-fix lint issues? (yes/no/skip): " fix_lint | ||
| case "$fix_lint" in | ||
| yes|y) | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_FIX_ARGS} ${CGW_LINT_EXCLUDES} | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| "${CGW_FORMAT_CMD}" ${CGW_FORMAT_FIX_ARGS} ${CGW_FORMAT_EXCLUDES} | ||
| fi | ||
| git add . | ||
| unstage_local_only_files |
There was a problem hiding this comment.
Several # shellcheck disable=SC2086 suppressions in the new/modified commit_enhanced.sh blocks are missing the required one-line justification comment.
This violates Shell Script Code Style [R6] (suppressions must include a justification).
Suggestion
Add justification comments everywhere SC2086 is suppressed, e.g.:
# shellcheck disable=SC2086 # Word splitting intentional: CGW_*_ARGS contains multiple flags
lint_output=$("${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1) || python_lint_error=1Reply with "@CharlieHelps yes please" if you want me to add a commit that updates the suppressions consistently across the modified blocks.
…sg, pr_url, CGW_SKIP_LINT, SC2086
Addressed Charlie CI Round-3 FeedbackAll 6 review items resolved in commit d044fdf:
|
|
@CharlieHelps review latest commits |
There was a problem hiding this comment.
The main functional issue is that commit_enhanced.sh still ties formatting checks to CGW_LINT_CMD, breaking “format-only” configurations and making behavior inconsistent with check_lint.sh/push_validated.sh. _config.sh now depends on err, which makes the config loader less self-contained than it should be. The test helper hide_gh() mutates PATH in a way that can remove unrelated system tools, risking environment-dependent failures. create_pr.sh’s URL extraction via regex is brittle compared to gh’s --json url output.
Summary of changes
Summary
This PR expands CGW’s workflow automation, CI, configuration precedence, and test coverage.
CI / repo hygiene
- Enforced LF endings for
*.bats/*.bashvia.gitattributes. - Added a Bats
testjob to.github/workflows/branch-protection.yml(pinned tags) and introduced.shellcheckrcfor local parity. - Extended
.gitignorewith local-only dev artifacts.
Config & precedence
scripts/git/_config.shnow supportsPROJECT_ROOToverrides and loads.cgw.confline-by-line so env vars take priority during evaluation.- Added config variables for
CGW_MARKDOWNLINT_CMD/CGW_MARKDOWNLINT_ARGSandCGW_MERGE_MODE.
Workflow scripts
check_lint.sh,commit_enhanced.sh, andpush_validated.shgained--skip-lint/--skip-md-lintand optional Markdown lint support.configure.shhook generation now escapes\,&, and|forsedreplacement safety.- Added new
scripts/git/create_pr.shto open PRs viaghwith branch validation +git fetch.
Tests
- Added unit + integration Bats suites plus shared helpers (
tests/helpers/setup.bash,tests/helpers/mocks.bash) and a.cgw.conffixture.
| if [[ -z "${CGW_LINT_CMD}" ]]; then | ||
| echo " (lint check skipped — CGW_LINT_CMD not set)" | ||
| else | ||
| log_section_start "LINT CHECK" "$logfile" | ||
|
|
||
| # Determine lint binary (venv or PATH) | ||
| local lint_cmd="${CGW_LINT_CMD}" | ||
| if [[ "${CGW_LINT_CMD}" == "ruff" ]]; then | ||
| get_python_path 2>/dev/null || true | ||
| if [[ -n "${PYTHON_BIN:-}" ]] && [[ -f "${PYTHON_BIN}/ruff${PYTHON_EXT:-}" ]]; then | ||
| lint_cmd="${PYTHON_BIN}/ruff${PYTHON_EXT:-}" | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| local lint_output format_output | ||
| local lint_output format_output | ||
|
|
||
| # shellcheck disable=SC2086 | ||
| lint_output=$("${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$lint_output" ]] && [[ "$lint_output" != *"All checks passed"* ]]; then | ||
| echo "[LINT ERRORS]" | tee -a "$logfile" | ||
| echo "$lint_output" | tee -a "$logfile" | ||
| fi | ||
|
|
||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| format_output=$("${CGW_FORMAT_CMD}" ${CGW_FORMAT_CHECK_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$format_output" ]] && [[ "$format_output" == *"would reformat"* ]]; then | ||
| echo "[FORMAT ERRORS]" | tee -a "$logfile" | ||
| echo "$format_output" | tee -a "$logfile" | ||
| # shellcheck disable=SC2086 # Word splitting intentional: CGW_LINT_CHECK_ARGS/CGW_LINT_EXCLUDES contain multiple flags | ||
| lint_output=$("${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$lint_output" ]] && [[ "$lint_output" != *"All checks passed"* ]]; then | ||
| echo "[LINT ERRORS]" | tee -a "$logfile" | ||
| echo "$lint_output" | tee -a "$logfile" | ||
| fi | ||
| fi | ||
|
|
||
| log_section_end "LINT CHECK" "$logfile" "$python_lint_error" | ||
|
|
||
| if [[ ${python_lint_error} -eq 1 ]]; then | ||
| echo "[!] Lint errors detected" | ||
| if [[ ${non_interactive} -eq 1 ]]; then | ||
| echo "[Non-interactive] Auto-fixing lint issues..." | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_FIX_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| "${CGW_FORMAT_CMD}" ${CGW_FORMAT_FIX_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 # Word splitting intentional: CGW_FORMAT_CHECK_ARGS/CGW_FORMAT_EXCLUDES contain multiple flags | ||
| format_output=$("${CGW_FORMAT_CMD}" ${CGW_FORMAT_CHECK_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$format_output" ]] && [[ "$format_output" == *"would reformat"* ]]; then | ||
| echo "[FORMAT ERRORS]" | tee -a "$logfile" | ||
| echo "$format_output" | tee -a "$logfile" | ||
| fi | ||
| fi | ||
|
|
||
| if [[ ${staged_only} -eq 0 ]]; then | ||
| git add . | ||
| unstage_local_only_files | ||
| fi | ||
| log_section_end "LINT CHECK" "$logfile" "$python_lint_error" | ||
|
|
||
| # Re-check | ||
| python_lint_error=0 | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" || python_lint_error=1 | ||
| if [[ ${python_lint_error} -eq 1 ]]; then | ||
| echo "[!] Lint errors detected" | ||
| if [[ ${non_interactive} -eq 1 ]]; then | ||
| echo "[Non-interactive] Auto-fixing lint issues..." | ||
| # shellcheck disable=SC2086 # Word splitting intentional: CGW_LINT_FIX_ARGS/CGW_LINT_EXCLUDES contain multiple flags | ||
| "${lint_cmd}" ${CGW_LINT_FIX_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 # Word splitting intentional: CGW_FORMAT_FIX_ARGS/CGW_FORMAT_EXCLUDES contain multiple flags | ||
| "${CGW_FORMAT_CMD}" ${CGW_FORMAT_FIX_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| fi |
There was a problem hiding this comment.
commit_enhanced.sh still treats formatting as a sub-part of the code lint block. If a repo intentionally disables code lint via CGW_LINT_CMD="" but keeps formatting enabled (CGW_FORMAT_CMD set), the format check (and format auto-fix) will never run here—while check_lint.sh/push_validated.sh will run format independently. That inconsistency makes “format-only” setups behave differently depending on which CGW entrypoint is used.
Suggestion
Refactor the code-quality step to run format independently of CGW_LINT_CMD (mirroring check_lint.sh). One approach:
- Track
lint_errorandformat_errorseparately. - Run the lint block only if
[[ -n "${CGW_LINT_CMD}" ]]. - Run the format block only if
[[ -n "${CGW_FORMAT_CMD}" ]]. - Combine them for the overall decision (
python_lint_error=1if either fails). - In the auto-fix path, run only the corresponding fix commands that are configured.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit implementing this refactor and updating/adding integration tests for the CGW_LINT_CMD="" + CGW_FORMAT_CMD!= "" case.
| if [[ -z "${CGW_LINT_CMD}" ]]; then | ||
| echo " (lint check skipped — CGW_LINT_CMD not set)" | ||
| else | ||
| log_section_start "LINT CHECK" "$logfile" | ||
|
|
||
| # Determine lint binary (venv or PATH) | ||
| local lint_cmd="${CGW_LINT_CMD}" | ||
| if [[ "${CGW_LINT_CMD}" == "ruff" ]]; then | ||
| get_python_path 2>/dev/null || true | ||
| if [[ -n "${PYTHON_BIN:-}" ]] && [[ -f "${PYTHON_BIN}/ruff${PYTHON_EXT:-}" ]]; then | ||
| lint_cmd="${PYTHON_BIN}/ruff${PYTHON_EXT:-}" | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| local lint_output format_output | ||
| local lint_output format_output | ||
|
|
||
| # shellcheck disable=SC2086 | ||
| lint_output=$("${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$lint_output" ]] && [[ "$lint_output" != *"All checks passed"* ]]; then | ||
| echo "[LINT ERRORS]" | tee -a "$logfile" | ||
| echo "$lint_output" | tee -a "$logfile" | ||
| fi | ||
|
|
||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| format_output=$("${CGW_FORMAT_CMD}" ${CGW_FORMAT_CHECK_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$format_output" ]] && [[ "$format_output" == *"would reformat"* ]]; then | ||
| echo "[FORMAT ERRORS]" | tee -a "$logfile" | ||
| echo "$format_output" | tee -a "$logfile" | ||
| # shellcheck disable=SC2086 # Word splitting intentional: CGW_LINT_CHECK_ARGS/CGW_LINT_EXCLUDES contain multiple flags | ||
| lint_output=$("${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$lint_output" ]] && [[ "$lint_output" != *"All checks passed"* ]]; then | ||
| echo "[LINT ERRORS]" | tee -a "$logfile" | ||
| echo "$lint_output" | tee -a "$logfile" | ||
| fi | ||
| fi | ||
|
|
||
| log_section_end "LINT CHECK" "$logfile" "$python_lint_error" | ||
|
|
||
| if [[ ${python_lint_error} -eq 1 ]]; then | ||
| echo "[!] Lint errors detected" | ||
| if [[ ${non_interactive} -eq 1 ]]; then | ||
| echo "[Non-interactive] Auto-fixing lint issues..." | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_FIX_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 | ||
| "${CGW_FORMAT_CMD}" ${CGW_FORMAT_FIX_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 # Word splitting intentional: CGW_FORMAT_CHECK_ARGS/CGW_FORMAT_EXCLUDES contain multiple flags | ||
| format_output=$("${CGW_FORMAT_CMD}" ${CGW_FORMAT_CHECK_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1) || python_lint_error=1 | ||
| if [[ -n "$format_output" ]] && [[ "$format_output" == *"would reformat"* ]]; then | ||
| echo "[FORMAT ERRORS]" | tee -a "$logfile" | ||
| echo "$format_output" | tee -a "$logfile" | ||
| fi | ||
| fi | ||
|
|
||
| if [[ ${staged_only} -eq 0 ]]; then | ||
| git add . | ||
| unstage_local_only_files | ||
| fi | ||
| log_section_end "LINT CHECK" "$logfile" "$python_lint_error" | ||
|
|
||
| # Re-check | ||
| python_lint_error=0 | ||
| # shellcheck disable=SC2086 | ||
| "${lint_cmd}" ${CGW_LINT_CHECK_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" || python_lint_error=1 | ||
| if [[ ${python_lint_error} -eq 1 ]]; then | ||
| echo "[!] Lint errors detected" | ||
| if [[ ${non_interactive} -eq 1 ]]; then | ||
| echo "[Non-interactive] Auto-fixing lint issues..." | ||
| # shellcheck disable=SC2086 # Word splitting intentional: CGW_LINT_FIX_ARGS/CGW_LINT_EXCLUDES contain multiple flags | ||
| "${lint_cmd}" ${CGW_LINT_FIX_ARGS} ${CGW_LINT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| if [[ -n "${CGW_FORMAT_CMD}" ]]; then | ||
| # shellcheck disable=SC2086 # Word splitting intentional: CGW_FORMAT_FIX_ARGS/CGW_FORMAT_EXCLUDES contain multiple flags | ||
| "${CGW_FORMAT_CMD}" ${CGW_FORMAT_FIX_ARGS} ${CGW_FORMAT_EXCLUDES} 2>&1 | tee -a "$logfile" | ||
| fi |
There was a problem hiding this comment.
commit_enhanced.sh still effectively couples formatting enforcement to whether CGW_LINT_CMD is set: when CGW_LINT_CMD="" (code lint disabled) but CGW_FORMAT_CMD is configured, the script prints “lint check skipped” and never runs the format check. That’s inconsistent with check_lint.sh (format runs independently) and makes “format-only” repos silently lose formatting protection at commit time.
Separately, the format invocation uses ${CGW_FORMAT_CMD} directly, while the lint path resolution special-cases ruff to use the .venv binary. If ruff is only available in .venv, lint will work but format can fail with command not found (or run the wrong global binary).
Suggestion
Refactor the “code quality” step so format runs independently of code lint (and resolve the format binary the same way as lint when CGW_FORMAT_CMD=ruff). For example:
# Resolve lint_cmd (only if CGW_LINT_CMD set)
local lint_cmd="${CGW_LINT_CMD}"
if [[ -n "${CGW_LINT_CMD}" && "${CGW_LINT_CMD}" == "ruff" ]]; then
get_python_path 2>/dev/null || true
if [[ -n "${PYTHON_BIN:-}" ]] && [[ -f "${PYTHON_BIN}/ruff${PYTHON_EXT:-}" ]]; then
lint_cmd="${PYTHON_BIN}/ruff${PYTHON_EXT:-}"
fi
fi
# Resolve format_cmd independently
local format_cmd="${CGW_FORMAT_CMD}"
if [[ -n "${CGW_FORMAT_CMD}" && "${CGW_FORMAT_CMD}" == "ruff" ]]; then
get_python_path 2>/dev/null || true
if [[ -n "${PYTHON_BIN:-}" ]] && [[ -f "${PYTHON_BIN}/ruff${PYTHON_EXT:-}" ]]; then
format_cmd="${PYTHON_BIN}/ruff${PYTHON_EXT:-}"
fi
fi
# Then run lint (if configured) and format (if configured) as separate checks.If you’d like, I can put up a commit that (1) decouples format from CGW_LINT_CMD, (2) resolves ruff for format via .venv like lint does, and (3) updates/extends the Bats coverage for “format-only” setups. Reply with "@CharlieHelps yes please" and I’ll add it.
| if [[ -f "${_CGW_CONF}" ]]; then | ||
| # shellcheck source=/dev/null | ||
| source "${_CGW_CONF}" | ||
| # Read .cgw.conf line-by-line, only applying variables not already set in the environment. | ||
| # This ensures env vars take priority AND derived values (e.g. CGW_PROTECTED_BRANCHES | ||
| # referencing CGW_TARGET_BRANCH) stay consistent with the values actually used. | ||
| while IFS= read -r _line; do | ||
| [[ "${_line}" =~ ^[[:space:]]*# ]] && continue # skip comments | ||
| [[ "${_line}" =~ ^[[:space:]]*$ ]] && continue # skip blank lines | ||
| # Only accept CGW_* assignment lines (optionally prefixed with export). | ||
| # Reject anything else to prevent eval of arbitrary shell statements. | ||
| if [[ "${_line}" =~ ^[[:space:]]*(export[[:space:]]+)?(CGW_[A-Z0-9_]+)= ]]; then | ||
| _cgw_var="${BASH_REMATCH[2]}" | ||
| # Only set if not already in environment (preserves env var priority) | ||
| if [[ -z "${!_cgw_var+x}" ]]; then | ||
| # shellcheck disable=SC2163 # eval required: _line contains "KEY=VALUE" or "export KEY=VALUE" | ||
| eval "${_line}" | ||
| fi | ||
| else | ||
| err "Ignoring unsupported line in .cgw.conf: ${_line}" | ||
| fi | ||
| done < "${_CGW_CONF}" | ||
| unset _line _cgw_var |
There was a problem hiding this comment.
_config.sh calls err when encountering an unsupported .cgw.conf line. That introduces an unnecessary dependency on _common.sh’s helper functions and makes _config.sh more fragile when sourced in isolation (e.g., unit tests or any future tooling that loads config without _common.sh).
Suggestion
Avoid calling err from _config.sh. Use a direct stderr write (or a tiny local _config_warn() helper) so _config.sh remains self-contained:
# inside the parser
printf '%s\n' "[WARN] Ignoring unsupported line in .cgw.conf: ${_line}" >&2Reply with "@CharlieHelps yes please" if you’d like me to add a commit to decouple _config.sh from _common.sh in this spot (and adjust tests if needed).
Addressed Charlie CI Round-4 FeedbackAll 3 items resolved in commit 0af3b42:
|
1 participant
Changes
Test Status
All 136 tests passing (44 unit + 92 integration).
Branch
development→main