From 5f96ece19121b00bd420e704590baef224679622 Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Wed, 21 Oct 2026 17:47:15 -0500 Subject: [PATCH 1/3] fix(dev-lead): push guard trusts its own records, not commit email Since ADR-0008 dev-lead commits as 36422719+don-petry@users.noreply.github.com, the same address the owner's machine and every Claude Code session use. The #1607 guard classified a remote commit as dev-lead's own from that email, so every owner commit pushed to a dev-lead branch counted as dev-lead's and was force-pushed over (combsmith/.github #1314 and #1315, 2026-10-11). _commit_is_dev_lead now trusts only evidence dev-lead records itself: - the SHAs this run published with push_no_clobber or push_with_merge_guard (_PUSH_GUARD_PUSHED), and - a Dev-Lead-Stamp trailer on the commits the harness creates, whose hash covers the commit's own tree and parents. An amended, cherry-picked or rebased copy carries the trailer text but no longer matches it. The committer identity is not consulted. Anything without a record is steering: rebased in, or the push stops. The decision is the pure _push_guard_ownership_verdict, with bats cases for an owner commit with the same email (kept), dev-lead's rewrite of a stamped commit (still force-pushes), and a stamp copied onto another author's commit (not trusted). Closes #2313 Co-Authored-By: Claude Opus 5.5 --- ...-write-mode-personas-on-a-shared-branch.md | 6 +- scripts/dev-lead-fix-ci.sh | 5 +- scripts/dev-lead-fix-issue.sh | 6 +- scripts/dev-lead-fix-reviews.sh | 4 +- scripts/lib/auto-merge.sh | 7 +- scripts/lib/git-identity.sh | 54 +++++ scripts/lib/git-push-guard.sh | 139 ++++++++++--- .../unit/test_incorporate_remote_head.bats | 192 ++++++++++++++++-- tests/dev-lead/unit/test_push_no_clobber.bats | 12 +- 9 files changed, 363 insertions(+), 62 deletions(-) diff --git a/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md b/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md index 7c6beb9de..da03e2acc 100644 --- a/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md +++ b/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md @@ -41,7 +41,11 @@ per ADR-0004), not by prompt text alone: 1. **Incorporate or stand down, never force.** Before pushing, the persona reconciles with the *true* remote head. It rebases a foreign commit in, or stops and escalates when that cannot be done cleanly. A foreign commit is - never discarded (`push_no_clobber`, `scripts/lib/git-push-guard.sh`). + never discarded (`push_no_clobber`, `scripts/lib/git-push-guard.sh`). A + commit is foreign unless the persona's own records say it wrote it: a SHA + this run pushed, or a `Dev-Lead-Stamp` trailer that matches the commit's + tree and parents. The committer identity is not a record, because the owner + commits as the same account (#2313). 2. **"Pushed" is a verdict, not an assumption.** After the push, the harness compares the pre-pass head, the pushed SHA and the re-fetched remote head (`cl_push_landed_verdict`, `scripts/lib/claim-landing.sh`). Any result other diff --git a/scripts/dev-lead-fix-ci.sh b/scripts/dev-lead-fix-ci.sh index 582756275..91c284af9 100755 --- a/scripts/dev-lead-fix-ci.sh +++ b/scripts/dev-lead-fix-ci.sh @@ -380,7 +380,10 @@ main() { # before committing or commit fails with "fatal: empty ident name". setup_git_identity git add -A - git commit -m "fix(ci): auto-fix for $(echo "$CHECKS_JSON" | jq -r '.[0].name // "CI failure"') [skip ci-relay]" + # Dev-Lead-Stamp: lets a later run's push guard recognise this commit as + # dev-lead's own (#2313). + git commit -m "fix(ci): auto-fix for $(echo "$CHECKS_JSON" | jq -r '.[0].name // "CI failure"') [skip ci-relay]" \ + -m "$(dev_lead_stamp_trailer)" fi # Sync-stub guard (#2185/#2212): on a `standards-sync` PR a CI fix must not # change a stub copied verbatim from combsmith/.github/standards/ (e.g. diff --git a/scripts/dev-lead-fix-issue.sh b/scripts/dev-lead-fix-issue.sh index 4e0fdd2ad..541d2cd52 100755 --- a/scripts/dev-lead-fix-issue.sh +++ b/scripts/dev-lead-fix-issue.sh @@ -224,7 +224,7 @@ checkpoint_push_partial_work() { if $has_uncommitted; then git add -A >/dev/null 2>&1 || return 1 git commit -m "${CHECKPOINT_COMMIT_PREFIX} partial work for issue #${ISSUE_NUMBER} (stage timeout — unreviewed, do not merge)" \ - >/dev/null 2>&1 || return 1 + -m "$(dev_lead_stamp_trailer)" >/dev/null 2>&1 || return 1 fi # Push the branch so the work lands on the remote before the run exits. @@ -669,7 +669,9 @@ ${lint_output} if $has_uncommitted; then git add -A - git commit -m "feat: implement issue #${ISSUE_NUMBER} — ${ISSUE_TITLE}" + # Dev-Lead-Stamp: lets a later run's push guard recognise this commit as + # dev-lead's own when it reconciles the PR branch (#2313). + git commit -m "feat: implement issue #${ISSUE_NUMBER} — ${ISSUE_TITLE}" -m "$(dev_lead_stamp_trailer)" fi # Empty net-diff guard (#1786, slice 1 of #1620): before pushing or opening a diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 922c9e1b6..fe0a6d1d6 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -3251,7 +3251,9 @@ commit_and_push() { # an if-statement condition, so git commit failures would be silently swallowed # otherwise. Using exit (not return) ensures CI fails visibly instead of posting a # false "Changes committed and pushed" comment. - git commit -m "$commit_msg" || { echo "::error::git commit failed — check git identity configuration on the runner" >&2; exit 1; } + # The Dev-Lead-Stamp trailer is how a later run's push guard knows this + # commit is dev-lead's own; the committer email cannot say (#2313). + git commit -m "$commit_msg" -m "$(dev_lead_stamp_trailer)" || { echo "::error::git commit failed — check git identity configuration on the runner" >&2; exit 1; } fi # No-op guard (#1340, extended #1786): a fix/review/rebase pass that reverts # the PR's own changes nets the base…head diff to zero. Pushing it would let a diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index 414f7982c..7f901d5fc 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -147,10 +147,14 @@ push_with_merge_guard() { return "$reconcile_rc" fi - local errf + local errf publishing errf="$(mktemp)" + # Record what this push publishes so a later reconcile in the run knows these + # commits are dev-lead's own (#2313). + publishing=$(_push_guard_publishing "${1:-}") if git push "$@" 2>"$errf"; then rm -f "$errf" + _push_guard_record_pushed "$publishing" # Refresh the held SHA after our own push so the EXIT-trap restore uses # --match-head-commit against the new head rather than the pre-push SHA. _AM_HEAD_SHA=$(git rev-parse HEAD 2>/dev/null || true) @@ -175,6 +179,7 @@ push_with_merge_guard() { echo "::notice::push rejected (non-fast-forward) and HEAD has diverged from ${upstream} — history was rewritten (likely a self-rebase); retrying with a pinned lease. Overwriting dev-lead's own commit(s): ${discarded:-}" >&2 if _pinned_force_push "$@" 2>>"$errf"; then rm -f "$errf" + _push_guard_record_pushed "$publishing" _AM_HEAD_SHA=$(git rev-parse HEAD 2>/dev/null || true) return 0 fi diff --git a/scripts/lib/git-identity.sh b/scripts/lib/git-identity.sh index ddfe69beb..0121c19d8 100644 --- a/scripts/lib/git-identity.sh +++ b/scripts/lib/git-identity.sh @@ -35,3 +35,57 @@ setup_git_identity() { fi git config user.name "$bot" } + +# dev-lead commit stamp (#2313). Since ADR-0008 dev-lead commits with the same +# noreply email as the owner and every Claude Code session on the owner's +# machine, so the committer identity cannot tell dev-lead's commits from theirs. +# dev-lead marks the commits it creates with a trailer the push guard can check: +# +# Dev-Lead-Stamp: \nparent

..." — the commit's own tree and +# parents, in the order a commit object lists them> +# +# The stamp is bound to the commit's content and position. Amending a stamped +# commit (new tree) or cherry-picking or rebasing it (new parents) keeps the +# trailer text but breaks the match, so a copied stamp marks nothing. It is a +# record dev-lead keeps, not a signature: it guards against copies made by +# accident, not against someone who computes a stamp on purpose. +# Not readonly: the push guard sources this file too, and a second `readonly` +# assignment would abort a script running under `set -e`. +DEV_LEAD_STAMP_KEY='Dev-Lead-Stamp' + +# dev_lead_stamp_of [parent...] — echo the stamp for a commit with this +# tree and these parents. Pure: hashes its arguments, reads no repository. +dev_lead_stamp_of() { + local tree="${1:-}" + [ -n "$tree" ] || return 1 + shift + { + printf 'tree %s\n' "$tree" + local p + for p in "$@"; do printf 'parent %s\n' "$p"; done + } | git hash-object --stdin +} + +# dev_lead_stamp_trailer — echo the `Dev-Lead-Stamp: ` line for the commit +# `git commit` is about to make from the current index: tree = the staged tree, +# parents = HEAD (none on an unborn branch) plus MERGE_HEAD when a merge is being +# concluded. Pass it as the last `-m` of the commit so it lands in the trailer +# block. Echoes nothing and returns 1 when the index cannot be written; the +# commit then goes out unstamped and the push guard treats it as foreign. +dev_lead_stamp_trailer() { + local tree head merge_head_file stamp + local -a parents=() + tree=$(git write-tree 2>/dev/null) || return 1 + if head=$(git rev-parse --verify --quiet HEAD 2>/dev/null); then + parents+=("$head") + fi + merge_head_file=$(git rev-parse --git-path MERGE_HEAD 2>/dev/null || true) + if [ -n "$merge_head_file" ] && [ -f "$merge_head_file" ]; then + local mh + while IFS= read -r mh; do + [ -n "$mh" ] && parents+=("$mh") + done < "$merge_head_file" + fi + stamp=$(dev_lead_stamp_of "$tree" ${parents[@]+"${parents[@]}"}) || return 1 + printf '%s: %s\n' "$DEV_LEAD_STAMP_KEY" "$stamp" +} diff --git a/scripts/lib/git-push-guard.sh b/scripts/lib/git-push-guard.sh index 126dc8e2c..7f79bc64f 100644 --- a/scripts/lib/git-push-guard.sh +++ b/scripts/lib/git-push-guard.sh @@ -19,14 +19,23 @@ # a. incorporate_remote_head re-fetches the true remote head before # pushing and either rebases HEAD onto it (incorporating a # concurrent commit) or, when it cannot be incorporated cleanly, -# STOPS and escalates. A commit authored by anyone other than -# dev-lead's own identity is treated as steering: never discarded, -# and called out in the run summary. +# STOPS and escalates. A commit dev-lead has no record of writing +# is treated as steering: never discarded, and called out in the +# run summary. # b. every force push adds --force-if-includes, so the force ABORTS # unless the commits being overwritten are reachable from a ref we # actually fetched (i.e. we truly incorporated them) — the lease # alone is not enough once a fetch has refreshed it. # +# #2313 "Is this commit dev-lead's?" was answered from the committer email. +# Since ADR-0008 dev-lead commits as the owner's account, with the same +# noreply address as the owner's own machine and every Claude Code +# session on it, so every owner commit counted as dev-lead's and was +# force-pushed over (combsmith/.github #1314, #1315). Ownership now comes +# only from evidence dev-lead records itself: the SHAs this run pushed, +# or a Dev-Lead-Stamp trailer bound to the commit's own tree and parents +# (git-identity.sh). The committer identity is never consulted. +# # Usage: push_no_clobber [git push args...] e.g. push_no_clobber origin main # _push_guard_summary MESSAGE — append a line to the GitHub run summary (if the @@ -37,31 +46,95 @@ _push_guard_summary() { return 0 } -# _commit_is_dev_lead SHA — true when the commit was authored by dev-lead's own -# identity. dev-lead commits with the bot's noreply email — exactly -# @users.noreply.github.com or +@users.noreply.github.com -# (see setup_git_identity in git-identity.sh) — so match ONLY those two forms. -# A bare "*@…" suffix match would also accept a spoofed local-part such as -# attacker-@users.noreply.github.com and misclassify a foreign commit as -# dev-lead-owned; anchoring the local part to the exact login (optionally an -# all-digit user-id prefix) closes that. Anything else — a human maintainer's -# real email, another bot — is foreign (steering) and must never be silently -# discarded. The email is the reliable signal: a human owner may share the -# `don-petry` *name* but not the bot noreply address. -_commit_is_dev_lead() { - local sha="$1" - local bot="${BOT_USER:-donpetry-bot}" - local email - email=$(git log -1 --format='%ce' "$sha" 2>/dev/null || true) - # Quoted "${bot}" is a literal in the regex; the id-prefix and domain are - # anchored so the login must be the WHOLE local part (or follow "+"). - if [[ "$email" == "${bot}@users.noreply.github.com" ]] \ - || [[ "$email" =~ ^[0-9]+\+"${bot}"@users\.noreply\.github\.com$ ]]; then - return 0 +# dev_lead_stamp_of (the Dev-Lead-Stamp hash) lives with the code that stamps +# dev-lead's commits. +if ! declare -F dev_lead_stamp_of >/dev/null 2>&1; then + # shellcheck source=scripts/lib/git-identity.sh + source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/git-identity.sh" +fi + +# Newline-separated SHAs this run has published with push_no_clobber or +# push_with_merge_guard (#2313). A commit on the remote is dev-lead's own when its +# SHA is here, whatever its author or committer says. Accumulates across pushes +# in one run. +_PUSH_GUARD_PUSHED="${_PUSH_GUARD_PUSHED:-}" + +# _push_guard_ownership_verdict — decide +# whether a commit is dev-lead's own from evidence dev-lead recorded itself +# (#2313). Echoes one token; returns 0 only for the two `own-*` tokens: +# own-pushed is one of the newline-separated SHAs in . +# own-stamped has a `Dev-Lead-Stamp:` line whose hash equals the +# stamp of and the space-separated . An amended, +# cherry-picked or rebased copy keeps the line but not the tree and +# parents it was computed from, so the copy does not match. +# foreign anything else, including a commit whose author and committer +# are exactly dev-lead's: the owner commits with the same identity. +# Pure apart from `git hash-object --stdin` (no repository, network or gh). +_push_guard_ownership_verdict() { + local sha="${1:-}" tree="${2:-}" parent_list="${3:-}" message="${4:-}" pushed="${5:-}" + if [ -n "$sha" ] && grep -qxF -- "$sha" <<<"$pushed"; then + echo own-pushed; return 0 fi + if [ -n "$tree" ]; then + local expected claimed + # Word-split on purpose: is the space-separated %P list. + # shellcheck disable=SC2086 + expected=$(dev_lead_stamp_of "$tree" $parent_list 2>/dev/null || true) + if [ -n "$expected" ]; then + while IFS= read -r claimed; do + if [ "$claimed" = "$expected" ]; then + echo own-stamped; return 0 + fi + done < <(sed -nE "s/^${DEV_LEAD_STAMP_KEY}:[[:space:]]*([0-9a-f]+)[[:space:]]*\$/\1/p" <<<"$message") + fi + fi + echo foreign return 1 } +# _commit_is_dev_lead SHA — true only when dev-lead's own records say it wrote the +# commit: this run pushed it, or it carries a matching Dev-Lead-Stamp (#2313). The +# committer email is deliberately NOT a signal: since ADR-0008 the owner, every +# Claude Code session on the owner's machine and dev-lead all commit as +# +don-petry@users.noreply.github.com. An unreadable commit is foreign. +_commit_is_dev_lead() { + local sha="$1" full meta tree parents message + full=$(git rev-parse --verify --quiet "${sha}^{commit}" 2>/dev/null) || return 1 + meta=$(git show -s --format='%T%x09%P' "$full" 2>/dev/null) || return 1 + tree="${meta%%$'\t'*}" + parents="${meta#*$'\t'}" + message=$(git show -s --format='%B' "$full" 2>/dev/null || true) + _push_guard_ownership_verdict "$full" "$tree" "$parents" "$message" "$_PUSH_GUARD_PUSHED" >/dev/null +} + +# _push_guard_publishing [remote] — echo the SHAs a push of HEAD is about to +# publish: the commits HEAD has beyond the remote head incorporate_remote_head +# verified, or, when no remote head was read (a new branch, a failed fetch), +# beyond every remote-tracking ref. Run it BEFORE the push: a successful push +# moves the remote-tracking ref to HEAD. +_push_guard_publishing() { + if [ -n "$_PUSH_GUARD_REMOTE_SHA" ]; then + git rev-list "${_PUSH_GUARD_REMOTE_SHA}..HEAD" 2>/dev/null || true + else + git rev-list HEAD --not --remotes="${1:-origin}" 2>/dev/null || true + fi +} + +# _push_guard_record_pushed — add a successful push's SHAs to the ledger. +_push_guard_record_pushed() { + local shas="${1:-}" sha + while IFS= read -r sha; do + [ -z "$sha" ] && continue + grep -qxF -- "$sha" <<<"$_PUSH_GUARD_PUSHED" && continue + if [ -n "$_PUSH_GUARD_PUSHED" ]; then + _PUSH_GUARD_PUSHED+=$'\n'"${sha}" + else + _PUSH_GUARD_PUSHED="$sha" + fi + done <<<"$shas" + return 0 +} + # _resolve_remote_branch [remote branch] — echo " " for the push # target. Uses explicit args when given; otherwise derives them from the current # branch's upstream (@{u}). Echoes nothing when neither is available. @@ -81,7 +154,8 @@ _resolve_remote_branch() { # # Returns: # 0 safe to push — either HEAD already contains the remote head, or the only -# commits the remote has that HEAD lacks are dev-lead's OWN (a legitimate +# commits the remote has that HEAD lacks are dev-lead's OWN by its records +# (_commit_is_dev_lead: pushed by this run, or stamped) (a legitimate # self-rewrite the caller will publish with --force-with-lease), or foreign # (steering) commits were cleanly rebased in (HEAD now contains them). # 2 STOP/escalate — the remote has a steering commit that could not be @@ -169,8 +243,8 @@ incorporate_remote_head() { done <<< "$unincorporated" if [ -z "$foreign" ]; then - # Every remote commit HEAD lacks is dev-lead's own — a legitimate history - # rewrite (e.g. amend/rebase of our prior push). The caller publishes it + # Every remote commit HEAD lacks is dev-lead's own by its records — a + # legitimate history rewrite (e.g. amend/rebase of our prior push). The caller publishes it # with --force-with-lease --force-if-includes and logs the discarded SHAs. return 0 fi @@ -181,7 +255,7 @@ incorporate_remote_head() { foreign_oneline=$(while IFS= read -r sha; do [ -n "$sha" ] && git log -1 --format='%h %an: %s' "$sha" 2>/dev/null done <<< "$foreign") - echo "::warning::steering detected on ${remote}/${branch} — commit(s) by a non-dev-lead identity present on the remote that HEAD did not incorporate; rebasing onto them so they are never discarded (#1607):" >&2 + echo "::warning::steering detected on ${remote}/${branch} — commit(s) dev-lead has no record of writing are on the remote and HEAD did not incorporate them; rebasing onto them so they are never discarded (#1607):" >&2 printf '%s\n' "$foreign_oneline" >&2 # Record dev-lead's commits before the rebase so their rebased successors can @@ -198,7 +272,7 @@ incorporate_remote_head() { _PUSH_GUARD_REWRITES="$rewrites" fi _push_guard_summary "### dev-lead: maintainer steering incorporated (#1607)" - _push_guard_summary "A commit by a non-dev-lead identity was on \`${remote}/${branch}\` and has been rebased into this push rather than discarded:" + _push_guard_summary "A commit dev-lead has no record of writing was on \`${remote}/${branch}\` and has been rebased into this push rather than discarded:" _push_guard_summary "" _push_guard_summary '```' _push_guard_summary "${foreign_oneline}" @@ -210,7 +284,7 @@ incorporate_remote_head() { git rebase --abort 2>/dev/null || true echo "::error::steering commit(s) on ${remote}/${branch} could not be rebased into dev-lead's changes cleanly. Refusing to force-push over unincorporated human work — stopping for human review (#1607)." >&2 _push_guard_summary "### dev-lead: STOPPED — un-incorporable maintainer steering (#1607)" - _push_guard_summary "A commit by a non-dev-lead identity is on \`${remote}/${branch}\` and conflicts with dev-lead's changes. dev-lead refused to force-push over it and stopped for human review:" + _push_guard_summary "A commit dev-lead has no record of writing is on \`${remote}/${branch}\` and conflicts with dev-lead's changes. dev-lead refused to force-push over it and stopped for human review:" _push_guard_summary "" _push_guard_summary '```' _push_guard_summary "${foreign_oneline}" @@ -236,11 +310,13 @@ push_no_clobber() { # steering commit, or stop/escalate if it cannot be incorporated cleanly. incorporate_remote_head "$@" || return $? - local errf + local errf publishing errf="$(mktemp)" + publishing=$(_push_guard_publishing "${1:-}") if git push "$@" 2>"$errf"; then rm -f "$errf" + _push_guard_record_pushed "$publishing" return 0 fi @@ -261,6 +337,7 @@ push_no_clobber() { echo "::notice::push rejected (non-fast-forward) and HEAD diverged from ${upstream} — history was rewritten (likely a self-rebase); retrying with a pinned lease (aborts unless the remote is still where dev-lead incorporated). Overwriting dev-lead's own commit(s): ${discarded:-}" >&2 if _pinned_force_push "$@" 2>>"$errf"; then rm -f "$errf" + _push_guard_record_pushed "$publishing" return 0 fi cat "$errf" >&2 diff --git a/tests/dev-lead/unit/test_incorporate_remote_head.bats b/tests/dev-lead/unit/test_incorporate_remote_head.bats index 56f7ba657..b4f21b140 100644 --- a/tests/dev-lead/unit/test_incorporate_remote_head.bats +++ b/tests/dev-lead/unit/test_incorporate_remote_head.bats @@ -7,15 +7,17 @@ # pushed to the branch after dev-lead's last fetch. Before pushing, dev-lead # must fetch the true remote head and INCORPORATE it (rebase onto it) — or, when # it cannot be incorporated cleanly, STOP and escalate rather than force over it. -# A commit authored by anyone other than dev-lead's own identity is steering: -# never discarded, and called out in the run summary. +# A commit dev-lead has no record of writing is steering: never discarded, and +# called out in the run summary. Since #2313 that record is the SHAs this run +# pushed or a Dev-Lead-Stamp trailer, never the committer email — the owner +# commits with the same noreply address dev-lead does. BOT_EMAIL_SUFFIX="don-petry@users.noreply.github.com" setup() { GUARD_LIB="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)/scripts/lib/git-push-guard.sh" - # dev-lead acts as `don-petry`; classify by the bot's noreply email. + # dev-lead acts as `don-petry` (ADR-0008), the same account the owner uses. export BOT_USER="don-petry" REMOTE="$BATS_TEST_TMPDIR/remote.git" @@ -40,7 +42,8 @@ setup() { ( cd "$WORK"; _dev_identity ) } -# dev-lead's own git identity (bot noreply email → classified as OWN). +# dev-lead's git identity. Since ADR-0008 the owner commits with it too, so it +# classifies nothing on its own (#2313). _dev_identity() { git config user.email "12345+${BOT_EMAIL_SUFFIX}" git config user.name "don-petry" @@ -145,31 +148,180 @@ _remote_log() { git --git-dir="$REMOTE" log --format='%s' main; } echo "$output" | grep -q "${discarded:0:12}" } -@test "security: only the exact bot noreply local-part is classified as dev-lead (#1610)" { +# ── #2313: ownership comes from dev-lead's records, not the committer email ── +# The owner, every Claude Code session on the owner's machine and dev-lead all +# commit as +don-petry@users.noreply.github.com. On combsmith/.github #1314 +# and #1315 the owner's commits were classified as dev-lead's and force-pushed +# over. + +# The owner pushing to the branch with dev-lead's EXACT identity. +_owner_push() { + local file="$1" content="$2" + local other="$BATS_TEST_TMPDIR/owner.$RANDOM" + git clone -q "$REMOTE" "$other" + ( + cd "$other" + _dev_identity + printf '%s\n' "$content" > "$file"; git add "$file" + git commit -q -m "owner: $file" + git push -q origin main + ) +} + +# An earlier dev-lead run: a stamped commit pushed from its own clone. +_earlier_run_push() { + local file="$1" content="$2" + local other="$BATS_TEST_TMPDIR/earlier.$RANDOM" + git clone -q "$REMOTE" "$other" + ( + cd "$other" + _dev_identity + source "$GUARD_LIB" + printf '%s\n' "$content" > "$file"; git add "$file" + git commit -q -m "dev-lead: $file" -m "$(dev_lead_stamp_trailer)" + git push -q origin main + ) +} + +@test "#2313: an owner commit with dev-lead's exact email is kept, not force-pushed over" { cd "$WORK" source "$GUARD_LIB" + # The #1314 shape: dev-lead builds a commit on the head it checked out... + echo devfix > devfix; git add devfix; git commit -q -m "dev-lead fix" + # ...while the owner pushes a fix to the same branch with the same identity. + _owner_push owner.txt "the approved fix" + local owner_sha; owner_sha=$(_remote_head) + [ "$(git --git-dir="$REMOTE" log -1 --format=%ce main)" = "$(git log -1 --format=%ce HEAD)" ] - # Exact login as the whole local part → OWN. - GIT_COMMITTER_EMAIL="don-petry@users.noreply.github.com" \ - git commit -q --allow-empty -m bare - run _commit_is_dev_lead HEAD + run push_no_clobber origin main [ "$status" -eq 0 ] + # The owner's commit is still on the branch, under dev-lead's. + git --git-dir="$REMOTE" merge-base --is-ancestor "$owner_sha" main + local head_tree; head_tree=$(git --git-dir="$REMOTE" ls-tree --name-only -r main) + echo "$head_tree" | grep -qx owner.txt + echo "$head_tree" | grep -qx devfix + # And the run says so. + grep -q 'owner: owner.txt' "$GITHUB_STEP_SUMMARY" +} - # Numeric user-id prefix + login → OWN. - GIT_COMMITTER_EMAIL="12345+don-petry@users.noreply.github.com" \ - git commit -q --allow-empty -m idprefix - run _commit_is_dev_lead HEAD +@test "#2313: an owner commit with dev-lead's email that conflicts stops the push" { + cd "$WORK" + source "$GUARD_LIB" + printf 'dev-lead line\n' >> base; git add base; git commit -q -m "dev-lead edits base" + _owner_push base "owner rewrite of base" + local preserved; preserved=$(_remote_head) + + run push_no_clobber origin main + [ "$status" -eq 2 ] + [ "$(_remote_head)" = "$preserved" ] + grep -q 'STOPPED' "$GITHUB_STEP_SUMMARY" +} + +@test "#2313: dev-lead's rewrite of a commit an earlier run stamped still force-pushes" { + _earlier_run_push work v1 + cd "$WORK" + git pull -q origin main + source "$GUARD_LIB" + local stamped; stamped=$(git rev-parse HEAD) + run _commit_is_dev_lead "$stamped" [ "$status" -eq 0 ] - # A spoofed local part that merely ENDS WITH the login is FOREIGN (steering). - GIT_COMMITTER_EMAIL="attacker-don-petry@users.noreply.github.com" \ - git commit -q --allow-empty -m spoof - run _commit_is_dev_lead HEAD + # This run amends the earlier run's commit: a legitimate self-rewrite. + echo v2 > work; git add work; git commit -q --amend -m "dev-lead: work v2" + local amended; amended=$(git rev-parse HEAD) + + run push_no_clobber origin main + [ "$status" -eq 0 ] + [ "$(_remote_head)" = "$amended" ] + echo "$output" | grep -q "${stamped:0:12}" +} + +@test "#2313: a commit this run pushed is its own even with no stamp" { + cd "$WORK" + source "$GUARD_LIB" + echo v1 > work; git add work; git commit -q -m "engine commit, unstamped" + local pushed; pushed=$(git rev-parse HEAD) + run _commit_is_dev_lead "$pushed" + [ "$status" -ne 0 ] + push_no_clobber origin main + run _commit_is_dev_lead "$pushed" + [ "$status" -eq 0 ] + grep -qxF "$pushed" <<<"$_PUSH_GUARD_PUSHED" +} + +@test "#2313: a stamp copied onto another author's commit is not trusted" { + _earlier_run_push work v1 + # The owner amends dev-lead's stamped commit: new content, same message, so the + # Dev-Lead-Stamp line is carried over verbatim onto a commit dev-lead never made. + local other="$BATS_TEST_TMPDIR/owner-amend" + git clone -q "$REMOTE" "$other" + ( + cd "$other" + git config user.email "12345+${BOT_EMAIL_SUFFIX}" + git config user.name "Don Petry" + echo "owner fix" > work; git add work + git commit -q --amend --no-edit + git push -q --force origin main + ) + local spoofed; spoofed=$(_remote_head) + git --git-dir="$REMOTE" log -1 --format=%B main | grep -q '^Dev-Lead-Stamp: ' + + cd "$WORK" + source "$GUARD_LIB" + run _commit_is_dev_lead "$spoofed" [ "$status" -ne 0 ] - # A non-numeric prefix before "+login" is also FOREIGN. - GIT_COMMITTER_EMAIL="evil+don-petry@users.noreply.github.com" \ - git commit -q --allow-empty -m evilprefix + # dev-lead, behind the owner, pushes its own unrelated fix: the owner's commit + # must survive. + echo devfix > devfix; git add devfix; git commit -q -m "dev-lead fix" + run push_no_clobber origin main + [ "$status" -eq 0 ] + git --git-dir="$REMOTE" merge-base --is-ancestor "$spoofed" main + [ "$(git --git-dir="$REMOTE" show main:work)" = "owner fix" ] +} + +@test "#2313: _push_guard_ownership_verdict — pushed SHA, matching stamp, and nothing else" { + source "$GUARD_LIB" + local tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904 + local p1=1111111111111111111111111111111111111111 + local p2=2222222222222222222222222222222222222222 + local stamp; stamp=$(dev_lead_stamp_of "$tree" "$p1") + + run _push_guard_ownership_verdict aaaa "$tree" "$p1" "msg" $'bbbb\naaaa' + [ "$status" -eq 0 ]; [ "$output" = own-pushed ] + + run _push_guard_ownership_verdict aaaa "$tree" "$p1" $'msg\n\nDev-Lead-Stamp: '"$stamp" "" + [ "$status" -eq 0 ]; [ "$output" = own-stamped ] + + # The same stamp on a commit with another parent (a cherry-pick or rebase). + run _push_guard_ownership_verdict aaaa "$tree" "$p2" $'msg\n\nDev-Lead-Stamp: '"$stamp" "" + [ "$status" -eq 1 ]; [ "$output" = foreign ] + + # A merge commit's stamp covers both parents, in order. + local merge_stamp; merge_stamp=$(dev_lead_stamp_of "$tree" "$p1" "$p2") + run _push_guard_ownership_verdict aaaa "$tree" "$p1 $p2" "Dev-Lead-Stamp: ${merge_stamp}" "" + [ "$status" -eq 0 ]; [ "$output" = own-stamped ] + run _push_guard_ownership_verdict aaaa "$tree" "$p2 $p1" "Dev-Lead-Stamp: ${merge_stamp}" "" + [ "$status" -eq 1 ] + + # A SHA that merely contains a pushed SHA is not it. + run _push_guard_ownership_verdict aaaa "$tree" "$p1" "msg" "aaaab" + [ "$status" -eq 1 ]; [ "$output" = foreign ] + + # No record at all: foreign, whatever identity made it. + run _push_guard_ownership_verdict aaaa "$tree" "$p1" "msg" "" + [ "$status" -eq 1 ]; [ "$output" = foreign ] +} + +@test "#2313: the committer email is not a signal — dev-lead's exact noreply address alone is foreign" { + cd "$WORK" + source "$GUARD_LIB" + GIT_COMMITTER_EMAIL="12345+don-petry@users.noreply.github.com" \ + git commit -q --allow-empty -m idprefix + run _commit_is_dev_lead HEAD + [ "$status" -ne 0 ] + GIT_COMMITTER_EMAIL="don-petry@users.noreply.github.com" \ + git commit -q --allow-empty -m bare run _commit_is_dev_lead HEAD [ "$status" -ne 0 ] } diff --git a/tests/dev-lead/unit/test_push_no_clobber.bats b/tests/dev-lead/unit/test_push_no_clobber.bats index 688bc1366..8bc93f74d 100644 --- a/tests/dev-lead/unit/test_push_no_clobber.bats +++ b/tests/dev-lead/unit/test_push_no_clobber.bats @@ -11,22 +11,24 @@ setup() { GUARD_LIB="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)/scripts/lib/git-push-guard.sh" - # dev-lead acts as `don-petry`; its own commits carry the bot noreply email so - # the #1607 identity-aware guard classifies them as OWN (a self-rewrite may be - # force-with-leased) rather than foreign steering (which is never discarded). + # dev-lead acts as `don-petry`. The seed commit stands for an earlier dev-lead + # run: it carries a Dev-Lead-Stamp, so the #1607/#2313 guard classifies it as + # OWN (a self-rewrite may be force-with-leased) rather than foreign steering + # (which is never discarded). The email alone would not (#2313). export BOT_USER="don-petry" REMOTE="$BATS_TEST_TMPDIR/remote.git" WORK="$BATS_TEST_TMPDIR/work" - # Bare remote seeded with commit A on `main` (authored as dev-lead). + # Bare remote seeded with commit A on `main` (stamped by dev-lead). git init -q --bare "$REMOTE" local seed="$BATS_TEST_TMPDIR/seed" git init -q "$seed" ( cd "$seed" + source "$GUARD_LIB" git config user.email "1+don-petry@users.noreply.github.com"; git config user.name don-petry - echo A > f; git add f; git commit -q -m A + echo A > f; git add f; git commit -q -m A -m "$(dev_lead_stamp_trailer)" git branch -M main git remote add origin "$REMOTE" git push -q -u origin main From ca32ebbfbfca3d8a276ee9c3fbf16629350d3e52 Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Wed, 21 Oct 2026 21:35:57 -0500 Subject: [PATCH 2/3] fix(dev-lead): stamp every commit dev-lead pushes, engine's included Review on #2385: commits the engine makes with its own `git commit` (prompts tell the model to commit; the rebase intent takes the same path) never got a Dev-Lead-Stamp. A later run has an empty in-run ledger, so its push guard treated them as foreign and rebased onto them; a conflict returned 2 and blocked dev-lead's rewrite of its own work. push_no_clobber and push_with_merge_guard now stamp every unpublished commit right before the push (dev_lead_stamp_unpublished), after the steering rebase so parents are final. Tree, author, dates and message are kept, and the stamp joins the existing trailer block. A commit already validly stamped is left alone, and commits already on the remote are never touched. The old-to-new rows join _PUSH_GUARD_REWRITES, so the claim sweep re-points a "Fixed in " reply at the commit that landed (#2032). fix-issue stamps before its two direct pushes. New bats: an engine-made commit is stamped on push and is dev-lead's own on a later run with an empty ledger (its amend force-pushes). Stamping leaves remote commits and valid stamps alone and keeps other trailers. An incorporated owner commit enters neither the ledger nor a stamp. Refs #2313 Co-Authored-By: Claude Opus 5.5 --- ...-write-mode-personas-on-a-shared-branch.md | 5 +- scripts/dev-lead-fix-issue.sh | 7 +- scripts/lib/auto-merge.sh | 2 + scripts/lib/git-identity.sh | 90 ++++++++++++- scripts/lib/git-push-guard.sh | 41 ++++-- .../unit/test_incorporate_remote_head.bats | 120 ++++++++++++++++-- tests/dev-lead/unit/test_push_no_clobber.bats | 12 +- 7 files changed, 246 insertions(+), 31 deletions(-) diff --git a/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md b/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md index da03e2acc..c82d17d9d 100644 --- a/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md +++ b/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md @@ -44,8 +44,9 @@ per ADR-0004), not by prompt text alone: never discarded (`push_no_clobber`, `scripts/lib/git-push-guard.sh`). A commit is foreign unless the persona's own records say it wrote it: a SHA this run pushed, or a `Dev-Lead-Stamp` trailer that matches the commit's - tree and parents. The committer identity is not a record, because the owner - commits as the same account (#2313). + tree and parents. The persona stamps every commit it is about to push, + including those the model made itself. The committer identity is not a + record, because the owner commits as the same account (#2313). 2. **"Pushed" is a verdict, not an assumption.** After the push, the harness compares the pre-pass head, the pushed SHA and the re-fetched remote head (`cl_push_landed_verdict`, `scripts/lib/claim-landing.sh`). Any result other diff --git a/scripts/dev-lead-fix-issue.sh b/scripts/dev-lead-fix-issue.sh index 541d2cd52..1ab18bad6 100755 --- a/scripts/dev-lead-fix-issue.sh +++ b/scripts/dev-lead-fix-issue.sh @@ -227,7 +227,9 @@ checkpoint_push_partial_work() { -m "$(dev_lead_stamp_trailer)" >/dev/null 2>&1 || return 1 fi - # Push the branch so the work lands on the remote before the run exits. + # Push the branch so the work lands on the remote before the run exits. Stamp + # the engine's own commits first so a later run knows them as dev-lead's (#2313). + dev_lead_stamp_unpublished --remotes=origin >/dev/null || true git push --set-upstream origin "$br" >/dev/null 2>&1 || return 1 printf '%s\n' "$br" @@ -684,6 +686,9 @@ ${lint_output} abort_empty_net_diff "$base_ref" fi + # Stamp the engine's own commits so a later run's push guard knows them as + # dev-lead's, not as steering it must rebase onto (#2313). + dev_lead_stamp_unpublished --remotes=origin >/dev/null || true git push --set-upstream origin "$branch" # Head SHA of the durable, pushed work — the verifiable artifact the completion diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index 7f901d5fc..c9fda2e97 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -147,6 +147,8 @@ push_with_merge_guard() { return "$reconcile_rc" fi + _push_guard_stamp_unpublished "${1:-}" + local errf publishing errf="$(mktemp)" # Record what this push publishes so a later reconcile in the run knows these diff --git a/scripts/lib/git-identity.sh b/scripts/lib/git-identity.sh index 0121c19d8..00dc251c9 100644 --- a/scripts/lib/git-identity.sh +++ b/scripts/lib/git-identity.sh @@ -44,11 +44,19 @@ setup_git_identity() { # Dev-Lead-Stamp: \nparent

..." — the commit's own tree and # parents, in the order a commit object lists them> # +# The harness stamps the commits it makes as it makes them, and stamps every +# other commit it is about to publish — the ones the engine made with its own +# `git commit` — just before the push (dev_lead_stamp_unpublished), so every +# commit dev-lead pushes carries a stamp a later run can check. +# # The stamp is bound to the commit's content and position. Amending a stamped # commit (new tree) or cherry-picking or rebasing it (new parents) keeps the # trailer text but breaks the match, so a copied stamp marks nothing. It is a # record dev-lead keeps, not a signature: it guards against copies made by -# accident, not against someone who computes a stamp on purpose. +# accident, not against someone who computes a stamp on purpose. A reword that +# changes only the message keeps tree and parents, so the stamp still matches: +# dev-lead may then rewrite that commit, which can lose the reworded message but +# never content that is not already in its own commit. # Not readonly: the push guard sources this file too, and a second `readonly` # assignment would abort a script running under `set -e`. DEV_LEAD_STAMP_KEY='Dev-Lead-Stamp' @@ -89,3 +97,83 @@ dev_lead_stamp_trailer() { stamp=$(dev_lead_stamp_of "$tree" ${parents[@]+"${parents[@]}"}) || return 1 printf '%s: %s\n' "$DEV_LEAD_STAMP_KEY" "$stamp" } + +# dev_lead_stamp_matches — true when has a +# Dev-Lead-Stamp line equal to the stamp of and the space-separated +# . Pure apart from `git hash-object --stdin`. +dev_lead_stamp_matches() { + local tree="${1:-}" parent_list="${2:-}" message="${3:-}" expected claimed + [ -n "$tree" ] || return 1 + # Word-split on purpose: is the space-separated %P list. + # shellcheck disable=SC2086 + expected=$(dev_lead_stamp_of "$tree" $parent_list 2>/dev/null) || return 1 + [ -n "$expected" ] || return 1 + while IFS= read -r claimed; do + [ "$claimed" = "$expected" ] && return 0 + done < <(sed -nE "s/^${DEV_LEAD_STAMP_KEY}:[[:space:]]*([0-9a-f]+)[[:space:]]*\$/\1/p" <<<"$message") + return 1 +} + +# dev_lead_stamp_commit [parent...] — write a copy of with the given +# parents and a Dev-Lead-Stamp for its tree and those parents, and echo the new +# SHA. Tree, author, committer, dates and message are kept; an existing +# Dev-Lead-Stamp line is replaced, not duplicated, and the stamp joins the +# message's trailer block so trailers such as Test-Change-Justification stay +# readable. Writes the object only: no ref moves. +dev_lead_stamp_commit() { + local sha="${1:-}" + [ -n "$sha" ] || return 1 + shift + local tree stamp msg ident p + local -a pargs=() who=() + tree=$(git rev-parse --verify --quiet "${sha}^{tree}") || return 1 + stamp=$(dev_lead_stamp_of "$tree" "$@") || return 1 + msg=$(git show -s --format='%B' "$sha" \ + | git interpret-trailers --if-exists replace --trailer "${DEV_LEAD_STAMP_KEY}: ${stamp}") || return 1 + ident=$(git show -s --date=raw --format='%an%n%ae%n%ad%n%cn%n%ce%n%cd' "$sha") || return 1 + while IFS= read -r p; do who+=("$p"); done <<<"$ident" + [ "${#who[@]}" -eq 6 ] || return 1 + for p in "$@"; do pargs+=(-p "$p"); done + printf '%s\n' "$msg" | GIT_AUTHOR_NAME="${who[0]}" GIT_AUTHOR_EMAIL="${who[1]}" GIT_AUTHOR_DATE="${who[2]}" \ + GIT_COMMITTER_NAME="${who[3]}" GIT_COMMITTER_EMAIL="${who[4]}" GIT_COMMITTER_DATE="${who[5]}" \ + git commit-tree "$tree" ${pargs[@]+"${pargs[@]}"} +} + +# dev_lead_stamp_unpublished [exclude...] — make every commit HEAD has beyond +# (rev-list arguments, e.g. a remote head and `--remotes=origin`) +# carry a valid Dev-Lead-Stamp, then move HEAD to the stamped copy (#2313). +# Call it right before a push: those commits are dev-lead's unpublished work, +# whoever ran `git commit`. A commit already validly stamped, with no rewritten +# parent, is kept as it is. Echoes one `\t` row per rewritten commit +# so a caller can re-point anything that cites the old SHA. Changes nothing and +# returns 1 if any step fails; the push then goes out unstamped, which a later +# run treats as foreign (rebase or stop, never discard). +dev_lead_stamp_unpublished() { + local head commits sha parent_list tree msg new rows="" mapped p + head=$(git rev-parse --verify --quiet HEAD 2>/dev/null) || return 0 + [ -n "$head" ] || return 0 + commits=$(git rev-list --reverse --topo-order HEAD --not "$@" 2>/dev/null) || return 1 + [ -n "$commits" ] || return 0 + while IFS= read -r sha; do + [ -z "$sha" ] && continue + parent_list=$(git show -s --format='%P' "$sha") || return 1 + local -a new_parents=() + for p in $parent_list; do + mapped=$(awk -F'\t' -v s="$p" '$1 == s { print $2; exit }' <<<"$rows") + new_parents+=("${mapped:-$p}") + done + if [ "${new_parents[*]-}" = "$parent_list" ]; then + tree=$(git rev-parse --verify --quiet "${sha}^{tree}") || return 1 + msg=$(git show -s --format='%B' "$sha") || return 1 + dev_lead_stamp_matches "$tree" "$parent_list" "$msg" && continue + fi + new=$(dev_lead_stamp_commit "$sha" ${new_parents[@]+"${new_parents[@]}"}) || return 1 + [ -n "$new" ] || return 1 + rows+="${sha}"$'\t'"${new}"$'\n' + done <<<"$commits" + [ -n "$rows" ] || return 0 + new=$(awk -F'\t' -v s="$head" '$1 == s { print $2; exit }' <<<"$rows") + [ -n "$new" ] || return 1 + git update-ref -m "dev-lead: stamp unpublished commits (#2313)" HEAD "$new" "$head" 2>/dev/null || return 1 + printf '%s' "$rows" +} diff --git a/scripts/lib/git-push-guard.sh b/scripts/lib/git-push-guard.sh index 7f79bc64f..f6c168308 100644 --- a/scripts/lib/git-push-guard.sh +++ b/scripts/lib/git-push-guard.sh @@ -75,18 +75,8 @@ _push_guard_ownership_verdict() { if [ -n "$sha" ] && grep -qxF -- "$sha" <<<"$pushed"; then echo own-pushed; return 0 fi - if [ -n "$tree" ]; then - local expected claimed - # Word-split on purpose: is the space-separated %P list. - # shellcheck disable=SC2086 - expected=$(dev_lead_stamp_of "$tree" $parent_list 2>/dev/null || true) - if [ -n "$expected" ]; then - while IFS= read -r claimed; do - if [ "$claimed" = "$expected" ]; then - echo own-stamped; return 0 - fi - done < <(sed -nE "s/^${DEV_LEAD_STAMP_KEY}:[[:space:]]*([0-9a-f]+)[[:space:]]*\$/\1/p" <<<"$message") - fi + if dev_lead_stamp_matches "$tree" "$parent_list" "$message"; then + echo own-stamped; return 0 fi echo foreign return 1 @@ -120,6 +110,31 @@ _push_guard_publishing() { fi } +# _push_guard_stamp_unpublished [remote] — stamp every commit the push is about +# to publish (dev_lead_stamp_unpublished), so a later run recognises the +# engine's own `git commit`s as dev-lead's, not just the harness's (#2313). Runs +# after incorporate_remote_head: a rebase changes parents, which would void a +# stamp made earlier. The old→new rows join _PUSH_GUARD_REWRITES, so the claim +# sweep re-points a "Fixed in " reply at the commit that landed (#2032). +_push_guard_stamp_unpublished() { + local remote="${1:-origin}" rows rc=0 + local -a exclude=(--remotes="$remote") + [ -n "$_PUSH_GUARD_REMOTE_SHA" ] && exclude+=("$_PUSH_GUARD_REMOTE_SHA") + rows=$(dev_lead_stamp_unpublished "${exclude[@]}") || rc=$? + if [ "$rc" -ne 0 ]; then + echo "::warning::could not stamp the commits about to be pushed; a later run will treat them as foreign (rebase or stop, never discard) (#2313)" >&2 + return 0 + fi + [ -z "$rows" ] && return 0 + rows="${rows%$'\n'}" + if [ -n "$_PUSH_GUARD_REWRITES" ]; then + _PUSH_GUARD_REWRITES+=$'\n'"${rows}" + else + _PUSH_GUARD_REWRITES="$rows" + fi + return 0 +} + # _push_guard_record_pushed — add a successful push's SHAs to the ledger. _push_guard_record_pushed() { local shas="${1:-}" sha @@ -310,6 +325,8 @@ push_no_clobber() { # steering commit, or stop/escalate if it cannot be incorporated cleanly. incorporate_remote_head "$@" || return $? + _push_guard_stamp_unpublished "${1:-}" + local errf publishing errf="$(mktemp)" publishing=$(_push_guard_publishing "${1:-}") diff --git a/tests/dev-lead/unit/test_incorporate_remote_head.bats b/tests/dev-lead/unit/test_incorporate_remote_head.bats index b4f21b140..c4ba34ce3 100644 --- a/tests/dev-lead/unit/test_incorporate_remote_head.bats +++ b/tests/dev-lead/unit/test_incorporate_remote_head.bats @@ -127,11 +127,12 @@ _remote_log() { git --git-dir="$REMOTE" log --format='%s' main; } echo v1 > work; git add work; git commit -q -m "dev-lead work" push_no_clobber origin main echo v2 > work; git add work; git commit -q --amend -m "dev-lead work v2" - local amended; amended=$(git rev-parse HEAD) run push_no_clobber origin main [ "$status" -eq 0 ] - [ "$(_remote_head)" = "$amended" ] + # The amend landed (as its stamped copy, #2313) and v1 is gone. + [ "$(git --git-dir="$REMOTE" show main:work)" = v2 ] + [ "$(git --git-dir="$REMOTE" log -1 --format=%s main)" = "dev-lead work v2" ] } @test "AC3: a force-push logs the discarded SHA(s)" { @@ -186,6 +187,7 @@ _earlier_run_push() { @test "#2313: an owner commit with dev-lead's exact email is kept, not force-pushed over" { cd "$WORK" source "$GUARD_LIB" + _PUSH_GUARD_PUSHED="" # The #1314 shape: dev-lead builds a commit on the head it checked out... echo devfix > devfix; git add devfix; git commit -q -m "dev-lead fix" # ...while the owner pushes a fix to the same branch with the same identity. @@ -202,6 +204,13 @@ _earlier_run_push() { echo "$head_tree" | grep -qx devfix # And the run says so. grep -q 'owner: owner.txt' "$GITHUB_STEP_SUMMARY" + # The owner's commit went into neither the ledger nor a stamp: a second push in + # this run still sees it as foreign. + run grep -qxF "$owner_sha" <<<"$_PUSH_GUARD_PUSHED" + [ "$status" -ne 0 ] + run _commit_is_dev_lead "$owner_sha" + [ "$status" -ne 0 ] + [ "$(git --git-dir="$REMOTE" log -1 --format=%B "$owner_sha" | grep -c '^Dev-Lead-Stamp: ')" -eq 0 ] } @test "#2313: an owner commit with dev-lead's email that conflicts stops the push" { @@ -228,22 +237,24 @@ _earlier_run_push() { # This run amends the earlier run's commit: a legitimate self-rewrite. echo v2 > work; git add work; git commit -q --amend -m "dev-lead: work v2" - local amended; amended=$(git rev-parse HEAD) run push_no_clobber origin main [ "$status" -eq 0 ] - [ "$(_remote_head)" = "$amended" ] + [ "$(git --git-dir="$REMOTE" show main:work)" = v2 ] + [ "$(git --git-dir="$REMOTE" log -1 --format=%s main)" = "dev-lead: work v2" ] echo "$output" | grep -q "${stamped:0:12}" } -@test "#2313: a commit this run pushed is its own even with no stamp" { +@test "#2313: a commit this run pushed is its own even when stamping failed" { cd "$WORK" source "$GUARD_LIB" + dev_lead_stamp_unpublished() { return 1; } echo v1 > work; git add work; git commit -q -m "engine commit, unstamped" local pushed; pushed=$(git rev-parse HEAD) run _commit_is_dev_lead "$pushed" [ "$status" -ne 0 ] - push_no_clobber origin main + push_no_clobber origin main 2>/dev/null + [ "$(_remote_head)" = "$pushed" ] run _commit_is_dev_lead "$pushed" [ "$status" -eq 0 ] grep -qxF "$pushed" <<<"$_PUSH_GUARD_PUSHED" @@ -280,6 +291,81 @@ _earlier_run_push() { [ "$(git --git-dir="$REMOTE" show main:work)" = "owner fix" ] } +# A later dev-lead run: a fresh clone of the branch, with an empty in-run ledger. +_later_run_clone() { + local dir="$BATS_TEST_TMPDIR/later.$RANDOM" + git clone -q "$REMOTE" "$dir" + ( cd "$dir"; _dev_identity ) + printf '%s\n' "$dir" +} + +@test "#2313: an engine-made (unstamped) commit is stamped on push and is dev-lead's own on a later run" { + cd "$WORK" + source "$GUARD_LIB" + # The engine commits with its own `git commit`: no trailer, no harness involved. + echo v1 > work; git add work; git commit -q -m "fix: engine's own commit" + local engine_sha engine_tree + engine_sha=$(git rev-parse HEAD); engine_tree=$(git rev-parse HEAD^{tree}) + push_no_clobber origin main + + # What landed is the stamped copy: same tree, author and subject, new SHA. + local landed; landed=$(_remote_head) + [ "$landed" != "$engine_sha" ] + [ "$(git --git-dir="$REMOTE" rev-parse main^{tree})" = "$engine_tree" ] + [ "$(git --git-dir="$REMOTE" log -1 --format=%s main)" = "fix: engine's own commit" ] + git --git-dir="$REMOTE" log -1 --format=%B main | grep -q '^Dev-Lead-Stamp: ' + # The claim sweep can follow the engine's SHA to the one that landed (#2032). + grep -qx "${engine_sha}"$'\t'"${landed}" <<<"$_PUSH_GUARD_REWRITES" + + # A later run starts with an empty ledger and amends that commit. + local later; later=$(_later_run_clone) + cd "$later" + _PUSH_GUARD_PUSHED="" _PUSH_GUARD_REWRITES="" + source "$GUARD_LIB" + [ -z "$_PUSH_GUARD_PUSHED" ] + run _commit_is_dev_lead "$landed" + [ "$status" -eq 0 ] + echo v2 > work; git add work; git commit -q --amend -m "fix: engine's own commit v2" + + run push_no_clobber origin main + [ "$status" -eq 0 ] + # dev-lead's own rewrite went through: v2 is the head and v1 is gone. + [ "$(git --git-dir="$REMOTE" show main:work)" = v2 ] + run git --git-dir="$REMOTE" merge-base --is-ancestor "$landed" main + [ "$status" -ne 0 ] + grep -q 'STOPPED' "$GITHUB_STEP_SUMMARY" && return 1 + return 0 +} + +@test "#2313: stamping before a push leaves commits already on the remote and valid stamps alone" { + cd "$WORK" + source "$GUARD_LIB" + # The owner's commit is on the remote and in HEAD: published, never restamped. + _owner_push owner.txt "owner work" + git pull -q origin main + local owner_sha; owner_sha=$(git rev-parse HEAD) + # A harness commit that is already stamped, then an engine commit on top. + echo h > harness; git add harness + git commit -q -m "fix(reviews): harness commit" -m "$(dev_lead_stamp_trailer)" + local harness_sha; harness_sha=$(git rev-parse HEAD) + echo e > engine; git add engine; git commit -q -m "engine commit" -m "Test-Change-Justification: review asked for it" + + push_no_clobber origin main + # The owner's commit and the stamped harness commit keep their SHAs. + git --git-dir="$REMOTE" merge-base --is-ancestor "$harness_sha" main + [ "$(git --git-dir="$REMOTE" rev-parse main~1)" = "$harness_sha" ] + [ "$(git --git-dir="$REMOTE" rev-parse main~2)" = "$owner_sha" ] + [ "$(git --git-dir="$REMOTE" log -1 --format=%B main~2 | grep -c '^Dev-Lead-Stamp: ')" -eq 0 ] + # Only the engine commit was rewritten; its other trailer is still a trailer. + [ "$(grep -c . <<<"$_PUSH_GUARD_REWRITES")" -eq 1 ] + [ "$(git --git-dir="$REMOTE" log -1 --format='%(trailers:key=Test-Change-Justification,valueonly)' main)" = "review asked for it" ] + run _commit_is_dev_lead "$(_remote_head)" + [ "$status" -eq 0 ] + # And the working tree is untouched by the restamp. + [ -z "$(git status --porcelain)" ] + [ "$(git rev-parse HEAD)" = "$(_remote_head)" ] +} + @test "#2313: _push_guard_ownership_verdict — pushed SHA, matching stamp, and nothing else" { source "$GUARD_LIB" local tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904 @@ -329,7 +415,7 @@ _earlier_run_push() { @test "no divergence: plain fast-forward push still works" { cd "$WORK" source "$GUARD_LIB" - echo ff > ff; git add ff; git commit -q -m "ff commit" + echo ff > ff; git add ff; git commit -q -m "ff commit" -m "$(dev_lead_stamp_trailer)" local head; head=$(git rev-parse HEAD) run push_no_clobber origin main @@ -342,6 +428,16 @@ _earlier_run_push() { # the guard rebases them onto a steering commit, those SHAs exist nowhere on the # remote, so the claim sweep needs the map to keep the claims that did land. +# _follow_rewrites — follow _PUSH_GUARD_REWRITES rows to the last SHA, the +# way cl_map_sha does. +_follow_rewrites() { + local cur="$1" next + while next=$(awk -F'\t' -v s="$cur" '$1 == s { print $2; exit }' <<<"$_PUSH_GUARD_REWRITES") && [ -n "$next" ]; do + cur="$next" + done + printf '%s\n' "$cur" +} + @test "#2032: a clean rebase onto steering records each dev-lead commit's rebased SHA" { cd "$WORK" source "$GUARD_LIB" @@ -356,15 +452,17 @@ _earlier_run_push() { new2=$(git rev-parse HEAD); new1=$(git rev-parse HEAD~1) [ "$new2" != "$old2" ] [ "$(_remote_head)" = "$new2" ] - grep -qx "${old1}"$'\t'"${new1}" <<<"$_PUSH_GUARD_REWRITES" - grep -qx "${old2}"$'\t'"${new2}" <<<"$_PUSH_GUARD_REWRITES" - [ "$(grep -c . <<<"$_PUSH_GUARD_REWRITES")" -eq 2 ] + # The rebase and then the stamp (#2313) each rewrote the commits; the claim + # sweep follows the chain (cl_map_sha), so each old SHA reaches what landed. + [ "$(_follow_rewrites "$old1")" = "$new1" ] + [ "$(_follow_rewrites "$old2")" = "$new2" ] } @test "#2032: no rebase (plain fast-forward) records no rewrites" { cd "$WORK" source "$GUARD_LIB" - echo ff > ff; git add ff; git commit -q -m "ff commit" + # A harness commit, stamped when made, so the push rewrites nothing. + echo ff > ff; git add ff; git commit -q -m "ff commit" -m "$(dev_lead_stamp_trailer)" push_no_clobber origin main [ -z "$_PUSH_GUARD_REWRITES" ] } diff --git a/tests/dev-lead/unit/test_push_no_clobber.bats b/tests/dev-lead/unit/test_push_no_clobber.bats index 8bc93f74d..6237f3b56 100644 --- a/tests/dev-lead/unit/test_push_no_clobber.bats +++ b/tests/dev-lead/unit/test_push_no_clobber.bats @@ -60,11 +60,14 @@ _remote_head() { git --git-dir="$REMOTE" rev-parse main; } cd "$WORK" source "$GUARD_LIB" echo B >> f; git add f; git commit -q -m B - local local_head; local_head=$(git rev-parse HEAD) + local local_tree; local_tree=$(git rev-parse HEAD^{tree}) run push_no_clobber origin main [ "$status" -eq 0 ] - [ "$(_remote_head)" = "$local_head" ] + # The push stamps the commit first (#2313): the remote holds HEAD as it is + # after the push, with the tree that was committed. + [ "$(_remote_head)" = "$(git rev-parse HEAD)" ] + [ "$(git --git-dir="$REMOTE" rev-parse main^{tree})" = "$local_tree" ] } @test "push_no_clobber: rewritten branch pushes with lease when remote is unchanged" { @@ -72,11 +75,12 @@ _remote_head() { git --git-dir="$REMOTE" rev-parse main; } source "$GUARD_LIB" # Rewrite history (rebase-like) so the branch diverges from origin/main. echo A2 > f; git add f; git commit -q --amend -m A2 - local local_head; local_head=$(git rev-parse HEAD) + local local_tree; local_tree=$(git rev-parse HEAD^{tree}) run push_no_clobber origin main [ "$status" -eq 0 ] - [ "$(_remote_head)" = "$local_head" ] + [ "$(_remote_head)" = "$(git rev-parse HEAD)" ] + [ "$(git --git-dir="$REMOTE" rev-parse main^{tree})" = "$local_tree" ] } @test "push_no_clobber: rewritten branch ABORTS when the remote moved under us — unseen commit survives" { From 2c5a5bd3f772f2cd511e9aba54306d716e2cf42a Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Wed, 21 Oct 2026 21:58:34 -0500 Subject: [PATCH 3/3] fix(dev-lead): record pushed SHAs in a ledger, never rewrite commits ca32ebbf stamped the engine's commits by rewriting them just before the push. That changed their SHAs, which broke the CI unit job: - the #2143 push job no longer pushed the work job's exact commit; - the #2269 this-pass checks no longer matched the commit the pass made. A commit is never rewritten after the fact now. Commits the engine makes with its own `git commit` are covered by a cross-run ledger instead: after each successful push, push_no_clobber, push_with_merge_guard and fix-issue's direct pushes add a git note per pushed SHA to refs/notes/dev-lead-pushed on the remote. The push fetches the ledger, annotates, and pushes as a fast-forward, retrying on a race. It never forces, so another run's notes survive. incorporate_remote_head fetches the ledger before it classifies commits. A note is keyed by the exact SHA, so an amended or rebased copy has none. A failed ledger read or write only warns: the commit then counts as foreign (rebase or stop). Harness commits keep their commit-time Dev-Lead-Stamp. New bats: an engine-made commit an earlier run pushed is dev-lead's own via the ledger, and its amend force-pushes with the exact SHA. An owner amend of a noted commit is foreign. Publishing keeps another run's notes. An incorporated owner commit enters neither ledger. Refs #2313 Co-Authored-By: Claude Opus 5.5 --- ...-write-mode-personas-on-a-shared-branch.md | 8 +- scripts/dev-lead-fix-issue.sh | 19 ++- scripts/lib/auto-merge.sh | 6 +- scripts/lib/git-identity.sh | 73 +-------- scripts/lib/git-push-guard.sh | 95 +++++++---- .../unit/test_incorporate_remote_head.bats | 152 +++++++++--------- tests/dev-lead/unit/test_push_no_clobber.bats | 12 +- 7 files changed, 165 insertions(+), 200 deletions(-) diff --git a/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md b/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md index c82d17d9d..1a5b9bdc7 100644 --- a/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md +++ b/docs/architecture/adr/0011-write-mode-personas-on-a-shared-branch.md @@ -43,10 +43,10 @@ per ADR-0004), not by prompt text alone: stops and escalates when that cannot be done cleanly. A foreign commit is never discarded (`push_no_clobber`, `scripts/lib/git-push-guard.sh`). A commit is foreign unless the persona's own records say it wrote it: a SHA - this run pushed, or a `Dev-Lead-Stamp` trailer that matches the commit's - tree and parents. The persona stamps every commit it is about to push, - including those the model made itself. The committer identity is not a - record, because the owner commits as the same account (#2313). + it pushed, in this run or an earlier one (the `refs/notes/dev-lead-pushed` + ledger on the remote), or a `Dev-Lead-Stamp` trailer that matches the + commit's tree and parents. The committer identity is not a record, because + the owner commits as the same account (#2313). 2. **"Pushed" is a verdict, not an assumption.** After the push, the harness compares the pre-pass head, the pushed SHA and the re-fetched remote head (`cl_push_landed_verdict`, `scripts/lib/claim-landing.sh`). Any result other diff --git a/scripts/dev-lead-fix-issue.sh b/scripts/dev-lead-fix-issue.sh index 1ab18bad6..bac8b014a 100755 --- a/scripts/dev-lead-fix-issue.sh +++ b/scripts/dev-lead-fix-issue.sh @@ -5,6 +5,9 @@ set -euo pipefail source "$(dirname "$0")/engine.sh" source "$(dirname "$0")/lib/git-identity.sh" +# _push_guard_record_pushed: record what this run pushes in the cross-run ledger +# a later run's push guard reads (#2313). +source "$(dirname "$0")/lib/git-push-guard.sh" # Pure completion-claim helpers (#1445): body_has_completion_claim / # claim_is_retracted / supersede_claim_body + the PC_CLAIM_RETRACTED_MARKER. source "$(dirname "$0")/lib/premature-closure-detect.sh" @@ -227,10 +230,12 @@ checkpoint_push_partial_work() { -m "$(dev_lead_stamp_trailer)" >/dev/null 2>&1 || return 1 fi - # Push the branch so the work lands on the remote before the run exits. Stamp - # the engine's own commits first so a later run knows them as dev-lead's (#2313). - dev_lead_stamp_unpublished --remotes=origin >/dev/null || true + # Push the branch so the work lands on the remote before the run exits, and + # record its commits as dev-lead's for later runs' push guard (#2313). + local published + published=$(git rev-list HEAD --not --remotes=origin 2>/dev/null || true) git push --set-upstream origin "$br" >/dev/null 2>&1 || return 1 + _push_guard_record_pushed "$published" origin printf '%s\n' "$br" return 0 @@ -686,10 +691,12 @@ ${lint_output} abort_empty_net_diff "$base_ref" fi - # Stamp the engine's own commits so a later run's push guard knows them as - # dev-lead's, not as steering it must rebase onto (#2313). - dev_lead_stamp_unpublished --remotes=origin >/dev/null || true + # Record the pushed commits (the engine's own included) as dev-lead's, so a + # later run's push guard does not mistake them for steering (#2313). + local published + published=$(git rev-list HEAD --not --remotes=origin 2>/dev/null || true) git push --set-upstream origin "$branch" + _push_guard_record_pushed "$published" origin # Head SHA of the durable, pushed work — the verifiable artifact the completion # claim references (#1445, AC #2). diff --git a/scripts/lib/auto-merge.sh b/scripts/lib/auto-merge.sh index c9fda2e97..f621bfbff 100644 --- a/scripts/lib/auto-merge.sh +++ b/scripts/lib/auto-merge.sh @@ -147,8 +147,6 @@ push_with_merge_guard() { return "$reconcile_rc" fi - _push_guard_stamp_unpublished "${1:-}" - local errf publishing errf="$(mktemp)" # Record what this push publishes so a later reconcile in the run knows these @@ -156,7 +154,7 @@ push_with_merge_guard() { publishing=$(_push_guard_publishing "${1:-}") if git push "$@" 2>"$errf"; then rm -f "$errf" - _push_guard_record_pushed "$publishing" + _push_guard_record_pushed "$publishing" "${1:-origin}" # Refresh the held SHA after our own push so the EXIT-trap restore uses # --match-head-commit against the new head rather than the pre-push SHA. _AM_HEAD_SHA=$(git rev-parse HEAD 2>/dev/null || true) @@ -181,7 +179,7 @@ push_with_merge_guard() { echo "::notice::push rejected (non-fast-forward) and HEAD has diverged from ${upstream} — history was rewritten (likely a self-rebase); retrying with a pinned lease. Overwriting dev-lead's own commit(s): ${discarded:-}" >&2 if _pinned_force_push "$@" 2>>"$errf"; then rm -f "$errf" - _push_guard_record_pushed "$publishing" + _push_guard_record_pushed "$publishing" "${1:-origin}" _AM_HEAD_SHA=$(git rev-parse HEAD 2>/dev/null || true) return 0 fi diff --git a/scripts/lib/git-identity.sh b/scripts/lib/git-identity.sh index 00dc251c9..397816e39 100644 --- a/scripts/lib/git-identity.sh +++ b/scripts/lib/git-identity.sh @@ -44,10 +44,11 @@ setup_git_identity() { # Dev-Lead-Stamp: \nparent

..." — the commit's own tree and # parents, in the order a commit object lists them> # -# The harness stamps the commits it makes as it makes them, and stamps every -# other commit it is about to publish — the ones the engine made with its own -# `git commit` — just before the push (dev_lead_stamp_unpublished), so every -# commit dev-lead pushes carries a stamp a later run can check. +# The harness stamps the commits it makes as it makes them. It never rewrites a +# commit after the fact to add one: the engine's claims, the #2143 test handoff +# and the #2269 this-pass checks all cite exact SHAs. Commits the engine makes +# with its own `git commit` are covered instead by the push guard's ledger of +# pushed SHAs (refs/notes/dev-lead-pushed, git-push-guard.sh). # # The stamp is bound to the commit's content and position. Amending a stamped # commit (new tree) or cherry-picking or rebasing it (new parents) keeps the @@ -113,67 +114,3 @@ dev_lead_stamp_matches() { done < <(sed -nE "s/^${DEV_LEAD_STAMP_KEY}:[[:space:]]*([0-9a-f]+)[[:space:]]*\$/\1/p" <<<"$message") return 1 } - -# dev_lead_stamp_commit [parent...] — write a copy of with the given -# parents and a Dev-Lead-Stamp for its tree and those parents, and echo the new -# SHA. Tree, author, committer, dates and message are kept; an existing -# Dev-Lead-Stamp line is replaced, not duplicated, and the stamp joins the -# message's trailer block so trailers such as Test-Change-Justification stay -# readable. Writes the object only: no ref moves. -dev_lead_stamp_commit() { - local sha="${1:-}" - [ -n "$sha" ] || return 1 - shift - local tree stamp msg ident p - local -a pargs=() who=() - tree=$(git rev-parse --verify --quiet "${sha}^{tree}") || return 1 - stamp=$(dev_lead_stamp_of "$tree" "$@") || return 1 - msg=$(git show -s --format='%B' "$sha" \ - | git interpret-trailers --if-exists replace --trailer "${DEV_LEAD_STAMP_KEY}: ${stamp}") || return 1 - ident=$(git show -s --date=raw --format='%an%n%ae%n%ad%n%cn%n%ce%n%cd' "$sha") || return 1 - while IFS= read -r p; do who+=("$p"); done <<<"$ident" - [ "${#who[@]}" -eq 6 ] || return 1 - for p in "$@"; do pargs+=(-p "$p"); done - printf '%s\n' "$msg" | GIT_AUTHOR_NAME="${who[0]}" GIT_AUTHOR_EMAIL="${who[1]}" GIT_AUTHOR_DATE="${who[2]}" \ - GIT_COMMITTER_NAME="${who[3]}" GIT_COMMITTER_EMAIL="${who[4]}" GIT_COMMITTER_DATE="${who[5]}" \ - git commit-tree "$tree" ${pargs[@]+"${pargs[@]}"} -} - -# dev_lead_stamp_unpublished [exclude...] — make every commit HEAD has beyond -# (rev-list arguments, e.g. a remote head and `--remotes=origin`) -# carry a valid Dev-Lead-Stamp, then move HEAD to the stamped copy (#2313). -# Call it right before a push: those commits are dev-lead's unpublished work, -# whoever ran `git commit`. A commit already validly stamped, with no rewritten -# parent, is kept as it is. Echoes one `\t` row per rewritten commit -# so a caller can re-point anything that cites the old SHA. Changes nothing and -# returns 1 if any step fails; the push then goes out unstamped, which a later -# run treats as foreign (rebase or stop, never discard). -dev_lead_stamp_unpublished() { - local head commits sha parent_list tree msg new rows="" mapped p - head=$(git rev-parse --verify --quiet HEAD 2>/dev/null) || return 0 - [ -n "$head" ] || return 0 - commits=$(git rev-list --reverse --topo-order HEAD --not "$@" 2>/dev/null) || return 1 - [ -n "$commits" ] || return 0 - while IFS= read -r sha; do - [ -z "$sha" ] && continue - parent_list=$(git show -s --format='%P' "$sha") || return 1 - local -a new_parents=() - for p in $parent_list; do - mapped=$(awk -F'\t' -v s="$p" '$1 == s { print $2; exit }' <<<"$rows") - new_parents+=("${mapped:-$p}") - done - if [ "${new_parents[*]-}" = "$parent_list" ]; then - tree=$(git rev-parse --verify --quiet "${sha}^{tree}") || return 1 - msg=$(git show -s --format='%B' "$sha") || return 1 - dev_lead_stamp_matches "$tree" "$parent_list" "$msg" && continue - fi - new=$(dev_lead_stamp_commit "$sha" ${new_parents[@]+"${new_parents[@]}"}) || return 1 - [ -n "$new" ] || return 1 - rows+="${sha}"$'\t'"${new}"$'\n' - done <<<"$commits" - [ -n "$rows" ] || return 0 - new=$(awk -F'\t' -v s="$head" '$1 == s { print $2; exit }' <<<"$rows") - [ -n "$new" ] || return 1 - git update-ref -m "dev-lead: stamp unpublished commits (#2313)" HEAD "$new" "$head" 2>/dev/null || return 1 - printf '%s' "$rows" -} diff --git a/scripts/lib/git-push-guard.sh b/scripts/lib/git-push-guard.sh index f6c168308..c752c91da 100644 --- a/scripts/lib/git-push-guard.sh +++ b/scripts/lib/git-push-guard.sh @@ -59,10 +59,22 @@ fi # in one run. _PUSH_GUARD_PUSHED="${_PUSH_GUARD_PUSHED:-}" +# The cross-run ledger (#2313): a git-notes ref on the remote holding one note per +# SHA dev-lead has pushed, in any run. A note is keyed by the exact commit, so an +# amended, cherry-picked or rebased copy has none. This is what covers commits the +# engine makes with its own `git commit`, which carry no Dev-Lead-Stamp; they are +# never rewritten to add one, because claims, the #2143 handoff and the #2269 +# this-pass checks cite their exact SHAs. Reading or writing it can fail (no +# remote, no permission); a commit the ledger cannot vouch for is foreign, which +# means rebase or stop, never discard. +_PUSH_GUARD_LEDGER_REF="refs/notes/dev-lead-pushed" + # _push_guard_ownership_verdict — decide # whether a commit is dev-lead's own from evidence dev-lead recorded itself -# (#2313). Echoes one token; returns 0 only for the two `own-*` tokens: +# (#2313). The optional 6th argument is `true` when the cross-run ledger holds a +# note for . Echoes one token; returns 0 only for the `own-*` tokens: # own-pushed is one of the newline-separated SHAs in . +# own-ledger is `true`: an earlier run pushed exactly this commit. # own-stamped has a `Dev-Lead-Stamp:` line whose hash equals the # stamp of and the space-separated . An amended, # cherry-picked or rebased copy keeps the line but not the tree and @@ -71,10 +83,13 @@ _PUSH_GUARD_PUSHED="${_PUSH_GUARD_PUSHED:-}" # are exactly dev-lead's: the owner commits with the same identity. # Pure apart from `git hash-object --stdin` (no repository, network or gh). _push_guard_ownership_verdict() { - local sha="${1:-}" tree="${2:-}" parent_list="${3:-}" message="${4:-}" pushed="${5:-}" + local sha="${1:-}" tree="${2:-}" parent_list="${3:-}" message="${4:-}" pushed="${5:-}" recorded="${6:-false}" if [ -n "$sha" ] && grep -qxF -- "$sha" <<<"$pushed"; then echo own-pushed; return 0 fi + if [ -n "$sha" ] && [ "$recorded" = true ]; then + echo own-ledger; return 0 + fi if dev_lead_stamp_matches "$tree" "$parent_list" "$message"; then echo own-stamped; return 0 fi @@ -83,7 +98,8 @@ _push_guard_ownership_verdict() { } # _commit_is_dev_lead SHA — true only when dev-lead's own records say it wrote the -# commit: this run pushed it, or it carries a matching Dev-Lead-Stamp (#2313). The +# commit: this run pushed it, the cross-run ledger has a note for it (fetched by +# incorporate_remote_head), or it carries a matching Dev-Lead-Stamp (#2313). The # committer email is deliberately NOT a signal: since ADR-0008 the owner, every # Claude Code session on the owner's machine and dev-lead all commit as # +don-petry@users.noreply.github.com. An unreadable commit is foreign. @@ -94,7 +110,38 @@ _commit_is_dev_lead() { tree="${meta%%$'\t'*}" parents="${meta#*$'\t'}" message=$(git show -s --format='%B' "$full" 2>/dev/null || true) - _push_guard_ownership_verdict "$full" "$tree" "$parents" "$message" "$_PUSH_GUARD_PUSHED" >/dev/null + local recorded=false + git notes --ref="$_PUSH_GUARD_LEDGER_REF" list "$full" >/dev/null 2>&1 && recorded=true + _push_guard_ownership_verdict "$full" "$tree" "$parents" "$message" "$_PUSH_GUARD_PUSHED" "$recorded" >/dev/null +} + +# _push_guard_ledger_fetch [remote] — replace the local ledger with the remote's. +# A remote without one (or an unreachable remote) leaves the local ref as it is. +_push_guard_ledger_fetch() { + git fetch --quiet "${1:-origin}" "+${_PUSH_GUARD_LEDGER_REF}:${_PUSH_GUARD_LEDGER_REF}" >/dev/null 2>&1 || true +} + +# _push_guard_ledger_publish — add a note for each SHA to the +# remote ledger. Fetch, annotate, push as a fast-forward; when another run moved +# the ledger in between, fetch again and redo it (three tries). Never forces, so a +# note another run wrote is never lost. A failure only warns: the SHAs are still +# in this run's _PUSH_GUARD_PUSHED, and a later run treats them as foreign. +_push_guard_ledger_publish() { + local remote="${1:-origin}" shas="${2:-}" sha _attempt + [ -n "$shas" ] || return 0 + local note="dev-lead pushed this commit (run ${GITHUB_RUN_ID:-local}, #2313)" + for _attempt in 1 2 3; do + _push_guard_ledger_fetch "$remote" + while IFS= read -r sha; do + [ -z "$sha" ] && continue + git notes --ref="$_PUSH_GUARD_LEDGER_REF" add -f -m "$note" "$sha" >/dev/null 2>&1 || continue 2 + done <<<"$shas" + if git push --quiet "$remote" "${_PUSH_GUARD_LEDGER_REF}:${_PUSH_GUARD_LEDGER_REF}" >/dev/null 2>&1; then + return 0 + fi + done + echo "::warning::could not record the pushed commit(s) in ${remote} ${_PUSH_GUARD_LEDGER_REF}; a later run will treat them as foreign (rebase or stop, never discard) (#2313)" >&2 + return 0 } # _push_guard_publishing [remote] — echo the SHAs a push of HEAD is about to @@ -110,34 +157,10 @@ _push_guard_publishing() { fi } -# _push_guard_stamp_unpublished [remote] — stamp every commit the push is about -# to publish (dev_lead_stamp_unpublished), so a later run recognises the -# engine's own `git commit`s as dev-lead's, not just the harness's (#2313). Runs -# after incorporate_remote_head: a rebase changes parents, which would void a -# stamp made earlier. The old→new rows join _PUSH_GUARD_REWRITES, so the claim -# sweep re-points a "Fixed in " reply at the commit that landed (#2032). -_push_guard_stamp_unpublished() { - local remote="${1:-origin}" rows rc=0 - local -a exclude=(--remotes="$remote") - [ -n "$_PUSH_GUARD_REMOTE_SHA" ] && exclude+=("$_PUSH_GUARD_REMOTE_SHA") - rows=$(dev_lead_stamp_unpublished "${exclude[@]}") || rc=$? - if [ "$rc" -ne 0 ]; then - echo "::warning::could not stamp the commits about to be pushed; a later run will treat them as foreign (rebase or stop, never discard) (#2313)" >&2 - return 0 - fi - [ -z "$rows" ] && return 0 - rows="${rows%$'\n'}" - if [ -n "$_PUSH_GUARD_REWRITES" ]; then - _PUSH_GUARD_REWRITES+=$'\n'"${rows}" - else - _PUSH_GUARD_REWRITES="$rows" - fi - return 0 -} - -# _push_guard_record_pushed — add a successful push's SHAs to the ledger. +# _push_guard_record_pushed [remote] — add a successful push's SHAs to this +# run's ledger and, when is given, to the cross-run ledger on it. _push_guard_record_pushed() { - local shas="${1:-}" sha + local shas="${1:-}" remote="${2:-}" sha while IFS= read -r sha; do [ -z "$sha" ] && continue grep -qxF -- "$sha" <<<"$_PUSH_GUARD_PUSHED" && continue @@ -147,6 +170,7 @@ _push_guard_record_pushed() { _PUSH_GUARD_PUSHED="$sha" fi done <<<"$shas" + [ -n "$remote" ] && _push_guard_ledger_publish "$remote" "$shas" return 0 } @@ -249,6 +273,9 @@ incorporate_remote_head() { unincorporated=$(git rev-list --reverse "HEAD..${remote_sha}" 2>/dev/null || true) [ -z "$unincorporated" ] && return 0 + # The cross-run ledger vouches for commits earlier runs pushed (#2313). + _push_guard_ledger_fetch "$remote" + local sha while IFS= read -r sha; do [ -z "$sha" ] && continue @@ -325,15 +352,13 @@ push_no_clobber() { # steering commit, or stop/escalate if it cannot be incorporated cleanly. incorporate_remote_head "$@" || return $? - _push_guard_stamp_unpublished "${1:-}" - local errf publishing errf="$(mktemp)" publishing=$(_push_guard_publishing "${1:-}") if git push "$@" 2>"$errf"; then rm -f "$errf" - _push_guard_record_pushed "$publishing" + _push_guard_record_pushed "$publishing" "${1:-origin}" return 0 fi @@ -354,7 +379,7 @@ push_no_clobber() { echo "::notice::push rejected (non-fast-forward) and HEAD diverged from ${upstream} — history was rewritten (likely a self-rebase); retrying with a pinned lease (aborts unless the remote is still where dev-lead incorporated). Overwriting dev-lead's own commit(s): ${discarded:-}" >&2 if _pinned_force_push "$@" 2>>"$errf"; then rm -f "$errf" - _push_guard_record_pushed "$publishing" + _push_guard_record_pushed "$publishing" "${1:-origin}" return 0 fi cat "$errf" >&2 diff --git a/tests/dev-lead/unit/test_incorporate_remote_head.bats b/tests/dev-lead/unit/test_incorporate_remote_head.bats index c4ba34ce3..debc87603 100644 --- a/tests/dev-lead/unit/test_incorporate_remote_head.bats +++ b/tests/dev-lead/unit/test_incorporate_remote_head.bats @@ -127,12 +127,11 @@ _remote_log() { git --git-dir="$REMOTE" log --format='%s' main; } echo v1 > work; git add work; git commit -q -m "dev-lead work" push_no_clobber origin main echo v2 > work; git add work; git commit -q --amend -m "dev-lead work v2" + local amended; amended=$(git rev-parse HEAD) run push_no_clobber origin main [ "$status" -eq 0 ] - # The amend landed (as its stamped copy, #2313) and v1 is gone. - [ "$(git --git-dir="$REMOTE" show main:work)" = v2 ] - [ "$(git --git-dir="$REMOTE" log -1 --format=%s main)" = "dev-lead work v2" ] + [ "$(_remote_head)" = "$amended" ] } @test "AC3: a force-push logs the discarded SHA(s)" { @@ -204,13 +203,14 @@ _earlier_run_push() { echo "$head_tree" | grep -qx devfix # And the run says so. grep -q 'owner: owner.txt' "$GITHUB_STEP_SUMMARY" - # The owner's commit went into neither the ledger nor a stamp: a second push in - # this run still sees it as foreign. + # The owner's commit entered neither ledger: a second push in this run, or a + # later run, still sees it as foreign. run grep -qxF "$owner_sha" <<<"$_PUSH_GUARD_PUSHED" [ "$status" -ne 0 ] + run git --git-dir="$REMOTE" notes --ref=refs/notes/dev-lead-pushed list "$owner_sha" + [ "$status" -ne 0 ] run _commit_is_dev_lead "$owner_sha" [ "$status" -ne 0 ] - [ "$(git --git-dir="$REMOTE" log -1 --format=%B "$owner_sha" | grep -c '^Dev-Lead-Stamp: ')" -eq 0 ] } @test "#2313: an owner commit with dev-lead's email that conflicts stops the push" { @@ -237,24 +237,22 @@ _earlier_run_push() { # This run amends the earlier run's commit: a legitimate self-rewrite. echo v2 > work; git add work; git commit -q --amend -m "dev-lead: work v2" + local amended; amended=$(git rev-parse HEAD) run push_no_clobber origin main [ "$status" -eq 0 ] - [ "$(git --git-dir="$REMOTE" show main:work)" = v2 ] - [ "$(git --git-dir="$REMOTE" log -1 --format=%s main)" = "dev-lead: work v2" ] + [ "$(_remote_head)" = "$amended" ] echo "$output" | grep -q "${stamped:0:12}" } -@test "#2313: a commit this run pushed is its own even when stamping failed" { +@test "#2313: a commit this run pushed is its own even with no stamp" { cd "$WORK" source "$GUARD_LIB" - dev_lead_stamp_unpublished() { return 1; } echo v1 > work; git add work; git commit -q -m "engine commit, unstamped" local pushed; pushed=$(git rev-parse HEAD) run _commit_is_dev_lead "$pushed" [ "$status" -ne 0 ] - push_no_clobber origin main 2>/dev/null - [ "$(_remote_head)" = "$pushed" ] + push_no_clobber origin main run _commit_is_dev_lead "$pushed" [ "$status" -eq 0 ] grep -qxF "$pushed" <<<"$_PUSH_GUARD_PUSHED" @@ -299,71 +297,82 @@ _later_run_clone() { printf '%s\n' "$dir" } -@test "#2313: an engine-made (unstamped) commit is stamped on push and is dev-lead's own on a later run" { +@test "#2313: an engine-made commit an earlier run pushed is dev-lead's own via the ledger; its amend force-pushes" { cd "$WORK" source "$GUARD_LIB" - # The engine commits with its own `git commit`: no trailer, no harness involved. + # The engine commits with its own `git commit`: no stamp. echo v1 > work; git add work; git commit -q -m "fix: engine's own commit" - local engine_sha engine_tree - engine_sha=$(git rev-parse HEAD); engine_tree=$(git rev-parse HEAD^{tree}) + local engine_sha; engine_sha=$(git rev-parse HEAD) push_no_clobber origin main + # Pushed exactly as committed — never rewritten — and noted in the remote ledger. + [ "$(_remote_head)" = "$engine_sha" ] + git --git-dir="$REMOTE" notes --ref=refs/notes/dev-lead-pushed list "$engine_sha" - # What landed is the stamped copy: same tree, author and subject, new SHA. - local landed; landed=$(_remote_head) - [ "$landed" != "$engine_sha" ] - [ "$(git --git-dir="$REMOTE" rev-parse main^{tree})" = "$engine_tree" ] - [ "$(git --git-dir="$REMOTE" log -1 --format=%s main)" = "fix: engine's own commit" ] - git --git-dir="$REMOTE" log -1 --format=%B main | grep -q '^Dev-Lead-Stamp: ' - # The claim sweep can follow the engine's SHA to the one that landed (#2032). - grep -qx "${engine_sha}"$'\t'"${landed}" <<<"$_PUSH_GUARD_REWRITES" - - # A later run starts with an empty ledger and amends that commit. + # A later run: fresh clone, empty in-run ledger. local later; later=$(_later_run_clone) cd "$later" - _PUSH_GUARD_PUSHED="" _PUSH_GUARD_REWRITES="" + _PUSH_GUARD_PUSHED="" source "$GUARD_LIB" - [ -z "$_PUSH_GUARD_PUSHED" ] - run _commit_is_dev_lead "$landed" - [ "$status" -eq 0 ] echo v2 > work; git add work; git commit -q --amend -m "fix: engine's own commit v2" + local amended; amended=$(git rev-parse HEAD) run push_no_clobber origin main [ "$status" -eq 0 ] - # dev-lead's own rewrite went through: v2 is the head and v1 is gone. - [ "$(git --git-dir="$REMOTE" show main:work)" = v2 ] - run git --git-dir="$REMOTE" merge-base --is-ancestor "$landed" main + [ "$(_remote_head)" = "$amended" ] + echo "$output" | grep -q "${engine_sha:0:12}" + run grep -q 'STOPPED' "$GITHUB_STEP_SUMMARY" [ "$status" -ne 0 ] - grep -q 'STOPPED' "$GITHUB_STEP_SUMMARY" && return 1 - return 0 + # The amended copy is noted as well; the copy dev-lead never pushed is not. + git --git-dir="$REMOTE" notes --ref=refs/notes/dev-lead-pushed list "$amended" } -@test "#2313: stamping before a push leaves commits already on the remote and valid stamps alone" { +@test "#2313: the ledger never vouches for a copy — an owner amend of a noted commit is foreign" { cd "$WORK" source "$GUARD_LIB" - # The owner's commit is on the remote and in HEAD: published, never restamped. - _owner_push owner.txt "owner work" - git pull -q origin main - local owner_sha; owner_sha=$(git rev-parse HEAD) - # A harness commit that is already stamped, then an engine commit on top. - echo h > harness; git add harness - git commit -q -m "fix(reviews): harness commit" -m "$(dev_lead_stamp_trailer)" - local harness_sha; harness_sha=$(git rev-parse HEAD) - echo e > engine; git add engine; git commit -q -m "engine commit" -m "Test-Change-Justification: review asked for it" - + echo v1 > work; git add work; git commit -q -m "fix: engine's own commit" push_no_clobber origin main - # The owner's commit and the stamped harness commit keep their SHAs. - git --git-dir="$REMOTE" merge-base --is-ancestor "$harness_sha" main - [ "$(git --git-dir="$REMOTE" rev-parse main~1)" = "$harness_sha" ] - [ "$(git --git-dir="$REMOTE" rev-parse main~2)" = "$owner_sha" ] - [ "$(git --git-dir="$REMOTE" log -1 --format=%B main~2 | grep -c '^Dev-Lead-Stamp: ')" -eq 0 ] - # Only the engine commit was rewritten; its other trailer is still a trailer. - [ "$(grep -c . <<<"$_PUSH_GUARD_REWRITES")" -eq 1 ] - [ "$(git --git-dir="$REMOTE" log -1 --format='%(trailers:key=Test-Change-Justification,valueonly)' main)" = "review asked for it" ] - run _commit_is_dev_lead "$(_remote_head)" + # The owner amends dev-lead's pushed commit with the same identity. + local other="$BATS_TEST_TMPDIR/owner-amend-noted" + git clone -q "$REMOTE" "$other" + ( + cd "$other"; _dev_identity + echo "owner fix" > work; git add work; git commit -q --amend --no-edit + git push -q --force origin main + ) + local owner_sha; owner_sha=$(_remote_head) + + local later; later=$(_later_run_clone) + cd "$later" + _PUSH_GUARD_PUSHED="" + source "$GUARD_LIB" + git reset -q --hard HEAD~1 + echo devfix > devfix; git add devfix; git commit -q -m "dev-lead fix" + run push_no_clobber origin main [ "$status" -eq 0 ] - # And the working tree is untouched by the restamp. - [ -z "$(git status --porcelain)" ] - [ "$(git rev-parse HEAD)" = "$(_remote_head)" ] + git --git-dir="$REMOTE" merge-base --is-ancestor "$owner_sha" main + [ "$(git --git-dir="$REMOTE" show main:work)" = "owner fix" ] +} + +@test "#2313: publishing to the ledger keeps notes another run wrote" { + cd "$WORK" + source "$GUARD_LIB" + echo one > one; git add one; git commit -q -m one + local first; first=$(git rev-parse HEAD) + push_no_clobber origin main + + # Another run, whose clone never fetched the ledger, pushes a second commit. + local later; later=$(_later_run_clone) + cd "$later" + _PUSH_GUARD_PUSHED="" + source "$GUARD_LIB" + run git notes --ref=refs/notes/dev-lead-pushed list + [ -z "$output" ] + echo two > two; git add two; git commit -q -m two + local second; second=$(git rev-parse HEAD) + push_no_clobber origin main + + git --git-dir="$REMOTE" notes --ref=refs/notes/dev-lead-pushed list "$first" + git --git-dir="$REMOTE" notes --ref=refs/notes/dev-lead-pushed list "$second" } @test "#2313: _push_guard_ownership_verdict — pushed SHA, matching stamp, and nothing else" { @@ -376,6 +385,11 @@ _later_run_clone() { run _push_guard_ownership_verdict aaaa "$tree" "$p1" "msg" $'bbbb\naaaa' [ "$status" -eq 0 ]; [ "$output" = own-pushed ] + run _push_guard_ownership_verdict aaaa "$tree" "$p1" "msg" "" true + [ "$status" -eq 0 ]; [ "$output" = own-ledger ] + run _push_guard_ownership_verdict aaaa "$tree" "$p1" "msg" "" false + [ "$status" -eq 1 ]; [ "$output" = foreign ] + run _push_guard_ownership_verdict aaaa "$tree" "$p1" $'msg\n\nDev-Lead-Stamp: '"$stamp" "" [ "$status" -eq 0 ]; [ "$output" = own-stamped ] @@ -415,7 +429,7 @@ _later_run_clone() { @test "no divergence: plain fast-forward push still works" { cd "$WORK" source "$GUARD_LIB" - echo ff > ff; git add ff; git commit -q -m "ff commit" -m "$(dev_lead_stamp_trailer)" + echo ff > ff; git add ff; git commit -q -m "ff commit" local head; head=$(git rev-parse HEAD) run push_no_clobber origin main @@ -428,16 +442,6 @@ _later_run_clone() { # the guard rebases them onto a steering commit, those SHAs exist nowhere on the # remote, so the claim sweep needs the map to keep the claims that did land. -# _follow_rewrites — follow _PUSH_GUARD_REWRITES rows to the last SHA, the -# way cl_map_sha does. -_follow_rewrites() { - local cur="$1" next - while next=$(awk -F'\t' -v s="$cur" '$1 == s { print $2; exit }' <<<"$_PUSH_GUARD_REWRITES") && [ -n "$next" ]; do - cur="$next" - done - printf '%s\n' "$cur" -} - @test "#2032: a clean rebase onto steering records each dev-lead commit's rebased SHA" { cd "$WORK" source "$GUARD_LIB" @@ -452,17 +456,15 @@ _follow_rewrites() { new2=$(git rev-parse HEAD); new1=$(git rev-parse HEAD~1) [ "$new2" != "$old2" ] [ "$(_remote_head)" = "$new2" ] - # The rebase and then the stamp (#2313) each rewrote the commits; the claim - # sweep follows the chain (cl_map_sha), so each old SHA reaches what landed. - [ "$(_follow_rewrites "$old1")" = "$new1" ] - [ "$(_follow_rewrites "$old2")" = "$new2" ] + grep -qx "${old1}"$'\t'"${new1}" <<<"$_PUSH_GUARD_REWRITES" + grep -qx "${old2}"$'\t'"${new2}" <<<"$_PUSH_GUARD_REWRITES" + [ "$(grep -c . <<<"$_PUSH_GUARD_REWRITES")" -eq 2 ] } @test "#2032: no rebase (plain fast-forward) records no rewrites" { cd "$WORK" source "$GUARD_LIB" - # A harness commit, stamped when made, so the push rewrites nothing. - echo ff > ff; git add ff; git commit -q -m "ff commit" -m "$(dev_lead_stamp_trailer)" + echo ff > ff; git add ff; git commit -q -m "ff commit" push_no_clobber origin main [ -z "$_PUSH_GUARD_REWRITES" ] } diff --git a/tests/dev-lead/unit/test_push_no_clobber.bats b/tests/dev-lead/unit/test_push_no_clobber.bats index 6237f3b56..8bc93f74d 100644 --- a/tests/dev-lead/unit/test_push_no_clobber.bats +++ b/tests/dev-lead/unit/test_push_no_clobber.bats @@ -60,14 +60,11 @@ _remote_head() { git --git-dir="$REMOTE" rev-parse main; } cd "$WORK" source "$GUARD_LIB" echo B >> f; git add f; git commit -q -m B - local local_tree; local_tree=$(git rev-parse HEAD^{tree}) + local local_head; local_head=$(git rev-parse HEAD) run push_no_clobber origin main [ "$status" -eq 0 ] - # The push stamps the commit first (#2313): the remote holds HEAD as it is - # after the push, with the tree that was committed. - [ "$(_remote_head)" = "$(git rev-parse HEAD)" ] - [ "$(git --git-dir="$REMOTE" rev-parse main^{tree})" = "$local_tree" ] + [ "$(_remote_head)" = "$local_head" ] } @test "push_no_clobber: rewritten branch pushes with lease when remote is unchanged" { @@ -75,12 +72,11 @@ _remote_head() { git --git-dir="$REMOTE" rev-parse main; } source "$GUARD_LIB" # Rewrite history (rebase-like) so the branch diverges from origin/main. echo A2 > f; git add f; git commit -q --amend -m A2 - local local_tree; local_tree=$(git rev-parse HEAD^{tree}) + local local_head; local_head=$(git rev-parse HEAD) run push_no_clobber origin main [ "$status" -eq 0 ] - [ "$(_remote_head)" = "$(git rev-parse HEAD)" ] - [ "$(git --git-dir="$REMOTE" rev-parse main^{tree})" = "$local_tree" ] + [ "$(_remote_head)" = "$local_head" ] } @test "push_no_clobber: rewritten branch ABORTS when the remote moved under us — unseen commit survives" {