fix: shell hook test suite passes with no ambient git identity - #43
Merged
Conversation
Every git-worktree-related test fixture in tests/run.sh chains `git commit --allow-empty` before `git worktree add` via &&, wrapped in 2>/dev/null. On a bare GitHub Actions runner (no global user.email/user.name - actions/checkout doesn't set one for the runner's global config), the commit fails with "Author identity unknown", the && chain short-circuits, git worktree add never runs, and every downstream worktree-detection assertion resolves against a directory that was never created - looking exactly like a bug in hooks/_lib.sh's worktree logic. It wasn't; that logic is untouched and correct. Reproduced locally via Docker (ubuntu:24.04, non-root, no git identity configured) - byte-for-byte matches the CI failure. Exporting GIT_AUTHOR_*/GIT_COMMITTER_* env vars at the top of the script (not global git config, so no side effects on the ambient environment) fixes it: 166/166 passing in the same reproduction, up from the cascading failures in #42.
session-onboard.sh's worktree-sharing note (issue #31) compared \$droot (canonicalized - see _tl_compute_data_root in _lib.sh) directly against \$root (deliberately raw - tl_root()'s own doc comment: it anchors file-path relativization elsewhere and must match tool input paths exactly). On any project whose raw path passes through a symlink - macOS's /tmp -> /private/tmp being the everyday case, which is exactly what mktemp -d resolves to in the test fixtures - the two strings never matched even for the literal main working tree, so onboard claimed data sharing on every single session there. This is a separate bug from the CI-only worktree-fixture failure this issue was originally filed for (fixed in tests/run.sh) - found while verifying that fix locally, where it was the one failure NOT explained by the missing git identity (it passes fine in CI/git 2.43, fails locally on git 2.55 due to the symlink shape of macOS's mktemp output). Fix: canonicalize \$root only at the comparison site, not the variable itself, so \$root's other uses (git -C "$root", live-git-state, file-path relativization) stay exactly as designed. 166/166 passing, both locally (macOS, git 2.55) and in the Linux reproduction used for the other #42 fix.
Review of this PR (two independent agents, converging on the same
finding) caught a regression in the session-onboard.sh fix: its premise
that "$droot is canonicalized" is false for two of
_tl_compute_data_root()'s five return paths - the worktree-list-
unparseable fallback and the migration-safety sticky branch both
returned the raw $_tl_wt instead of $_tl_wt_canonical, unlike the other
three. Comparing that raw value against the now-canonicalized $root in
session-onboard.sh meant a worktree hitting the migration-safety branch
(deliberately NOT sharing with main - the whole point of that branch)
would print a false "shared with the main working tree" note on any
symlinked path, exactly inverting the intended behavior.
Fix: canonicalize both raw-returning branches, so every exit path of
_tl_compute_data_root() honors its own documented contract ("gd == cd
means ... return the canonical path") rather than three of five. This is
the root-cause fix rather than a second canonicalize-at-the-call-site
patch - droot is now guaranteed canonical by construction for every
future caller, not just this one comparison.
Also adds the regression test both reviewers flagged as missing (pins
session-onboard.sh's printed message for the migration-safety branch,
not just tl_data_dir()'s return value, which 5e6 already covered) and
loud fixture-setup failure checks on the three git-worktree fixtures
(the exact pattern that made both this bug and the original #42 failure
read as product bugs instead of broken test setup).
Verified: the new test fails without this fix (confirmed by reverting
hooks/_lib.sh and re-running) and passes with it. 167/167 both locally
(macOS, git 2.55) and in the Linux reproduction used for the rest of #42.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #42, which reported the
shellCI job failing on GitHub Actions after re-enabling CI in #41. Two separate, unrelated bugs — both only manifest without an already-provided git identity / on a symlinked path, which is why they never surfaced until CI actually ran again.Fix 1: test fixtures assumed an ambient git identity
Every
git worktreetest fixture intests/run.shchainsgit commit --allow-emptybeforegit worktree addvia&&, wrapped in2>/dev/null. A bare GitHub Actions runner has no globaluser.email/user.nameconfigured (actions/checkoutdoesn't set one for the runner's global config, only for its own internal auth). The commit fails with "Author identity unknown", the&&chain short-circuits,git worktree addnever runs, and every downstream worktree-detection assertion resolves against a directory that was never created — looking exactly like a bug inhooks/_lib.sh's actual worktree logic. It wasn't; that logic is untouched.Fix: export
GIT_AUTHOR_NAME/GIT_AUTHOR_EMAIL/GIT_COMMITTER_NAME/GIT_COMMITTER_EMAILat the top oftests/run.sh(env vars, not global git config — no side effects on the ambient environment). Makes the suite hermetic regardless of what identity (if any) the host has configured.Fix 2: onboard's sharing-note compared a canonical path to a raw one
Found while verifying fix 1 locally — the one failure that showed up on macOS (git 2.55) but not in the Linux reproduction (git 2.43), so it isn't the same bug.
session-onboard.sh's worktree-sharing note (issue #31) compares$droot(canonicalized — see_tl_compute_data_root) directly against$root(deliberately raw, pertl_root()'s own doc comment — it anchors file-path relativization elsewhere and must match tool input paths exactly). On any project whose raw path passes through a symlink — macOS's/tmp→/private/tmpbeing the everyday case, which is exactly whatmktemp -dresolves to in the test fixtures — the two strings never match even for the literal main working tree, so onboard claimed data sharing on every session there.Fix: canonicalize
$rootonly at the comparison site, not the variable itself, so its other uses stay exactly as designed.Verification
ubuntu:24.04, non-root, no git identity configured) — byte-for-byte matches the CI failure log.local-ci --strict: all checks pass (shellcheck, manifest validation, hook tests).Related
Fixes #42.