From 1df08e86bbd164265e502665f4a7723b5babc82b Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 17 Jul 2026 19:51:58 +0200 Subject: [PATCH] ci: pre-push guard against pushing an uncommitted working-tree change (H13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A file in the pushed diff that still has uncommitted working-tree/index changes means the pushed commit doesn't match what you built/reviewed — the #416 index/worktree trap, where a --no-renames review fix lived only in the working tree and shipped without being committed (CI, the push, and the reviewer each saw a different tree). New fail-open pre-push hook blocks that precisely (only files in the branch's diff vs origin/main), after the H11 rebase check. Escape: ETV_ALLOW_DIRTY_PUSH=1. Documented in decisions.md. Refs #416 --- .claude/hooks/prepush-clean-worktree-check.sh | 43 +++++++++++++++++++ .husky/pre-push | 6 +++ docs/decisions.md | 20 +++++++++ 3 files changed, 69 insertions(+) create mode 100755 .claude/hooks/prepush-clean-worktree-check.sh diff --git a/.claude/hooks/prepush-clean-worktree-check.sh b/.claude/hooks/prepush-clean-worktree-check.sh new file mode 100755 index 000000000..4b49ae874 --- /dev/null +++ b/.claude/hooks/prepush-clean-worktree-check.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +# H13 (ersatztv#416 session) — refuse to push when a file in the pushed diff still has UNCOMMITTED +# changes in the working tree or index. That is the "I left part of my intended change behind" +# failure: a fix edited into the working file but never committed (e.g. after a `git reset --soft` +# that re-staged a stale index) gets pushed WITHOUT the fix — while local tests and a working-tree +# review both see the fix that never shipped. This bit the #416 session: a `--no-renames` review fix +# lived only in the working tree, so the pushed commit, CI, and the first re-review each saw a +# different tree, and a PR went out still carrying the bug the review had "confirmed" fixed. +# +# Scope is deliberately PRECISE to keep false positives near zero: it blocks only when a dirty +# tracked file is ALSO part of this branch's diff vs origin/main. Unrelated uncommitted scratch in a +# file the push doesn't touch is fine; untracked files are ignored. +# +# Fail-OPEN on anything we can't decide (a git pre-push hook has no "ask"): not a git repo, offline / +# no origin/main, HEAD unresolved -> allow. Deliberate escape: ETV_ALLOW_DIRTY_PUSH=1. +set -uo pipefail + +[ "${ETV_ALLOW_DIRTY_PUSH:-}" = "1" ] && exit 0 +git rev-parse --git-dir >/dev/null 2>&1 || exit 0 + +# Files with uncommitted changes vs HEAD — unstaged AND staged-but-uncommitted, tracked only. +dirty="$( { git diff --name-only; git diff --cached --name-only; } 2>/dev/null | sort -u )" +[ -z "$dirty" ] && exit 0 # clean tree -> nothing to guard + +# The set of files this branch introduces vs origin/main (the "pushed diff"). Best-effort fetch; +# if origin/main is unavailable we cannot scope precisely -> fail open rather than over-block. +git fetch origin main --quiet 2>/dev/null || exit 0 +git rev-parse --verify --quiet origin/main >/dev/null 2>&1 || exit 0 +pushed="$( git diff --name-only "origin/main...HEAD" 2>/dev/null | sort -u )" +[ -z "$pushed" ] && exit 0 + +# Intersection: dirty files that are part of the pushed diff. +both="$( comm -12 <(printf '%s\n' "$dirty") <(printf '%s\n' "$pushed") )" +[ -z "$both" ] && exit 0 + +branch=$(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo HEAD) +echo "husky - push blocked (H13): '$branch' has UNCOMMITTED changes to file(s) that are part of" +echo " what you're pushing — the pushed commit does NOT match your working tree, so a local fix" +echo " or review may be shipping without its change (the #416 index/worktree trap):" +printf '%s\n' "$both" | sed 's/^/ /' +echo " Commit them (or 'git checkout --' to discard), then push. If the difference is intentional" +echo " and unrelated, bypass with: ETV_ALLOW_DIRTY_PUSH=1 git push" +exit 1 diff --git a/.husky/pre-push b/.husky/pre-push index ef691778f..345e96343 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -15,6 +15,12 @@ unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE # hook on code that isn't yours). Fail-open; escape with ETV_SKIP_REBASE_CHECK=1. ./.claude/hooks/prepush-rebase-check.sh || exit 1 +# H13 (ersatztv#416 session): refuse to push when a file in the pushed diff still has uncommitted +# working-tree/index changes — the pushed commit wouldn't match what you built/reviewed (the #416 +# index/worktree trap: a review fix left in the working tree shipped without being committed). +# Runs before the slow CI-parity checks so it fails fast. Fail-open; escape ETV_ALLOW_DIRTY_PUSH=1. +./.claude/hooks/prepush-clean-worktree-check.sh || exit 1 + # CI-parity checks: catch "green locally, red in CI" before the push leaves the machine. # check:api guards the generated OpenAPI types (v1.json / v1.d.ts drift); the full # lint/typecheck/build catch a staged change that breaks an UNstaged file (lint-staged diff --git a/docs/decisions.md b/docs/decisions.md index b05454556..1c6d8eb99 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1537,3 +1537,23 @@ because they use `fetch-depth: 0` — a difference the first cut missed. Meta-le gating change can pass every local test and merge green while being a complete no-op in CI; only real-PR verification that **measures the effect** (job durations, not just a green check) catches it — which is exactly what #416's Done-when demanded. Fixed in the #416 follow-up PR. +## 2026-07-17 — Pre-push guard: don't push a file whose working-tree copy is uncommitted (H13, #416 session) + +A review fix (`--no-renames`) was edited into the working file and empirically verified there, but a +`git reset --soft` + `git add` then committed the *stale index*, leaving the fix as an uncommitted +working-tree diff. The push, the CI run, and a cold reviewer each saw a **different tree**: CI/push +had the OLD code; the reviewer read the working file and "confirmed" a fix that never shipped. A PR +went out still carrying the bug the review had cleared. Root cause: local build/test/review all +operate on the working tree, but what ships is the *committed* tree — nothing enforced that they +match. + +Decision: a fail-open **pre-push hook** (`.claude/hooks/prepush-clean-worktree-check.sh`, wired into +`.husky/pre-push` after the H11 rebase check) blocks a push when a file that is part of the branch's +diff vs `origin/main` **also** has uncommitted working-tree or index changes. Scope is deliberately +precise — only files in the pushed diff, so unrelated uncommitted scratch (or untracked files) never +false-block. Fail-open on anything undecidable (not a repo, offline, no `origin/main`); deliberate +escape `ETV_ALLOW_DIRTY_PUSH=1`. This is the mechanized half of the working-tree-vs-committed lesson; +the review-process half (point reviewers at `git show :`, never the bare working file) stays +guidance. Same hook family as H11 (rebase-not-merge) / H12 (issue-qualification), same "#303 make the +rule a hook, not prose to remember" throughline. Verified: dirty PR-file → block; clean tree → allow; +dirty non-PR file → allow; escape hatch → allow. -- 2.47.3