Skip to content

🏗️🔧：survive a failed call to the GitHub API #1816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DerekNonGeneric merged 2 commits into from
Aug 15, 2026
Merged

🏗️🔧：survive a failed call to the GitHub API #1816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4c65c3
🏗️🔧：survive a failed call to the GitHub API
DerekNonGeneric Aug 15, 2026
90a68f7
🏗️🔧：stop the queue waiting for itself
DerekNonGeneric Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion build/shared/landing.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export type CheckRun = {
name: string;
status: string;
conclusion: string | null;
detailsUrl?: string;
};

/** What GitHub reports about one commit status, which is the older kind. */
Expand All @@ -108,11 +109,25 @@ export type CommitStatus = { context: string; state: string };
* running counts against it: a label applied while a check was in flight says
* nothing about how that check turned out. Neutral and skipped do not count
* against it, since neither is a complaint.
* The queue is itself a check, so its own run is left out. Waiting for it
* would be waiting for a job that cannot finish until it stops waiting.
* @param {CheckRun[]} runs The check runs reported on the commit.
* @param {CommitStatus[]} statuses The commit statuses reported on it.
* @param {string} ownRunId The workflow run doing the asking, if it is one.
* @returns {string} The reason, or an empty string if there is none.
*/
export function checksVerdict(runs: CheckRun[], statuses: CommitStatus[]) {
export function checksVerdict(
all: CheckRun[],
statuses: CommitStatus[],
ownRunId = ''
) {
const runs =
ownRunId === ''
? all
: all.filter(
(run) =>
!(run.detailsUrl ?? '').includes(`/actions/runs/${ownRunId}/`)
);
const pending = [
...runs.filter((run) => run.status !== 'completed').map((run) => run.name),
...statuses
Expand Down
33 changes: 33 additions & 0 deletions build/shared/landing.test.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,36 @@ describe('checksVerdict', () => {
);
});
});

describe('checksVerdict and its own run', () => {
test('does not wait for the queue that is doing the asking', () => {
// The queue reports as a check itself, so counting it would mean waiting
// for a job that cannot finish until it stops waiting.
const own = {
name: 'Land',
status: 'in_progress',
conclusion: null,
detailsUrl: 'https://github.com/o/r/actions/runs/999/job/1',
};

deepStrictEqual(
checksVerdict(
[own, { name: 'Lint', status: 'completed', conclusion: 'success' }],
[],
'999'
),
''
);
});

test('still waits for a different run', () => {
const other = {
name: 'Lint',
status: 'in_progress',
conclusion: null,
detailsUrl: 'https://github.com/o/r/actions/runs/1000/job/1',
};

match(checksVerdict([other], [], '999'), /have not finished: Lint/);
});
});
232 changes: 141 additions & 91 deletions build/tasks/land-pull-request.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,30 +32,69 @@ type PullRequest = {
const [number, ...flags] = process.argv.slice(2);
const dryRun = flags.includes('--dry-run');

const gh = (...args: string[]) =>
execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim();
const git = (...args: string[]) =>
execFileSync('git', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim();
const run = (command: string, args: string[]) =>
execFileSync(command, args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim();

/**
* Runs `gh`, and tries a second time before giving up. A read that fails
* because GitHub returned a 502 or throttled the request is not a reason to
* refuse to land, and it happens often enough to have happened here: an
* earlier version crashed on one and worked when run again unchanged.
* @param {...string} args What to pass to `gh`.
* @returns {string} Its output.
*/
const gh = (...args: string[]) => {
try {
return run('gh', args);
} catch (first) {
// A merge is not safe to repeat blindly: if the first attempt reached
// GitHub, the second would report an already-merged pull request as a
// failure. Reads are.
if (args.includes('PUT')) throw first;

return run('gh', args);
}
};

const git = (...args: string[]) => run('git', args);

/**
* Works something out once, the first time it is wanted. Asked for at the top
* of the file instead, these would run before anything could catch them
* failing, and a broken `gh` would print a stack trace rather than a reason.
* @param {() => T} work How to find the answer.
* @returns {() => T} A function returning it, computed at most once.
*/
const once = <T,>(work: () => T) => {
let answer: T | undefined;

return () => {
answer ??= work();

return answer;
};
};

// Nothing here names a repository. A workflow sets GITHUB_REPOSITORY, and a
// terminal has a remote to read it from, so a copy of this file lands in
// another repository without being edited first.
const REPOSITORY =
process.env.GITHUB_REPOSITORY ??
git('remote', 'get-url', 'origin').match(
/github\.com[/:](?<repo>[^/]+\/[^/]+?)(?:\.git)?$/
)?.groups?.repo ??
'';

const DEFAULT_BRANCH = gh(
'api',
`repos/${REPOSITORY}`,
'--jq',
'.default_branch'
const repository = once(
() =>
process.env.GITHUB_REPOSITORY ??
git('remote', 'get-url', 'origin').match(
/github\.com[/:](?<repo>[^/]+\/[^/]+?)(?:\.git)?$/
)?.groups?.repo ??
''
);

const defaultBranch = once(() =>
gh('api', `repos/${repository()}`, '--jq', '.default_branch')
);

/** Who is asking for this to land: the labeller, or whoever is at the keyboard. */
const ACTOR = process.env.LAND_ACTOR ?? gh('api', 'user', '--jq', '.login');
const actor = once(
() => process.env.LAND_ACTOR ?? gh('api', 'user', '--jq', '.login')
);

/**
* Reads the pull request, waiting for GitHub to work out whether it merges.
Expand All @@ -69,7 +108,7 @@ const readPull = async (pull: string): Promise<PullRequest> => {
JSON.parse(
gh(
'api',
`repos/${REPOSITORY}/pulls/${pull}`,
`repos/${repository()}/pulls/${pull}`,
'--jq',
'{title, base: .base.ref, head: .head.sha, state, draft, mergeableState: .mergeable_state}'
)
Expand Down Expand Up @@ -105,19 +144,20 @@ const checksRefuse = (sha: string) =>
JSON.parse(
gh(
'api',
`repos/${REPOSITORY}/commits/${sha}/check-runs`,
`repos/${repository()}/commits/${sha}/check-runs`,
'--jq',
'[.check_runs[] | {name, status, conclusion}]'
'[.check_runs[] | {name, status, conclusion, detailsUrl: .details_url}]'
)
),
JSON.parse(
gh(
'api',
`repos/${REPOSITORY}/commits/${sha}/status`,
`repos/${repository()}/commits/${sha}/status`,
'--jq',
'[.statuses[] | {context, state}]'
)
)
),
process.env.GITHUB_RUN_ID ?? ''
);

/**
Expand All @@ -129,14 +169,14 @@ const checksRefuse = (sha: string) =>
const actorRefuses = () => {
const permission = gh(
'api',
`repos/${REPOSITORY}/collaborators/${ACTOR}/permission`,
`repos/${repository()}/collaborators/${actor()}/permission`,
'--jq',
'.permission'
);

return ['admin', 'maintain', 'write'].includes(permission)
? ''
: `${ACTOR} has ${permission} access, which does not carry the right to push`;
: `${actor()} has ${permission} access, which does not carry the right to push`;
};

/**
Expand All @@ -150,98 +190,108 @@ const refuse = (pull: PullRequest) => {
}

if (pull.mergeableState === 'dirty') {
return `it conflicts with ${DEFAULT_BRANCH}; rebase the branch onto it and push first`;
return `it conflicts with ${defaultBranch()}; rebase the branch onto it and push first`;
}

if (pull.base !== DEFAULT_BRANCH) {
if (pull.base !== defaultBranch()) {
return `it targets ${pull.base}; retarget it, or land its base first`;
}

if (pull.mergeableState === 'unknown') {
return 'GitHub has not worked out whether it merges yet; try again shortly';
}

const actor = actorRefuses();
const unentitled = actorRefuses();

if (actor !== '') return actor;
if (unentitled !== '') return unentitled;

const checks = checksRefuse(pull.head);

return checks === '' ? '' : `${checks}`;
};

if (number === undefined || !/^\d+$/.test(number)) {
console.error('Usage: nps "land <number> [--dry-run]"');
process.exitCode = 1;
} else {
const pull = await readPull(number);
const reason = refuse(pull);

if (reason !== '') {
console.error(`#${number} cannot be landed: ${reason}.`);
// Anything unexpected -- a network failure that outlived its retry, a
// command that is not installed -- should say so in a line rather than
// print a stack trace at whoever applied a label.
try {
if (number === undefined || !/^\d+$/.test(number)) {
console.error('Usage: nps "land <number> [--dry-run]"');
process.exitCode = 1;
} else {
// Oldest first, so the landed message reads in the order the work was
// done rather than the order git lists it.
const shas = git(
'rev-list',
'--reverse',
'--no-merges',
`origin/${DEFAULT_BRANCH}..${pull.head}`
)
.split('\n')
.filter(Boolean);
const parts = shas.map((sha) =>
partsOfMessage(git('log', '-1', '--format=%B', sha))
);
const message = composeLandingMessage(
parts,
`https://github.com/${REPOSITORY}/pull/${number}`
);
const problems = validateCommitMessage(`${pull.title}\n\n${message}`);
const rule = '='.repeat(72);
const pull = await readPull(number);
const reason = refuse(pull);

console.log(`${rule}\n${pull.title}\n\n${message}\n${rule}`);
console.log(
`#${number}: ${shas.length} commit${shas.length === 1 ? '' : 's'}, ${pull.mergeableState}`
);

if (shas.length === 0) {
console.error(`\n#${number} has no commits over ${DEFAULT_BRANCH}.`);
process.exitCode = 1;
} else if (problems.length > 0) {
console.error(
'\nThe message this would land does not pass its own rules:'
);
for (const problem of problems) console.error(` ${problem}`);
if (reason !== '') {
console.error(`#${number} cannot be landed: ${reason}.`);
process.exitCode = 1;
} else if (dryRun) {
console.log('\nDry run; nothing landed.');
} else {
const merged = JSON.parse(
gh(
'api',
'-X',
'PUT',
`repos/${REPOSITORY}/pulls/${number}/merge`,
'-f',
'merge_method=squash',
'-f',
`commit_title=${pull.title}`,
'-f',
`commit_message=${message}`,
'-f',
`sha=${pull.head}`,
'--jq',
'{merged, sha}'
)
// Oldest first, so the landed message reads in the order the work was
// done rather than the order git lists it.
const shas = git(
'rev-list',
'--reverse',
'--no-merges',
`origin/${defaultBranch()}..${pull.head}`
)
.split('\n')
.filter(Boolean);
const parts = shas.map((sha) =>
partsOfMessage(git('log', '-1', '--format=%B', sha))
);
const message = composeLandingMessage(
parts,
`https://github.com/${repository()}/pull/${number}`
);
const problems = validateCommitMessage(`${pull.title}\n\n${message}`);
const rule = '='.repeat(72);

console.log(`${rule}\n${pull.title}\n\n${message}\n${rule}`);
console.log(
`#${number}: ${shas.length} commit${shas.length === 1 ? '' : 's'}, ${pull.mergeableState}`
);

if (merged.merged === true) console.log(`\nLanded as ${merged.sha}`);
else {
console.error(`\n#${number} did not merge.`);
if (shas.length === 0) {
console.error(`\n#${number} has no commits over ${defaultBranch()}.`);
process.exitCode = 1;
} else if (problems.length > 0) {
console.error(
'\nThe message this would land does not pass its own rules:'
);
for (const problem of problems) console.error(` ${problem}`);
process.exitCode = 1;
} else if (dryRun) {
console.log('\nDry run; nothing landed.');
} else {
const merged = JSON.parse(
gh(
'api',
'-X',
'PUT',
`repos/${repository()}/pulls/${number}/merge`,
'-f',
'merge_method=squash',
'-f',
`commit_title=${pull.title}`,
'-f',
`commit_message=${message}`,
'-f',
`sha=${pull.head}`,
'--jq',
'{merged, sha}'
)
);

if (merged.merged === true) console.log(`\nLanded as ${merged.sha}`);
else {
console.error(`\n#${number} did not merge.`);
process.exitCode = 1;
}
}
}
}
} catch (error) {
console.error(
`Could not land #${number}: ${error instanceof Error ? error.message.split(String.fromCharCode(10))[0] : String(error)}`
);
process.exitCode = 1;
}