diff --git a/.githooks/tests/test-pre-commit-real-sdk.sh b/.githooks/tests/test-pre-commit-real-sdk.sh new file mode 100755 index 00000000..6b13e58a --- /dev/null +++ b/.githooks/tests/test-pre-commit-real-sdk.sh @@ -0,0 +1,577 @@ +#!/usr/bin/env bash +# Real-SDK coverage for .githooks/pre-commit's Flutter path (issue #678). +# +# WHAT THIS IS, AND WHY IT IS SEPARATE FROM test-pre-commit-hook.sh +# ---------------------------------------------------------------- +# test-pre-commit-hook.sh runs the real hook against *fake* flutter/dart +# launchers that hand-model the SDK's own self-resolution (#594/#644). That +# suite is the fast red/green detector for the hook's LOGIC — it runs on four +# shells in milliseconds and goes red against the unfixed hook — but it is +# blind to the SDK CHANGING. If a future Flutter release resolves its own +# revision through a different channel, the stubs keep passing while the #644 +# protection quietly stops matching reality. +# +# This script closes that blind spot with ONE additive check: it drives the +# hook's Flutter path against a real, pinned, DISPOSABLE Flutter SDK, in the +# linked-worktree + real-`git commit` configuration that reproduces #644. It +# does not replace any stub case and it changes neither the hook nor the stub +# suite (both out of scope for #678). +# +# BLAST RADIUS — READ BEFORE POINTING THIS AT ANYTHING +# ---------------------------------------------------- +# The bug it exercises DELETES SHARED SDK STATE: bin/internal/shared.sh judges +# its tool cache stale (a leaked GIT_DIR misdirects `git -C "$FLUTTER_ROOT" +# rev-parse HEAD`) and deletes bin/cache/flutter.version.json AND +# "$FLUTTER_ROOT/version", then never rewrites them for a `dart` call. Run +# against a SHARED SDK, that wedges `dart pub` for every other worker on the +# machine. So this script: +# * uses ONLY a disposable SDK, designated explicitly via JEEVES_REAL_SDK +# (CI passes the runner-local subosito/flutter-action install); it never +# auto-resolves the machine's shared SDK; +# * HARD-REFUSES the hook's own shared fallbacks and the FVM store +# (~/fvm/versions/*, ~/development/flutter, ~/flutter, /opt/flutter) — +# refusal, not a warning; +# * repairs the disposable SDK on exit via a clean-env `flutter --version`, +# so even a run that corrupts it heals before returning; +# * makes no multi-GB per-run SDK copies (the workaround #676 retired). +# +# RECORDED AC-NARROWING (issue #678 acceptance criterion 3) +# --------------------------------------------------------- +# AC #3 asks one check to assert BOTH that flutter.version.json survives AND +# that no `git rev-parse --local-env-vars` name reaches a Flutter/Dart child. +# This check SPLITS them deliberately: +# * Survival -> asserted here directly (byte-compare of the version file). +# * "no repo-local var reaches a launcher's own environment" -> stays the +# STUB suite's job (assert_no_local_git_env_leaked, on four shells), which +# records the child's env with no interposition conflict. Here the real +# launchers run UNMODIFIED, so nothing can read a launcher's own env; a +# *harmful* leak is still caught through the survival outcome (a leak that +# corrupts changes the cache -> byte-compare reds), and a non-interposing +# git-shim adds an AC #4 mechanism-drift sentinel (below). +# +# TEETH-PROOF (development-order step 1 — done by hand, never in this file) +# ------------------------------------------------------------------------ +# A green check that cannot go red proves nothing. Against a DISPOSABLE SDK +# and an unfixed copy of the hook (the `unset` narrowed back into the +# self-heal subshell), the byte-compare MUST red on two independent paths: +# the leaked GIT_DIR makes the self-heal stamp the worktree revision (#594), +# and `dart` hits shared.sh's revision mismatch and deletes flutter.version.json +# with no rewrite. Do not take that on faith — RUN the teeth-proof against a +# disposable SDK before shipping any change here, and confirm the byte-compare +# goes red. If a future pinned SDK does NOT corrupt against the unfixed hook, +# STOP and escalate (comment on the issue, flag maintainers) — do not ship a +# green-by-construction check. Run the teeth-proof with: +# HOOK=/tmp/unfixed-pre-commit \ +# JEEVES_REAL_SDK=/tmp/throwaway-flutter-clone \ +# ./.githooks/tests/test-pre-commit-real-sdk.sh +# +# WHERE IT RUNS / COST +# -------------------- +# Executed in the Flutter workflow (.github/workflows/flutter-ci.yml, job +# `hook-real-sdk`), NOT the fast SDK-less `Infra & hooks` job. Its shebang means +# the backend-ci shell-lint sweep covers it too. Cost is recorded in that +# job's YAML comment. +# +# Usage (local): JEEVES_REAL_SDK=/path/to/disposable/sdk ./test-pre-commit-real-sdk.sh +# No JEEVES_REAL_SDK -> loud SKIP + exit 0 locally; a hard failure under CI +# (or REQUIRE_REAL_SDK=1, which the CI job sets). + +set -uo pipefail + +# ----- Configuration (named module-level constants, no inline magic) -------- + +# The Flutter package the fixture generates. Both fixtures (main checkout and +# linked worktree) use it; the linked-worktree commit is what triggers the hook. +FIXTURE_PACKAGE_NAME="jeeves_precommit_fixture" + +# build_runner pinned EXACT: an unpinned range would let a future pub.dev +# release red this job for reasons unrelated to the hook. Kept in step with the +# app's own resolution (app/pubspec.lock) so it is known to resolve against the +# SDK pinned in app/.fvmrc. +# not configurable: this is the fixture's own dependency pin, not a tunable. +PINNED_BUILD_RUNNER_VERSION="2.15.0" + +# A restricted PATH so the hook can only see whatever SDK bin dir it resolves +# and puts on PATH itself, never a real system Flutter/Dart. Mirrors the stub +# suite's BARE_PATH. +BARE_PATH="/usr/bin:/bin:/usr/sbin:/sbin" + +TESTS_DIR=$(cd -- "$(dirname -- "$0")" && pwd) +# HOOK is overridable so the teeth-proof can point it at an unfixed copy. +HOOK="${HOOK:-${TESTS_DIR}/../pre-commit}" + +# The real HOME and pub cache, captured before any HOME rewriting. The commit +# runs under an empty HOME (to neutralise the hook's $HOME/*flutter fallbacks), +# but the fixtures' `pub get` ran under the real HOME, so the commit must be +# handed the real pub cache or build_runner/analyze/test resolve nothing. +REAL_HOME="${HOME}" +REAL_PUB_CACHE="${PUB_CACHE:-${REAL_HOME}/.pub-cache}" + +# The names git considers repo-local — exactly what the hook unsets, and +# exactly what would misresolve a Flutter/Dart child to the wrong repository. +# Fail CLOSED if git cannot produce this list (the call fails, or answers +# empty): the leak assertion at the end would otherwise iterate over nothing +# and the job would green vacuously on `sdk_targeted_calls >= 1`. The hook +# itself fails closed on the identical call (pre-commit:191-199); mirror that. +if ! LOCAL_GIT_ENV_VARS=$(git -C "${TESTS_DIR}" rev-parse --local-env-vars) || + [ -z "${LOCAL_GIT_ENV_VARS}" ]; then + echo "FAIL — could not enumerate git's repo-local env vars" >&2 + echo " (\`git rev-parse --local-env-vars\` failed or answered empty); the" >&2 + echo " leak assertion cannot be made, so refusing to run — cf." >&2 + echo " pre-commit:191-199, which fails closed on the same call." >&2 + exit 1 +fi + +# Defensive: never let a repo-local git env inherited by THIS script misdirect +# the SDK's own self-resolution during setup. The hook clears its own for the +# committing child; we clear ours for the setup commands. Guaranteed non-empty +# by the fail-closed check above. +# shellcheck disable=SC2086 +unset ${LOCAL_GIT_ENV_VARS} + +# ----- Bookkeeping ---------------------------------------------------------- + +FAILURES=0 +start_case() { printf '\n%s\n' "$1"; } +ok() { printf ' ok — %s\n' "$1"; } +bad() { printf ' FAIL — %s\n' "$1"; FAILURES=$((FAILURES + 1)); } +skip() { printf ' SKIP — %s\n' "$1"; } + +report() { + echo + if [ "${FAILURES}" -eq 0 ]; then + echo "All checks passed." + exit 0 + fi + echo "${FAILURES} check(s) failed." + exit 1 +} + +WORK=$(mktemp -d) +mkdir -p "${WORK}/empty-home" + +sdk="" # resolved (pwd -P) disposable SDK root +sdk_confirmed_disposable=0 # only repair a SDK we proved is disposable + +# Trap-repair on exit: a clean-env `flutter --version` from inside the SDK +# regenerates flutter.version.json (and version) even if the run corrupted it, +# then WORK is removed. Only fires once the SDK has passed the hard-refuse. +# shellcheck disable=SC2317,SC2329 # body runs via the EXIT trap, not inline +# (SC2317 on <0.11, SC2329 "never invoked" on >=0.11 — disable both so the +# `--severity=style` CI sweep stays green across shellcheck versions). +cleanup() { + if [ "${sdk_confirmed_disposable}" -eq 1 ] && [ -n "${sdk}" ] && [ -x "${sdk}/bin/flutter" ]; then + ( cd "${sdk}" && ./bin/flutter --version >/dev/null 2>&1 ) || true + fi + rm -rf "${WORK}" +} +trap cleanup EXIT + +# ----- Premise resolution (skip loudly; fail in CI) ------------------------- + +# Missing designated SDK: skip locally, fail under CI / REQUIRE_REAL_SDK — a +# silently skipped real-SDK check is exactly how SDK drift would stay invisible. +# Same shape as backend/tests/sync/test_ops_author_chain_race_postgres.py, which +# skips without a Postgres DATABASE_URL but pytest.fail()s under CI for the same +# reason (a silent skip in CI retires the only coverage unnoticed). +premise_unmet() { + local reason="$1" + if [ -n "${CI:-}" ] || [ -n "${REQUIRE_REAL_SDK:-}" ]; then + start_case "real-SDK coverage: premise unmet under CI" + bad "${reason} (CI requires a designated disposable git-checkout SDK)" + report + fi + start_case "real-SDK coverage: no disposable SDK designated" + skip "${reason}" + echo + echo "Set JEEVES_REAL_SDK to a throwaway git-checkout Flutter SDK to run this" + echo "locally; CI runs it in the Flutter workflow against the runner-local" + echo "subosito/flutter-action install." + exit 0 +} + +# Hard error regardless of environment — an explicitly designated SDK that is +# non-git or shared is a mistake or drift, never something to skip past. +fatal() { + start_case "real-SDK coverage: designated SDK is unusable" + bad "$1" + report +} + +designated="${JEEVES_REAL_SDK:-}" +[ -n "${designated}" ] || premise_unmet "JEEVES_REAL_SDK is not set" +[ -d "${designated}" ] || premise_unmet "JEEVES_REAL_SDK=${designated} is not a directory" +sdk=$(cd "${designated}" && pwd -P) || premise_unmet "JEEVES_REAL_SDK=${designated} could not be resolved" + +start_case "real-SDK coverage: driving the hook's Flutter path against ${sdk}" + +# AC-anchor: the mechanism #644 protects is `git -C "$FLUTTER_ROOT" rev-parse +# HEAD`. If the designated SDK is not a git checkout, that mechanism has +# changed — red loudly rather than going vacuously green. shared.sh itself bails +# when `$FLUTTER_ROOT/.git` is absent ("The Flutter directory is not a clone of +# the GitHub project"), so a usable real SDK necessarily carries one. +if ! git -C "${sdk}" rev-parse HEAD >/dev/null 2>&1 || [ ! -e "${sdk}/.git" ]; then + fatal "designated SDK ${sdk} is not a git checkout — the #644 self-resolution mechanism (git -C FLUTTER_ROOT rev-parse HEAD) no longer applies; a release archive keeps its .git for flutter upgrade/channel, so a missing one means the mechanism changed and must be investigated, not skipped" +fi +sdk_head=$(git -C "${sdk}" rev-parse HEAD) + +# Hard-refuse the shared SDKs a mis-designation would land on: the FVM store and +# the hook's own $HOME/system fallbacks. Refusal, not a warning. +# +# FVM keeps its versions under /versions/. The cache defaults to +# ~/fvm or ~/.fvm but is configurable via FVM_CACHE_PATH (current) or FVM_HOME +# (legacy fallback), so a shared SDK can live outside the two default roots. +# Enumerate the configured caches too — a designation that lands on any of them +# must be refused, exactly like the hard-coded roots. +refuse_if_shared() { + local candidate resolved + local -a shared_candidates=( + "${REAL_HOME}/fvm/versions"/* + "${REAL_HOME}/.fvm/versions"/* + ) + [ -n "${FVM_CACHE_PATH:-}" ] && shared_candidates+=( "${FVM_CACHE_PATH}/versions"/* ) + [ -n "${FVM_HOME:-}" ] && shared_candidates+=( "${FVM_HOME}/versions"/* ) + shared_candidates+=( + "${REAL_HOME}/development/flutter" + "${REAL_HOME}/flutter" + "/opt/flutter" + ) + for candidate in "${shared_candidates[@]}"; do + [ -d "${candidate}" ] || continue + resolved=$(cd "${candidate}" && pwd -P) || continue + if [ "${resolved}" = "${sdk}" ]; then + fatal "designated SDK ${sdk} resolves to a SHARED SDK (${candidate}); running this against it would corrupt it for every worktree and worker on the machine (#644). Designate a DISPOSABLE SDK." + fi + done +} +refuse_if_shared +sdk_confirmed_disposable=1 +ok "designated SDK is a git checkout (HEAD ${sdk_head}) and not a shared SDK" + +# ----- Warm the SDK, then snapshot the healthy baseline --------------------- + +# shared.sh's invalidation predicate has FOUR conditions (missing snapshot, +# missing/empty stamp, stamp != compilekey, and pubspec.yaml newer than +# pubspec.lock). A freshly installed SDK that has not built flutter_tools yet +# would lose flutter.version.json on the hook's first `dart` call — a false red +# unrelated to any leak. Warm the tool first (building it also runs a `pub get` +# that touches pubspec.lock, covering the mtime clause too), leaving the +# revision mismatch (the #644 signal) as the only remaining invalidation. +if ! ( cd "${sdk}" && ./bin/flutter --version >/dev/null 2>&1 ); then + bad "could not warm the designated SDK (\`flutter --version\` failed from inside it)" + report +fi + +version_json="${sdk}/bin/cache/flutter.version.json" +version_file="${sdk}/version" # absent on 3.44.1; snapshotted only if present +baseline_json="${WORK}/baseline-flutter.version.json" +baseline_version="${WORK}/baseline-version" + +if [ ! -f "${version_json}" ]; then + bad "warming did not produce ${version_json} — cannot establish a healthy baseline" + report +fi +cp "${version_json}" "${baseline_json}" +[ -f "${version_file}" ] && cp "${version_file}" "${baseline_version}" + +# Parse frameworkRevision (the SHA lives here in a real file; the real JSON puts +# a space after the colon, so the pattern tolerates it). +framework_revision_of() { + grep -o '"frameworkRevision":[[:space:]]*"[^"]*"' "$1" 2>/dev/null | head -1 | cut -d'"' -f4 +} +baseline_revision=$(framework_revision_of "${baseline_json}") +if [ "${baseline_revision}" = "${sdk_head}" ]; then + ok "baseline flutter.version.json is healthy (frameworkRevision == SDK HEAD)" +else + bad "baseline frameworkRevision (${baseline_revision:-}) != SDK HEAD (${sdk_head}) after warming — the baseline is not trustworthy" + report +fi + +# ----- Fixture: a minimal real Flutter package ------------------------------ + +build_fixture_app() { + local app_dir="$1" + mkdir -p "${app_dir}/lib" "${app_dir}/test" + + cat > "${app_dir}/pubspec.yaml" <=3.11.0 <4.0.0" + +dependencies: + flutter: + sdk: flutter + +dev_dependencies: + flutter_test: + sdk: flutter + build_runner: ${PINNED_BUILD_RUNNER_VERSION} + +flutter: + uses-material-design: true +EOF + + cat > "${app_dir}/lib/${FIXTURE_PACKAGE_NAME}.dart" <<'EOF' +/// Trivial library so the fixture analyzes cleanly. +int answer() => 42; +EOF + + cat > "${app_dir}/test/${FIXTURE_PACKAGE_NAME}_test.dart" < "${app_dir}/.gitignore" <<'EOF' +.dart_tool/ +.fvm/ +build/ +EOF +} + +git_init_repo() { + local dir="$1" branch="$2" + git -C "${dir}" init -q -b "${branch}" + git -C "${dir}" config user.email 'jeeves@example.invalid' + git -C "${dir}" config user.name 'Jeeves Dev' + git -C "${dir}" config commit.gpgsign false +} + +# Runs `flutter pub get` in a fixture app dir, from OUTSIDE the SDK, with a +# clean git env (the script already cleared its own). The SDK computes its own +# revision here — no cwd/GIT_DIR misdirection — so setup never corrupts it. +fixture_pub_get() { + local label="$1" app_dir="$2" out + if ! out=$( cd "${app_dir}" && "${sdk}/bin/flutter" pub get 2>&1 ); then + bad "${label}: \`flutter pub get\` failed: ${out}" + report + fi +} + +main_checkout="${WORK}/main-checkout" +mkdir -p "${main_checkout}/app" +build_fixture_app "${main_checkout}/app" +git_init_repo "${main_checkout}" main +git -C "${main_checkout}" add -A +git -C "${main_checkout}" commit -q -m 'seed fixture app' + +# The FVM symlink lives ONLY in the main checkout, gitignored so a linked +# worktree never inherits it — the exact shape that makes the hook resolve the +# main checkout's SDK (#644's configuration). A plain symlink, nothing written +# under it, so the hook's `pwd -P` self-heal lands on the real disposable SDK. +mkdir -p "${main_checkout}/app/.fvm" +ln -s "${sdk}" "${main_checkout}/app/.fvm/flutter_sdk" +fixture_pub_get "main checkout" "${main_checkout}/app" + +# A real linked worktree, with its own app/.fvm removed (a fresh one never +# carries it anyway — it is gitignored), so the hook must fall back to the main +# checkout's SDK. +commit_wt="${WORK}/linked-wt" +git -C "${main_checkout}" worktree add -q -b wt-real-sdk "${commit_wt}" +rm -rf "${commit_wt}/app/.fvm" +fixture_pub_get "linked worktree" "${commit_wt}/app" + +# ----- Prove, by construction, which SDK the hook will resolve -------------- + +# The hook picks the SDK from a fixed candidate order (pre-commit:139-144). With +# candidate 1 (worktree app/.fvm/flutter_sdk) absent and candidate 2 (the main +# checkout's) resolving to the designated disposable SDK and executable, the +# hook MUST resolve candidate 2 and can never reach the absolute /opt/flutter +# path (which BARE_PATH cannot mask), because an earlier valid candidate wins. +worktree_candidate1="${commit_wt}/app/.fvm/flutter_sdk" +main_fvm_link="${main_checkout}/app/.fvm/flutter_sdk" +resolved_candidate2=$(cd "${main_fvm_link}" 2>/dev/null && pwd -P) + +if [ ! -e "${worktree_candidate1}" ]; then + ok "worktree carries no app/.fvm/flutter_sdk (candidate 1 absent — the #644 shape)" +else + bad "worktree still carries app/.fvm/flutter_sdk — the fixture is not the #644 shape" +fi +if [ "${resolved_candidate2}" = "${sdk}" ] && [ -x "${main_fvm_link}/bin/flutter" ]; then + ok "main checkout's app/.fvm/flutter_sdk resolves to the designated SDK and is executable (candidate 2 wins by construction)" +else + bad "candidate 2 does not resolve to the designated SDK (${resolved_candidate2:-} != ${sdk}) — resolution is not pinned" + report +fi + +# ----- Non-interposing git-shim (AC #4 mechanism-drift sentinel) ------------ + +# A recording `git` that logs each call's cwd, argv and any repo-local git env, +# then execs the real git unchanged. It touches nothing under the SDK, so it +# does not disturb the `pwd -P` self-heal. Its unique value is the case a +# byte-compare cannot see: a future SDK that self-resolves through a DIFFERENT +# channel (so `git -C FLUTTER_ROOT rev-parse HEAD` vanishes) reds on the "≥1 +# SDK-targeted call" assertion even when the cache is not corrupted. +# +# A bare shim first on PATH is NOT enough: git prepends its own exec-path +# (which contains a `git`) to a hook's PATH, so the hook's own `git` would hit +# that, not the shim. The fix is to make the shim dir BE the exec-path — +# symlink git-core's helpers into it so external subcommands still resolve, then +# override only `git` — and point GIT_EXEC_PATH at it in the commit below. +git_shim_dir="${WORK}/git-shim" +git_probes="${WORK}/git-probes" +mkdir -p "${git_shim_dir}" "${git_probes}" +real_git=$(command -v git) +real_exec_path=$(git --exec-path) +if [ -d "${real_exec_path}" ]; then + for helper in "${real_exec_path}"/*; do + [ -e "${helper}" ] || continue + ln -s "${helper}" "${git_shim_dir}/${helper##*/}" + done +fi +rm -f "${git_shim_dir}/git" +cat > "${git_shim_dir}/git" < "\$_probe" 2>/dev/null +exec ${real_git} "\$@" +SHIM +chmod +x "${git_shim_dir}/git" + +# ----- Stage the app change and drive a real `git commit` ------------------- + +# Install ONLY pre-commit into the common hooks dir a linked worktree shares — +# pointing core.hooksPath at .githooks would enlist commit-msg/prepare-commit-msg +# and interfere. A non-executable hook is silently ignored by git (a real risk +# for a hand-prepared teeth-proof copy), so fail clearly rather than letting the +# liveness gate report a mysterious "Flutter block never ran". +# git resolves a hook symlink against the hooks dir, not this script's cwd, so a +# relative HOOK (an overridden teeth-proof copy) would install a DANGLING link +# that git silently ignores — surfacing later as a mysterious "Flutter block +# never ran". Resolve to an absolute path FIRST, then validate and symlink that +# exact path (the default HOOK is already absolute; this pins an overridden one). +if ! hook_abs=$(cd -- "$(dirname -- "${HOOK}")" 2>/dev/null && + printf '%s/%s\n' "$(pwd -P)" "$(basename -- "${HOOK}")"); then + bad "could not resolve the hook path ${HOOK} to an absolute location" + report +fi +# A non-executable hook is silently ignored by git (a real risk for a +# hand-prepared teeth-proof copy), so fail clearly rather than letting the +# liveness gate report a mysterious "Flutter block never ran". +if [ ! -x "${hook_abs}" ]; then + bad "the hook at ${hook_abs} is not executable — git would ignore it; run 'chmod +x' on it (teeth-proof copies especially)" + report +fi +ln -sf "${hook_abs}" "${main_checkout}/.git/hooks/pre-commit" + +# Stage a single app/ file so the diff unambiguously matches the hook's +# `^app/` gate, without dragging in pub artefacts. +printf '\n// edited by the real-SDK check\n' >> "${commit_wt}/app/lib/${FIXTURE_PACKAGE_NAME}.dart" +git -C "${commit_wt}" add "app/lib/${FIXTURE_PACKAGE_NAME}.dart" + +# `env -i` so every GIT_* the probes see was exported by git itself, not +# inherited from this suite. GIT_EXEC_PATH points git's own exec-path at the +# shim dir (see above), so the `git` git prepends to the hook's PATH — and the +# one every flutter/dart child inherits — is the recording shim. HOME is empty +# to neutralise the hook's $HOME/*flutter fallbacks. PUB_CACHE is pinned to the +# real, warmed cache the fixtures resolved against. SHIM_PROBE_DIR is not a +# repo-local git var, so it survives the hook's unset and reaches the shim in +# every child. GIT_EXEC_PATH and SHIM_PROBE_DIR are likewise not repo-local, so +# they too survive the unset. The outer git is the REAL binary (absolute path). +commit_out=$(env -i \ + PATH="${git_shim_dir}:${BARE_PATH}" \ + HOME="${WORK}/empty-home" \ + PUB_CACHE="${REAL_PUB_CACHE}" \ + GIT_EXEC_PATH="${git_shim_dir}" \ + SHIM_PROBE_DIR="${git_probes}" \ + "${real_git}" -C "${commit_wt}" commit -q -m 'trigger the hook' 2>&1) +commit_rc=$? + +# ----- Assertions ----------------------------------------------------------- + +# 1. The commit succeeded and landed. This alone does NOT prove the Flutter +# block ran (pre-commit:117 gates it and a skipped block still exits 0) — +# assertion 2 is what proves the block executed. +if [ "${commit_rc}" -eq 0 ]; then + ok "real git commit: the commit succeeded with no manual SDK repair" +else + bad "real git commit: hook rejected the commit (rc=${commit_rc}): ${commit_out}" +fi +if git -C "${commit_wt}" log --oneline -1 2>/dev/null | grep -qF 'trigger the hook'; then + ok "real git commit: the commit actually landed" +else + bad "real git commit: nothing was committed" +fi + +# 2. Liveness — the Flutter block actually ran. Without this, every survival / +# revision assertion below would pass trivially on a hook that skipped the +# block entirely. +if printf '%s' "${commit_out}" | grep -qF 'Flutter app files modified'; then + ok "liveness: the Flutter block was entered (pre-commit:117 gate matched)" +else + bad "liveness: the Flutter block never ran — the survival assertions below would be vacuous: ${commit_out}" +fi +if printf '%s' "${commit_out}" | grep -qF 'Running build_runner'; then + ok "liveness: the hook reached the dart invocation past the unset (build_runner)" +else + bad "liveness: the hook did not reach build_runner — the corruption path was not exercised: ${commit_out}" +fi + +# 3. Survival — byte-identical version file(s). Catches deletion AND mutation, +# schema-agnostic (the real file carries no wall-clock/path fields). +if [ -f "${version_json}" ] && cmp -s "${baseline_json}" "${version_json}"; then + ok "survival: bin/cache/flutter.version.json survived byte-for-byte" +elif [ ! -f "${version_json}" ]; then + bad "survival: bin/cache/flutter.version.json was DELETED — every other worker on the machine would be broken (#644)" +else + bad "survival: bin/cache/flutter.version.json was MUTATED (not byte-identical to the healthy baseline)" +fi +if [ -f "${baseline_version}" ]; then + if [ -f "${version_file}" ] && cmp -s "${baseline_version}" "${version_file}"; then + ok "survival: \$FLUTTER_ROOT/version survived byte-for-byte" + else + bad "survival: \$FLUTTER_ROOT/version was deleted or mutated" + fi +fi + +# 4. Human-readable diagnostic on top of the byte-compare (matches TESTING.md). +after_revision=$(framework_revision_of "${version_json}") +if [ "${after_revision}" = "${sdk_head}" ]; then + ok "frameworkRevision still matches the SDK's own HEAD (${sdk_head})" +else + bad "frameworkRevision (${after_revision:-}) != the SDK's own HEAD (${sdk_head}) — the cache no longer identifies the SDK" +fi + +# 5/6. The git-shim's AC #4 sentinel: at least one SDK-targeted self-resolution +# call, and none carrying a repo-local git var. "SDK-targeted" = argv +# `-C ` OR cwd inside the SDK, compared on pwd -P-resolved paths +# because the fixture SDK path is a symlink. +sdk_targeted_calls=0 +for probe in "${git_probes}"/call.*; do + [ -f "${probe}" ] || continue + probe_cwd=$(grep -m1 '^CWD=' "${probe}" 2>/dev/null); probe_cwd="${probe_cwd#CWD=}" + probe_argv=$(grep -m1 '^ARGV=' "${probe}" 2>/dev/null); probe_argv="${probe_argv#ARGV=}" + is_sdk_targeted=0 + case " ${probe_argv} " in *" -C ${sdk} "*) is_sdk_targeted=1 ;; esac + case "${probe_cwd}" in "${sdk}" | "${sdk}"/*) is_sdk_targeted=1 ;; esac + [ "${is_sdk_targeted}" -eq 1 ] || continue + sdk_targeted_calls=$((sdk_targeted_calls + 1)) + while IFS= read -r var_name; do + [ -n "${var_name}" ] || continue + if grep -q "^${var_name}=" "${probe}"; then + bad "AC #4: an SDK-targeted git call carried repo-local ${var_name} (argv: ${probe_argv}) — the unset did not protect it" + fi + done <<< "${LOCAL_GIT_ENV_VARS}" +done + +if [ "${sdk_targeted_calls}" -ge 1 ]; then + ok "AC #4: the hook made ${sdk_targeted_calls} SDK-targeted git call(s) to the designated SDK — the self-resolution mechanism the stubs model still fires, against the real one" +else + bad "AC #4: no SDK-targeted git call was recorded — a real SDK that resolves its revision through a different channel would silently stop matching the stubs" +fi + +report diff --git a/.github/workflows/flutter-ci.yml b/.github/workflows/flutter-ci.yml index 7eda38cd..5cb985e2 100644 --- a/.github/workflows/flutter-ci.yml +++ b/.github/workflows/flutter-ci.yml @@ -1,16 +1,76 @@ name: Flutter CI +# Triggered by app/ changes, and — since #678 — by edits to the pre-commit hook +# and the real-SDK check that guards it, and to this workflow file. The last is +# what lets the PR that adds a job actually run it. A hook-only change runs only +# the real-SDK job (analyze/android-build are gated on `app` below); an app/** +# or workflow-file change runs everything it is relevant to. on: push: branches: [main] - paths: ["app/**"] + paths: + - "app/**" + - ".githooks/pre-commit" + - ".githooks/tests/test-pre-commit-real-sdk.sh" + - ".github/workflows/flutter-ci.yml" pull_request: branches: [main] - paths: ["app/**"] + paths: + - "app/**" + - ".githooks/pre-commit" + - ".githooks/tests/test-pre-commit-real-sdk.sh" + - ".github/workflows/flutter-ci.yml" + +# Every job here only reads the repository (checkout with persist-credentials: +# false, build, test), so grant the least-privilege token scope once at the +# workflow level rather than inheriting the broad default. +permissions: + contents: read + +# Cancel a superseded PR run rather than let it hold a runner — the real-SDK job +# downloads a full uncached SDK, so a stale run is expensive. Pushes to main are +# left to complete. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} jobs: + # Path detector. Named for its original android-build role and kept that name + # stable so a required-check name never moves (#678), but it now also gates + # the Flutter analyze job and the real-SDK hook check via the `app`/`hook` + # outputs — a hook-only change must not spend a full analyze/test. + android-build-changes: + name: Detect Android-relevant changes + runs-on: ubuntu-latest + outputs: + relevant: ${{ steps.filter.outputs.relevant }} + app: ${{ steps.filter.outputs.app }} + hook: ${{ steps.filter.outputs.hook }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: dorny/paths-filter@7b450fff21473bca461d4b92ce414b9d0420d706 # v4.0.2 + id: filter + with: + filters: | + relevant: + - 'app/android/**' + - 'app/pubspec.yaml' + - 'app/pubspec.lock' + - 'app/.fvmrc' + - '.github/workflows/flutter-ci.yml' + app: + - 'app/**' + - '.github/workflows/flutter-ci.yml' + hook: + - '.githooks/pre-commit' + - '.githooks/tests/test-pre-commit-real-sdk.sh' + analyze: name: Analyze & Test + needs: android-build-changes + if: needs.android-build-changes.outputs.app == 'true' runs-on: ubuntu-latest defaults: run: @@ -51,25 +111,77 @@ jobs: - run: flutter analyze - run: flutter test - android-build-changes: - name: Detect Android-relevant changes + # Real-SDK coverage for the pre-commit hook's Flutter path (#678). Lives here, + # in the Flutter workflow, rather than the fast SDK-less `Infra & hooks` job + # in backend-ci.yml: it needs a real, pinned Flutter SDK so that a change in + # the SDK's own launcher scripts cannot silently invalidate the #644 + # protection while the stub-based suite keeps passing. The stub suite (linted + # and run in `Infra & hooks`) stays the fast red/green detector for the hook's + # logic; this is the slower real-SDK check alongside it, not a replacement. + # + # `cache: false` is load-bearing: analyze/android-build use `cache: true`, + # which saves the SDK across runs. A corrupted SDK saved and restored later + # (the trap cannot beat a cancelled/timed-out job's save) is exactly the + # hazard this check exists to catch, so it downloads a FRESH SDK each run and + # never saves it. The fresh install is also why the script warms the tool + # before snapshotting its baseline. + # + # Cost (parallel job; PR wall-clock grows by max(0, this_job − analyze_job), + # compute by one runner-job). With cache:false the dominant cost is the full + # SDK download each run, then the tool warm/precache, a ~/.pub-cache + # partial-restore fetch, and a no-op build_runner + trivial analyze/test + # driven through the hook. Measured: ~1m33s, against ~5m29s for `analyze` in + # the same run. When an `app/**` change also runs `analyze`, this finishes + # well inside its window and adds ~0 to PR wall-clock. For a hook-only change + # `analyze` is skipped (it is gated on `app`), so this job is on the critical + # path and can extend PR wall-clock by its own runtime. Either way: one + # runner-job of compute. + hook-real-sdk: + name: Pre-commit hook — real SDK + needs: android-build-changes + if: needs.android-build-changes.outputs.app == 'true' || needs.android-build-changes.outputs.hook == 'true' runs-on: ubuntu-latest - outputs: - relevant: ${{ steps.filter.outputs.relevant }} + # Bounds the uncached SDK download + warm + commit-driven build_runner well + # above its ~2-minute observed runtime, so a hang cannot hold a runner for + # the 360-minute default (and bounds the window the exit trap cannot repair). + timeout-minutes: 30 + steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - - uses: dorny/paths-filter@7b450fff21473bca461d4b92ce414b9d0420d706 # v4.0.2 - id: filter + + - uses: subosito/flutter-action@1a449444c387b1966244ae4d4f8c696479add0b2 # v2 with: - filters: | - relevant: - - 'app/android/**' - - 'app/pubspec.yaml' - - 'app/pubspec.lock' - - 'app/.fvmrc' - - '.github/workflows/flutter-ci.yml' + flutter-version-file: app/.fvmrc + channel: stable + # A fresh, disposable, runner-local SDK each run — never saved. See + # the job comment above. + cache: false + + # Pub cache is a distinct cache from the SDK. This job resolves the + # GENERATED fixture's deps (a subset of the app's), so it gets its OWN key + # prefix `pub-hook-` rather than sharing android-build's `pub-` exact key: + # a shared key would collide — whichever job saved first would win, and + # android-build could restore this job's fixture-only cache as an exact + # hit. `restore-keys: pub-` still lets it warm partially from any `pub-*` + # cache (including android-build's full one), and the fixture pins the + # same build_runner the app does, so the overlap is high. + - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ~/.pub-cache + key: pub-hook-${{ hashFiles('app/pubspec.lock') }} + restore-keys: pub- + + - name: Real-SDK pre-commit hook coverage + env: + # A missing/non-git/shared SDK is a hard failure here, not a skip. + REQUIRE_REAL_SDK: "1" + run: | + sdk="$(dirname "$(dirname "$(command -v flutter)")")" + echo "Disposable runner-local SDK: ${sdk}" + JEEVES_REAL_SDK="${sdk}" PUB_CACHE="${HOME}/.pub-cache" \ + ./.githooks/tests/test-pre-commit-real-sdk.sh android-build: name: Android Gradle Build diff --git a/NOTES.md b/NOTES.md index 283d5f3e..21fe97cf 100644 --- a/NOTES.md +++ b/NOTES.md @@ -89,6 +89,11 @@ Format: `- : Instinct: X. Here: Y — .` ## 2026-08-03 (issue #675 — the COMMIT_SOURCE guard does not cover fixups) - Instinct: the `prepare-commit-msg` `[ "$COMMIT_SOURCE" != "message" ]` guard already skips every git-generated message, so `git commit --fixup`/`--squash` are safe. Here: those (and `--fixup=amend:`/`reword:`) all arrive with `COMMIT_SOURCE=message` — the plain `-m` value — so the guard lets them through and the hook appends `(#NNN)` to the `fixup!`/`squash!`/`amend!` subject git built. `git rebase --autosquash` matches that subject byte-for-byte, so the append makes the fixup never squash. The only signal is the **subject prefix**, not `COMMIT_SOURCE`. (git's documented `squash` COMMIT_SOURCE value is for `git merge --squash`, not `git commit --squash` — a name collision that makes `--squash` look already-handled.) +## 2026-08-03 (issue #678 — real-SDK coverage for the pre-commit hook) +- Instinct: point the real-SDK hook test at the machine's FVM SDK (`~/fvm/versions/`) or `/opt/flutter`. Here: the bug it exercises *deletes shared SDK state* (`shared.sh` drops `bin/cache/flutter.version.json` + `$FLUTTER_ROOT/version`), so a run against a shared SDK wedges `dart pub` for every worker on the machine — the check uses a disposable SDK only (designated via `JEEVES_REAL_SDK`), hard-refuses the shared fallbacks by name, and trap-repairs the disposable SDK on exit. +- Instinct: to observe the launchers' environment, interpose `flutter`/`dart` wrappers that read their own env. Here: the hook's self-heal runs `flutter --version` from *inside* the SDK, resolved with `pwd -P`, so a wrapper either recurses through the `app/.fvm/flutter_sdk` symlink or moves the self-heal off the SDK and defeats the very protection under test — keep `flutter_sdk` a plain symlink and observe through a non-interposing `git`-shim (record, then `exec` the real git) instead. +- Instinct: `commit_rc == 0` from the triggering `git commit` proves the hook's Flutter block ran. Here: `pre-commit:117` gates the whole block on `git diff --cached --name-only | grep -q "^app/"`, so a mis-staged fixture exits 0 having touched nothing and every survival/revision assertion passes trivially — assert the hook's own `Running build_runner` output as a liveness gate before trusting them. + ## 2026-08-07 (issue #533 — a `mounted` guard is not teardown protection) - Instinct: a focus-loss listener guarded on `mounted` is safe from teardown, and a route pop that unmounts a focused field loses the edit. Here: `State.mounted` is still **true** inside `dispose()` (`state._element` is nulled after, while `context.mounted`/`ref` die before), so the guard protects nothing there — and the listener never fires from teardown anyway, because a disposed `FocusNode` detaches and `_notify()` early-returns once `_parent` is null. The pop path saves for an unrelated reason: the newly-current route calls `setFirstFocus` while the popped subtree is still mounted, so the listener fires normally. Both halves of the instinct are wrong in opposite directions. - Instinct: a `dispose()` flush that saves a pending edit is a pure win. Here: it must be gated on a latched "subject confirmed missing", or the surface that watched its row get deleted writes to the deleted row on the way out — and `TodoDao` authors the sync op with no rows-affected check, so an op is authored for a deleted entity. A flush is a deferred write, and a deferred write inherits every guard the immediate one has. diff --git a/docs/TESTING.md b/docs/TESTING.md index c8aef4c8..bd5f0cd6 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -57,7 +57,7 @@ We follow a strict Test-Driven Development (TDD) cycle in a Top-Down approach. T Two things make this expensive to diagnose. It reports as a *build* error, not a test failure, so the run produces no test counts at all; and piping the command through `tail` discards the non-zero exit status, so a run that executed zero tests reads as a pass. Create agent worktrees at a short path (`/tmp/`), and capture exit codes explicitly (`cmd > log 2>&1; echo "exit=$?"`) rather than trusting a piped summary line. -- **Pre-commit hook and the shared FVM SDK cache**: the SDK is shared between every worktree and worker on the machine, and both its launchers (`bin/flutter`, `bin/dart`) resolve their own identity through `git`. That makes `bin/cache/flutter.version.json` a piece of *global* state that a single careless commit can wreck for everyone, so `.githooks/pre-commit` carries two distinct protections for it. Regression coverage for both lives in `.githooks/tests/test-pre-commit-hook.sh`. +- **Pre-commit hook and the shared FVM SDK cache**: the SDK is shared between every worktree and worker on the machine, and both its launchers (`bin/flutter`, `bin/dart`) resolve their own identity through `git`. That makes `bin/cache/flutter.version.json` a piece of *global* state that a single careless commit can wreck for everyone, so `.githooks/pre-commit` carries two distinct protections for it. Regression coverage lives in two files: `.githooks/tests/test-pre-commit-hook.sh` (fast, stub-based, four shells — the red/green detector for the hook's *logic*) and, since #678, `.githooks/tests/test-pre-commit-real-sdk.sh` (slower, against a real pinned SDK — the detector for the *SDK* changing, described at the end of this section). The first is *where* the self-heal runs. That step rewrites `bin/cache/flutter.version.json` before `dart run build_runner`, since a sibling worker's run can knock the file out mid-commit — and it runs `flutter --version` from *inside the resolved SDK's own directory*, never from `app/`. Flutter derives the identity it writes into that cache from the git repo of its cwd, so running it from `app/` would stamp *Jeeves's* revision into the shared cache instead of Flutter's own (`{"flutterVersion":"0.0.0-unknown","repositoryUrl":".../Jeeves.git",...}`), after which `dart pub` fails version solving machine-wide until the cache is repaired. @@ -71,6 +71,14 @@ We follow a strict Test-Driven Development (TDD) cycle in a Top-Down approach. T If the shared cache is ever corrupted by some other means (e.g. running `flutter --version` by hand from inside `app/`), confirm the diagnosis by reading `frameworkRevision` in `bin/cache/flutter.version.json`: if it matches a *Jeeves* commit rather than a Flutter one, the SDK computed its version from this repo. Repair by running `flutter --version` from inside the SDK's own directory (e.g. `(cd ~/fvm/versions/ && bin/flutter --version)`), not from `app/`. + **SDK drift vs hook drift — the real-SDK check.** The stub suite is a strong detector for the hook's *logic*: its fake `flutter`/`dart` launchers hand-model the SDK's self-resolution and go red against the unfixed hook. What they cannot notice is the *SDK* changing — if a future Flutter release resolves its own revision through a different channel, the stubs keep passing while the #644 protection quietly stops matching reality. `.githooks/tests/test-pre-commit-real-sdk.sh` closes that blind spot with one additive check: it drives the hook's Flutter path against a real, pinned, **disposable** Flutter SDK, from a real linked worktree, through a real `git commit` — the exact #644 configuration — using a minimal generated Flutter package (`build_runner` pinned to the version `app/pubspec.lock` resolves). It asserts `bin/cache/flutter.version.json` survives byte-for-byte (catching deletion *and* mutation, schema-agnostic because the real file carries no wall-clock fields) and that its `frameworkRevision` still matches the SDK's own `git rev-parse HEAD`. It fails if a future SDK stops resolving its revision through `git -C "$FLUTTER_ROOT" rev-parse HEAD` (a non-interposing `git`-shim asserts at least one such SDK-targeted call happened) or stops being a git checkout at all (asserted as a precondition — a release archive keeps its `.git` because `flutter upgrade`/`channel` need it). It does not replace the stub suite; it runs alongside it. + + **Where it runs, and why the cache is off.** In the Flutter workflow (`.github/workflows/flutter-ci.yml`, job `hook-real-sdk`), not the fast SDK-less `Infra & hooks` job in `backend-ci.yml`, because it needs a real SDK. The job runs `flutter-action` with `cache: false`: analyze/android-build cache the SDK across runs, and a *corrupted* SDK saved and restored later — the trap below cannot beat a cancelled/timed-out job's save — is precisely the hazard this check exists to catch, so it downloads a fresh disposable SDK each run and never saves it. That fresh install is **warmed** first (a clean-env `flutter --version` builds `flutter_tools.snapshot`/`.stamp`, and building the tool runs a `pub get` that touches `pubspec.lock`), so all four of `shared.sh`'s cache-invalidation conditions are satisfied before the baseline is snapshotted, leaving the revision mismatch — the #644 signal — as the only remaining invalidation. The triggering commit runs under `env -i` with an empty `HOME` (which neutralises the hook's `$HOME/*flutter` fallbacks) and `PUB_CACHE` pinned to the runner's warmed `~/.pub-cache`, because the fixtures' `pub get` ran under the real `HOME`. As a parallel job the PR wall-clock grows by `max(0, this_job − analyze_job)`, dominated by the uncached SDK download. + + **Disposable-SDK-only safety model.** The bug *deletes shared SDK state*, so a mis-designation against a shared SDK would wedge `dart pub` for every worker on the machine. The check therefore uses **only** the SDK explicitly designated via `JEEVES_REAL_SDK` (CI passes the runner-local `flutter-action` install), never auto-resolving the machine's SDK, and **hard-refuses** — refusal, not a warning — any designation that resolves to the FVM store or the hook's own shared fallbacks. The FVM store is enumerated at both default roots (`~/fvm/versions/*`, `~/.fvm/versions/*`) and at any configured cache (`$FVM_CACHE_PATH/versions/*`, and the legacy `$FVM_HOME/versions/*`), alongside `~/development/flutter`, `~/flutter`, and `/opt/flutter`. A `trap … EXIT` repairs the disposable SDK with a clean-env `flutter --version` even if a run corrupts it. It **skips loudly** (exit 0) when no disposable SDK is designated, so a developer with no throwaway SDK is not blocked, but **fails rather than skips** under `CI` (or `REQUIRE_REAL_SDK=1`, which the job sets) — a silently skipped real-SDK check is exactly how SDK drift would stay invisible. Before shipping any change here, prove the check has teeth by running it with `HOOK` pointed at a deliberately-unfixed copy of the hook against a disposable SDK: the byte-compare must go red. If a pinned SDK ever fails to corrupt against the unfixed hook, the check is green-by-construction — stop and escalate, do not ship it. + + **A recorded acceptance-criterion narrowing.** #678's criterion asks one check to assert *both* that `flutter.version.json` survives *and* that no `git rev-parse --local-env-vars` name reaches a Flutter/Dart child. The real-SDK check deliberately **splits** these. Survival is asserted here directly. The "no repo-local var reaches a launcher's own environment" clause stays the **stub suite's** job (`assert_no_local_git_env_leaked`, precisely on four shells), because the real launchers run *unmodified* and so nothing can record a launcher's own environment; the real-SDK check still catches any *harmful* leak through the survival outcome (a leak that corrupts changes the cache → the byte-compare reds), and its non-interposing `git`-shim adds the mechanism-drift sentinel above. A **liveness gate** keeps the survival assertions from being vacuous: because `pre-commit`'s `^app/` gate lets a mis-staged fixture exit 0 having never touched Flutter, the check asserts the hook's own `Flutter app files modified` and `Running build_runner` output actually appeared before trusting any survival or revision assertion. + - **Pre-commit hook and an unusable `backend/.venv`**: `backend/.venv` is gitignored and only ever materialized by `uv sync`, so a freshly created linked worktree has none — run `cd backend && uv sync --extra dev` before your first commit that touches `backend/`. Rather than sourcing the venv blind, the hook fails closed through four checks, because **existence is not activation** and each step between them is another chance to fall through to the outer `PATH`: 1. `backend/.venv/bin/activate` **exists** (`[ -f ]`, which also rejects a broken symlink).