From 1b32405085c175a82717537bd7166ebd6feb14e9 Mon Sep 17 00:00:00 2001 From: lodar Date: Tue, 8 Sep 2026 12:09:59 +0000 Subject: [PATCH] DIVE-4090: derive the file-size ratchet base from the merge commit, not a branch name and a pasted sha The weekly upstream-sync PR carries a merge commit whose second parent is upstream's tip; the ratchet's default base (HEAD^1, our pre-merge main) reads every upstream commit as this PR's diff (DIVE-3850). Two fixes keyed the override on the branch name and pasted the sha: the name slipped in week 4 (DIVE-4069), the widened predicate now over-matches any `dive-*-sync` feature branch onto week 3's stale sha (DIVE-4090). - scripts/resolve-file-size-base.sh runs before `File size policy`: on a pull_request run it fetches exactly this PR's commits on top of the depth-2 checkout (--shallow-exclude=, trees only), walks the PR's first-parent chain, and exports the first foreign second parent as CHECK_FILE_SIZES_BASE. Anything else keeps HEAD^1. It prints one `ratchet-base:` line saying which base the run got and why. - scripts/test-resolve-file-size-base.sh: four shapes at fetch-depth 2 from a bare origin (plain feature, sync + fix-up, feature that merged main, push) plus the full-history path. - ci.yml: fetch-depth back to a plain 2 (upstream-identical), the env literal and both name predicates removed. Receipts: on the week-3 tree (PR #14's merge-ref shape, the case where the bases disagree) HEAD^1 is rc=1 on `agent_models_tests.rs: 965 -> 1001` and the derived base c3132c3ee is rc=0 on desktop/web/mobile; a depth-2 emulation of week 4's merge ref resolves to 3c7f288c6 in 1.6s; live PRs #9 and #10 resolve to HEAD^1. --- .github/workflows/ci.yml | 37 +++----- scripts/resolve-file-size-base.sh | 106 ++++++++++++++++++++++ scripts/test-resolve-file-size-base.sh | 118 +++++++++++++++++++++++++ 3 files changed, 238 insertions(+), 23 deletions(-) create mode 100755 scripts/resolve-file-size-base.sh create mode 100755 scripts/test-resolve-file-size-base.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b4e38df623e..96725740342 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,13 +30,7 @@ jobs: steps: - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: - # DIVE-3850: the file-size ratchet below asserts its base object - # exists (`git cat-file -e`), and on an upstream-sync PR that base is - # the sync merge's upstream parent -- outside a depth-2 clone. Full - # history for sync branches ONLY; every other PR keeps depth 2. - # NB the guard is inverted on purpose: 0 is FALSY in GitHub - # expressions, so `cond && 0 || 2` would always yield 2. - fetch-depth: ${{ (!(startsWith(github.head_ref, 'dive-') && contains(github.head_ref, '-sync'))) && 2 || 0 }} + fetch-depth: 2 - uses: cashapp/activate-hermit@cea9af7913204a965fd488637a8d1811bba2e616 # v1 - uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3 id: filter @@ -120,23 +114,20 @@ jobs: scripts/test-rust-cache-contract-regressions.sh - name: CI required-context isolation contract run: scripts/test-ci-required-context-isolation.sh + - name: Resolve file size ratchet base + # DIVE-3850 / DIVE-4069 / DIVE-4090: on a weekly wholesale upstream-sync + # PR the ratchet's default base (HEAD^1, our PRE-merge main) reads every + # upstream commit since the last sync as this PR's own diff and + # false-reds on files with zero lines of ours. The script reads the + # right base off the merge commit itself -- no branch-name predicate to + # miss or over-match, no pasted sha to go stale -- fetching only this + # PR's commits on top of the depth-2 checkout, and prints a one-line + # `ratchet-base:` receipt saying which base this run got and why. + # See community/wiki/a-diff-ratchet-based-on-head-caret-1-false-reds-every-wholesale-sync-merge.md + run: | + scripts/test-resolve-file-size-base.sh + scripts/resolve-file-size-base.sh - name: File size policy - env: - # DIVE-3850: on a weekly upstream-sync merge, HEAD^1 is our PRE-MERGE - # main, so the ratchet reads ~100 upstream commits as this PR's own - # diff and false-reds on files containing zero lines of ours. Graded - # and reproduced by quinn (DIVE-3851): same checker, same tree, only - # the base changed -- HEAD^1 rc=1, merge-base rc=1, upstream parent - # rc=0. Points the ratchet at the sync merge's upstream parent for - # sync branches ONLY; every other PR sees an empty string, - # which is falsy, and falls through to HEAD^1 unchanged. - # DIVE-4069: matched on `contains(...,'-sync')`, not a fixed - # `-upstream-sync` suffix -- week-4 branches were `dive--buzz-sync` - # / `-chat-sync` and slipped the old suffix, silently disengaging this - # protection. Any `dive-*-sync` head now qualifies. - # The sha is per-sync and MUST be updated by each weekly sync row. - # See community/wiki/a-diff-ratchet-based-on-head-caret-1-false-reds-every-wholesale-sync-merge.md - CHECK_FILE_SIZES_BASE: ${{ (startsWith(github.head_ref, 'dive-') && contains(github.head_ref, '-sync')) && 'c3132c3ee982d194cd0198ad07b57ec8bd726e4e' || '' }} run: just file-size-check dead-token-guard: diff --git a/scripts/resolve-file-size-base.sh b/scripts/resolve-file-size-base.sh new file mode 100755 index 00000000000..d39b93fceac --- /dev/null +++ b/scripts/resolve-file-size-base.sh @@ -0,0 +1,106 @@ +#!/usr/bin/env bash +# Resolve the base commit for `just file-size-check` and export it as +# CHECK_FILE_SIZES_BASE (appended to $GITHUB_ENV on Actions, printed elsewhere). +# +# Why this exists (DIVE-3850, DIVE-4069, DIVE-4090): the file-size ratchet +# diffs the tree against HEAD^1. On an ordinary PR that is the base branch tip +# and the diff is the author's own work. On a weekly wholesale upstream sync +# the PR carries a merge commit whose second parent is upstream's tip, and +# HEAD^1 is our PRE-merge main -- so the ratchet reads every upstream commit +# since the last sync as this PR's diff and false-reds on files with zero +# lines of ours. The correct base for that shape is the sync merge's UPSTREAM +# parent (measured on one tree with only the base changing: HEAD^1 rc=1, +# merge-base rc=1, upstream parent rc=0). +# +# Two earlier fixes keyed this on the BRANCH NAME and pasted the sha by hand. +# A name predicate that misses is a silently disengaged guard (week 4), one +# that over-matches false-reds an ordinary feature branch, and a pasted sha is +# stale the week after it was written. This script reads the base off the +# MERGE ITSELF: +# +# 1. Only a pull_request run has a merge ref to inspect: HEAD is GitHub's +# merge of the base tip (HEAD^1) and the PR head (HEAD^2). +# 2. The checkout is depth 2, so the PR head is a shallow boundary and its +# own history is absent. Fetch exactly the PR's commits -- everything +# reachable from the head that is NOT reachable from the base branch +# (--shallow-exclude) -- as trees only (--filter=blob:none; the ratchet +# lazily pulls the handful of base blobs it actually reads). +# 3. Walk the PR's first-parent chain. The first merge commit on it whose +# second parent is NOT on the base branch is a wholesale merge of a +# foreign side; that second parent is the base. A shallow boundary +# commit is grafted parentless, so parents are read from the raw object. +# 4. Anything else -- a plain feature branch, or a branch that merged the +# base branch into itself -- keeps HEAD^1 (empty value), unchanged. +# +# The single `ratchet-base:` line this prints is the receipt: read it in the +# job log to see which base a given PR actually got, and why. +set -euo pipefail + +emit() { + if [ -n "${GITHUB_ENV:-}" ]; then + printf 'CHECK_FILE_SIZES_BASE=%s\n' "$1" >>"$GITHUB_ENV" + else + printf 'CHECK_FILE_SIZES_BASE=%s\n' "$1" + fi +} + +receipt() { + printf 'ratchet-base: %s\n' "$1" +} + +event="${GITHUB_EVENT_NAME:-}" +if [ "$event" != "pull_request" ]; then + receipt "HEAD^1 (event '${event:-none}' is not a pull request; nothing to derive)" + emit "" + exit 0 +fi + +if ! head=$(git rev-parse --verify --quiet 'HEAD^2'); then + receipt "HEAD^1 (HEAD has one parent, so this checkout is not a pull-request merge ref)" + emit "" + exit 0 +fi +base_tip=$(git rev-parse 'HEAD^1') +base_ref="${GITHUB_BASE_REF:-main}" + +if [ "$(git rev-parse --is-shallow-repository)" = "true" ]; then + if ! git fetch --quiet --filter=blob:none --shallow-exclude="$base_ref" origin "$head"; then + echo "::error::resolve-file-size-base: could not fetch this PR's commits" \ + "(git fetch --shallow-exclude=$base_ref origin $head failed); refusing to guess the base" >&2 + exit 1 + fi +fi + +chain=$(git rev-list --first-parent "$head" --not "$base_tip") +count=$(printf '%s\n' "$chain" | grep -c . || true) + +for commit in $chain; do + # Raw parents: `git rev-parse ^2` fails on a shallow boundary commit, but + # the commit object itself still lists every parent. + parents=$(git cat-file -p "$commit" | sed -n 's/^parent //p') + second=$(printf '%s\n' "$parents" | sed -n '2p') + [ -n "$second" ] || continue + + if ! git cat-file -e "${second}^{commit}" 2>/dev/null; then + # Absent after a fetch that excluded the base branch => reachable from the + # base branch => this merge brought the base branch in, not a foreign side. + on_base=true + elif git merge-base --is-ancestor "$second" "$base_tip"; then + on_base=true + else + on_base=false + fi + + if [ "$on_base" = "true" ]; then + receipt "HEAD^1 (merge commit ${commit:0:9} on this PR merges the base branch in, not a foreign side; ${count} first-parent commit(s) on this PR)" + emit "" + exit 0 + fi + + receipt "${second} (second parent of merge commit ${commit:0:9}, the foreign side of a wholesale merge; ${count} first-parent commit(s) on this PR)" + emit "$second" + exit 0 +done + +receipt "HEAD^1 (no merge commit among this PR's ${count} first-parent commit(s))" +emit "" diff --git a/scripts/test-resolve-file-size-base.sh b/scripts/test-resolve-file-size-base.sh new file mode 100755 index 00000000000..31b3d8701c5 --- /dev/null +++ b/scripts/test-resolve-file-size-base.sh @@ -0,0 +1,118 @@ +#!/usr/bin/env bash +# Contract test for scripts/resolve-file-size-base.sh (DIVE-4090). +# +# Builds a synthetic fork -- an "upstream" line, our main forked from it -- and +# checks out four pull-request shapes the way actions/checkout does on a +# pull_request run: refs/pull/N/merge at fetch-depth 2 from a bare origin, so +# the PR head is a shallow boundary exactly as in CI. +# +# plain feature branch -> HEAD^1 (empty) +# wholesale sync merge + a fix-up -> the sync merge's upstream parent +# feature branch that merged main -> HEAD^1 (empty) +# push event -> HEAD^1 (empty) +# +# The sync and merged-main shapes are then re-run from a full clone, which +# exercises the no-fetch path and the ancestry test instead of the +# absent-after-shallow-exclude inference. +set -euo pipefail + +repo_root=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) +resolve="${repo_root}/scripts/resolve-file-size-base.sh" +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' EXIT + +g() { git -C "$1" -c user.name=test -c user.email=test@example.com "${@:2}"; } +commit() { + echo "$2" >>"$1/file-$2" + g "$1" add -A + g "$1" commit -qm "$2" +} + +src="$tmp/src" +bare="$tmp/origin.git" +mkdir -p "$src" +g "$src" init -q -b main +commit "$src" u1 +g "$src" branch upstream +commit "$src" o1 +g "$src" checkout -q upstream +commit "$src" u2 +commit "$src" u3 +upstream_tip=$(g "$src" rev-parse upstream) + +# Shape A: plain feature branch off main. +g "$src" checkout -q -b feat main +commit "$src" f1 + +# Shape B: wholesale sync -- merge upstream into a branch off main, fix-up on top. +g "$src" checkout -q -b sync main +g "$src" merge -q --no-ff -m "sync upstream" upstream +commit "$src" fixup + +# Shape C: feature branch that merged main into itself after main moved. +g "$src" checkout -q -b feat2 main +commit "$src" f2 +g "$src" checkout -q main +commit "$src" o2 +g "$src" checkout -q feat2 +g "$src" merge -q --no-ff -m "merge main" main + +# GitHub's refs/pull/N/merge: a merge of the base tip and the PR head. +mergeref() { + g "$src" checkout -q --detach main + g "$src" merge -q --no-ff -m "pull request merge ref for $1" "$1" >/dev/null + g "$src" update-ref "refs/pull/$2/merge" HEAD +} +mergeref feat 1 +mergeref sync 2 +mergeref feat2 3 + +git clone -q --bare "$src" "$bare" +g "$src" push -q "$bare" "refs/pull/*:refs/pull/*" +git -C "$bare" config uploadpack.allowReachableSHA1InWant true +git -C "$bare" config uploadpack.allowFilter true + +# name pull-ref event expected +run_case() { + local work="$tmp/work-$1" envfile out got + mkdir -p "$work" + git -C "$work" init -q + git -C "$work" remote add origin "$bare" + git -C "$work" fetch -q --depth=2 origin "+$2:refs/remotes/origin/pr-merge" + git -C "$work" checkout -q --detach refs/remotes/origin/pr-merge + [ "$(git -C "$work" rev-parse --is-shallow-repository)" = "true" ] || + { echo "case $1: checkout is not shallow, test would not cover the CI shape" >&2; exit 1; } + envfile="$work/github.env" + : >"$envfile" + out=$(cd "$work" && GITHUB_EVENT_NAME="$3" GITHUB_BASE_REF=main GITHUB_ENV="$envfile" "$resolve") + got=$(sed -n 's/^CHECK_FILE_SIZES_BASE=//p' "$envfile") + [[ "$out" == ratchet-base:* ]] || + { echo "case $1: no ratchet-base receipt line (got: $out)" >&2; exit 1; } + [ "$got" = "$4" ] || + { echo "case $1: expected base '$4', got '$got' ($out)" >&2; exit 1; } + echo "ok $1: $out" +} + +run_case plain-feature refs/pull/1/merge pull_request "" +run_case sync-with-fixup refs/pull/2/merge pull_request "$upstream_tip" +run_case feature-merged-main refs/pull/3/merge pull_request "" +run_case push-event refs/pull/2/merge push "" + +# Full-history path: no shallow fetch, ancestry decided by merge-base. +run_full() { + local work="$tmp/full-$1" envfile out got + git -c advice.detachedHead=false clone -q "$bare" "$work" + git -C "$work" fetch -q origin "+$2:refs/remotes/origin/pr-merge" + git -C "$work" -c advice.detachedHead=false checkout -q --detach refs/remotes/origin/pr-merge + envfile="$work/github.env" + : >"$envfile" + out=$(cd "$work" && GITHUB_EVENT_NAME=pull_request GITHUB_BASE_REF=main GITHUB_ENV="$envfile" "$resolve") + got=$(sed -n 's/^CHECK_FILE_SIZES_BASE=//p' "$envfile") + [ "$got" = "$3" ] || + { echo "full $1: expected base '$3', got '$got' ($out)" >&2; exit 1; } + echo "ok full-history $1: $out" +} +run_full sync-with-fixup refs/pull/2/merge "$upstream_tip" +run_full feature-merged-main refs/pull/3/merge "" + +echo "resolve-file-size-base contract: all cases passed"