mirror of
https://github.com/kuhyx/todo-app.git
synced 2026-11-10 19:07:02 +01:00
Fix three gate defects found reviewing the cap refactor
Moving bump_patch_version.sh into .pre-commit-config.yaml introduced three bugs, all reproduced in clean clones of HEAD before fixing and re-verified after: 1. DATA LOSS. pre-commit stashes unstaged changes and restores them with `git apply`. The bump rewrites pubspec.yaml inside that window, so an uncommitted edit near the `version:` line made the restore conflict: the commit aborted AND the developer's edit vanished from the working tree, recoverable only from $PRE_COMMIT_HOME. Only non-mutating checks belong in that file. 2. A commit rejected by the file-length gate still had its version bumped and staged, riding into the next successful commit. `always_run: true` with no `fail_fast` meant "ordered last" did not mean "runs last only on success" -- which is exactly what the comment above it claimed. 3. `files: ^pubspec\.yaml$` narrowed the crdt_sync override check from unconditional to staged-only, so a poisoned pubspec arriving by pull or merge was invisible to every later commit that did not touch pubspec. The hand-written hook this replaced checked it every time; that narrowing quietly widened the hole the check exists to close. The fix keeps the framework for the read-only gates and puts the mutation back outside it: install_hooks.sh now writes a wrapper that runs pre-commit's generated hook first and bump_patch_version.sh only if it passed. Plus fail_fast, and always_run on the crdt check. Also fixes three stale references this refactor created: a load-bearing layout comment citing the renamed _buildRaw, bin/todo_desktop.dart naming two split-away test files, and CLAUDE.md giving two different suite runtimes. Verified: unstaged pubspec edit preserved through a commit; a rejected commit leaves nothing staged; a merged-in override is caught on an unrelated commit. 313 tests, 100% coverage, gate exits 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
5856617cbf
commit
34b2c1c87c
@ -2,25 +2,43 @@
|
||||
#
|
||||
# This repo previously used a hand-written .git/hooks/pre-commit installed by
|
||||
# scripts/install_hooks.sh. It moved to the pre-commit framework when the
|
||||
# 250-line file-length gate was added, so all three checks live in one place
|
||||
# with one install path. The two pre-existing checks are ported verbatim
|
||||
# below -- dropping them to add a new gate would trade one silent failure for
|
||||
# another.
|
||||
# 250-line file-length gate was added, so the *checks* live in one place with
|
||||
# one install path.
|
||||
#
|
||||
# Install with: pre-commit install
|
||||
# Only non-mutating checks belong here. pre-commit stashes unstaged changes
|
||||
# for the duration of the run and restores them with `git apply` afterwards;
|
||||
# a hook that rewrites a tracked file inside that window makes the restore
|
||||
# conflict, which aborts the commit AND drops the developer's uncommitted
|
||||
# edit. scripts/bump_patch_version.sh rewrites pubspec.yaml, so it stays out
|
||||
# of this file and runs from the wrapper hook instead -- see
|
||||
# scripts/install_hooks.sh.
|
||||
#
|
||||
# fail_fast so a later check cannot run after an earlier one has already
|
||||
# rejected the commit.
|
||||
#
|
||||
# Install with: scripts/install_hooks.sh
|
||||
# The hooks are `language: system` because they are plain bash/python already
|
||||
# on this machine; pre-commit must not build an isolated env for them.
|
||||
fail_fast: true
|
||||
repos:
|
||||
- repo: local
|
||||
hooks:
|
||||
# Gate first: a local crdt_sync override left in place makes CI build a
|
||||
# library this checkout has never tested against, silently and in both
|
||||
# directions. Blocking the commit is the only thing that prevents it.
|
||||
#
|
||||
# always_run/pass_filenames rather than `files:`: a poisoned pubspec can
|
||||
# arrive by pull, merge or --no-verify, and a staged-only trigger would
|
||||
# then never look at it again for any commit that does not happen to
|
||||
# touch pubspec.yaml. The old hand-written hook checked it every time;
|
||||
# narrowing that to staged-only silently widened the hole this gate
|
||||
# exists to close.
|
||||
- id: no-crdt-sync-override
|
||||
name: no local crdt_sync dependency_override
|
||||
entry: scripts/check_no_crdt_sync_override.sh
|
||||
language: system
|
||||
files: ^pubspec\.yaml$
|
||||
pass_filenames: false
|
||||
always_run: true
|
||||
|
||||
# The shared 250-line cap. Checks only the files in the commit, so an
|
||||
# unrelated commit never breaks; exits 1 rather than warning. No
|
||||
@ -32,12 +50,3 @@ repos:
|
||||
name: file length <= 250 lines
|
||||
entry: scripts/check_file_length.sh
|
||||
language: system
|
||||
|
||||
# Version bump LAST: it rewrites pubspec.yaml and `git add`s it, so it
|
||||
# must not run before a gate that could still reject the commit.
|
||||
- id: bump-patch-version
|
||||
name: bump patch version
|
||||
entry: scripts/bump_patch_version.sh
|
||||
language: system
|
||||
pass_filenames: false
|
||||
always_run: true
|
||||
|
||||
11
CLAUDE.md
11
CLAUDE.md
@ -26,7 +26,7 @@ and sync peer-to-peer through a GitHub repo used as dumb storage.
|
||||
|
||||
## Commands
|
||||
|
||||
- Run tests + coverage: `flutter test --coverage` (suite runs in 1–2min; keep it
|
||||
- Run tests + coverage: `flutter test --coverage` (suite runs in under a minute; keep it
|
||||
quick — see Testing below).
|
||||
- Coverage summary: `lcov --list coverage/lcov.info`.
|
||||
- Static analysis: `flutter analyze` (must be clean before committing).
|
||||
@ -138,7 +138,14 @@ migrate → Settings → Import).
|
||||
pre-commit hook (`scripts/check_file_length.sh`, delegating to the shared
|
||||
gate in `~/utils`) and by `.github/workflows/file-length.yml`, so it fails
|
||||
the commit rather than being a note anyone can ignore. Install the hook on
|
||||
a fresh clone with `scripts/install_hooks.sh`. There is deliberately no
|
||||
a fresh clone with `scripts/install_hooks.sh`.
|
||||
- **Only non-mutating checks go in `.pre-commit-config.yaml`.** pre-commit
|
||||
stashes unstaged changes and restores them with `git apply`; a hook that
|
||||
rewrites a tracked file inside that window makes the restore conflict,
|
||||
which aborts the commit *and drops the developer's uncommitted edit*. That
|
||||
is why `bump_patch_version.sh` runs from the wrapper hook that
|
||||
`install_hooks.sh` writes, after pre-commit has exited and only when it
|
||||
passed — see the comments in both files. There is deliberately no
|
||||
baseline and no allowlist — split the file instead. Exemptions (generated,
|
||||
vendored, data files) live with the checker so the gate and the survey
|
||||
cannot disagree.
|
||||
|
||||
@ -1,8 +1,8 @@
|
||||
// coverage:ignore-file
|
||||
// Entry point for the desktop wrapper: resolves real paths, starts the server,
|
||||
// and launches the browser. The serving logic it delegates to is covered by
|
||||
// test/wrapper_server_test.dart, and the busy-port recovery it delegates to by
|
||||
// test/port_guard_test.dart. Keep this file thin wiring for that reason.
|
||||
// test/wrapper_server_*_test.dart, and the busy-port recovery it delegates to
|
||||
// by test/port_guard_*_test.dart. Keep this file thin wiring for that reason.
|
||||
import 'dart:io';
|
||||
|
||||
import 'package:path/path.dart' as p;
|
||||
|
||||
@ -175,7 +175,7 @@ class _NoteEditorState extends State<NoteEditor> {
|
||||
|
||||
// Guided hides the template/mode chrome entirely. Return the flat step
|
||||
// page directly so the TextField sits in THIS column's Expanded — same
|
||||
// depth as _buildRaw, avoiding the nested-Column/Expanded layout issue
|
||||
// depth as the raw field, avoiding the nested-Column/Expanded layout issue
|
||||
// that silently collapses the inner flex space in release builds.
|
||||
if (_mode == NoteEditorMode.guided) return _buildStepPage();
|
||||
|
||||
|
||||
@ -16,7 +16,7 @@ publish_to: 'none' # Remove this line if you wish to publish to pub.dev
|
||||
# https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html
|
||||
# In Windows, build-name is used as the major, minor, and patch parts
|
||||
# of the product and file versions while build-number is used as the build suffix.
|
||||
version: 1.0.19+1
|
||||
version: 1.0.20+1
|
||||
|
||||
environment:
|
||||
sdk: ^3.12.2
|
||||
|
||||
@ -6,12 +6,22 @@
|
||||
# .git/hooks/ is not tracked, so a fresh clone has no hooks. Run this once
|
||||
# after cloning.
|
||||
#
|
||||
# This repo used to write a hand-rolled .git/hooks/pre-commit here. It moved
|
||||
# to the pre-commit framework when the 250-line file-length gate was added:
|
||||
# three checks in one hand-written hook is the point where "just a plain
|
||||
# script" stops paying for itself, and pre-commit already solves passing only
|
||||
# the staged files to each check. The checks themselves are unchanged and
|
||||
# still live in scripts/ -- see .pre-commit-config.yaml.
|
||||
# Two layers, deliberately:
|
||||
#
|
||||
# 1. pre-commit (the framework) runs the read-only gates in
|
||||
# .pre-commit-config.yaml -- the crdt_sync override check and the
|
||||
# 250-line file-length cap.
|
||||
# 2. A thin wrapper hook installed here runs pre-commit first and, only if
|
||||
# it passed, runs scripts/bump_patch_version.sh.
|
||||
#
|
||||
# The bump cannot live in .pre-commit-config.yaml. pre-commit stashes
|
||||
# unstaged changes for the duration of its run and restores them with
|
||||
# `git apply`; the bump rewrites pubspec.yaml inside that window, so an
|
||||
# uncommitted edit anywhere near the `version:` line makes the restore
|
||||
# conflict -- which aborts the commit and DROPS the developer's edit from the
|
||||
# working tree. Running it after pre-commit has exited keeps the mutation
|
||||
# outside the stash cycle, and gating it on pre-commit's exit status stops a
|
||||
# rejected commit from leaving a stray bump staged for the next one.
|
||||
# ============================================================================
|
||||
|
||||
set -euo pipefail
|
||||
@ -22,8 +32,7 @@ readonly HOOK="$REPO_ROOT/.git/hooks/pre-commit"
|
||||
readonly LEGACY_MARKER='bump_patch_version.sh'
|
||||
|
||||
# The old hand-written hook has to go before pre-commit will install over it;
|
||||
# pre-commit refuses to clobber a foreign hook, and leaving it would run the
|
||||
# version bump twice per commit.
|
||||
# pre-commit refuses to clobber a foreign hook.
|
||||
remove_legacy_hook() {
|
||||
if [[ -e "$HOOK" ]] && grep -q "$LEGACY_MARKER" "$HOOK" \
|
||||
&& ! grep -q 'pre-commit' "$HOOK"; then
|
||||
@ -44,10 +53,34 @@ ensure_pre_commit() {
|
||||
fi
|
||||
}
|
||||
|
||||
# Wraps pre-commit's own generated hook so the version bump runs after it,
|
||||
# and only on success. pre-commit install --hook-type pre-commit writes the
|
||||
# generated script; we move it aside and call it from ours.
|
||||
install_wrapper() {
|
||||
local generated="$REPO_ROOT/.git/hooks/pre-commit.pre-commit-generated"
|
||||
mv "$HOOK" "$generated"
|
||||
cat > "$HOOK" <<'HOOK_BODY'
|
||||
#!/bin/bash
|
||||
# Installed by scripts/install_hooks.sh — see that file for why the version
|
||||
# bump runs here rather than as a pre-commit hook.
|
||||
set -euo pipefail
|
||||
ROOT="$(git rev-parse --show-toplevel)"
|
||||
readonly ROOT
|
||||
|
||||
# The read-only gates. A non-zero exit here rejects the commit, and the bump
|
||||
# below never runs, so a rejected attempt leaves no stray version change.
|
||||
"$ROOT/.git/hooks/pre-commit.pre-commit-generated" "$@"
|
||||
|
||||
exec bash "$ROOT/scripts/bump_patch_version.sh"
|
||||
HOOK_BODY
|
||||
chmod 755 "$HOOK"
|
||||
}
|
||||
|
||||
main() {
|
||||
ensure_pre_commit
|
||||
remove_legacy_hook
|
||||
pre-commit install
|
||||
install_wrapper
|
||||
echo "Installed pre-commit hooks: $HOOK"
|
||||
}
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user