From bdbeb9c21b9ca21c568f92882aeff63955439da2 Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah Date: Tue, 7 Jul 2026 16:50:50 -0700 Subject: [PATCH] Harden GitHub helper outputs --- CHANGELOG.md | 3 + lib/bash/gh/lib_gh.sh | 188 ++++++++++++++-------------------- lib/bash/gh/tests/lib_gh.bats | 138 +++++++++++++++++++++++++ 3 files changed, 219 insertions(+), 110 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e66ea1e..ec570c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ and versions are tracked in the repo-root `VERSION` file. - Made `gh_run` report failed GitHub CLI commands even when callers enable `set -e`, and preserved argument boundaries in GitHub command failure logs. - Documented and tested `str_split` trailing-separator behavior. +- Hardened GitHub helper pass-by-name outputs against internal variable + shadowing, added a non-optional `gh_infer_repo_from_origin` error message, + and deduplicated worktree parsing loops. ## [1.2.0] - 2026-07-04 diff --git a/lib/bash/gh/lib_gh.sh b/lib/bash/gh/lib_gh.sh index 9d8c5e2..644ac65 100644 --- a/lib/bash/gh/lib_gh.sh +++ b/lib/bash/gh/lib_gh.sh @@ -96,61 +96,62 @@ gh_repo_from_remote_url() { } gh_infer_repo_from_origin() { - local repo_dir="$1" - local result_var="${2:-}" - local optional=0 - local inferred_repo remote_url + local __gh_infer_repo_dir="$1" + local __gh_infer_result_name="${2:-}" + local __gh_infer_optional=0 + local __gh_infer_repo __gh_infer_remote_url - if [[ -z "$repo_dir" || -z "$result_var" ]]; then + if [[ -z "$__gh_infer_repo_dir" || -z "$__gh_infer_result_name" ]]; then log_error "Usage: gh_infer_repo_from_origin [--optional]" return 1 fi - assert_variable_name "$result_var" + assert_variable_name "$__gh_infer_result_name" if [[ "${3:-}" == "--optional" ]]; then - optional=1 + __gh_infer_optional=1 fi - remote_url="$(git -C "$repo_dir" remote get-url origin 2>/dev/null || true)" - if [[ -z "$remote_url" ]] || ! gh_repo_from_remote_url "$remote_url" inferred_repo; then - if ((optional)); then - printf -v "$result_var" '%s' "" + __gh_infer_remote_url="$(git -C "$__gh_infer_repo_dir" remote get-url origin 2>/dev/null || true)" + if [[ -z "$__gh_infer_remote_url" ]] || ! gh_repo_from_remote_url "$__gh_infer_remote_url" __gh_infer_repo; then + if ((__gh_infer_optional)); then + printf -v "$__gh_infer_result_name" '%s' "" return 0 fi + log_error "Could not infer GitHub repository from '$__gh_infer_repo_dir' origin remote." return 1 fi - printf -v "$result_var" '%s' "$inferred_repo" + printf -v "$__gh_infer_result_name" '%s' "$__gh_infer_repo" } gh_detect_default_branch() { - local repo_dir="$1" - local result_var="${2:-}" - local detected_branch + local __gh_detect_repo_dir="$1" + local __gh_detect_result_name="${2:-}" + local __gh_detect_branch - if [[ -z "$repo_dir" || -z "$result_var" ]]; then + if [[ -z "$__gh_detect_repo_dir" || -z "$__gh_detect_result_name" ]]; then log_error "Usage: gh_detect_default_branch " return 1 fi - assert_variable_name "$result_var" + assert_variable_name "$__gh_detect_result_name" - if detected_branch="$(git -C "$repo_dir" symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null)"; then - detected_branch="${detected_branch#origin/}" - if [[ -n "$detected_branch" ]]; then - printf -v "$result_var" '%s' "$detected_branch" + if __gh_detect_branch="$(git -C "$__gh_detect_repo_dir" symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null)"; then + __gh_detect_branch="${__gh_detect_branch#origin/}" + if [[ -n "$__gh_detect_branch" ]]; then + printf -v "$__gh_detect_result_name" '%s' "$__gh_detect_branch" return 0 fi fi - if git -C "$repo_dir" show-ref --verify --quiet refs/remotes/origin/main || - git -C "$repo_dir" show-ref --verify --quiet refs/heads/main; then - printf -v "$result_var" '%s' main + if git -C "$__gh_detect_repo_dir" show-ref --verify --quiet refs/remotes/origin/main || + git -C "$__gh_detect_repo_dir" show-ref --verify --quiet refs/heads/main; then + printf -v "$__gh_detect_result_name" '%s' main return 0 fi - if git -C "$repo_dir" show-ref --verify --quiet refs/remotes/origin/master || - git -C "$repo_dir" show-ref --verify --quiet refs/heads/master; then - printf -v "$result_var" '%s' master + if git -C "$__gh_detect_repo_dir" show-ref --verify --quiet refs/remotes/origin/master || + git -C "$__gh_detect_repo_dir" show-ref --verify --quiet refs/heads/master; then + printf -v "$__gh_detect_result_name" '%s' master return 0 fi @@ -158,29 +159,28 @@ gh_detect_default_branch() { } gh_repo_default_branch() { - local repo="$1" - local result_var="${2:-}" - local remote_default_branch status + local __gh_repo="$1" + local __gh_repo_result_name="${2:-}" + local __gh_repo_default_branch __gh_repo_status=0 - if [[ -z "$repo" || -z "$result_var" ]]; then + if [[ -z "$__gh_repo" || -z "$__gh_repo_result_name" ]]; then log_error "Usage: gh_repo_default_branch " return 1 fi - assert_variable_name "$result_var" + assert_variable_name "$__gh_repo_result_name" gh_require_cli || return 1 - remote_default_branch="$(gh repo view "$repo" --json defaultBranchRef --jq .defaultBranchRef.name 2>/dev/null)" - status=$? - if ((status != 0)); then - gh_report_command_failure "$status" repo view "$repo" --json defaultBranchRef --jq .defaultBranchRef.name + __gh_repo_default_branch="$(gh repo view "$__gh_repo" --json defaultBranchRef --jq .defaultBranchRef.name 2>/dev/null)" || __gh_repo_status=$? + if ((__gh_repo_status != 0)); then + gh_report_command_failure "$__gh_repo_status" repo view "$__gh_repo" --json defaultBranchRef --jq .defaultBranchRef.name return $? fi - if [[ -z "$remote_default_branch" ]]; then - log_error "GitHub repository '$repo' does not report a default branch." + if [[ -z "$__gh_repo_default_branch" ]]; then + log_error "GitHub repository '$__gh_repo' does not report a default branch." return 1 fi - printf -v "$result_var" '%s' "$remote_default_branch" + printf -v "$__gh_repo_result_name" '%s' "$__gh_repo_default_branch" } __gh_api_failure_retryable() { @@ -255,43 +255,29 @@ gh_worktree_path_for_branch() { local repo_dir="${2:-}" local target_ref="refs/heads/$branch" local line path="" ref + local -a git_cmd=(git) [[ -n "$branch" ]] || { log_error "Usage: gh_worktree_path_for_branch [repo_dir]" return 1 } - if [[ -n "$repo_dir" ]]; then - while IFS= read -r line; do - case "$line" in - "worktree "*) - path="${line#worktree }" - ;; - "branch "*) - ref="${line#branch }" - if [[ "$ref" == "$target_ref" ]]; then - printf '%s\n' "$path" - return 0 - fi - ;; - esac - done < <(git -C "$repo_dir" worktree list --porcelain) - else - while IFS= read -r line; do - case "$line" in - "worktree "*) - path="${line#worktree }" - ;; - "branch "*) - ref="${line#branch }" - if [[ "$ref" == "$target_ref" ]]; then - printf '%s\n' "$path" - return 0 - fi - ;; - esac - done < <(git worktree list --porcelain) - fi + [[ -z "$repo_dir" ]] || git_cmd=(git -C "$repo_dir") + + while IFS= read -r line; do + case "$line" in + "worktree "*) + path="${line#worktree }" + ;; + "branch "*) + ref="${line#branch }" + if [[ "$ref" == "$target_ref" ]]; then + printf '%s\n' "$path" + return 0 + fi + ;; + esac + done < <("${git_cmd[@]}" worktree list --porcelain) return 1 } @@ -299,46 +285,28 @@ gh_worktree_path_for_branch() { gh_list_worktree_branches() { local repo_dir="${1:-}" local line path="" branch="" - - if [[ -n "$repo_dir" ]]; then - while IFS= read -r line; do - case "$line" in - "") - if [[ -n "$path" && -n "$branch" ]]; then - branch="${branch#refs/heads/}" - printf '%s\t%s\n' "$path" "$branch" - fi - path="" - branch="" - ;; - "worktree "*) - path="${line#worktree }" - ;; - "branch "*) - branch="${line#branch }" - ;; - esac - done < <(git -C "$repo_dir" worktree list --porcelain; printf '\n') - else - while IFS= read -r line; do - case "$line" in - "") - if [[ -n "$path" && -n "$branch" ]]; then - branch="${branch#refs/heads/}" - printf '%s\t%s\n' "$path" "$branch" - fi - path="" - branch="" - ;; - "worktree "*) - path="${line#worktree }" - ;; - "branch "*) - branch="${line#branch }" - ;; - esac - done < <(git worktree list --porcelain; printf '\n') - fi + local -a git_cmd=(git) + + [[ -z "$repo_dir" ]] || git_cmd=(git -C "$repo_dir") + + while IFS= read -r line; do + case "$line" in + "") + if [[ -n "$path" && -n "$branch" ]]; then + branch="${branch#refs/heads/}" + printf '%s\t%s\n' "$path" "$branch" + fi + path="" + branch="" + ;; + "worktree "*) + path="${line#worktree }" + ;; + "branch "*) + branch="${line#branch }" + ;; + esac + done < <("${git_cmd[@]}" worktree list --porcelain; printf '\n') } gh_branch_upstream() { diff --git a/lib/bash/gh/tests/lib_gh.bats b/lib/bash/gh/tests/lib_gh.bats index 3b05bc2..bb4d77e 100644 --- a/lib/bash/gh/tests/lib_gh.bats +++ b/lib/bash/gh/tests/lib_gh.bats @@ -226,6 +226,30 @@ EOF [ "$repo" = "owner/repo" ] } +@test "gh_infer_repo_from_origin supports inferred_repo as the result variable name" { + local repo_dir="$TEST_TMPDIR/repo" + local inferred_repo="" + + init_git_repo "$repo_dir" + git -C "$repo_dir" remote add origin "git@github.com:owner/repo.git" + + gh_infer_repo_from_origin "$repo_dir" inferred_repo + + [ "$inferred_repo" = "owner/repo" ] +} + +@test "gh_infer_repo_from_origin supports remote_url as the result variable name" { + local repo_dir="$TEST_TMPDIR/repo" + local remote_url="" + + init_git_repo "$repo_dir" + git -C "$repo_dir" remote add origin "git@github.com:owner/repo.git" + + gh_infer_repo_from_origin "$repo_dir" remote_url + + [ "$remote_url" = "owner/repo" ] +} + @test "gh_infer_repo_from_origin returns empty success for non-GitHub remotes when optional" { local repo_dir="$TEST_TMPDIR/repo" local repo="sentinel" @@ -238,6 +262,20 @@ EOF [ "$repo" = "" ] } +@test "gh_infer_repo_from_origin logs non-optional inference failures" { + local repo_dir="$TEST_TMPDIR/repo" + local repo="sentinel" + + init_git_repo "$repo_dir" + git -C "$repo_dir" remote add origin "https://example.com/owner/repo.git" + + bats_run gh_infer_repo_from_origin "$repo_dir" repo + + [ "$status" -eq 1 ] + [ "$repo" = "sentinel" ] + [[ "$output" == *"Could not infer GitHub repository from '$repo_dir' origin remote."* ]] +} + @test "gh_detect_default_branch prefers origin HEAD and falls back to local main or master" { local repo_dir="$TEST_TMPDIR/repo" local branch="" @@ -271,6 +309,19 @@ EOF [ "$default_branch" = "main" ] } +@test "gh_detect_default_branch supports detected_branch as the result variable name" { + local repo_dir="$TEST_TMPDIR/repo" + local detected_branch="" + + init_git_repo "$repo_dir" + printf 'base\n' > "$repo_dir/data.txt" + commit_all "$repo_dir" "Initial commit" + + gh_detect_default_branch "$repo_dir" detected_branch + + [ "$detected_branch" = "main" ] +} + @test "gh_detect_default_branch reports failure when no default branch can be detected" { local repo_dir="$TEST_TMPDIR/repo" local branch="sentinel" @@ -319,6 +370,40 @@ EOF [ "$default_branch" = "develop" ] } +@test "gh_repo_default_branch supports remote_default_branch as the result variable name" { + local remote_default_branch="" + + create_fake_gh <<'EOF' +#!/usr/bin/env bash +if [[ "$1" == "repo" && "$2" == "view" ]]; then + printf 'develop\n' + exit 0 +fi +exit 99 +EOF + + gh_repo_default_branch "owner/repo" remote_default_branch + + [ "$remote_default_branch" = "develop" ] +} + +@test "gh_repo_default_branch supports status as the result variable name" { + local status="" + + create_fake_gh <<'EOF' +#!/usr/bin/env bash +if [[ "$1" == "repo" && "$2" == "view" ]]; then + printf 'develop\n' + exit 0 +fi +exit 99 +EOF + + gh_repo_default_branch "owner/repo" status + + [ "$status" = "develop" ] +} + @test "gh_api_with_retry retries retryable API pressure once" { create_fake_gh <<'EOF' #!/usr/bin/env bash @@ -380,6 +465,32 @@ EOF [ "$output" = "/tmp/feature" ] } +@test "gh_worktree_path_for_branch uses repo_dir when provided" { + create_fake_git <<'EOF' +#!/usr/bin/env bash +if [[ "$1" == "-C" && "$3" == "worktree" && "$4" == "list" ]]; then + printf 'repo-dir=%s\n' "$2" > "${TEST_TMPDIR:?}/git-repo-dir" + cat <<'OUT' +worktree /tmp/main +HEAD abc123 +branch refs/heads/main + +worktree /tmp/feature +HEAD def456 +branch refs/heads/feature/test +OUT + exit 0 +fi +exit 99 +EOF + + capture_command gh_worktree_path_for_branch feature/test "$TEST_TMPDIR/repo" + + [ "$status" -eq 0 ] + [ "$output" = "/tmp/feature" ] + [ "$(cat "$TEST_TMPDIR/git-repo-dir")" = "repo-dir=$TEST_TMPDIR/repo" ] +} + @test "gh_list_worktree_branches emits path and branch pairs" { create_fake_git <<'EOF' #!/usr/bin/env bash @@ -405,6 +516,33 @@ EOF [[ "$output" == *$'/tmp/feature\tfeature/test'* ]] } +@test "gh_list_worktree_branches uses repo_dir when provided" { + create_fake_git <<'EOF' +#!/usr/bin/env bash +if [[ "$1" == "-C" && "$3" == "worktree" && "$4" == "list" ]]; then + printf 'repo-dir=%s\n' "$2" > "${TEST_TMPDIR:?}/git-repo-dir" + cat <<'OUT' +worktree /tmp/main +HEAD abc123 +branch refs/heads/main + +worktree /tmp/feature +HEAD def456 +branch refs/heads/feature/test +OUT + exit 0 +fi +exit 99 +EOF + + capture_command gh_list_worktree_branches "$TEST_TMPDIR/repo" + + [ "$status" -eq 0 ] + [[ "$output" == *$'/tmp/main\tmain'* ]] + [[ "$output" == *$'/tmp/feature\tfeature/test'* ]] + [ "$(cat "$TEST_TMPDIR/git-repo-dir")" = "repo-dir=$TEST_TMPDIR/repo" ] +} + @test "gh_branch_upstream prints the configured upstream" { local repo_dir="$TEST_TMPDIR/repo" local remote_dir="$TEST_TMPDIR/remote.git"