Skip to content

Commit b68e3ed

Browse files
committed
fix: move the merge hooks off GraphQL onto REST
Both PostToolUse hooks that fire after `gh pr merge` read GitHub through `gh pr list` / `gh pr view`, and every `gh pr *` porcelain command goes through the GraphQL API. That budget is scored in points rather than requests, and agent sessions here exhaust it routinely, at which point both hooks fail silently: the cleanup hook stops seeing squash-merged PRs so every merged worktree leaks, and the release hook never fires its global-CLI reminder. Both were failing this way when measured. The REST pulls endpoint answers the same two questions on a separate budget, so move them there. Also stop capturing JSON into a shell variable and parsing it. A `gh` earlier on PATH may be a wrapper that prints a banner to stdout before exec'ing the real binary, and that text lands inside the capture and breaks the parse. Asking for one scalar and taking the last line is immune to it. The squash-merge path had no test at all, because the harness makes gh a no-op, so it grew a stub that answers the REST call.
1 parent c0917df commit b68e3ed

4 files changed

Lines changed: 174 additions & 18 deletions

File tree

.claude/hooks/cleanup-merged-worktree.sh

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,25 @@ is_merged() {
6161
if git merge-base --is-ancestor "refs/heads/$br" "$base" 2>/dev/null; then return 0; fi
6262
# A merged GitHub PR for this head branch (squash merges, which are NOT an
6363
# ancestor of base). Network; skipped when gh is absent or unauthenticated.
64+
#
65+
# REST, not `gh pr list`. Every `gh pr *` porcelain command goes through the
66+
# GraphQL API, whose budget is scored in POINTS and which agent sessions here
67+
# routinely exhaust; when it is spent this lookup returns nothing, the branch
68+
# reads as unmerged, and the worktree leaks, which is the exact failure this
69+
# hook exists to prevent. The REST pulls endpoint is a separate budget.
70+
# `{owner}`/`{repo}` expand from the current repo, and resolve to nothing when
71+
# there is no remote (the test harness), so the call fails closed to the
72+
# ancestor check above rather than erroring.
73+
#
74+
# Read the number through `grep -E '^[0-9]+$'` rather than trusting the whole
75+
# capture: a `gh` earlier on PATH may be a wrapper that prints a banner to
76+
# STDOUT before exec'ing the real binary, which would otherwise land inside
77+
# this variable.
6478
if command -v gh >/dev/null 2>&1; then
6579
local n
66-
n=$(gh pr list --state merged --head "$br" --json number --jq '.[0].number' 2>/dev/null | grep -E '^[0-9]+$' || true)
80+
n=$(gh api "repos/{owner}/{repo}/pulls?state=closed&head={owner}:$br&per_page=100" \
81+
--jq '[.[] | select(.merged_at != null)] | .[0].number // empty' 2>/dev/null \
82+
| grep -E '^[0-9]+$' || true)
6783
[ -n "$n" ] && return 0
6884
fi
6985
return 1

.claude/hooks/release-global-update.sh

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,16 +39,23 @@ command -v gh >/dev/null 2>&1 || exit 0
3939
num=$(printf '%s' "$cmd" | grep -oE 'gh pr merge[[:space:]]+#?[0-9]+' | grep -oE '[0-9]+' | head -1)
4040
if [ -z "$num" ]; then exit 0; fi
4141

42-
info=$(gh pr view "$num" --json headRefName,title 2>/dev/null || true)
43-
if [ -z "$info" ]; then exit 0; fi
44-
head=$(printf '%s' "$info" | jq -r '.headRefName // ""' 2>/dev/null || true)
45-
title=$(printf '%s' "$info" | jq -r '.title // ""' 2>/dev/null || true)
42+
# REST, not `gh pr view`. Every `gh pr *` porcelain command goes through the
43+
# GraphQL API, whose budget is scored in POINTS and which agent sessions here
44+
# routinely exhaust; when it is spent this fetch returns nothing and the reminder
45+
# silently never fires. The REST pulls endpoint is a separate budget.
46+
#
47+
# Ask for the ONE field this hook reads and take the LAST line, rather than
48+
# capturing JSON and parsing it here. A `gh` earlier on PATH may be a wrapper
49+
# that prints a banner to STDOUT before exec'ing the real binary; prepended to
50+
# JSON that breaks the parse outright, while a scalar survives `tail -n1`.
51+
title=$(gh api "repos/{owner}/{repo}/pulls/$num" --jq '.title // empty' 2>/dev/null | tail -n1)
52+
if [ -z "$title" ]; then exit 0; fi
4653

4754
# A real release PR carries the canonical "chore: release <pkgs>" title (the
4855
# release process always titles it exactly that). Match the TITLE, not the
4956
# branch prefix: a `chore/release-*` branch that is NOT a package release (a hook
5057
# tweak, a doc change) would otherwise fire a false reminder with nothing to
51-
# publish. `head` is unused now but kept in the fetch for future signals.
58+
# publish.
5259
if ! printf '%s' "$title" | grep -qiE '^chore: release '; then exit 0; fi
5360

5461
read -r -d '' MSG <<'EOF' || true

test/hooks/cleanup-merged-worktree.test.mjs

Lines changed: 87 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@
1313
import { test } from 'node:test';
1414
import assert from 'node:assert/strict';
1515
import { execFileSync, spawnSync } from 'node:child_process';
16-
import { mkdtempSync, writeFileSync, existsSync } from 'node:fs';
16+
import { mkdtempSync, writeFileSync, existsSync, chmodSync } from 'node:fs';
1717
import { tmpdir } from 'node:os';
18-
import { join, dirname, resolve } from 'node:path';
18+
import { join, dirname, resolve, delimiter } from 'node:path';
1919
import { fileURLToPath } from 'node:url';
2020

2121
const HOOK = resolve(
@@ -46,14 +46,50 @@ function addWorktree({ git, dir, main }, name, { merged, dirty } = {}) {
4646
return path;
4747
}
4848

49+
/**
50+
* A fake `gh` on PATH emulating the REST call the hook makes for squash merges,
51+
* `gh api "repos/{owner}/{repo}/pulls?...&head={owner}:<branch>&..." --jq ...`.
52+
* It prints a PR number when `<branch>` is in `mergedBranches`, and nothing
53+
* otherwise. `bannerLine`, when set, is printed to STDOUT first, reproducing a
54+
* PATH wrapper (a mise shim does this locally) that would otherwise land inside
55+
* the hook's `$(gh ...)` capture.
56+
*/
57+
function fakeGhDir(mergedBranches, bannerLine = '') {
58+
const dir = mkdtempSync(join(tmpdir(), 'webjs-wtgh-'));
59+
const gh = join(dir, 'gh');
60+
writeFileSync(
61+
gh,
62+
[
63+
'#!/usr/bin/env bash',
64+
bannerLine ? `echo ${JSON.stringify(bannerLine)}` : '',
65+
'for a in "$@"; do',
66+
' case "$a" in',
67+
' *head=*)',
68+
' br="${a##*:}"; br="${br%%&*}"',
69+
` for m in ${mergedBranches.map((b) => `'${b}'`).join(' ')}; do`,
70+
' if [ "$br" = "$m" ]; then echo 4242; exit 0; fi',
71+
' done ;;',
72+
' esac',
73+
'done',
74+
'',
75+
].join('\n'),
76+
);
77+
chmodSync(gh, 0o755);
78+
return dir;
79+
}
80+
4981
/** Run the hook with a given command, from a given cwd. Returns {code, out}. */
50-
function runHook(command, cwd) {
82+
function runHook(command, cwd, { mergedBranches = null, bannerLine = '' } = {}) {
83+
// Default: no stub, so the no-remote temp repo makes gh a harmless no-op
84+
// regardless of host auth, and only the ancestor-of-base signal fires.
85+
const ghDir = mergedBranches ? fakeGhDir(mergedBranches, bannerLine) : null;
86+
const env = { ...process.env, GH_NO_UPDATE_NOTIFIER: '1' };
87+
if (ghDir) env.PATH = `${ghDir}${delimiter}${process.env.PATH}`;
5188
const r = spawnSync('bash', [HOOK], {
5289
cwd,
5390
input: JSON.stringify({ tool_input: { command } }),
5491
encoding: 'utf8',
55-
// Force the no-remote temp repo to make gh a harmless no-op regardless of host auth.
56-
env: { ...process.env, GH_NO_UPDATE_NOTIFIER: '1' },
92+
env,
5793
});
5894
return { code: r.status, out: (r.stdout || '') + (r.stderr || '') };
5995
}
@@ -73,6 +109,52 @@ test('removes a merged + clean worktree, keeps dirty and unmerged ones', () => {
73109
assert.ok(existsSync(repo.main), 'primary checkout is never removed');
74110
});
75111

112+
// A squash merge leaves the branch NOT an ancestor of base, so the git signal
113+
// cannot see it and the REST lookup is the only thing that can. This is the path
114+
// that silently stopped working while it went through GraphQL: an exhausted
115+
// point budget returned nothing, every squash-merged branch read as unmerged,
116+
// and its worktree leaked, which is the failure the hook exists to prevent.
117+
test('removes a squash-merged worktree that git alone cannot see as merged', () => {
118+
const repo = makeRepo();
119+
const squashed = addWorktree(repo, 'feat-squashed', {});
120+
const unmerged = addWorktree(repo, 'feat-really-unmerged', {});
121+
122+
// Neither branch is an ancestor of main; only `feat-squashed` has a merged PR.
123+
const { code } = runHook('gh pr merge 1 --squash', repo.main, {
124+
mergedBranches: ['feat-squashed'],
125+
});
126+
127+
assert.equal(code, 0);
128+
assert.ok(!existsSync(squashed), 'a squash-merged branch is detected over REST and removed');
129+
assert.ok(existsSync(unmerged), 'a branch with no merged PR is still kept');
130+
});
131+
132+
test('squash-merge detection survives a `gh` wrapper that banners to stdout', () => {
133+
const repo = makeRepo();
134+
const squashed = addWorktree(repo, 'feat-squashed', {});
135+
136+
const { code } = runHook('gh pr merge 1 --squash', repo.main, {
137+
mergedBranches: ['feat-squashed'],
138+
bannerLine: 'mise ~/.config/mise/config.toml tools: gh@2.97.0',
139+
});
140+
141+
assert.equal(code, 0);
142+
assert.ok(!existsSync(squashed), 'a stdout banner must not hide the PR number');
143+
});
144+
145+
test('a banner with no PR number does not make an unmerged branch look merged', () => {
146+
const repo = makeRepo();
147+
const unmerged = addWorktree(repo, 'feat-unmerged', {});
148+
149+
const { code } = runHook('gh pr merge 1 --squash', repo.main, {
150+
mergedBranches: [],
151+
bannerLine: 'mise ~/.config/mise/config.toml tools: gh@2.97.0',
152+
});
153+
154+
assert.equal(code, 0);
155+
assert.ok(existsSync(unmerged), 'banner text must never be read as a PR number');
156+
});
157+
76158
test('does nothing on a command that is not `gh pr merge`', () => {
77159
const repo = makeRepo();
78160
const clean = addWorktree(repo, 'feat-merged-clean', { merged: true });

test/hooks/release-global-update.test.mjs

Lines changed: 58 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,16 @@
55
// publish lands. A normal PR merge, a non-merge command, and the escape hatch
66
// produce no reminder. It never blocks the tool (always exits 0).
77
//
8-
// `gh pr view` is stubbed with a fake `gh` on PATH so the test is offline and
9-
// deterministic.
8+
// The hook reads the PR title over the REST pulls endpoint (`gh api`), NOT
9+
// `gh pr view`, because every `gh pr *` porcelain command spends the GraphQL
10+
// point budget that agent sessions here exhaust. A fake `gh` on PATH stubs that
11+
// call so the test is offline and deterministic.
12+
//
13+
// The fake also covers a trap the real environment has: a `gh` earlier on PATH
14+
// may be a WRAPPER that prints a banner to stdout before exec'ing the real
15+
// binary (a mise shim does exactly this locally). That banner lands inside any
16+
// `$(gh ...)` capture, so the hook asks for a single scalar and takes the last
17+
// line instead of capturing JSON and parsing it. `bannerLine` exercises that.
1018

1119
import { test } from 'node:test';
1220
import assert from 'node:assert/strict';
@@ -21,17 +29,40 @@ const HOOK = resolve(
2129
'../../.claude/hooks/release-global-update.sh',
2230
);
2331

24-
/** A fake `gh` on PATH whose `pr view` prints the given headRefName + title. */
25-
function fakeGhDir(headRefName, title) {
32+
/**
33+
* A fake `gh` on PATH emulating `gh api <endpoint> --jq <expr>`: it reads the
34+
* `--jq` expression and prints the matching scalar, the way the real command
35+
* does. `bannerLine`, when set, is printed to STDOUT first, reproducing a PATH
36+
* wrapper that announces itself before running.
37+
*/
38+
function fakeGhDir(headRefName, title, bannerLine = '') {
2639
const dir = mkdtempSync(join(tmpdir(), 'webjs-relhook-'));
2740
const gh = join(dir, 'gh');
28-
writeFileSync(gh, `#!/usr/bin/env bash\necho '${JSON.stringify({ headRefName, title })}'\n`);
41+
writeFileSync(
42+
gh,
43+
[
44+
'#!/usr/bin/env bash',
45+
bannerLine ? `echo ${JSON.stringify(bannerLine)}` : '',
46+
'expr=""',
47+
'prev=""',
48+
'for a in "$@"; do',
49+
' if [ "$prev" = "--jq" ]; then expr="$a"; fi',
50+
' prev="$a"',
51+
'done',
52+
'case "$expr" in',
53+
` *.title*) echo ${JSON.stringify(title)} ;;`,
54+
` *head.ref*) echo ${JSON.stringify(headRefName)} ;;`,
55+
' *) ;;',
56+
'esac',
57+
'',
58+
].join('\n'),
59+
);
2960
chmodSync(gh, 0o755);
3061
return dir;
3162
}
3263

33-
function runHook(command, { headRefName = '', title = '', env = {} } = {}) {
34-
const ghDir = fakeGhDir(headRefName, title);
64+
function runHook(command, { headRefName = '', title = '', bannerLine = '', env = {} } = {}) {
65+
const ghDir = fakeGhDir(headRefName, title, bannerLine);
3566
try {
3667
const r = spawnSync('bash', [HOOK], {
3768
input: JSON.stringify({ tool_input: { command } }),
@@ -81,6 +112,26 @@ test('does NOTHING for a chore/release-* branch that is not a package release (t
81112
assert.doesNotMatch(out, /webjsdev/, 'a chore/release-* branch with a non-release title must not fire');
82113
});
83114

115+
test('survives a `gh` wrapper that prints a banner to stdout before the payload', () => {
116+
const { code, out } = runHook('gh pr merge 839 --squash', {
117+
headRefName: 'chore/release-2026-07-08b',
118+
title: 'chore: release server 0.8.43',
119+
bannerLine: 'mise ~/.config/mise/config.toml tools: gh@2.97.0',
120+
});
121+
assert.equal(code, 0);
122+
assert.match(out, /npm update -g webjsdev/, 'a stdout banner must not swallow the title');
123+
});
124+
125+
test('a banner alone, with no title, does not fire the reminder', () => {
126+
const { code, out } = runHook('gh pr merge 839 --squash', {
127+
headRefName: '',
128+
title: '',
129+
bannerLine: 'mise ~/.config/mise/config.toml tools: gh@2.97.0',
130+
});
131+
assert.equal(code, 0);
132+
assert.doesNotMatch(out, /webjsdev/, 'the banner must never be mistaken for a release title');
133+
});
134+
84135
test('does NOTHING for a command that is not `gh pr merge`', () => {
85136
const { out } = runHook('git status', { headRefName: 'chore/release-x', title: 'chore: release x' });
86137
assert.doesNotMatch(out, /webjsdev/);

0 commit comments

Comments
 (0)