Recovered review findings for dotfiles from the pending-issues fallback queue (13 items)

[twistedmelonman]

Recovered from the ~/.claude/pending-issues/ fallback queue during #231 triage. These findings belong to this repo and were never filed, because the fallback writer records no target repo.

Batched into one issue so a review's findings stay one unit of attention. Tick items off as they are addressed; close when the list is done or the remainder is judged not worth doing.

Where a finding was verified against current code during triage, the verification is noted inline.

---

• Tag pinning is less supply-chain-resistant than SHA pinning for reusable workflows (.github/workflows/claude-blocking-review.yml, .github/workflows/dependabot-auto-merge.yml)

  Switching from commit-SHA pins to tag pins (e.g. @v3.1.0, @dependabot-auto-merge-v2) trades supply-chain immutability for readability/maintainability, since tags can be moved or re-pointed by the upstream repo owner. This is presumably an intentional tradeoff given the PR title says "standardize on tag-pinning," but worth confirming that smartwatermelon/github-workflows tags are treated as immutable/protected upstream, or that this risk is otherwise accepted.

  Verified 2026-08-24: still present — claude-blocking-review.yml@v3, claude-assistant.yml@v3, dependabot-auto-merge.yml@dependabot-auto-merge-v2. All mutable refs.

• tolerate_upgrade_failure=true silently swallows all formula failures in non-interactive mode, not just sudo-related ones (bash/functions.sh:377-379)

  In non-interactive mode the error tolerance was originally intended to handle cask sudo-failures gracefully. Now that casks are skipped entirely, tolerate_upgrade_failure=true applies to all brew upgrade --formula failures — including a genuinely broken formula (network error, checksum mismatch, etc.). The call chain at line 663 (_homebrew_update || return $?) relies on _homebrew_update propagating real errors, but in non-interactive mode it will always return 0 even if no formulae upgraded successfully. The log captures the exit code ("brew upgrade (formulae) completed with errors (exit N) - check log"), so visibility exists, but the upstream updates() orchestrator won't abort on a total formula failure. This is a pre-existing design trade-off that was present in the old code too — it's not introduced by this diff — but it's worth tracking. If a formula consistently fails, nightly runs will silently no-op on it indefinitely.

  Verified 2026-08-24: still present at bash/functions.sh:368,371,409.

• Malformed 37-character fake SHA in stale-CI test case (bash/tests/test-pre-push-stale-ci.sh:309)

  OLD_SHA_2="1111111111111111111111111111111111111" is 37 characters, not 40. Git SHAs are 40 hex characters. The test still passes correctly because the hook's staleness detection only needs headRefOid != pushed_sha, and a 37-char string is definitionally ≠ a real 40-char SHA. But the mock doesn't resemble a realistic stale-SHA scenario (e.g., a prior commit's SHA). Consider using a full 40-char string like "1111111111111111111111111111111111111111" to match the shape of real values the hook will encounter in production. No correctness impact on either the test or the hook.

  Verified 2026-08-24: confirmed — OLD_SHA_2 at bash/tests/test-pre-push-stale-ci.sh:127 is 37 characters, not 40.

• test-gpush-wrapper-guard.sh has no Case 2 (bash/tests/test-gpush-wrapper-guard.sh:36)

  Merged from two findings covering the same gap.

  Verified 2026-08-24: confirmed — the case comments run Case 1 (line 33) to Case 3 (line 36) to Case 4 (line 69). Case 2 is absent entirely, so the numbering implies coverage that does not exist. One of the two original findings additionally noted that a Case 2 asserting rejection of extra positional args would pass for the wrong reason, since gpush does not validate them.

• Temp-file leak in test-gpush-wrapper-guard.sh (bash/tests/test-gpush-wrapper-guard.sh:46-67)

  Merged from two findings covering the same defect.

  The rm -f "/tmp/gpush-test-main-$$" at line 62 is dead code — it's placed after exit 0 and exit 1 branches, so it never executes. The outer rm -f "/tmp/gpush-test-main-$$" at line 70 runs in the outer shell where $$ is a different PID, so it names a different file. The temp file /tmp/gpush-test-main-<subshell-PID> leaks on every test run. Low severity (temp files, small, named for cleanup by OS on reboot), but fixable by using trap 'rm -f "${tmpfile}"' EXIT with a named variable inside the subshell, or by redirecting output to a variable instead of a temp file.

  The /bin/bash -c subshell at line 42 creates /tmp/gpush-test-main-$$ where $$ is the subshell's PID. The outer script's cleanup at line 67 removes /tmp/gpush-test-main-$$ where $$ is the outer script's PID — a different value. Because $$ is inside single quotes in the heredoc-like -c string, it expands at subshell runtime, not at interpolation time, meaning the PIDs never match. The result is a temp file left in /tmp after each test run. Not a test correctness issue (the test outcome is unaffected), but accumulates cruft over many runs. Fix: pass the filename from the outer shell (e.g., via an env var: GPUSH_TMP_MAIN=$(mktemp) /bin/bash -c '... gpush >"${GPUSH_TMP_MAIN}" 2>&1 ...').

• Test aborts silently on unexpected git command failure rather than printing "FAILED" (bash/tests/test-git-wrapper-init-hook.sh:13)

  The test uses set -euo pipefail (line 13) together with a fail=0 accumulation pattern. If any git init subshell (e.g. lines 64, 66, 71, 73, 84) fails unexpectedly due to an environment problem (missing git, bad temp dir, etc.), the script exits immediately via -e without ever printing "FAILED" or an exit code of 1 from that path — it just stops. This is a minor test-harness UX issue: a failing environment looks like a clean exit to callers that don't check stderr. The other test files in this suite have the same pattern, so this is consistent pre-existing behavior, not introduced by this diff. If test infrastructure robustness matters, wrapping the per-case git init calls in if ! (cd ... && git init) 2>&1; then echo "FAIL: git init failed"; fail=1; fi would give better error attribution on environmental failures.

• GIT_CONFIG_COUNT requires git ≥ 2.31; undocumented minimum version assumption (bash/tests/test-git-wrapper-init-hook.sh:40)

  GIT_CONFIG_COUNT/GIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_N was introduced in git 2.31 (March 2021). Other tests in this repo use -c key=value per-command flags which work on older git. This test now silently fails in a confusing way on any machine with git < 2.31 — the env vars are silently ignored, template hooks still get symlinked in, and the cat > clobber bug returns. A comment noting the minimum git version, or a version guard (git --version | awk ... check at test startup), would prevent future debugging confusion. Low urgency given it's 2026, but worth a note.

• Existing test files use unguarded cd for REPO_ROOT resolution — inconsistent with new CDPATH-hardening pattern (bash/tests/test-git-wrapper-init-hook.sh:17)

  The four existing test files (test-gpush-wrapper-guard.sh, test-path-order.sh, test-git-wrapper-init-hook.sh, test-gh-wrapper-draft-off-org.sh, test-gh-wrapper-identity.sh) still use $(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd) without CDPATH=''. They each mitigate via unset CDPATH at the top, which prevents CDPATH resolution entirely — so they're functionally safe. However, this creates an inconsistency: the new test uses CDPATH='' cd (per-invocation guard), while older tests use unset CDPATH (script-scope). The dirname output for these scripts is always something like /Users/.../.../bash/tests, which is an absolute path, so CDPATH resolution can't trigger on it regardless. Still, as the codebase documents the CDPATH='' cd idiom as the preferred per-invocation pattern (env.sh comment, CDPATH without "." still echoes path for CDPATH-resolved directories #176), it would be worth updating the existing tests for consistency in a follow-up.

• Test file: subshell fail variable cannot propagate back to parent via cwd_fail_file if the entire subshell exits non-zero before the touch line (bash/tests/test-gh-wrapper-draft-off-org.sh:93)

  The subshell is run under set -euo pipefail. If assert_args itself throws an unhandled error (not via fail=1 but via an actual non-zero exit), the subshell exits immediately before reaching the if [[ "${fail}" == "1" ]]; then touch "${cwd_fail_file}"; fi sentinel. The parent then sees no cwd_fail_file, incorrectly treating the test as passing. This is a low-probability test-reliability concern (not a correctness concern in the wrapper itself), but the silent-pass-on-error pattern is worth noting. The || true guard on assert_args calls, or wrapping in ( set +e; ... ), would make failures visible either way.

• git init does not invoke post-checkout; the hook comment and README mislead on this (pre-push whole-codebase review, git/template/hooks/post-checkout:6-21)

• Case H: first run_hook does not pass user.email/user.name, making it dependent on global git config (pre-push whole-codebase review, bash/tests/test-post-checkout-gitignore.sh:272-274)

    The file header says "This hook runs after: git init (if template configured)" and the comment on line 21 says "For git init, prev_head is all zeros (0000000...)". Both claims are incorrect: `git init` does not invoke `post-checkout` at all — that hook is only called by `git clone` (during its internal initial-checkout step) and by explicit `git checkout`/`git checkout -b` commands. The `init.templateDir` mechanism copies the hook file into `.git/hooks/`, but `git init` never runs it; only a subsequent `git checkout` would. The test suite accounts for this correctly by using `run_hook` (direct invocation) for cases E–K and real `git clone` for A–D, so there is no functional defect in the hook itself. The misleading comment could cause a future maintainer to incorrectly assume `git init` fires the hook and add an `init`-only code path that never runs.
    Cases A–D use `make_origin` which sets `user.email` and `user.name`, but cases E–K (including Case H) initialize bare repos with `git init` and do not set them. The `run_hook` function invokes the hook directly without making a commit, so no git identity is actually needed for these cases. However, if a future test extends Case H to make a commit (e.g., to test the tracked-`.claude/` guard in a committed repo), the test will fail on machines with no global git identity configured — a CI environment behavior that's not tested locally. This is a latent fragility, not a current failure.
  

• SKIP message goes to stdout, not stderr — may be swallowed in some CI log formats (pre-push whole-codebase review, bash/tests/test-git-config-hygiene.sh:96)

    The SKIP line echoes to stdout (`echo "  SKIP: ..."`), consistent with how PASS lines are emitted. This is fine and intentional, but if a CI pipeline only captures stderr for test output, the SKIP will be invisible. The existing `_fail` helper goes to stderr; `_pass` goes to stdout — so this is consistent with the existing pattern. No change needed, but worth knowing if CI log parsing is ever added.
  

• Stale .ralph/ references in historical plan doc (pre-push whole-codebase review, docs/plans/2026-03-26-symlink-migration-plan.md:74,112)

    Two mentions of `.ralph/pre-push` remain in the migration plan file after the rename to `.project-hooks/`. These are in a dated historical document that describes past state rather than current behavior, so they carry no runtime risk — no hook or CI step reads them. Worth updating for future readers who might search for `.ralph` to understand the hook extension seam and land on stale context, but this doesn't block the push.
  

  Verified 2026-08-24: confirmed at lines 79 and 117. Note line 5 of that doc already carries a disclaimer that .ralph/ was renamed in Rename the .ralph/ project-local hook seam — the experiment it is named for is over #233, which reduces the severity.

[twistedmelonman]

Triaged all 13 findings against current `main`. Six are already fixed, two are non-findings, and the one substantive item belongs to an existing plan.

Already fixed — verified in current code

• .3 37-char fake SHA → now `OLD_SHA_2="1111111111111111111111111111111111111111"`, measured 40 chars (`test-pre-push-stale-ci.sh:156`)
• .4 missing Case 2 → `# Case 2: refuses to run on main` exists with a real fixture and asserted preconditions (`test-gpush-wrapper-guard.sh:82-90`)
• .5 temp-file leak via mismatched `$$` → capture file moved inside `WORKDIR` (commit `08789fa`)
• .7 `GIT_CONFIG_COUNT` version requirement → explicit guard parses `git --version` and hard-fails below 2.31, citing this issue (`test-git-wrapper-init-hook.sh:51-68`)
• .9 subshell sentinel failing open → inverted to a fail-closed completion sentinel (commit `8d878db`)
• .10 post-checkout comment wrongly claiming `git init` fires the hook → corrected to "On `git init`, git itself never fires post-checkout" (commit `08789fa`)

Non-findings

• .12 SKIP to stdout — the issue itself concludes "No change needed"; consistent with `_pass`
• .13 stale `.ralph/` refs in a plan doc — mitigated by the disclaimer at `:5`

Remaining, low priority

.2 (`tolerate_upgrade_failure=true` swallows formula failures, `bash/functions.sh:409`), .6 (test aborts silently on unexpected `git init` failure), .8 (CDPATH pattern inconsistency, "functionally safe" by the issue's own admission), .11 (Case H lacks `user.email`/`user.name` — latent only, `run_hook` makes no commit).

The substantive item is already planned

.1 (SHA-pinning the four `smartwatermelon/github-workflows` refs) is real — all four are still mutable tags:

• `claude-blocking-review.yml:21,34` → `@v3`
• `claude.yml:23` → `@v3`
• `dependabot-auto-merge.yml:31` → `@dependabot-auto-merge-v2`

This should not be fixed here in isolation. `dev-env` has a plan for it — `docs/plans/2026-08-27-workflow-sha-pinning.md` — which sequences `dev-env` first as end-to-end validation, then propagates to the other 15 repos, and folds `dependabot-auto-merge.yml` into a centralized reusable workflow. Pinning dotfiles alone would leave it inconsistent with the rest of the fleet and skip that plan's Phase 1 prerequisite.

One trap worth recording for whoever executes it: `v3` and `v3.2.1` both resolve to commit `688e0288`, but `v3.2.1` is an annotated tag whose own object SHA is `6b3f74a8`. The correct pin is the dereferenced commit (`git ls-remote ... "refs/tags/v3.2.1^{}"`), not the tag object.

Leaving this issue open, scoped to .1 (tracked by the dev-env plan) plus the four low-priority items above.