ci: harden GitHub API workflows (#3709)
This commit is contained in:
@@ -18,8 +18,8 @@ class PullRequestDocumentationWorkflowContractTest(unittest.TestCase):
|
||||
self.assertTrue(WORKFLOW.is_file(), "Documentation coverage workflow is missing")
|
||||
self.workflow = WORKFLOW.read_text(encoding="utf-8")
|
||||
|
||||
# @covers AC-CI-PR-DOCS-001.1, AC-CI-PR-DOCS-002.2
|
||||
def test_runs_for_all_pr_and_manual_retry_events(self) -> None:
|
||||
# @covers AC-CI-PR-DOCS-001.1, AC-CI-PR-DOCS-002.2, AC-CI-PR-DOCS-004.4
|
||||
def test_runs_for_relevant_pr_and_manual_retry_events(self) -> None:
|
||||
trigger = self.workflow.partition("on:\n")[2].partition("\nconcurrency:")[0]
|
||||
self.assertIn("pull_request_target:", trigger)
|
||||
for event in (
|
||||
@@ -31,13 +31,32 @@ class PullRequestDocumentationWorkflowContractTest(unittest.TestCase):
|
||||
"unlabeled",
|
||||
"ready_for_review",
|
||||
):
|
||||
self.assertIn(event, trigger)
|
||||
if event == "ready_for_review":
|
||||
self.assertNotIn(event, trigger)
|
||||
else:
|
||||
self.assertIn(event, trigger)
|
||||
self.assertIn("merge_group:", trigger)
|
||||
self.assertIn("checks_requested", trigger)
|
||||
self.assertIn("workflow_dispatch:", trigger)
|
||||
self.assertIn("pr_number:", trigger)
|
||||
self.assertNotIn("paths:", trigger)
|
||||
|
||||
# @covers AC-CI-PR-DOCS-002.2, AC-CI-PR-DOCS-004.4
|
||||
def test_unrelated_label_events_are_rejected_at_the_job_boundary(self) -> None:
|
||||
coverage_job = self.workflow.partition("jobs:\n")[2]
|
||||
self.assertIn("github.event_name != 'pull_request_target'", coverage_job)
|
||||
self.assertIn("github.event.action != 'labeled'", coverage_job)
|
||||
self.assertIn("github.event.action != 'unlabeled'", coverage_job)
|
||||
self.assertIn("github.event.label.name == 'no-docs-allow'", coverage_job)
|
||||
self.assertLess(coverage_job.index("github.event.label.name"), coverage_job.index("runs-on:"))
|
||||
|
||||
# @covers AC-CI-PR-DOCS-004.4
|
||||
def test_description_edits_are_rejected_but_base_retargets_run(self) -> None:
|
||||
coverage_job = self.workflow.partition("jobs:\n")[2]
|
||||
self.assertIn("github.event.action != 'edited'", coverage_job)
|
||||
self.assertIn("github.event.changes.base != null", coverage_job)
|
||||
self.assertLess(coverage_job.index("github.event.changes.base"), coverage_job.index("runs-on:"))
|
||||
|
||||
# @covers AC-CI-PR-DOCS-003.1, AC-CI-PR-DOCS-003.4
|
||||
def test_serializes_prs_without_cancelling_merge_group_runs(self) -> None:
|
||||
self.assertIn("group: >-", self.workflow)
|
||||
|
||||
+509
-58
@@ -67,10 +67,147 @@ const MAX_TOTAL_DOCUMENT_BYTES = 4 * 1024 * 1024;
|
||||
const MAX_RESPONSE_BYTES = 8 * 1024 * 1024;
|
||||
const MAX_CHANGED_FILES = 3000;
|
||||
const REQUEST_TIMEOUT_MS = 30_000;
|
||||
const MAX_REQUEST_ATTEMPTS = 3;
|
||||
const RETRY_BASE_DELAY_MS = 250;
|
||||
const MAX_RETRY_SLEEP_MS = 180_000;
|
||||
const SECONDARY_RATE_LIMIT_DELAYS_MS = [60_000, 120_000];
|
||||
const NO_DOCS_LABEL = 'no-docs-allow';
|
||||
const STATUS_CONTEXT = 'PR documentation coverage';
|
||||
const GITHUB_API = 'https://api.github.com';
|
||||
|
||||
function defaultSleep(delay) {
|
||||
return new Promise(resolve => setTimeout(resolve, delay));
|
||||
}
|
||||
|
||||
function defaultLog(message) {
|
||||
console.error(message);
|
||||
}
|
||||
|
||||
function responseHeader(response, name) {
|
||||
if (typeof response?.headers?.get === 'function') {
|
||||
return response.headers.get(name) ?? undefined;
|
||||
}
|
||||
if (!response?.headers || typeof response.headers !== 'object') {
|
||||
return undefined;
|
||||
}
|
||||
const key = Object.keys(response.headers).find(header =>
|
||||
header.toLowerCase() === name.toLowerCase(),
|
||||
);
|
||||
return key === undefined ? undefined : String(response.headers[key]);
|
||||
}
|
||||
|
||||
function retryAfterDelay(response) {
|
||||
const value = responseHeader(response, 'retry-after');
|
||||
if (value === undefined) {
|
||||
return undefined;
|
||||
}
|
||||
const trimmed = value.trim();
|
||||
if (/^\d+(?:\.\d+)?$/.test(trimmed)) {
|
||||
const seconds = Number(trimmed);
|
||||
return Number.isFinite(seconds) ? Math.max(0, seconds * 1000) : undefined;
|
||||
}
|
||||
const timestamp = Date.parse(trimmed);
|
||||
return Number.isFinite(timestamp) ? Math.max(0, timestamp - Date.now()) : undefined;
|
||||
}
|
||||
|
||||
function primaryRateLimitResetDelay(response) {
|
||||
const remaining = Number(responseHeader(response, 'x-ratelimit-remaining'));
|
||||
if (!Number.isFinite(remaining) || remaining > 0) {
|
||||
return undefined;
|
||||
}
|
||||
const reset = Number(responseHeader(response, 'x-ratelimit-reset'));
|
||||
if (!Number.isFinite(reset) || reset <= 0) {
|
||||
return undefined;
|
||||
}
|
||||
return Math.max(0, reset * 1000 - Date.now());
|
||||
}
|
||||
|
||||
function isGraphqlRateLimitPayload(payload) {
|
||||
return Array.isArray(payload?.errors)
|
||||
&& payload.errors.some(error => error?.type === 'RATE_LIMITED');
|
||||
}
|
||||
|
||||
function isRateLimitResponse(response, payload, requestClass) {
|
||||
if (requestClass === 'merge-queue' && isGraphqlRateLimitPayload(payload)) {
|
||||
return true;
|
||||
}
|
||||
if (response?.status === 429) {
|
||||
return true;
|
||||
}
|
||||
if (response?.status !== 403) {
|
||||
return false;
|
||||
}
|
||||
const remaining = Number(responseHeader(response, 'x-ratelimit-remaining'));
|
||||
return (
|
||||
(Number.isFinite(remaining) && remaining <= 0)
|
||||
|| responseHeader(response, 'retry-after') !== undefined
|
||||
|| /rate limit|secondary rate limit|abuse detection|temporarily unavailable/i.test(
|
||||
String(payload?.message ?? ''),
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
function isRetryableResponse(response, payload, requestClass) {
|
||||
const status = response?.status;
|
||||
return (
|
||||
status === 408
|
||||
|| status === 429
|
||||
|| (Number.isInteger(status) && status >= 500 && status <= 599)
|
||||
|| isRateLimitResponse(response, payload, requestClass)
|
||||
);
|
||||
}
|
||||
|
||||
function retryPlan(response, payload, attemptIndex, category, requestClass) {
|
||||
const rateLimited = isRateLimitResponse(response, payload, requestClass);
|
||||
const serverDelay = retryAfterDelay(response);
|
||||
if (serverDelay !== undefined) {
|
||||
return { delay: serverDelay, reason: 'server-wait' };
|
||||
}
|
||||
if (rateLimited) {
|
||||
const resetDelay = primaryRateLimitResetDelay(response);
|
||||
if (resetDelay !== undefined) {
|
||||
return { delay: resetDelay, reason: 'primary-rate-limit-reset' };
|
||||
}
|
||||
return {
|
||||
delay: SECONDARY_RATE_LIMIT_DELAYS_MS[attemptIndex] ?? MAX_RETRY_SLEEP_MS,
|
||||
reason: 'secondary-rate-limit-fallback',
|
||||
};
|
||||
}
|
||||
if (category === 'transport' || category === 'timeout' || isRetryableResponse(response, payload)) {
|
||||
return {
|
||||
delay: Math.min(RETRY_BASE_DELAY_MS * (2 ** attemptIndex), 5_000),
|
||||
reason: 'short-exponential-backoff',
|
||||
};
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
function requestClassForEndpoint(endpoint, method) {
|
||||
const pathname = endpoint.split('?', 1)[0];
|
||||
if (method === 'POST' && pathname.includes('/statuses/')) {
|
||||
return 'commit-status';
|
||||
}
|
||||
if (pathname.includes('/pulls/') && pathname.endsWith('/files')) {
|
||||
return 'changed-files';
|
||||
}
|
||||
if (pathname === '/search/code') {
|
||||
return 'code-search';
|
||||
}
|
||||
if (pathname === '/graphql') {
|
||||
return 'merge-queue';
|
||||
}
|
||||
if (pathname.includes('/pulls/')) {
|
||||
return 'pull-request';
|
||||
}
|
||||
return 'github-api';
|
||||
}
|
||||
|
||||
function transportCategory(error) {
|
||||
return error?.name === 'TimeoutError' || error?.name === 'AbortError'
|
||||
? 'timeout'
|
||||
: 'transport';
|
||||
}
|
||||
|
||||
function normalizeRepoPath(value) {
|
||||
if (typeof value !== 'string' || value.length === 0) {
|
||||
throw new Error('repository path must be a non-empty string');
|
||||
@@ -775,7 +912,14 @@ function requirePullRequestNumber(value) {
|
||||
}
|
||||
|
||||
class GitHubClient {
|
||||
constructor({ owner, repo, token, fetchImpl = globalThis.fetch } = {}) {
|
||||
constructor({
|
||||
owner,
|
||||
repo,
|
||||
token,
|
||||
fetchImpl = globalThis.fetch,
|
||||
sleepImpl = defaultSleep,
|
||||
logImpl = defaultLog,
|
||||
} = {}) {
|
||||
if (typeof owner !== 'string' || typeof repo !== 'string' || owner === '' || repo === '') {
|
||||
throw new Error('GitHub repository owner and name are required');
|
||||
}
|
||||
@@ -785,16 +929,28 @@ class GitHubClient {
|
||||
if (typeof fetchImpl !== 'function') {
|
||||
throw new Error('fetch implementation is required');
|
||||
}
|
||||
if (typeof sleepImpl !== 'function') {
|
||||
throw new Error('sleep implementation is required');
|
||||
}
|
||||
if (typeof logImpl !== 'function') {
|
||||
throw new Error('log implementation is required');
|
||||
}
|
||||
this.owner = owner;
|
||||
this.repo = repo;
|
||||
this.token = token;
|
||||
this.fetchImpl = fetchImpl;
|
||||
this.sleepImpl = sleepImpl;
|
||||
this.logImpl = logImpl;
|
||||
this.sleptMs = 0;
|
||||
}
|
||||
|
||||
async request(endpoint, { method = 'GET', body } = {}) {
|
||||
async request(endpoint, { method = 'GET', body, requestClass: requestedClass } = {}) {
|
||||
if (typeof endpoint !== 'string' || !endpoint.startsWith('/')) {
|
||||
throw new Error('GitHub endpoint must be an absolute API path');
|
||||
}
|
||||
const requestClass = typeof requestedClass === 'string' && requestedClass.length > 0
|
||||
? requestedClass
|
||||
: requestClassForEndpoint(endpoint, method);
|
||||
const headers = {
|
||||
Accept: 'application/vnd.github+json',
|
||||
Authorization: `Bearer ${this.token}`,
|
||||
@@ -803,51 +959,252 @@ class GitHubClient {
|
||||
if (body !== undefined) {
|
||||
headers['Content-Type'] = 'application/json';
|
||||
}
|
||||
const timeoutSignal = typeof AbortSignal === 'function'
|
||||
&& typeof AbortSignal.timeout === 'function'
|
||||
? AbortSignal.timeout(REQUEST_TIMEOUT_MS)
|
||||
: undefined;
|
||||
let response;
|
||||
try {
|
||||
response = await this.fetchImpl(`${GITHUB_API}${endpoint}`, {
|
||||
method,
|
||||
headers,
|
||||
body: body === undefined ? undefined : JSON.stringify(body),
|
||||
...(timeoutSignal ? { signal: timeoutSignal } : {}),
|
||||
});
|
||||
} catch (error) {
|
||||
if (error?.name === 'TimeoutError' || error?.name === 'AbortError') {
|
||||
throw new Error(`GitHub API request timed out after ${REQUEST_TIMEOUT_MS}ms`);
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
if (!response || typeof response.text !== 'function') {
|
||||
throw new Error('GitHub API returned an invalid response');
|
||||
}
|
||||
const text = await response.text();
|
||||
if (Buffer.byteLength(text, 'utf8') > MAX_RESPONSE_BYTES) {
|
||||
throw new Error(`GitHub API response exceeds the ${MAX_RESPONSE_BYTES}-byte limit`);
|
||||
}
|
||||
let payload = null;
|
||||
if (text.length > 0) {
|
||||
|
||||
const statusForLog = response => Number.isInteger(response?.status)
|
||||
? `HTTP ${response.status}`
|
||||
: 'none';
|
||||
const log = message => {
|
||||
try {
|
||||
payload = JSON.parse(text);
|
||||
this.logImpl(message);
|
||||
} catch {
|
||||
throw new Error('GitHub API returned invalid JSON');
|
||||
// Diagnostics must not hide the request result.
|
||||
}
|
||||
};
|
||||
const terminalError = ({
|
||||
category,
|
||||
response,
|
||||
attempt,
|
||||
outcome,
|
||||
message,
|
||||
nextDelay,
|
||||
}) => {
|
||||
const status = statusForLog(response);
|
||||
const diagnostic = [
|
||||
`class=${requestClass}`,
|
||||
`category=${category}`,
|
||||
`status=${status}`,
|
||||
`attempts=${attempt}`,
|
||||
`outcome=${outcome}`,
|
||||
...(nextDelay === undefined ? [] : [`next_delay_ms=${nextDelay}`]),
|
||||
].join(' ');
|
||||
log(`GitHub API request stopped: ${diagnostic}`);
|
||||
if (message) {
|
||||
return new Error(`${message} (${diagnostic})`);
|
||||
}
|
||||
return new Error(`GitHub API request failed (${diagnostic})`);
|
||||
};
|
||||
const waitBeforeRetry = async ({ response, category, attempt, plan }) => {
|
||||
const remainingMs = MAX_RETRY_SLEEP_MS - this.sleptMs;
|
||||
if (plan.delay > remainingMs) {
|
||||
return false;
|
||||
}
|
||||
log(
|
||||
`GitHub API request retry: class=${requestClass} category=${category} `
|
||||
+ `status=${statusForLog(response)} attempt=${attempt + 1}/${MAX_REQUEST_ATTEMPTS} `
|
||||
+ `delay_ms=${plan.delay} reason=${plan.reason}`,
|
||||
);
|
||||
try {
|
||||
await this.sleepImpl(plan.delay);
|
||||
} catch {
|
||||
throw terminalError({
|
||||
category: 'sleep',
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
outcome: 'retry-aborted',
|
||||
});
|
||||
}
|
||||
this.sleptMs += plan.delay;
|
||||
return true;
|
||||
};
|
||||
|
||||
for (let attempt = 0; attempt < MAX_REQUEST_ATTEMPTS; attempt += 1) {
|
||||
const timeoutSignal = typeof AbortSignal === 'function'
|
||||
&& typeof AbortSignal.timeout === 'function'
|
||||
? AbortSignal.timeout(REQUEST_TIMEOUT_MS)
|
||||
: undefined;
|
||||
let response;
|
||||
try {
|
||||
response = await this.fetchImpl(`${GITHUB_API}${endpoint}`, {
|
||||
method,
|
||||
headers,
|
||||
body: body === undefined ? undefined : JSON.stringify(body),
|
||||
...(timeoutSignal ? { signal: timeoutSignal } : {}),
|
||||
});
|
||||
} catch (error) {
|
||||
const category = transportCategory(error);
|
||||
const plan = retryPlan(undefined, undefined, attempt, category, requestClass);
|
||||
if (attempt + 1 < MAX_REQUEST_ATTEMPTS && plan) {
|
||||
if (await waitBeforeRetry({ category, attempt, plan })) {
|
||||
continue;
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
attempt: attempt + 1,
|
||||
outcome: 'wait-budget-exhausted',
|
||||
nextDelay: plan.delay,
|
||||
});
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
attempt: attempt + 1,
|
||||
message: category === 'timeout'
|
||||
? `GitHub API request timed out after ${REQUEST_TIMEOUT_MS}ms`
|
||||
: undefined,
|
||||
outcome: 'retry-exhausted',
|
||||
});
|
||||
}
|
||||
if (!response || typeof response.text !== 'function') {
|
||||
throw terminalError({
|
||||
category: 'protocol',
|
||||
attempt: attempt + 1,
|
||||
message: 'GitHub API returned an invalid response',
|
||||
outcome: 'permanent',
|
||||
});
|
||||
}
|
||||
|
||||
let text;
|
||||
try {
|
||||
text = await response.text();
|
||||
} catch (error) {
|
||||
const retryable = response.ok || isRetryableResponse(response, undefined, requestClass);
|
||||
const category = retryable ? transportCategory(error) : 'http';
|
||||
const plan = retryable
|
||||
? retryPlan(response, undefined, attempt, category, requestClass)
|
||||
: undefined;
|
||||
if (retryable && attempt + 1 < MAX_REQUEST_ATTEMPTS && plan) {
|
||||
if (await waitBeforeRetry({ response, category, attempt, plan })) {
|
||||
continue;
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
outcome: 'wait-budget-exhausted',
|
||||
nextDelay: plan.delay,
|
||||
});
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
outcome: retryable ? 'retry-exhausted' : 'permanent',
|
||||
});
|
||||
}
|
||||
if (Buffer.byteLength(text, 'utf8') > MAX_RESPONSE_BYTES) {
|
||||
throw terminalError({
|
||||
category: 'response-too-large',
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
message: `GitHub API response exceeds the ${MAX_RESPONSE_BYTES}-byte limit`,
|
||||
outcome: 'permanent',
|
||||
});
|
||||
}
|
||||
let payload = null;
|
||||
if (text.length > 0) {
|
||||
try {
|
||||
payload = JSON.parse(text);
|
||||
} catch {
|
||||
const retryable = !response.ok && isRetryableResponse(response, undefined, requestClass);
|
||||
// The body is unavailable after parsing fails, so headers are the only signal.
|
||||
const category = isRateLimitResponse(response, undefined, requestClass)
|
||||
? 'rate-limit'
|
||||
: response.status >= 500
|
||||
? 'server'
|
||||
: 'protocol';
|
||||
const plan = retryable
|
||||
? retryPlan(response, undefined, attempt, category, requestClass)
|
||||
: undefined;
|
||||
if (
|
||||
retryable
|
||||
&& attempt + 1 < MAX_REQUEST_ATTEMPTS
|
||||
&& plan
|
||||
) {
|
||||
if (await waitBeforeRetry({ response, category, attempt, plan })) {
|
||||
continue;
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
outcome: 'wait-budget-exhausted',
|
||||
nextDelay: plan.delay,
|
||||
});
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
message: response.ok
|
||||
? 'GitHub API returned invalid JSON'
|
||||
: `GitHub API request failed with HTTP ${response.status}: invalid JSON`,
|
||||
outcome: retryable ? 'retry-exhausted' : 'permanent',
|
||||
});
|
||||
}
|
||||
}
|
||||
if (response.ok && isRateLimitResponse(response, payload, requestClass)) {
|
||||
const category = 'rate-limit';
|
||||
const plan = retryPlan(response, payload, attempt, category, requestClass);
|
||||
if (attempt + 1 < MAX_REQUEST_ATTEMPTS && plan) {
|
||||
if (await waitBeforeRetry({ response, category, attempt, plan })) {
|
||||
continue;
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
outcome: 'wait-budget-exhausted',
|
||||
nextDelay: plan.delay,
|
||||
});
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
outcome: 'retry-exhausted',
|
||||
});
|
||||
}
|
||||
if (!response.ok) {
|
||||
const category = isRateLimitResponse(response, payload, requestClass)
|
||||
? 'rate-limit'
|
||||
: response.status >= 500
|
||||
? 'server'
|
||||
: 'http';
|
||||
const plan = retryPlan(response, payload, attempt, category, requestClass);
|
||||
if (
|
||||
attempt + 1 < MAX_REQUEST_ATTEMPTS
|
||||
&& isRetryableResponse(response, payload, requestClass)
|
||||
&& plan
|
||||
) {
|
||||
if (await waitBeforeRetry({ response, category, attempt, plan })) {
|
||||
continue;
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
outcome: 'wait-budget-exhausted',
|
||||
nextDelay: plan.delay,
|
||||
});
|
||||
}
|
||||
throw terminalError({
|
||||
category,
|
||||
response,
|
||||
attempt: attempt + 1,
|
||||
message: `GitHub API request failed with HTTP ${response.status}`,
|
||||
outcome: isRetryableResponse(response, payload, requestClass)
|
||||
? 'retry-exhausted'
|
||||
: 'permanent',
|
||||
});
|
||||
}
|
||||
return payload;
|
||||
}
|
||||
if (!response.ok) {
|
||||
const detail =
|
||||
payload && typeof payload.message === 'string' ? `: ${payload.message.slice(0, 200)}` : '';
|
||||
throw new Error(`GitHub API request failed with HTTP ${response.status}${detail}`);
|
||||
}
|
||||
return payload;
|
||||
}
|
||||
|
||||
async getPullRequest(number) {
|
||||
requirePullRequestNumber(number);
|
||||
const pullRequest = await this.request(
|
||||
`/repos/${encodeURIComponent(this.owner)}/${encodeURIComponent(this.repo)}/pulls/${number}`,
|
||||
{ requestClass: 'pull-request' },
|
||||
);
|
||||
if (!pullRequest || pullRequest.number !== number) {
|
||||
throw new Error('GitHub returned a pull request with the wrong number');
|
||||
@@ -876,6 +1233,7 @@ class GitHubClient {
|
||||
for (let page = 1; page <= 30; page += 1) {
|
||||
const pageFiles = await this.request(
|
||||
`/repos/${encodeURIComponent(this.owner)}/${encodeURIComponent(this.repo)}/pulls/${number}/files?per_page=100&page=${page}`,
|
||||
{ requestClass: 'changed-files' },
|
||||
);
|
||||
if (!Array.isArray(pageFiles)) {
|
||||
throw new Error('GitHub changed-file response is not a list');
|
||||
@@ -919,6 +1277,7 @@ class GitHubClient {
|
||||
const encodedPath = normalized.split('/').map(encodeURIComponent).join('/');
|
||||
const response = await this.request(
|
||||
`/repos/${encodeURIComponent(this.owner)}/${encodeURIComponent(this.repo)}/contents/${encodedPath}?ref=${encodeURIComponent(ref)}`,
|
||||
{ requestClass: 'file-content' },
|
||||
);
|
||||
if (typeof response?.path !== 'string' || normalizeRepoPath(response.path) !== normalized) {
|
||||
throw new Error(`${normalized} response has a mismatched returned path`);
|
||||
@@ -949,6 +1308,7 @@ class GitHubClient {
|
||||
const encodedPath = normalized.split('/').map(encodeURIComponent).join('/');
|
||||
const response = await this.request(
|
||||
`/repos/${encodeURIComponent(this.owner)}/${encodeURIComponent(this.repo)}/contents/${encodedPath}?ref=${encodeURIComponent(ref)}`,
|
||||
{ requestClass: 'directory-listing' },
|
||||
);
|
||||
if (!Array.isArray(response)) {
|
||||
throw new Error(`${normalized} is not a directory listing`);
|
||||
@@ -973,6 +1333,7 @@ class GitHubClient {
|
||||
const query = `"${text}" repo:${this.owner}/${this.repo} path:${normalizedDirectory}`;
|
||||
const response = await this.request(
|
||||
`/search/code?q=${encodeURIComponent(query)}&per_page=100`,
|
||||
{ requestClass: 'code-search' },
|
||||
);
|
||||
if (
|
||||
!response
|
||||
@@ -1008,6 +1369,7 @@ class GitHubClient {
|
||||
`/repos/${encodeURIComponent(this.owner)}/${encodeURIComponent(this.repo)}/statuses/${sha}`,
|
||||
{
|
||||
method: 'POST',
|
||||
requestClass: 'commit-status',
|
||||
body: {
|
||||
state: status.state,
|
||||
context: STATUS_CONTEXT,
|
||||
@@ -1021,6 +1383,7 @@ class GitHubClient {
|
||||
async graphql(query, variables) {
|
||||
const response = await this.request('/graphql', {
|
||||
method: 'POST',
|
||||
requestClass: 'merge-queue',
|
||||
body: { query, variables },
|
||||
});
|
||||
if (Array.isArray(response?.errors) && response.errors.length > 0) {
|
||||
@@ -1117,36 +1480,88 @@ function isMissingResourceError(error) {
|
||||
return /\bHTTP 404\b/.test(String(error?.message ?? error));
|
||||
}
|
||||
|
||||
async function loadCoverageContents({ client, changedFiles, headSha }) {
|
||||
function hasRequirementHeading(content, requirementId) {
|
||||
return typeof content === 'string' && requirementHeadingPattern(requirementId).test(content);
|
||||
}
|
||||
|
||||
function changedRequirementSources(changedFiles, requirementDirectory) {
|
||||
const sources = new Map();
|
||||
const prefix = `${requirementDirectory}/`;
|
||||
for (const change of changedFiles ?? []) {
|
||||
const currentPath = typeof change === 'string' ? change : change?.filename;
|
||||
if (typeof currentPath !== 'string') {
|
||||
continue;
|
||||
}
|
||||
let normalizedCurrent;
|
||||
try {
|
||||
normalizedCurrent = normalizeRepoPath(currentPath);
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
const status = typeof change === 'string' ? undefined : change?.status;
|
||||
const headPath = status === 'removed' ? undefined : normalizedCurrent;
|
||||
let basePath = normalizedCurrent;
|
||||
if (status === 'renamed' && typeof change.previous_filename === 'string') {
|
||||
try {
|
||||
basePath = normalizeRepoPath(change.previous_filename);
|
||||
} catch {
|
||||
basePath = undefined;
|
||||
}
|
||||
}
|
||||
const normalizedHead = headPath && headPath.startsWith(prefix) && headPath.endsWith('.md')
|
||||
? headPath
|
||||
: undefined;
|
||||
const normalizedBase = basePath && basePath.startsWith(prefix) && basePath.endsWith('.md')
|
||||
? basePath
|
||||
: undefined;
|
||||
if (!normalizedHead && !normalizedBase) {
|
||||
continue;
|
||||
}
|
||||
const key = normalizedHead ?? `base:${normalizedBase}`;
|
||||
if (!sources.has(key)) {
|
||||
sources.set(key, { basePath: normalizedBase, headPath: normalizedHead });
|
||||
}
|
||||
}
|
||||
return [...sources.values()];
|
||||
}
|
||||
|
||||
async function loadCoverageContents({ client, changedFiles, headSha, baseSha }) {
|
||||
if (!classifyChangedFiles(changedFiles).requiresCoverage) {
|
||||
return {};
|
||||
}
|
||||
const contents = {};
|
||||
const loaded = new Set();
|
||||
const loaded = new Map();
|
||||
const requirementSearches = new Map();
|
||||
const requirementDirectories = new Map();
|
||||
let documentCount = 0;
|
||||
let totalBytes = 0;
|
||||
async function load(pathname) {
|
||||
async function load(pathname, ref, targetContents) {
|
||||
const normalized = normalizeRepoPath(pathname);
|
||||
if (loaded.has(normalized)) {
|
||||
return contents[normalized];
|
||||
const cacheKey = `${ref}\u0000${normalized}`;
|
||||
if (loaded.has(cacheKey)) {
|
||||
const content = loaded.get(cacheKey);
|
||||
if (targetContents) {
|
||||
targetContents[normalized] = content;
|
||||
}
|
||||
return content;
|
||||
}
|
||||
if (documentCount >= MAX_DOCUMENTS) {
|
||||
throw new Error(`Referenced documents exceed the ${MAX_DOCUMENTS}-document limit`);
|
||||
}
|
||||
let content;
|
||||
try {
|
||||
content = await client.getFile(normalized, headSha);
|
||||
content = await client.getFile(normalized, ref);
|
||||
} catch (error) {
|
||||
if (!isMissingResourceError(error)) {
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
loaded.add(normalized);
|
||||
loaded.set(cacheKey, content);
|
||||
documentCount += 1;
|
||||
if (targetContents) {
|
||||
targetContents[normalized] = content;
|
||||
}
|
||||
if (typeof content !== 'string' || content.trim().length === 0) {
|
||||
contents[normalized] = undefined;
|
||||
return undefined;
|
||||
}
|
||||
const size = Buffer.byteLength(content, 'utf8');
|
||||
@@ -1157,12 +1572,11 @@ async function loadCoverageContents({ client, changedFiles, headSha }) {
|
||||
if (totalBytes > MAX_TOTAL_DOCUMENT_BYTES) {
|
||||
throw new Error(`Referenced documents exceed the ${MAX_TOTAL_DOCUMENT_BYTES}-byte total limit`);
|
||||
}
|
||||
contents[normalized] = content;
|
||||
return content;
|
||||
}
|
||||
|
||||
for (const workOrderPath of selectChangedWorkOrders(changedFiles)) {
|
||||
const workOrderContent = await load(workOrderPath);
|
||||
const workOrderContent = await load(workOrderPath, headSha, contents);
|
||||
if (workOrderContent === undefined) {
|
||||
continue;
|
||||
}
|
||||
@@ -1178,7 +1592,7 @@ async function loadCoverageContents({ client, changedFiles, headSha }) {
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
const planContent = await load(planPath);
|
||||
const planContent = await load(planPath, headSha, contents);
|
||||
if (planContent === undefined) {
|
||||
continue;
|
||||
}
|
||||
@@ -1201,7 +1615,7 @@ async function loadCoverageContents({ client, changedFiles, headSha }) {
|
||||
}
|
||||
}
|
||||
for (const designPath of designPaths) {
|
||||
const designContent = await load(designPath);
|
||||
const designContent = await load(designPath, headSha, contents);
|
||||
if (designContent === undefined) {
|
||||
continue;
|
||||
}
|
||||
@@ -1222,19 +1636,44 @@ async function loadCoverageContents({ client, changedFiles, headSha }) {
|
||||
}
|
||||
const requirementDirectory = `docs/specs/${system}/requirements`;
|
||||
const requirementPaths = new Set();
|
||||
const changedRepositoryPaths = changedPaths(changedFiles).paths;
|
||||
for (const pathname of changedRepositoryPaths) {
|
||||
if (pathname.startsWith(`${requirementDirectory}/`) && pathname.endsWith('.md')) {
|
||||
requirementPaths.add(pathname);
|
||||
const baseContentByHeadPath = new Map();
|
||||
for (const source of changedRequirementSources(changedFiles, requirementDirectory)) {
|
||||
if (source.headPath) {
|
||||
requirementPaths.add(source.headPath);
|
||||
await load(source.headPath, headSha, contents);
|
||||
}
|
||||
if (source.basePath) {
|
||||
const baseContent = await load(source.basePath, baseSha);
|
||||
if (source.headPath) {
|
||||
baseContentByHeadPath.set(source.headPath, baseContent);
|
||||
}
|
||||
}
|
||||
}
|
||||
const unresolvedRequirementIds = [];
|
||||
const workOrderRequirements = new Set(workOrder.requirements ?? []);
|
||||
const referencedRequirementIds = designRequirements.filter(requirementId =>
|
||||
workOrderRequirements.has(requirementId)
|
||||
);
|
||||
const verifiedRequirementIds = new Set();
|
||||
for (const requirementId of referencedRequirementIds) {
|
||||
const definitions = requirementDefinitions(contents, requirementId, system);
|
||||
if (definitions.length !== 1) {
|
||||
continue;
|
||||
}
|
||||
const definition = definitions[0];
|
||||
const baseContent = baseContentByHeadPath.get(definition.pathname);
|
||||
if (
|
||||
hasRequirementHeading(definition.content, requirementId)
|
||||
&& hasRequirementHeading(baseContent, requirementId)
|
||||
) {
|
||||
verifiedRequirementIds.add(requirementId);
|
||||
}
|
||||
}
|
||||
const unresolvedRequirementIds = [];
|
||||
if (typeof client.searchCode === 'function') {
|
||||
for (const requirementId of referencedRequirementIds) {
|
||||
if (verifiedRequirementIds.has(requirementId)) {
|
||||
continue;
|
||||
}
|
||||
const searchKey = JSON.stringify([requirementDirectory, requirementId]);
|
||||
if (!requirementSearches.has(searchKey)) {
|
||||
requirementSearches.set(
|
||||
@@ -1251,7 +1690,9 @@ async function loadCoverageContents({ client, changedFiles, headSha }) {
|
||||
}
|
||||
}
|
||||
} else {
|
||||
unresolvedRequirementIds.push(...referencedRequirementIds);
|
||||
unresolvedRequirementIds.push(
|
||||
...referencedRequirementIds.filter(requirementId => !verifiedRequirementIds.has(requirementId)),
|
||||
);
|
||||
}
|
||||
if (unresolvedRequirementIds.length > 0 && typeof client.listDirectory === 'function') {
|
||||
if (!requirementDirectories.has(requirementDirectory)) {
|
||||
@@ -1277,7 +1718,7 @@ async function loadCoverageContents({ client, changedFiles, headSha }) {
|
||||
}
|
||||
}
|
||||
for (const pathname of requirementPaths) {
|
||||
const requirementContent = await load(pathname);
|
||||
const requirementContent = await load(pathname, headSha, contents);
|
||||
if (requirementContent === undefined) {
|
||||
continue;
|
||||
}
|
||||
@@ -1318,6 +1759,7 @@ async function evaluatePullRequestSnapshot({ client, pullRequest, expectedHeadSh
|
||||
}
|
||||
const changedFiles = await client.listFiles(pullRequest.number, pullRequest.changed_files);
|
||||
const fileContents = await loadCoverageContents({
|
||||
baseSha: pullRequest.base.sha,
|
||||
changedFiles,
|
||||
client,
|
||||
headSha: pullRequest.head.sha,
|
||||
@@ -1328,16 +1770,24 @@ async function evaluatePullRequestSnapshot({ client, pullRequest, expectedHeadSh
|
||||
};
|
||||
}
|
||||
|
||||
async function evaluatePullRequest({ client, pullNumber, expectedHeadSha, maxAttempts = 3 } = {}) {
|
||||
async function evaluatePullRequest({
|
||||
client,
|
||||
pullNumber,
|
||||
expectedHeadSha,
|
||||
maxAttempts = 3,
|
||||
initialPullRequest,
|
||||
} = {}) {
|
||||
requirePullRequestNumber(pullNumber);
|
||||
if (!Number.isInteger(maxAttempts) || maxAttempts < 1 || maxAttempts > 5) {
|
||||
throw new Error('maxAttempts must be an integer from 1 through 5');
|
||||
}
|
||||
let lastMetadata;
|
||||
let firstSnapshot = initialPullRequest;
|
||||
for (let attempt = 0; attempt < maxAttempts; attempt += 1) {
|
||||
let pullRequest;
|
||||
try {
|
||||
pullRequest = await client.getPullRequest(pullNumber);
|
||||
pullRequest = firstSnapshot ?? await client.getPullRequest(pullNumber);
|
||||
firstSnapshot = undefined;
|
||||
const before = resultMetadata(pullRequest);
|
||||
if (expectedHeadSha && pullRequest.head.sha !== expectedHeadSha) {
|
||||
return errorCoverageResult(
|
||||
@@ -1734,6 +2184,7 @@ async function run({ client, env = process.env, event, eventName, writeSummary }
|
||||
});
|
||||
result = await evaluatePullRequest({
|
||||
client: apiClient,
|
||||
initialPullRequest: current,
|
||||
pullNumber,
|
||||
});
|
||||
await publishResult(apiClient, result.headSha ?? pendingSha, result, targetUrl);
|
||||
|
||||
@@ -730,6 +730,560 @@ test('GitHub client rejects incomplete pages and decodes bounded file contents',
|
||||
assert.equal(requests[1].options.headers.Authorization, 'Bearer token-is-not-logged');
|
||||
});
|
||||
|
||||
test('GitHub client retries a transient response with the server delay', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
if (calls === 1) {
|
||||
return {
|
||||
ok: false,
|
||||
status: 503,
|
||||
headers: { 'retry-after': '0' },
|
||||
async text() {
|
||||
return JSON.stringify({ message: 'Service Unavailable' });
|
||||
},
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
number: 42,
|
||||
head: { sha: SHA_B },
|
||||
base: { sha: SHA_A, ref: 'main' },
|
||||
labels: [],
|
||||
changed_files: 0,
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
const result = await client.getPullRequest(42);
|
||||
|
||||
assert.equal(result.head.sha, SHA_B);
|
||||
assert.equal(calls, 2);
|
||||
assert.deepEqual(delays, [0]);
|
||||
});
|
||||
|
||||
test('GitHub client uses the documented secondary-limit delays without headers', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const logs = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'secret-token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
logImpl: message => logs.push(message),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
return {
|
||||
ok: false,
|
||||
status: 429,
|
||||
async text() {
|
||||
return JSON.stringify({ message: 'private response details' });
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
await assert.rejects(client.getPullRequest(42), /HTTP 429/);
|
||||
|
||||
assert.equal(calls, 3);
|
||||
assert.deepEqual(delays, [60_000, 120_000]);
|
||||
assert.equal(logs.some(log => log.includes('delay_ms=60000')), true);
|
||||
assert.equal(logs.some(log => log.includes('delay_ms=120000')), true);
|
||||
assert.equal(logs.every(log => !log.includes('secret-token')), true);
|
||||
assert.equal(logs.every(log => !log.includes('private response details')), true);
|
||||
assert.equal(logs.every(log => !log.includes('q=')), true);
|
||||
assert.equal(logs.some(log => log.includes('outcome=retry-exhausted')), true);
|
||||
});
|
||||
|
||||
test('GitHub client shares the rate-limit sleep budget across requests', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
if (calls <= 2 || calls === 4) {
|
||||
return {
|
||||
ok: false,
|
||||
status: 429,
|
||||
async text() {
|
||||
return JSON.stringify({ message: 'secondary rate limit' });
|
||||
},
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
number: 42,
|
||||
head: { sha: SHA_B },
|
||||
base: { sha: SHA_A, ref: 'main' },
|
||||
labels: [],
|
||||
changed_files: 0,
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
await client.getPullRequest(42);
|
||||
await assert.rejects(client.getPullRequest(42), /wait-budget-exhausted/);
|
||||
|
||||
assert.equal(calls, 4);
|
||||
assert.deepEqual(delays, [60_000, 120_000]);
|
||||
});
|
||||
|
||||
test('GitHub client retries rate-limited GraphQL error payloads', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
if (calls === 1) {
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
data: null,
|
||||
errors: [{ type: 'RATE_LIMITED', message: 'rate limit exceeded' }],
|
||||
});
|
||||
},
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
data: { repository: { mergeQueue: { entries: { nodes: [] } } } },
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
const result = await client.graphql('query { repository { name } }', {});
|
||||
|
||||
assert.deepEqual(result, { repository: { mergeQueue: { entries: { nodes: [] } } } });
|
||||
assert.equal(calls, 2);
|
||||
assert.deepEqual(delays, [60_000]);
|
||||
});
|
||||
|
||||
test('GitHub client does not retry unrelated GraphQL errors', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
data: null,
|
||||
errors: [{ type: 'FORBIDDEN', message: 'permission denied' }],
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
await assert.rejects(client.graphql('query { repository { name } }', {}), /permission denied/);
|
||||
|
||||
assert.equal(calls, 1);
|
||||
assert.deepEqual(delays, []);
|
||||
});
|
||||
|
||||
test('GitHub client retries status writes with the original request payload', async () => {
|
||||
const requests = [];
|
||||
let calls = 0;
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async () => {},
|
||||
logImpl: () => {},
|
||||
fetchImpl: async (url, options) => {
|
||||
calls += 1;
|
||||
requests.push({ url, body: options.body });
|
||||
return {
|
||||
ok: calls > 1,
|
||||
status: calls > 1 ? 201 : 503,
|
||||
headers: { 'retry-after': '0' },
|
||||
async text() {
|
||||
return calls > 1 ? '{}' : JSON.stringify({ message: 'temporary failure' });
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
await client.createCommitStatus(SHA_B, {
|
||||
description: 'Coverage failed',
|
||||
state: 'failure',
|
||||
targetUrl: 'https://github.com/kdlbs/kandev/actions/runs/42',
|
||||
});
|
||||
|
||||
assert.equal(calls, 2);
|
||||
assert.deepEqual(requests[1], requests[0]);
|
||||
});
|
||||
|
||||
test('GitHub client uses a due primary rate-limit reset', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
if (calls === 1) {
|
||||
return {
|
||||
ok: false,
|
||||
status: 403,
|
||||
headers: {
|
||||
'x-ratelimit-remaining': '0',
|
||||
'x-ratelimit-reset': String(Math.floor(Date.now() / 1000)),
|
||||
},
|
||||
async text() {
|
||||
return JSON.stringify({ message: 'API rate limit exceeded' });
|
||||
},
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
number: 42,
|
||||
head: { sha: SHA_B },
|
||||
base: { sha: SHA_A, ref: 'main' },
|
||||
labels: [],
|
||||
changed_files: 0,
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
const result = await client.getPullRequest(42);
|
||||
|
||||
assert.equal(result.number, 42);
|
||||
assert.equal(calls, 2);
|
||||
assert.deepEqual(delays, [0]);
|
||||
});
|
||||
|
||||
test('GitHub client stops before an explicit wait exceeds the sleep budget', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const logs = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
logImpl: message => logs.push(message),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
return {
|
||||
ok: false,
|
||||
status: 503,
|
||||
headers: { 'retry-after': '181' },
|
||||
async text() {
|
||||
return JSON.stringify({ message: 'Service Unavailable' });
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
await assert.rejects(client.getPullRequest(42), /wait-budget-exhausted/);
|
||||
|
||||
assert.equal(calls, 1);
|
||||
assert.deepEqual(delays, []);
|
||||
assert.equal(logs.some(log => log.includes('next_delay_ms=181000')), true);
|
||||
});
|
||||
|
||||
test('GitHub client retries transport failures with short exponential backoff', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
if (calls < 3) {
|
||||
throw new Error('network unavailable');
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
number: 42,
|
||||
head: { sha: SHA_B },
|
||||
base: { sha: SHA_A, ref: 'main' },
|
||||
labels: [],
|
||||
changed_files: 0,
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
const result = await client.getPullRequest(42);
|
||||
|
||||
assert.equal(result.number, 42);
|
||||
assert.equal(calls, 3);
|
||||
assert.deepEqual(delays, [250, 500]);
|
||||
});
|
||||
|
||||
test('GitHub client retries HTTP 408 and secondary rate-limit 403 responses', async t => {
|
||||
const cases = [
|
||||
{
|
||||
name: '408',
|
||||
response: { ok: false, status: 408 },
|
||||
expectedDelay: 250,
|
||||
},
|
||||
{
|
||||
name: 'secondary 403',
|
||||
response: {
|
||||
ok: false,
|
||||
status: 403,
|
||||
async text() {
|
||||
return JSON.stringify({ message: 'You have exceeded a secondary rate limit.' });
|
||||
},
|
||||
},
|
||||
expectedDelay: 60_000,
|
||||
},
|
||||
];
|
||||
|
||||
for (const scenario of cases) {
|
||||
await t.test(scenario.name, async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
if (calls === 1) {
|
||||
return {
|
||||
...scenario.response,
|
||||
async text() {
|
||||
return scenario.response.text
|
||||
? scenario.response.text()
|
||||
: JSON.stringify({ message: 'Request timed out' });
|
||||
},
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
number: 42,
|
||||
head: { sha: SHA_B },
|
||||
base: { sha: SHA_A, ref: 'main' },
|
||||
labels: [],
|
||||
changed_files: 0,
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
const result = await client.getPullRequest(42);
|
||||
|
||||
assert.equal(result.number, 42);
|
||||
assert.equal(calls, 2);
|
||||
assert.deepEqual(delays, [scenario.expectedDelay]);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
test('GitHub client retries an unreadable transient body and invalid retryable JSON', async t => {
|
||||
for (const mode of ['body', 'json']) {
|
||||
await t.test(mode, async () => {
|
||||
let calls = 0;
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async () => {},
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
if (calls === 1) {
|
||||
return {
|
||||
ok: false,
|
||||
status: 503,
|
||||
async text() {
|
||||
if (mode === 'body') {
|
||||
throw new Error('response body stream closed');
|
||||
}
|
||||
return '<html>temporarily unavailable</html>';
|
||||
},
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
number: 42,
|
||||
head: { sha: SHA_B },
|
||||
base: { sha: SHA_A, ref: 'main' },
|
||||
labels: [],
|
||||
changed_files: 0,
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
const result = await client.getPullRequest(42);
|
||||
|
||||
assert.equal(result.number, 42);
|
||||
assert.equal(calls, 2);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
test('GitHub client fails permanent 4xx body and JSON errors without retry', async t => {
|
||||
for (const mode of ['body', 'json']) {
|
||||
await t.test(mode, async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const logs = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
logImpl: message => logs.push(message),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
return {
|
||||
ok: false,
|
||||
status: 404,
|
||||
async text() {
|
||||
if (mode === 'body') {
|
||||
throw new Error('response body stream closed');
|
||||
}
|
||||
return '<html>not found</html>';
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
await assert.rejects(client.getPullRequest(42), /HTTP 404/);
|
||||
|
||||
assert.equal(calls, 1);
|
||||
assert.deepEqual(delays, []);
|
||||
assert.equal(logs.some(log => log.includes('status=HTTP 404')), true);
|
||||
assert.equal(logs.some(log => log.includes('attempts=1')), true);
|
||||
assert.equal(logs.some(log => log.includes('outcome=permanent')), true);
|
||||
assert.equal(logs.some(log => log.includes('request retry')), false);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
test('GitHub client fails permanent responses after one attempt', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const logs = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
logImpl: message => logs.push(message),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
return {
|
||||
ok: false,
|
||||
status: 404,
|
||||
async text() {
|
||||
return JSON.stringify({ message: 'Not Found' });
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
await assert.rejects(client.getPullRequest(42), /HTTP 404/);
|
||||
|
||||
assert.equal(calls, 1);
|
||||
assert.deepEqual(delays, []);
|
||||
assert.equal(logs.some(log => log.includes('category=http')), true);
|
||||
assert.equal(logs.some(log => log.includes('outcome=permanent')), true);
|
||||
});
|
||||
|
||||
test('GitHub client retries request timeouts as transport failures', async () => {
|
||||
let calls = 0;
|
||||
const delays = [];
|
||||
const client = new validator.GitHubClient({
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async delay => delays.push(delay),
|
||||
fetchImpl: async () => {
|
||||
calls += 1;
|
||||
if (calls === 1) {
|
||||
const error = new Error('request timed out');
|
||||
error.name = 'TimeoutError';
|
||||
throw error;
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
async text() {
|
||||
return JSON.stringify({
|
||||
number: 42,
|
||||
head: { sha: SHA_B },
|
||||
base: { sha: SHA_A, ref: 'main' },
|
||||
labels: [],
|
||||
changed_files: 0,
|
||||
});
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
const result = await client.getPullRequest(42);
|
||||
|
||||
assert.equal(result.number, 42);
|
||||
assert.equal(calls, 2);
|
||||
assert.deepEqual(delays, [250]);
|
||||
});
|
||||
|
||||
test('GitHub client rejects malformed changed-file entries and mismatched content paths', async () => {
|
||||
const responses = [
|
||||
{
|
||||
@@ -804,6 +1358,8 @@ test('GitHub client converts a request timeout into a bounded error', async () =
|
||||
owner: 'kdlbs',
|
||||
repo: 'kandev',
|
||||
token: 'token',
|
||||
sleepImpl: async () => {},
|
||||
logImpl: () => {},
|
||||
fetchImpl: async (_url, options) => {
|
||||
signal = options.signal;
|
||||
const error = new Error('request timed out');
|
||||
@@ -859,6 +1415,119 @@ test('coverage loading searches only the referenced requirement files', async ()
|
||||
]);
|
||||
});
|
||||
|
||||
test('a changed requirement body with the same identity avoids code search', async () => {
|
||||
const baseContents = uiFixtureContents();
|
||||
const headContents = uiFixtureContents();
|
||||
const requirementPath = 'docs/specs/ui/requirements/ui-coverage.md';
|
||||
headContents[requirementPath] = headContents[requirementPath].replace(
|
||||
'Runtime changes have a work order.',
|
||||
'Runtime changes have an updated work order.',
|
||||
);
|
||||
const changed = [
|
||||
{ filename: 'apps/web/lib/runtime.ts', status: 'modified' },
|
||||
{
|
||||
filename: 'docs/plans/recovery/task-01-recovery.md',
|
||||
status: 'modified',
|
||||
additions: 1,
|
||||
changes: 1,
|
||||
},
|
||||
{ filename: requirementPath, status: 'modified', additions: 1, changes: 1 },
|
||||
];
|
||||
const loaded = [];
|
||||
const searches = [];
|
||||
const client = {
|
||||
async getPullRequest() {
|
||||
return pullRequest(42, SHA_B, [], changed.length);
|
||||
},
|
||||
async listFiles() {
|
||||
return changed;
|
||||
},
|
||||
async getFile(pathname, ref) {
|
||||
loaded.push({ pathname, ref });
|
||||
if (ref === SHA_A) {
|
||||
return baseContents[pathname];
|
||||
}
|
||||
if (Object.hasOwn(headContents, pathname)) {
|
||||
return headContents[pathname];
|
||||
}
|
||||
throw new Error('GitHub API request failed with HTTP 404: Not Found');
|
||||
},
|
||||
async searchCode(requirementId, directory) {
|
||||
searches.push({ requirementId, directory });
|
||||
throw new Error('code search should not run for an unchanged identity');
|
||||
},
|
||||
};
|
||||
|
||||
const result = await validator.evaluatePullRequest({ client, pullNumber: 42 });
|
||||
|
||||
assert.equal(result.status, 'covered', result.errors.join('; '));
|
||||
assert.deepEqual(searches, []);
|
||||
assert.deepEqual(
|
||||
loaded.filter(entry => entry.pathname === requirementPath),
|
||||
[
|
||||
{ pathname: requirementPath, ref: SHA_B },
|
||||
{ pathname: requirementPath, ref: SHA_A },
|
||||
],
|
||||
);
|
||||
});
|
||||
|
||||
test('a requirement rename can reuse its unchanged base identity', async () => {
|
||||
const requirementPath = 'docs/specs/ui/requirements/ui-coverage.md';
|
||||
const renamedPath = 'docs/specs/ui/requirements/renamed-coverage.md';
|
||||
const baseContents = uiFixtureContents();
|
||||
const headContents = uiFixtureContents();
|
||||
headContents[renamedPath] = headContents[requirementPath];
|
||||
delete headContents[requirementPath];
|
||||
const changed = [
|
||||
{ filename: 'apps/web/lib/runtime.ts', status: 'modified' },
|
||||
{
|
||||
filename: 'docs/plans/recovery/task-01-recovery.md',
|
||||
status: 'modified',
|
||||
additions: 1,
|
||||
changes: 1,
|
||||
},
|
||||
{
|
||||
filename: renamedPath,
|
||||
previous_filename: requirementPath,
|
||||
status: 'renamed',
|
||||
additions: 1,
|
||||
deletions: 1,
|
||||
changes: 2,
|
||||
},
|
||||
];
|
||||
const loaded = [];
|
||||
const client = {
|
||||
async getPullRequest() {
|
||||
return pullRequest(42, SHA_B, [], changed.length);
|
||||
},
|
||||
async listFiles() {
|
||||
return changed;
|
||||
},
|
||||
async getFile(pathname, ref) {
|
||||
loaded.push({ pathname, ref });
|
||||
const source = ref === SHA_A ? baseContents : headContents;
|
||||
if (Object.hasOwn(source, pathname)) {
|
||||
return source[pathname];
|
||||
}
|
||||
throw new Error('GitHub API request failed with HTTP 404: Not Found');
|
||||
},
|
||||
async searchCode() {
|
||||
throw new Error('code search should not run for an unchanged rename');
|
||||
},
|
||||
};
|
||||
|
||||
const result = await validator.evaluatePullRequest({ client, pullNumber: 42 });
|
||||
|
||||
assert.equal(result.status, 'covered', result.errors.join('; '));
|
||||
assert.deepEqual(
|
||||
loaded.filter(entry => entry.pathname === renamedPath || entry.pathname === requirementPath),
|
||||
[
|
||||
{ pathname: renamedPath, ref: SHA_B },
|
||||
{ pathname: requirementPath, ref: SHA_A },
|
||||
],
|
||||
);
|
||||
});
|
||||
|
||||
function repeatedCoverageFixture() {
|
||||
const contents = uiFixtureContents();
|
||||
const planPath = 'docs/plans/recovery/plan.md';
|
||||
@@ -899,7 +1568,13 @@ function coverageClient(contents, changed, overrides = {}) {
|
||||
return changed;
|
||||
},
|
||||
async getFile(pathname, ref) {
|
||||
assert.equal(ref, SHA_B);
|
||||
assert.ok([SHA_A, SHA_B].includes(ref));
|
||||
if (
|
||||
ref === SHA_A
|
||||
&& changed.some(change => change?.filename === pathname && change?.status === 'added')
|
||||
) {
|
||||
throw new Error('GitHub API request failed with HTTP 404: Not Found');
|
||||
}
|
||||
if (!Object.hasOwn(contents, pathname)) {
|
||||
throw new Error('GitHub API request failed with HTTP 404: Not Found');
|
||||
}
|
||||
@@ -1430,6 +2105,35 @@ test('run publishes pending and final status for the stable current pull-request
|
||||
assert.match(summaries[0], /docs\/guide\.md/);
|
||||
});
|
||||
|
||||
test('run reuses the snapshot used for the pending status', async () => {
|
||||
let metadataCalls = 0;
|
||||
const statuses = [];
|
||||
const client = {
|
||||
async getPullRequest() {
|
||||
metadataCalls += 1;
|
||||
return pullRequest(42, SHA_B);
|
||||
},
|
||||
async listFiles() {
|
||||
return [{ filename: 'docs/guide.md', status: 'modified' }];
|
||||
},
|
||||
async createCommitStatus(sha, status) {
|
||||
statuses.push({ sha, state: status.state });
|
||||
},
|
||||
};
|
||||
|
||||
const result = await validator.run({
|
||||
client,
|
||||
env: {},
|
||||
event: { pull_request: { number: 42 } },
|
||||
eventName: 'pull_request_target',
|
||||
writeSummary: () => {},
|
||||
});
|
||||
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.equal(metadataCalls, 2);
|
||||
assert.deepEqual(statuses.map(status => status.state), ['pending', 'success']);
|
||||
});
|
||||
|
||||
test('workflow dispatch reads the pull-request number from its input', async () => {
|
||||
const statuses = [];
|
||||
const client = {
|
||||
|
||||
@@ -290,11 +290,17 @@ class PullRequestSizeLabelWorkflowContractTest(unittest.TestCase):
|
||||
):
|
||||
self.assertRegex(self.workflow, rf"{name}: \{{")
|
||||
self.assertIn(f"color: '{color}'", self.workflow)
|
||||
self.assertIn("github.rest.issues.getLabel", self.workflow)
|
||||
self.assertIn("github.rest.issues.createLabel", self.workflow)
|
||||
self.assertIn("github.rest.issues.listLabelsOnIssue", self.workflow)
|
||||
self.assertIn("github.rest.issues.addLabels", self.workflow)
|
||||
self.assertIn("github.rest.issues.removeLabel", self.workflow)
|
||||
self.assertIn("isMissingLabelDefinitionError", self.workflow)
|
||||
self.assertNotIn(
|
||||
"for (const [name, definition] of Object.entries(sizeLabelDefinitions))",
|
||||
self.workflow,
|
||||
)
|
||||
self.assertEqual(self.workflow.count("github.rest.issues.listLabelsOnIssue"), 1)
|
||||
self.assertEqual(self.workflow.count("github.rest.issues.getLabel"), 0)
|
||||
|
||||
stale_labels = extract_function(self.workflow, "staleSizeLabels")
|
||||
self.assertEqual(
|
||||
@@ -333,10 +339,90 @@ class PullRequestSizeLabelWorkflowContractTest(unittest.TestCase):
|
||||
)
|
||||
)
|
||||
create_index = self.workflow.index("github.rest.issues.createLabel")
|
||||
retry_read_index = self.workflow.index(
|
||||
"await github.rest.issues.getLabel", create_index
|
||||
first_add_index = self.workflow.index("github.rest.issues.addLabels")
|
||||
retry_add_index = self.workflow.index(
|
||||
"github.rest.issues.addLabels", first_add_index + 1
|
||||
)
|
||||
list_index = self.workflow.index("github.rest.issues.listLabelsOnIssue")
|
||||
self.assertLess(list_index, first_add_index)
|
||||
self.assertLess(first_add_index, create_index)
|
||||
self.assertLess(create_index, retry_add_index)
|
||||
|
||||
missing_definition = extract_function(
|
||||
self.workflow, "isMissingLabelDefinitionError"
|
||||
)
|
||||
self.assertTrue(
|
||||
run_javascript(
|
||||
missing_definition,
|
||||
"isMissingLabelDefinitionError",
|
||||
[
|
||||
{
|
||||
"status": 422,
|
||||
"response": {
|
||||
"data": {
|
||||
"errors": [
|
||||
{
|
||||
"resource": "Label",
|
||||
"code": "invalid",
|
||||
"field": "name",
|
||||
"value": "small",
|
||||
}
|
||||
]
|
||||
}
|
||||
},
|
||||
},
|
||||
"small",
|
||||
],
|
||||
)
|
||||
)
|
||||
self.assertFalse(
|
||||
run_javascript(
|
||||
missing_definition,
|
||||
"isMissingLabelDefinitionError",
|
||||
[
|
||||
{
|
||||
"status": 422,
|
||||
"response": {
|
||||
"data": {
|
||||
"errors": [
|
||||
{
|
||||
"resource": "Label",
|
||||
"code": "invalid",
|
||||
"field": "name",
|
||||
"value": "medium",
|
||||
}
|
||||
]
|
||||
}
|
||||
},
|
||||
},
|
||||
"small",
|
||||
],
|
||||
)
|
||||
)
|
||||
self.assertFalse(
|
||||
run_javascript(
|
||||
missing_definition,
|
||||
"isMissingLabelDefinitionError",
|
||||
[
|
||||
{
|
||||
"status": 422,
|
||||
"response": {
|
||||
"data": {
|
||||
"errors": [
|
||||
{
|
||||
"resource": "Issue",
|
||||
"code": "invalid",
|
||||
"field": "name",
|
||||
"value": "small",
|
||||
}
|
||||
]
|
||||
}
|
||||
},
|
||||
},
|
||||
"small",
|
||||
],
|
||||
)
|
||||
)
|
||||
self.assertLess(create_index, retry_read_index)
|
||||
|
||||
# @covers AC-CI-PR-SIZE-001.5
|
||||
def test_summary_row_contains_two_string_cells(self) -> None:
|
||||
@@ -374,7 +460,6 @@ class PullRequestSizeLabelWorkflowContractTest(unittest.TestCase):
|
||||
limit_index = self.workflow.index("changedFiles.length >= 3000")
|
||||
self.assertIn("throw new Error", self.workflow[limit_index : limit_index + 240])
|
||||
for mutation in (
|
||||
"github.rest.issues.getLabel",
|
||||
"github.rest.issues.createLabel",
|
||||
"github.rest.issues.addLabels",
|
||||
"github.rest.issues.removeLabel",
|
||||
|
||||
@@ -4,7 +4,7 @@ name: Pull request documentation coverage
|
||||
# The checked-out validator is always from a trusted workflow/base revision.
|
||||
on:
|
||||
pull_request_target: # zizmor: ignore[dangerous-triggers] base-controlled metadata-only workflow
|
||||
types: [opened, reopened, synchronize, edited, labeled, unlabeled, ready_for_review]
|
||||
types: [opened, reopened, synchronize, edited, labeled, unlabeled]
|
||||
merge_group:
|
||||
types: [checks_requested]
|
||||
workflow_dispatch:
|
||||
@@ -36,8 +36,20 @@ jobs:
|
||||
coverage:
|
||||
name: Publish PR documentation coverage status
|
||||
if: >-
|
||||
github.event_name != 'workflow_dispatch' ||
|
||||
github.ref == format('refs/heads/{0}', github.event.repository.default_branch)
|
||||
(
|
||||
github.event_name != 'workflow_dispatch' ||
|
||||
github.ref == format('refs/heads/{0}', github.event.repository.default_branch)
|
||||
) &&
|
||||
(
|
||||
github.event_name != 'pull_request_target' ||
|
||||
(github.event.action != 'labeled' && github.event.action != 'unlabeled') ||
|
||||
github.event.label.name == 'no-docs-allow'
|
||||
) &&
|
||||
(
|
||||
github.event_name != 'pull_request_target' ||
|
||||
github.event.action != 'edited' ||
|
||||
github.event.changes.base != null
|
||||
)
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 10
|
||||
env:
|
||||
|
||||
@@ -174,6 +174,20 @@ jobs:
|
||||
);
|
||||
}
|
||||
|
||||
function isMissingLabelDefinitionError(error, targetLabel) {
|
||||
const status = error?.status ?? error?.response?.status;
|
||||
return (
|
||||
status === 422 &&
|
||||
Array.isArray(error?.response?.data?.errors) &&
|
||||
error.response.data.errors.some(detail =>
|
||||
detail?.resource === 'Label' &&
|
||||
detail?.code === 'invalid' &&
|
||||
detail?.field === 'name' &&
|
||||
detail?.value === targetLabel,
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
function summaryTableRow(countedFileCount, targetLabel) {
|
||||
return [String(countedFileCount), targetLabel];
|
||||
}
|
||||
@@ -203,39 +217,6 @@ jobs:
|
||||
).length;
|
||||
const targetLabel = classifySize(countedFileCount);
|
||||
|
||||
for (const [name, definition] of Object.entries(sizeLabelDefinitions)) {
|
||||
try {
|
||||
await github.rest.issues.getLabel({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
name,
|
||||
});
|
||||
} catch (error) {
|
||||
const status = error?.status ?? error?.response?.status;
|
||||
if (status !== 404) {
|
||||
throw error;
|
||||
}
|
||||
try {
|
||||
await github.rest.issues.createLabel({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
name,
|
||||
color: definition.color,
|
||||
description: definition.description,
|
||||
});
|
||||
} catch (createError) {
|
||||
if (!isAlreadyExistingLabelError(createError)) {
|
||||
throw createError;
|
||||
}
|
||||
await github.rest.issues.getLabel({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
name,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const currentLabels = await github.paginate(
|
||||
github.rest.issues.listLabelsOnIssue,
|
||||
{
|
||||
@@ -250,12 +231,38 @@ jobs:
|
||||
.filter(name => typeof name === 'string');
|
||||
|
||||
if (!currentLabelNames.includes(targetLabel)) {
|
||||
await github.rest.issues.addLabels({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pullNumber,
|
||||
labels: [targetLabel],
|
||||
});
|
||||
try {
|
||||
await github.rest.issues.addLabels({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pullNumber,
|
||||
labels: [targetLabel],
|
||||
});
|
||||
} catch (error) {
|
||||
if (!isMissingLabelDefinitionError(error, targetLabel)) {
|
||||
throw error;
|
||||
}
|
||||
const definition = sizeLabelDefinitions[targetLabel];
|
||||
try {
|
||||
await github.rest.issues.createLabel({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
name: targetLabel,
|
||||
color: definition.color,
|
||||
description: definition.description,
|
||||
});
|
||||
} catch (createError) {
|
||||
if (!isAlreadyExistingLabelError(createError)) {
|
||||
throw createError;
|
||||
}
|
||||
}
|
||||
await github.rest.issues.addLabels({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pullNumber,
|
||||
labels: [targetLabel],
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
for (const label of staleSizeLabels(currentLabelNames, targetLabel)) {
|
||||
|
||||
@@ -0,0 +1,137 @@
|
||||
---
|
||||
created: 2026-09-15
|
||||
status: done
|
||||
requirements:
|
||||
- REQ-CI-PR-DOCS-002
|
||||
- REQ-CI-PR-DOCS-003
|
||||
- REQ-CI-PR-DOCS-004
|
||||
- REQ-CI-PR-SIZE-001
|
||||
system_design:
|
||||
- ../../specs/ci/system-design/pull-request-documentation-coverage.md
|
||||
- ../../specs/ci/system-design/pull-request-size-labels.md
|
||||
legacy_specs: []
|
||||
---
|
||||
|
||||
# Implementation plan: GitHub API resilience
|
||||
|
||||
## Overview
|
||||
|
||||
Reduce avoidable GitHub API traffic and recover correctly when GitHub applies a
|
||||
transient or secondary rate limit. Two documentation-coverage runs failed with
|
||||
HTTP 429 while evaluating otherwise valid pull requests. Read-only replay showed
|
||||
about 14 requests for the smaller example and 33 for the larger example,
|
||||
including status publication. Per-requirement code searches and repeated pull
|
||||
request reads account for much of the avoidable traffic.
|
||||
|
||||
The CI system owns this package because it owns the trusted workflow event,
|
||||
request, mutation, and failure contracts.
|
||||
|
||||
## Scope
|
||||
|
||||
In scope: rate-aware retries and diagnostics in the custom documentation
|
||||
evaluator. The package also covers request reuse, event gates, and lazy
|
||||
size-label definition creation.
|
||||
|
||||
Out of scope: replacing `GITHUB_TOKEN`, changing documentation policy, or
|
||||
changing size rules. The package does not change merge-queue membership or
|
||||
repository rulesets. It does not include low-frequency workflow cleanup.
|
||||
|
||||
## Technical approach
|
||||
|
||||
Extend the documentation evaluator's existing bounded client instead of adding
|
||||
a second request library. Classify errors before each retry. Follow GitHub wait
|
||||
headers. Use the documented one-minute minimum for a secondary limit without a
|
||||
usable header. Stop within a fixed total wait budget. Emit sanitized diagnostics
|
||||
to the runner log.
|
||||
|
||||
Pass the wrapper's initial pull request snapshot into the evaluator. Build a
|
||||
per-snapshot requirement cache from changed exact-head documents and their base
|
||||
versions before using code search. Keep code search as the bounded fallback for
|
||||
new, moved, unresolved, and ambiguous identities.
|
||||
|
||||
Narrow documentation coverage to events that can change its inputs. Keep
|
||||
`labeled` and `unlabeled` triggers for GitHub routing, use a job-level gate for
|
||||
the exact `no-docs-allow` label, and admit `edited` only for base-branch
|
||||
retargets. Exclude title and description edits and ready-for-review changes.
|
||||
|
||||
For size labels, list current pull request labels once and optimistically apply
|
||||
the target. Create only the target definition after a narrowly recognized
|
||||
missing-label response. Existing target labels require no repository
|
||||
label-definition requests.
|
||||
|
||||
Open PR #3693 is implementation input for Task 01. Reuse its bounded client and
|
||||
diagnostic tests after rebasing. Replace its 250/500 ms no-header fallback with
|
||||
the documented 60/120 second waits and total budget.
|
||||
|
||||
## Tests
|
||||
|
||||
| Criteria | Evidence |
|
||||
| --- | --- |
|
||||
| AC-CI-PR-DOCS-003.6 through .7 | Client fixtures for transport, 408, 429, rate-limit 403, 5xx, headers, no-header 60/120 second waits, exhaustion, permanent errors, wait-budget overflow, and sanitized logs. |
|
||||
| AC-CI-PR-DOCS-004.1 through .3 | Exact-head/base fixtures count calls, reuse the initial snapshot, skip search for verified changed requirements, and retain new-ID, missing-ID, and duplicate-ID failures. |
|
||||
| AC-CI-PR-DOCS-004.4 | Workflow contract verifies the minimal event list and exact override-label job gate. |
|
||||
| AC-CI-PR-SIZE-001.5, .6, .9, .10 | Workflow contract verifies one current-label read, direct target apply, lazy target creation, concurrent creation recovery, and no definition preflight on the steady path. |
|
||||
|
||||
Use fixtures from the two failed run shapes. One fixture has four requirement
|
||||
IDs in one changed document. The other has eleven IDs in several changed
|
||||
documents. Assert request classes and cache keys. Do not use live pull request
|
||||
numbers or mutable GitHub state.
|
||||
|
||||
## End-to-end evidence
|
||||
|
||||
No application UI changes. Local evidence comes from mocked GitHub responses and
|
||||
workflow contract tests. After deployment, manually dispatch coverage for a
|
||||
covered pull request and an uncovered pull request. Exercise the override label
|
||||
once. Record retry diagnostics for any real throttle. Compare request counts
|
||||
with the pre-change 14- and 33-request replays. Do not intentionally cause live
|
||||
rate limiting.
|
||||
|
||||
## Work orders
|
||||
|
||||
- [x] [Task 01: Add rate-aware API recovery](task-01-rate-aware-api-recovery.md)
|
||||
- [x] [Task 02: Consolidate documentation API reads](task-02-consolidate-documentation-api-reads.md)
|
||||
- [x] [Task 03: Remove size-label definition preflights](task-03-remove-size-label-definition-preflights.md)
|
||||
|
||||
Execute sequentially. No subagents are authorized.
|
||||
|
||||
## Verification
|
||||
|
||||
Run the exact commands in each work order. After all tasks:
|
||||
|
||||
```bash
|
||||
node --test .github/scripts/pr-docs.test.cjs
|
||||
python3 .github/scripts/pr-docs-workflow-contract_test.py
|
||||
python3 .github/scripts/pr-size-label-workflow-contract_test.py
|
||||
python3 .github/scripts/lint-action-pinning_test.py
|
||||
python3 .github/scripts/lint-action-pinning.py
|
||||
python3 scripts/list-docs.py validate
|
||||
python3 scripts/lint-spec-files.test.py
|
||||
python3 scripts/lint-spec-files.py --all
|
||||
zizmor .github/workflows/pr-docs.yml
|
||||
zizmor .github/workflows/pr-size-label.yml
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Risks
|
||||
|
||||
- Retrying a status write after an uncertain response can create a duplicate
|
||||
status record with the same context and state.
|
||||
- A conservative secondary-limit delay consumes runner time. The 180-second
|
||||
sleep budget keeps the workflow inside its ten-minute timeout.
|
||||
- Exact-head requirement optimization relies on the trusted base catalog's
|
||||
unique-ID validation and must scan every changed requirement document in the
|
||||
referenced directory.
|
||||
- Missing-label errors must be identified narrowly so validation and permission
|
||||
failures do not create labels.
|
||||
- Label-event gating must preserve merge-group and manual-dispatch paths.
|
||||
|
||||
## References
|
||||
|
||||
- [Documentation coverage requirements](../../specs/ci/requirements/pull-request-documentation-coverage.md)
|
||||
- [Documentation coverage design](../../specs/ci/system-design/pull-request-documentation-coverage.md)
|
||||
- [Pull request size requirements](../../specs/ci/requirements/pull-request-size-labels.md)
|
||||
- [Pull request size design](../../specs/ci/system-design/pull-request-size-labels.md)
|
||||
- [Documentation coverage decision](../../decisions/2026-09-10-pr-documentation-coverage.md)
|
||||
- [GitHub REST API rate limits](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api)
|
||||
- [GitHub REST API best practices](https://docs.github.com/rest/guides/best-practices-for-integrators)
|
||||
- [Open retry implementation PR #3693](https://github.com/kdlbs/kandev/pull/3693)
|
||||
@@ -0,0 +1,109 @@
|
||||
---
|
||||
id: "01-rate-aware-api-recovery"
|
||||
title: "Add rate-aware API recovery"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-CI-PR-DOCS-003
|
||||
acceptance_criteria:
|
||||
- AC-CI-PR-DOCS-003.6
|
||||
- AC-CI-PR-DOCS-003.7
|
||||
system_design:
|
||||
- ../../specs/ci/system-design/pull-request-documentation-coverage.md
|
||||
---
|
||||
|
||||
# Task 01: Add rate-aware API recovery
|
||||
|
||||
## Summary
|
||||
|
||||
Make the custom documentation evaluator recover from transient GitHub failures
|
||||
and expose useful, sanitized failure diagnostics.
|
||||
|
||||
## In scope
|
||||
|
||||
- `.github/scripts/pr-docs.cjs`
|
||||
- `.github/scripts/pr-docs.test.cjs`
|
||||
- This work order's implementation results
|
||||
|
||||
## Out of scope
|
||||
|
||||
Request consolidation, workflow trigger changes, size-label behavior, token
|
||||
changes, and live GitHub mutations outside normal workflow status publication.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Make at most three attempts for classified transport, 408, 429, retryable
|
||||
5xx, and rate-limit 403 errors. Fail permanent client errors immediately.
|
||||
- Honor usable `Retry-After` and primary reset headers. Use 60 then 120 seconds
|
||||
for a secondary rate limit without usable guidance, with a 180-second total
|
||||
sleep budget.
|
||||
- Log bounded request class, status/category, attempt, delay, and stop reason.
|
||||
Never log credentials, response bodies, query text, or document contents.
|
||||
|
||||
## Verification
|
||||
|
||||
Run from the repository root:
|
||||
|
||||
```bash
|
||||
node --test .github/scripts/pr-docs.test.cjs
|
||||
python3 .github/scripts/pr-docs-workflow-contract_test.py
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `.github/scripts/pr-docs.cjs`
|
||||
- `.github/scripts/pr-docs.test.cjs`
|
||||
- `docs/plans/github-api-resilience/task-01-rate-aware-api-recovery.md`
|
||||
|
||||
## Dependencies
|
||||
|
||||
None. Reconcile and reuse PR #3693 rather than independently duplicating its
|
||||
client and diagnostic changes.
|
||||
|
||||
## Risks
|
||||
|
||||
Do not apply the short transport/server backoff to a secondary rate limit.
|
||||
Retry status writes only with the exact original payload.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- [Requirements](../../specs/ci/requirements/pull-request-documentation-coverage.md)
|
||||
- [System design](../../specs/ci/system-design/pull-request-documentation-coverage.md)
|
||||
- [Plan](plan.md)
|
||||
- [PR #3693](https://github.com/kdlbs/kandev/pull/3693)
|
||||
|
||||
## Results
|
||||
|
||||
Implemented the bounded GitHub API client recovery path in
|
||||
`.github/scripts/pr-docs.cjs` and covered it with focused fixtures in
|
||||
`.github/scripts/pr-docs.test.cjs`.
|
||||
|
||||
- Classified transport and timeout failures, HTTP 408, 429, retryable 5xx,
|
||||
rate-limit 403 responses, and merge-queue GraphQL `RATE_LIMITED` payloads for
|
||||
at most three attempts.
|
||||
- Shared the 180-second sleep budget across every request made by one client
|
||||
evaluation.
|
||||
- Permanent 4xx responses fail immediately when their body cannot be read or
|
||||
contains invalid JSON.
|
||||
- Honored numeric and date `Retry-After` values and primary reset headers.
|
||||
Secondary rate-limit responses without usable headers wait 60 seconds and
|
||||
then 120 seconds, within a 180-second total sleep budget.
|
||||
- Added bounded request-class diagnostics for retries, permanent failures,
|
||||
exhausted attempts, and wait-budget stops. Diagnostics contain no tokens,
|
||||
query text, response bodies, or document contents.
|
||||
|
||||
Verification:
|
||||
|
||||
- `node --test .github/scripts/pr-docs.test.cjs` (75 passed)
|
||||
- `python3 .github/scripts/pr-docs-workflow-contract_test.py` (7 passed)
|
||||
- `python3 .github/scripts/lint-action-pinning_test.py` (9 passed)
|
||||
- `python3 .github/scripts/lint-action-pinning.py` (24 workflows passed)
|
||||
- `zizmor .github/workflows/pr-docs.yml` (no findings)
|
||||
- `git diff --check` (passed)
|
||||
@@ -0,0 +1,125 @@
|
||||
---
|
||||
id: "02-consolidate-documentation-api-reads"
|
||||
title: "Consolidate documentation API reads"
|
||||
status: done
|
||||
wave: 2
|
||||
depends_on:
|
||||
- "01-rate-aware-api-recovery"
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-CI-PR-DOCS-002
|
||||
- REQ-CI-PR-DOCS-003
|
||||
- REQ-CI-PR-DOCS-004
|
||||
acceptance_criteria:
|
||||
- AC-CI-PR-DOCS-002.2
|
||||
- AC-CI-PR-DOCS-003.1
|
||||
- AC-CI-PR-DOCS-003.2
|
||||
- AC-CI-PR-DOCS-004.1
|
||||
- AC-CI-PR-DOCS-004.2
|
||||
- AC-CI-PR-DOCS-004.3
|
||||
- AC-CI-PR-DOCS-004.4
|
||||
system_design:
|
||||
- ../../specs/ci/system-design/pull-request-documentation-coverage.md
|
||||
---
|
||||
|
||||
# Task 02: Consolidate documentation API reads
|
||||
|
||||
## Summary
|
||||
|
||||
Reuse exact-head and base data, remove a duplicate pull request read, and avoid
|
||||
full evaluations for events that cannot change documentation coverage.
|
||||
|
||||
## In scope
|
||||
|
||||
- `.github/scripts/pr-docs.cjs`
|
||||
- `.github/scripts/pr-docs.test.cjs`
|
||||
- `.github/workflows/pr-docs.yml`
|
||||
- `.github/scripts/pr-docs-workflow-contract_test.py`
|
||||
- This work order's implementation results
|
||||
|
||||
## Out of scope
|
||||
|
||||
Coverage-policy exemptions, artifact frontmatter, merge-group membership,
|
||||
rulesets, tokens, and other GitHub workflows.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Reuse the pending-status pull request snapshot. Perform one later consistency
|
||||
read for a stable pull request. Read each `(revision, path)` once.
|
||||
- Resolve a verified existing requirement from changed head/base documents with
|
||||
zero code-search calls. Keep bounded fallback and duplicate detection for new,
|
||||
moved, missing, or ambiguous IDs.
|
||||
- Route opened, reopened, synchronize, base-branch retargets, exact
|
||||
`no-docs-allow` label transitions, dispatch, and merge-group checks. Skip
|
||||
title, description, readiness, and unrelated label events before checkout or
|
||||
API use.
|
||||
|
||||
## Verification
|
||||
|
||||
Run from the repository root:
|
||||
|
||||
```bash
|
||||
node --test .github/scripts/pr-docs.test.cjs
|
||||
python3 .github/scripts/pr-docs-workflow-contract_test.py
|
||||
python3 .github/scripts/lint-action-pinning_test.py
|
||||
python3 .github/scripts/lint-action-pinning.py
|
||||
zizmor .github/workflows/pr-docs.yml
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `.github/scripts/pr-docs.cjs`
|
||||
- `.github/scripts/pr-docs.test.cjs`
|
||||
- `.github/workflows/pr-docs.yml`
|
||||
- `.github/scripts/pr-docs-workflow-contract_test.py`
|
||||
- `docs/plans/github-api-resilience/task-02-consolidate-documentation-api-reads.md`
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01.
|
||||
|
||||
## Risks
|
||||
|
||||
Start a fresh cache after metadata instability and for every merge-group member.
|
||||
Scan all changed requirement documents in a referenced directory before relying
|
||||
on trusted-base uniqueness.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- [Requirements](../../specs/ci/requirements/pull-request-documentation-coverage.md)
|
||||
- [System design](../../specs/ci/system-design/pull-request-documentation-coverage.md)
|
||||
- [Plan](plan.md)
|
||||
- `.github/AGENTS.md`
|
||||
|
||||
## Results
|
||||
|
||||
Implemented the exact-revision read strategy in `.github/scripts/pr-docs.cjs`
|
||||
and the relevant event gates in `.github/workflows/pr-docs.yml`.
|
||||
|
||||
- The initial pull request snapshot used for the pending status is passed into
|
||||
evaluation, leaving one later consistency read for a stable run.
|
||||
- Content reads use a `(revision, path)` cache. Changed requirement documents
|
||||
are loaded at the exact head before search, and their corresponding base
|
||||
paths are loaded once for identity comparison.
|
||||
- Existing or renamed requirement definitions with the same requirement
|
||||
heading/ID in their trusted base path skip code search, including body or
|
||||
acceptance-text changes. New, moved without a trusted base identity, missing,
|
||||
and ambiguous identities keep the bounded search and directory fallback.
|
||||
- The workflow admits only opened, reopened, synchronize, base-branch retargets,
|
||||
exact override-label transitions, manual dispatch, and merge-group checks.
|
||||
Title and description edits, readiness changes, and unrelated label events
|
||||
are rejected at the job gate.
|
||||
|
||||
Verification:
|
||||
|
||||
- `node --test .github/scripts/pr-docs.test.cjs` (75 passed)
|
||||
- `python3 .github/scripts/pr-docs-workflow-contract_test.py` (7 passed)
|
||||
- `python3 .github/scripts/lint-action-pinning_test.py` (9 passed)
|
||||
- `python3 .github/scripts/lint-action-pinning.py` (24 workflows passed)
|
||||
- `zizmor .github/workflows/pr-docs.yml` (no findings)
|
||||
- `git diff --check` (passed)
|
||||
@@ -0,0 +1,107 @@
|
||||
---
|
||||
id: "03-remove-size-label-definition-preflights"
|
||||
title: "Remove size-label definition preflights"
|
||||
status: done
|
||||
wave: 3
|
||||
depends_on:
|
||||
- "02-consolidate-documentation-api-reads"
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-CI-PR-SIZE-001
|
||||
acceptance_criteria:
|
||||
- AC-CI-PR-SIZE-001.5
|
||||
- AC-CI-PR-SIZE-001.6
|
||||
- AC-CI-PR-SIZE-001.7
|
||||
- AC-CI-PR-SIZE-001.9
|
||||
- AC-CI-PR-SIZE-001.10
|
||||
system_design:
|
||||
- ../../specs/ci/system-design/pull-request-size-labels.md
|
||||
---
|
||||
|
||||
# Task 03: Remove size-label definition preflights
|
||||
|
||||
## Summary
|
||||
|
||||
Remove three unconditional repository label reads from each pull request size
|
||||
calculation while preserving missing-label recovery and convergence.
|
||||
|
||||
## In scope
|
||||
|
||||
- `.github/workflows/pr-size-label.yml`
|
||||
- `.github/scripts/pr-size-label-workflow-contract_test.py`
|
||||
- This work order's implementation results
|
||||
|
||||
## Out of scope
|
||||
|
||||
Size thresholds, file classification, event routing, label colors and text,
|
||||
other workflow labels, and documentation coverage.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Read current pull request labels once and perform no repository label-definition
|
||||
reads on a steady run whose target label is already present.
|
||||
- Apply an absent target directly. After a narrowly identified missing-definition
|
||||
error, create only that definition and retry the apply.
|
||||
- Preserve add-before-remove ordering, concurrent `already_exists` recovery,
|
||||
unrelated labels, least privilege, and trusted execution.
|
||||
|
||||
## Verification
|
||||
|
||||
Run from the repository root:
|
||||
|
||||
```bash
|
||||
python3 .github/scripts/pr-size-label-workflow-contract_test.py
|
||||
python3 .github/scripts/lint-action-pinning_test.py
|
||||
python3 .github/scripts/lint-action-pinning.py
|
||||
zizmor .github/workflows/pr-size-label.yml
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `.github/workflows/pr-size-label.yml`
|
||||
- `.github/scripts/pr-size-label-workflow-contract_test.py`
|
||||
- `docs/plans/github-api-resilience/task-03-remove-size-label-definition-preflights.md`
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 02. The source files are disjoint, but sequential execution keeps delivery
|
||||
and live request-count evidence in one ordered package.
|
||||
|
||||
## Risks
|
||||
|
||||
Match missing-definition responses narrowly. A permission or validation error
|
||||
must fail without attempting label creation.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- [Requirements](../../specs/ci/requirements/pull-request-size-labels.md)
|
||||
- [System design](../../specs/ci/system-design/pull-request-size-labels.md)
|
||||
- [Plan](plan.md)
|
||||
- `.github/AGENTS.md`
|
||||
|
||||
## Results
|
||||
|
||||
Implemented lazy size-label provisioning in `.github/workflows/pr-size-label.yml`
|
||||
and updated its contract tests.
|
||||
|
||||
- The workflow reads current pull request labels once and skips all repository
|
||||
label-definition reads when the target is already present.
|
||||
- An absent target is applied directly. Only a narrowly identified missing
|
||||
definition response (`Label`/`name`/`invalid` for the exact target) creates
|
||||
the selected target, then retries the same apply.
|
||||
- Concurrent `already_exists` creation is accepted before the retry. The
|
||||
existing add-before-remove order, unrelated labels, permissions, and trusted
|
||||
execution remain intact.
|
||||
|
||||
Verification:
|
||||
|
||||
- `python3 .github/scripts/pr-size-label-workflow-contract_test.py` (9 passed)
|
||||
- `python3 .github/scripts/lint-action-pinning_test.py` (9 passed)
|
||||
- `python3 .github/scripts/lint-action-pinning.py` (24 workflows passed)
|
||||
- `zizmor .github/workflows/pr-size-label.yml` (no findings)
|
||||
- `git diff --check` (passed)
|
||||
@@ -14,6 +14,11 @@ Contributors must include the delivery record for changes that need design conte
|
||||
The CI system owns the coverage check, automatic exemptions, and label override.
|
||||
Contributors can write the artifacts manually; use of the repository harness is optional.
|
||||
|
||||
## Terminology
|
||||
|
||||
- **Full evaluation:** Reading the current pull request state, changed files, and linked delivery artifacts to calculate a documentation coverage result.
|
||||
- **Transient GitHub failure:** A transport failure, HTTP 408 or 429 response, retryable server response, HTTP 403 response that GitHub identifies as rate limiting, or a merge-queue GraphQL error with type `RATE_LIMITED`.
|
||||
|
||||
## Requirements
|
||||
|
||||
### REQ-CI-PR-DOCS-001: Predictable artifact coverage
|
||||
@@ -51,6 +56,19 @@ Contributors can write the artifacts manually; use of the repository harness is
|
||||
- **AC-CI-PR-DOCS-003.3:** The check shall evaluate contributor content as data and shall not execute contributor code with repository write credentials.
|
||||
- **AC-CI-PR-DOCS-003.4:** A merge group shall pass only when every included pull request meets its own coverage policy or has its own override. One pull request's artifacts or label shall not exempt another.
|
||||
- **AC-CI-PR-DOCS-003.5:** Required-check rollout shall include evidence that ordinary PRs, label changes, forks, and merge groups report the expected result.
|
||||
- **AC-CI-PR-DOCS-003.6:** When a GitHub request has a transient failure, the evaluator shall retry a bounded number of times. It shall honor usable server wait guidance. For a secondary rate limit without usable guidance, it shall wait at least 60 seconds before retrying.
|
||||
- **AC-CI-PR-DOCS-003.7:** When a GitHub request has a permanent failure, requires an excessive wait, or exhausts its retries, the result shall be an infrastructure error. The bounded diagnostic shall identify the request class, response status, and retry outcome. It shall not expose credentials or document contents.
|
||||
|
||||
### REQ-CI-PR-DOCS-004: Request-efficient evaluation
|
||||
|
||||
**Intent:** Reduce GitHub API pressure without weakening exact-revision validation.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-CI-PR-DOCS-004.1:** Within one pull request revision or merge-group member evaluation, the system shall reuse the initial pull request snapshot and each repeated artifact lookup.
|
||||
- **AC-CI-PR-DOCS-004.2:** When a changed requirement keeps its trusted base identity, the system shall resolve it from the exact-head and base documents. It shall not make a GitHub code-search request for that requirement.
|
||||
- **AC-CI-PR-DOCS-004.3:** When a requirement is new, moved, unresolved, or ambiguous, the system shall use bounded fallback lookup and shall preserve missing-definition and duplicate-definition failures.
|
||||
- **AC-CI-PR-DOCS-004.4:** A full evaluation shall run after pull request creation, reopening, revision changes, base-branch retargets, exact exception-label transitions, manual retries, and merge-group checks. It shall not run after title or description edits, draft-readiness changes, or unrelated label changes.
|
||||
|
||||
## Out of scope
|
||||
|
||||
@@ -62,3 +80,4 @@ Contributors can write the artifacts manually; use of the repository harness is
|
||||
## Implementation plans
|
||||
|
||||
- [PR documentation coverage](../../../plans/pr-documentation-coverage/plan.md)
|
||||
- [GitHub API resilience](../../../plans/github-api-resilience/plan.md)
|
||||
|
||||
@@ -34,10 +34,11 @@ Maintainers use pull request size labels to review smaller changes first. The CI
|
||||
- **AC-CI-PR-SIZE-001.3:** When the system calculates the count, it shall include application source, test source, and web translation catalogs under `apps/`.
|
||||
- **AC-CI-PR-SIZE-001.4:** When the system calculates the count, it shall exclude documentation, workflows, scripts, generated files, dependency files, linter files, tool settings, and assets. Assets are files with known image, font, or binary extensions or files under these asset directories: `apps/backend/internal/notifications/providers/assets/`, `apps/desktop/src-tauri/icons/`, `apps/web/lib/assets/`, `apps/web/public/`, and `apps/web/src/assets/`.
|
||||
- **AC-CI-PR-SIZE-001.5:** After a successful recalculation, the pull request shall have exactly one size label. The system shall preserve all unrelated labels.
|
||||
- **AC-CI-PR-SIZE-001.6:** When any size label definition does not exist, the system shall create all missing definitions before it changes pull request labels.
|
||||
- **AC-CI-PR-SIZE-001.6:** When the selected size label definition does not exist, the system shall create that definition before it applies the label. It shall not modify an existing definition.
|
||||
- **AC-CI-PR-SIZE-001.7:** When GitHub does not return a complete changed-file list, the system shall fail without changing the pull request size labels.
|
||||
- **AC-CI-PR-SIZE-001.8:** The workflow shall use trusted base-branch code. It shall not check out or execute pull request content.
|
||||
- **AC-CI-PR-SIZE-001.9:** After concurrent recalculations complete, the size label shall match the current pull request diff.
|
||||
- **AC-CI-PR-SIZE-001.10:** When the selected size label is already present, the recalculation shall not read or write repository label definitions.
|
||||
|
||||
## Out of scope
|
||||
|
||||
@@ -46,3 +47,7 @@ Maintainers use pull request size labels to review smaller changes first. The CI
|
||||
- Automatic updates for open pull requests that receive no new pull request event after deployment.
|
||||
- Changes to review, preview, merge, or walkthrough workflows.
|
||||
- Pull request sizing for repositories other than `kdlbs/kandev`.
|
||||
|
||||
## Implementation plans
|
||||
|
||||
- [GitHub API resilience](../../../plans/github-api-resilience/plan.md)
|
||||
|
||||
@@ -5,6 +5,7 @@ requirements:
|
||||
- REQ-CI-PR-DOCS-001
|
||||
- REQ-CI-PR-DOCS-002
|
||||
- REQ-CI-PR-DOCS-003
|
||||
- REQ-CI-PR-DOCS-004
|
||||
---
|
||||
|
||||
# Pull request documentation coverage system design
|
||||
@@ -18,10 +19,11 @@ CI owns the contributor coverage policy and its trusted execution. Application s
|
||||
| REQ-CI-PR-DOCS-001 | Classification; Artifact contract |
|
||||
| REQ-CI-PR-DOCS-002 | Events and overrides |
|
||||
| REQ-CI-PR-DOCS-003 | Reporting and consistency; Merge queue; Security |
|
||||
| REQ-CI-PR-DOCS-004 | API read strategy and events and overrides |
|
||||
|
||||
## Components
|
||||
|
||||
Proposed files:
|
||||
Files:
|
||||
|
||||
- `.github/workflows/pr-docs.yml`: trusted orchestration, PR and merge-group entry points.
|
||||
- `.github/scripts/pr-docs.cjs`: pure classification and reference validation, plus a bounded GitHub API adapter.
|
||||
@@ -72,9 +74,48 @@ A handful of unrelated documentation changes does not establish the reference ch
|
||||
No PR-body schema is needed. The existing repository frontmatter supplies machine-readable links.
|
||||
Update the PR template and contributor guide with examples and the distinction between structural coverage and semantic review.
|
||||
|
||||
## API read strategy
|
||||
|
||||
The evaluator keeps a cache for one exact pull request head or one merge-group
|
||||
member. A retry after unstable pull request metadata starts a new cache. The
|
||||
stable pull request path has this request budget before transient retries:
|
||||
|
||||
| Request class | Stable evaluation budget |
|
||||
| --- | --- |
|
||||
| Pull request metadata | Two reads: the initial snapshot and the consistency read. |
|
||||
| Changed files | One request per 100-file page. |
|
||||
| File contents | At most one read for each `(revision, path)` pair. |
|
||||
| Requirement search | At most one search for each unresolved `(directory, requirement ID)` pair. |
|
||||
| Commit status | One pending write and one terminal write. |
|
||||
|
||||
The initial pull request snapshot used to choose the pending-status revision is
|
||||
also the evaluator's first snapshot. Linked work orders, plans, designs, and
|
||||
requirements reuse content already loaded at the exact head.
|
||||
|
||||
Before code search, collect every changed Markdown requirement document in each
|
||||
referenced system directory and load it at the exact head. For an existing or
|
||||
renamed document, load its corresponding base path once. A referenced ID that
|
||||
has one exact-head definition and the same requirement heading/ID in the
|
||||
corresponding trusted base document needs no code search. The trusted base
|
||||
catalog's unique-ID validation rules out an unchanged duplicate, and scanning
|
||||
all changed requirement documents detects a duplicate introduced by the pull request.
|
||||
|
||||
Use the existing code-search and directory fallback for a new, moved, absent,
|
||||
or unresolved ID. A moved ID needs fallback when it has no verified base
|
||||
identity. Search results name candidates only. Read every candidate
|
||||
at the exact head. Then use structural validation to establish the definition.
|
||||
Missing, incomplete, or ambiguous results fail closed. Document-count, response,
|
||||
and byte limits apply across the head and base reads.
|
||||
|
||||
## Events and overrides
|
||||
|
||||
Use `pull_request_target` events `opened`, `reopened`, `synchronize`, `edited`, `labeled`, `unlabeled`, and `ready_for_review` without path filters.
|
||||
Use `pull_request_target` events `opened`, `reopened`, `synchronize`, `edited`,
|
||||
`labeled`, and `unlabeled` without path filters.
|
||||
At the job boundary, admit label events only when the event label is exactly
|
||||
`no-docs-allow`. Admit an `edited` event only when its `changes.base` field is
|
||||
present, because GitHub reports base-branch retargets as edited events. Title
|
||||
and description edits and draft-readiness transitions do not change any
|
||||
evaluator input, so they do not start the job.
|
||||
Read current PR metadata and labels, not just the event snapshot. Drafts follow the same policy.
|
||||
The exact current label `no-docs-allow` returns an override success before expensive file reads.
|
||||
Failure to read current metadata or labels is still an infrastructure error.
|
||||
@@ -92,6 +133,10 @@ Use a distinct job name to avoid a status/check-name collision.
|
||||
Set pending before evaluation; publish success, failure for missing coverage, or error for incomplete data.
|
||||
The status target URL points to the run summary, which lists reasons, paths, references, and remediation.
|
||||
Also fail the workflow job on policy failure or infrastructure error. Do not post PR comments.
|
||||
On each retry and terminal request failure, write a bounded runner-log
|
||||
diagnostic. Include the request class, HTTP status or transport category,
|
||||
attempt count, and selected delay or stop reason. Do not log authorization
|
||||
headers, raw response bodies, file contents, or query text.
|
||||
|
||||
Serialize all events by the normalized target branch with `queue: max` and
|
||||
`cancel-in-progress: false`. GitHub retains up to 100 pending runs in this
|
||||
@@ -106,6 +151,24 @@ Paginate file lists and compare the count to current `changed_files`. Reject res
|
||||
Reject truncated responses, invalid encodings, oversized documents, unsupported frontmatter, symlinks, submodules, and ambiguous IDs.
|
||||
Bound artifact reads to 100 documents, 256 KiB each, and 4 MiB total. Report limits explicitly; never silently discard documents.
|
||||
|
||||
The API adapter makes at most three attempts for transient transport failures,
|
||||
HTTP 408 or 429, and retryable 5xx responses. It also retries HTTP 403 responses
|
||||
that GitHub identifies as rate limiting and successful merge-queue GraphQL
|
||||
responses with an error of type `RATE_LIMITED`. Other 4xx responses and other
|
||||
GraphQL errors fail immediately. A usable `Retry-After` value takes
|
||||
precedence. A primary limit with no remaining quota uses `X-RateLimit-Reset`.
|
||||
A secondary limit without usable guidance waits 60 seconds before the second
|
||||
attempt and 120 seconds before the third. Transport, 408, and 5xx failures use
|
||||
short exponential backoff. The 180-second sleep budget is shared across all
|
||||
requests in one client evaluation. A server wait beyond the remaining budget
|
||||
fails instead of exceeding the job timeout.
|
||||
|
||||
Each attempt has its own request timeout. A status retry uses the same revision,
|
||||
context, state, and target URL. An uncertain response can create a duplicate
|
||||
status record. It cannot change the intended result. Request retries
|
||||
do not replace the separate bounded reevaluation used when pull request metadata
|
||||
changes.
|
||||
|
||||
## Merge queue
|
||||
|
||||
Handle `merge_group: checks_requested` and publish the same status on `merge_group.head_sha`.
|
||||
@@ -148,3 +211,5 @@ Existing open PRs need a supported event or dispatch to receive their first resu
|
||||
- [GitHub workflow events](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows)
|
||||
- [GitHub required status checks](https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks)
|
||||
- [GitHub merge queue API fields](https://docs.github.com/en/graphql/reference/pulls)
|
||||
- [GitHub REST API rate limits](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api)
|
||||
- [GitHub REST API best practices](https://docs.github.com/rest/guides/best-practices-for-integrators)
|
||||
|
||||
@@ -92,7 +92,9 @@ The workflow owns three repository label definitions:
|
||||
| `medium` | `FBCA04` | Pull request changes 11-50 application files |
|
||||
| `big` | `D93F0B` | Pull request changes 51 or more application files |
|
||||
|
||||
The workflow creates a missing definition before it changes pull request labels. It does not replace the color or description of an existing label.
|
||||
The workflow creates the selected definition only when applying that label shows
|
||||
that its repository definition is missing. It does not preflight all three
|
||||
definitions and does not replace the color or description of an existing label.
|
||||
|
||||
## Control flow
|
||||
|
||||
@@ -100,8 +102,11 @@ The workflow creates a missing definition before it changes pull request labels.
|
||||
2. The workflow reads all available changed-file pages for the current pull request.
|
||||
3. The workflow stops before mutations if the changed-file result can be incomplete.
|
||||
4. The workflow filters the paths and calculates the target size label.
|
||||
5. The workflow reads the three repository label definitions and creates missing definitions. If another run created a label after the read, a 422 `already_exists` response is re-read as success; other errors fail the job.
|
||||
6. The workflow adds the target label if the pull request does not have it.
|
||||
5. The workflow reads the current pull request labels once.
|
||||
6. If the target label is absent, the workflow applies it directly. A narrowly
|
||||
identified missing-definition response creates only the target definition and
|
||||
retries the apply. A concurrent `already_exists` creation converges on the
|
||||
same retry path.
|
||||
7. The workflow removes the other size labels if the pull request has them.
|
||||
8. The workflow writes the counted-file total and final label to the step summary.
|
||||
|
||||
@@ -113,7 +118,7 @@ The GitHub files endpoint returns at most 3,000 files. A result with 3,000 files
|
||||
|
||||
A changed-file API error also fails before a label mutation. The current size labels remain unchanged in both cases.
|
||||
|
||||
A label-definition or label-mutation error fails the job. A concurrent label-definition creation that returns 422 with `already_exists` is re-read and does not fail the run. A retry reads current GitHub state and converges on one target label.
|
||||
A label-definition or label-mutation error fails the job. A concurrent label-definition creation that returns 422 with `already_exists` is accepted before the target label apply is retried. A later workflow retry reads current GitHub state and converges on one target label.
|
||||
|
||||
The workflow adds the target before it removes stale labels. A partial mutation can temporarily leave two size labels, but it does not leave the pull request unlabeled.
|
||||
|
||||
@@ -132,6 +137,8 @@ The workflow passes repository and pull request values as API parameters. It doe
|
||||
The step summary shows the pull request number, counted-file total, and selected label. API errors remain visible in the workflow result.
|
||||
|
||||
The contract test covers the event list, permission block, pagination, file filter, limits, label definitions, mutation order, and trusted execution boundary.
|
||||
It also rejects unconditional repository label-definition reads and verifies the
|
||||
missing-target recovery path.
|
||||
|
||||
## Related decisions
|
||||
|
||||
|
||||
Reference in New Issue
Block a user