From d9378b5d8dbc2e1197061a36033e69ddfb037557 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:08:25 -0500 Subject: [PATCH 1/3] fix(source-control): excuse empty base commits in landed-work's patch-id count An empty squash merge on main (654f1964fb, #13641) has no patch, so the base-side count check failed by one on every worktree branched before it. Each worktree then deleted the memoized base set, recomputed the full base patch log and fell through to the merged-PR read. The 2026-11-06 reap run took 56 min with a 19.7 GB memory peak and logged 165 gh-pr-list failures. Commits patch-id did not hash are now excused only when git diff-tree shows their diff is empty. The branch-side check is unchanged. A failed merged-PR read now carries gh's first error line in its reason. Co-Authored-By: Claude Opus 5.5 --- .../fix-landed-work-empty-base-commit.md | 7 +++ plugins/source-control/scripts/landed-work.sh | 45 ++++++++++++++----- .../scripts/landed-work.test.sh | 12 +++++ 3 files changed, 54 insertions(+), 10 deletions(-) create mode 100644 .changes/source-control/fix-landed-work-empty-base-commit.md diff --git a/.changes/source-control/fix-landed-work-empty-base-commit.md b/.changes/source-control/fix-landed-work-empty-base-commit.md new file mode 100644 index 00000000000..b3880cbc841 --- /dev/null +++ b/.changes/source-control/fix-landed-work-empty-base-commit.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- The landed-work engine behind `/source-control:worktree cleanup` and the daily worktree reap no longer discards its base patch-id set when the base holds an empty commit, such as an empty squash merge. Such a commit has no patch to hash; it is now excused once `git diff-tree` shows its diff is empty, so every worktree stops rebuilding the base set and falling through to the merged-PR read. A failed merged-PR read now names gh's own error in its reason. diff --git a/plugins/source-control/scripts/landed-work.sh b/plugins/source-control/scripts/landed-work.sh index 9bafa5457d9..5fd8109b95c 100755 --- a/plugins/source-control/scripts/landed-work.sh +++ b/plugins/source-control/scripts/landed-work.sh @@ -364,15 +364,33 @@ resolve_base() { # equals the squash commit's, and still matches after the base advances. Both # verified in a fixture rather than assumed. patch_ids() { - local p="$1" range="$2" out="$3" patches="$WORKDIR/patches.$$" rc=0 + local p="$1" range="$2" out="$3" commits_out="${4:-}" patches="$WORKDIR/patches.$$" pairs="$WORKDIR/pairs.$$" rc=0 : >"$out" + [[ -n "$commits_out" ]] && : >"$commits_out" git -C "$p" log -p --no-merges --no-color "$range" >"$patches" 2>/dev/null || rc=1 # An empty stream is a legitimate empty range, so only a non-empty one is fed # to patch-id; $out stays truncated either way. if ((rc == 0)) && [[ -s "$patches" ]]; then - git patch-id --verbatim <"$patches" | cut -d' ' -f1 | sort >"$out" || rc=1 + git patch-id --verbatim <"$patches" >"$pairs" || rc=1 + cut -d' ' -f1 "$pairs" | sort >"$out" + [[ -n "$commits_out" ]] && cut -d' ' -f2 "$pairs" | sort >"$commits_out" fi - rm -f "$patches" + rm -f "$patches" "$pairs" + return "$rc" +} + +# only_empty_commits_unhashed : succeed when every +# non-merge commit in that patch-id did not hash has an empty diff. Such a +# commit has no patch to hash, so it cannot be a rendering failure. +only_empty_commits_unhashed() { + local p="$1" range="$2" hashed="$3" all="$WORKDIR/range-commits.$$" c rc=0 + git -C "$p" rev-list --no-merges "$range" 2>/dev/null | sort >"$all" || rc=1 + if ((rc == 0)); then + while read -r c; do + git -C "$p" diff-tree --quiet --root --no-commit-id -r "$c" 2>/dev/null || { rc=1; break; } + done < <(comm -23 "$all" "$hashed") + fi + rm -f "$all" return "$rc" } @@ -435,8 +453,8 @@ classify_landed() { L_REASON="base-range-unreadable" return 0 fi - if ! patch_ids "$p" "$mb..$base_sha" "$base_ids"; then - rm -f "$base_ids" + if ! patch_ids "$p" "$mb..$base_sha" "$base_ids" "$base_ids.commits"; then + rm -f "$base_ids" "$base_ids.commits" L_REASON="base-patchid-failed" return 0 fi @@ -445,14 +463,17 @@ classify_landed() { # between `yes` and `no` today — it is here so the two sides cannot silently # diverge if either is ever refactored, and so a base range that failed to # render is named rather than quietly narrowing the id set it is compared - # against. + # against. An empty commit on the base (an empty squash merge) has no patch, + # so it is excused only after diff-tree shows its diff really is empty. local base_count base_count=$(wc -l <"$base_ids" | tr -d '[:space:]') - if [[ "$base_count" -ne "$base_expected" ]]; then - rm -f "$base_ids" + if [[ "$base_count" -ne "$base_expected" ]] && + ! only_empty_commits_unhashed "$p" "$mb..$base_sha" "$base_ids.commits"; then + rm -f "$base_ids" "$base_ids.commits" L_REASON="base-patchid-incomplete-$base_count-of-$base_expected-commits" return 0 fi + rm -f "$base_ids.commits" fi # Branch side. The COMPLETENESS check runs before any verdict, because both @@ -615,8 +636,12 @@ merged_pr_proof() { fi if ! (cd "$p" && gh pr list --state merged --head "$branch" --limit 20 \ --json headRefOid,baseRefName,commits \ - --jq '.[] | "pr " + .baseRefName, "head " + .headRefOid, (.commits[] | "commit " + .oid)') >"$out" 2>/dev/null; then - L_REASON="${L_REASON:+$L_REASON; }merged-pr-read-failed:gh-pr-list" + --jq '.[] | "pr " + .baseRefName, "head " + .headRefOid, (.commits[] | "commit " + .oid)') >"$out" 2>"$out.err"; then + # Name gh's own error so a failed run states its cause; tabs and semicolons + # would break the record's columns and reason list. + local err + err=$(head -n 1 "$out.err" | tr '\t;' ' ' | cut -c1-120) + L_REASON="${L_REASON:+$L_REASON; }merged-pr-read-failed:gh-pr-list${err:+ ($err)}" return 1 fi local in_base=false diff --git a/plugins/source-control/scripts/landed-work.test.sh b/plugins/source-control/scripts/landed-work.test.sh index a13ab8bde97..790737eebc1 100755 --- a/plugins/source-control/scripts/landed-work.test.sh +++ b/plugins/source-control/scripts/landed-work.test.sh @@ -141,6 +141,18 @@ assert_eq "squash: still landed after the base advances" "yes" "$(col "$R" $C_LA assert_eq "squash: still by range patch-id after the base advances" \ "range-patchid" "$(col "$R" $C_METHOD)" +# An empty commit on the base (an empty squash merge) has no patch to hash. It is +# excused from the base-side count, so the base set stays usable; a commit whose +# diff fails to render still is not (base-patchid-incomplete). +git -C "$W" commit -q --allow-empty -m "empty squash on the base" >/dev/null 2>&1 +git -C "$W" push -q origin main >/dev/null 2>&1 + +OUT="$(bash "$ENGINE" --repo-dir "$W" --no-peers)" +R="$(row "$OUT" "wt-squash")" +assert_eq "squash: still landed past an empty base commit" "yes" "$(col "$R" $C_LANDED)" +assert_not_contains "and the base set is not reported incomplete" \ + "$(col "$R" $C_REASON)" "base-patchid-incomplete" + # -------------------------------------------------------------------------- # Negative control — a genuinely unmerged branch is NOT landed # -------------------------------------------------------------------------- From 0298b3d2a7595954fb5d3d7b26cb4cbdb08fa2d1 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:09:18 -0500 Subject: [PATCH 2/3] chore(source-control): name the landed-work fragment by branch slug and suffix Co-Authored-By: Claude Opus 5.5 --- ...se-commit.md => fix-landed-work-empty-base-commit-3c9e71a4.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changes/source-control/{fix-landed-work-empty-base-commit.md => fix-landed-work-empty-base-commit-3c9e71a4.md} (100%) diff --git a/.changes/source-control/fix-landed-work-empty-base-commit.md b/.changes/source-control/fix-landed-work-empty-base-commit-3c9e71a4.md similarity index 100% rename from .changes/source-control/fix-landed-work-empty-base-commit.md rename to .changes/source-control/fix-landed-work-empty-base-commit-3c9e71a4.md From fd3628564ab47f2e6df7091899cd9962603be453 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:24:01 -0500 Subject: [PATCH 3/3] test(source-control): cover gh's error text in the merged-PR read reason The stub's error now holds a tab, a semicolon and a second line; the tests check the first line is kept, blanked of both, and adds no column. Co-Authored-By: Claude Opus 5.5 --- plugins/source-control/scripts/landed-work.test.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/plugins/source-control/scripts/landed-work.test.sh b/plugins/source-control/scripts/landed-work.test.sh index 790737eebc1..c230e059d6f 100755 --- a/plugins/source-control/scripts/landed-work.test.sh +++ b/plugins/source-control/scripts/landed-work.test.sh @@ -633,7 +633,7 @@ PR_BIN="$TEST_TMPDIR/pr-bin" mkdir -p "$PR_BIN" cat >"$PR_BIN/gh" <<'STUB' #!/usr/bin/env bash -[[ -n "${GH_STUB_FAIL:-}" ]] && { echo "gh stub: forced failure" >&2; exit 1; } +[[ -n "${GH_STUB_FAIL:-}" ]] && { printf 'gh stub:\tforced; failure\nsecond line\n' >&2; exit 1; } cat "${GH_STUB_OUT:-/dev/null}" STUB chmod +x "$PR_BIN/gh" @@ -688,6 +688,12 @@ assert_contains "pr proof: the reason says why" "$(col "$R" $C_REASON)" "local-c R="$(row "$(run_pr "$TEST_TMPDIR/pr-commits-only" 1)" "wt-pr")" assert_eq "pr proof: a failed read keeps the prior verdict" "no" "$(col "$R" $C_LANDED)" assert_contains "pr proof: the failed read is named" "$(col "$R" $C_REASON)" "merged-pr-read-failed:gh-pr-list" +# gh's first error line rides along, its tab and semicolon blanked so the row's +# columns and the reason list keep their shape; later lines are dropped. +assert_contains "pr proof: gh's own error is named" "$(col "$R" $C_REASON)" "gh-pr-list (gh stub: forced failure)" +assert_not_contains "pr proof: only gh's first error line is kept" "$(col "$R" $C_REASON)" "second line" +assert_eq "pr proof: the error text adds no column" \ + "$(head -n 1 <<<"$OUT" | awk -F'\t' '{print NF}')" "$(awk -F'\t' '{print NF}' <<<"$R")" OUT="$(bash "$ENGINE" --repo-dir "$W" --no-peers)" # Bare-name invocation from the scripts directory still loads the library.