diff --git a/src/github/client.ts b/src/github/client.ts index 77f4ef4d6b..677bfa4693 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -317,7 +317,20 @@ function isVolatileSingleFlightEligibleGithubUrl(url: string, headers: Headers): const path = githubApiPath(url); return ( !/^\/repos\/[^/]+\/[^/]+\/contents(?:\/|$|[?#])/.test(path) && - !/^\/repos\/[^/]+\/[^/]+\/git\/(?:trees|blobs)\//.test(path) + !/^\/repos\/[^/]+\/[^/]+\/git\/(?:trees|blobs)\//.test(path) && + // A bare single-issue read and a collaborator-permission check (#regression-safe-propagation) each gate a + // TRUST decision (linked-issue label propagation's own-merge-closed check, and its maintainer-authored-issue + // relaxation) rather than merely reducing redundant reads within one review pass, which is what this + // coalescing mechanism was built for. Sharing one in-flight promise's outcome -- success OR a transient + // failure -- across genuinely INDEPENDENT callers (a webhook re-review racing a sweep tick, or simply two + // near-simultaneous webhook deliveries for the same PR merge) means one caller's momentary fetch/rate-limit + // hiccup silently becomes every concurrent caller's answer too, not just its own -- exactly the mechanism + // that let a transient GitHub hiccup permanently strip a correct propagated label (confirmed in production: + // `sensitive`-class coalescing observed inside the exact incident window, see #regression-safe-propagation). + // Excluding these two endpoint shapes costs at most one extra GitHub call when two truly-identical reads + // genuinely overlap -- worth it for a check whose wrong answer silently corrupts gittensor scoring. + !/^\/repos\/[^/]+\/[^/]+\/issues\/\d+(?:$|[?#])/.test(path) && + !/^\/repos\/[^/]+\/[^/]+\/collaborators\/[^/]+\/permission(?:$|[?#])/.test(path) ); } @@ -436,6 +449,14 @@ async function fetchWithGitHubRetry(input: RequestInfo | URL, init?: GitHubTimeo // Retry a transient rate-limit (with backoff) instead of surfacing it; stop once exhausted or it's not a limit. const rateLimited = await isRateLimitedResponse(response); if (!rateLimited) break; + // Deliberately UNCONDITIONAL, unlike observeGitHubRestRateLimit two lines above (#regression-safe-propagation): + // an existing Grafana alert/runbook queries this exact metric BY key_scope specifically to catch a caller + // that never opted into admission tracking -- key_scope="unknown" is the diagnostic signal that surfaces + // exactly that class of bug (it's how the src/github/public.ts and src/review/rag-index.ts / grounding- + // wire.ts wiring bugs landed alongside this fix were actually found in production). Gating this on + // `admissionKey` would make a FUTURE unattributed caller's rate-limiting invisible instead of diagnosable -- + // strictly worse than a merely-imprecise "unknown" bucket. Fix the caller's wiring (as those three were), + // don't hide the symptom here. recordGitHubRateLimitResponseMetric( response.status, admissionKey, diff --git a/src/github/public.ts b/src/github/public.ts index 01d5997c7e..2f538d997c 100644 --- a/src/github/public.ts +++ b/src/github/public.ts @@ -1,4 +1,4 @@ -import { timeoutFetch } from "./client"; +import { githubRateLimitAdmissionKeyForPublicToken, timeoutFetch, type GitHubRateLimitAdmissionKey } from "./client"; export type PublicContributorProfile = { login: string; @@ -72,9 +72,22 @@ export async function fetchPublicContributorProfile(login: string, env?: Pick => - timeoutFetch(url, { headers, signal: AbortSignal.timeout(GITHUB_PUBLIC_FETCH_TIMEOUT_MS) }); + timeoutFetch(url, { + headers, + signal: AbortSignal.timeout(GITHUB_PUBLIC_FETCH_TIMEOUT_MS), + githubRateLimitAdmission: admissionKey !== undefined, + ...(admissionKey ? { githubRateLimitAdmissionKey: admissionKey } : {}), + }); const [userResponse, reposResponse] = await Promise.all([ fetchWithTimeout(`https://api.github.com/users/${safeLogin}`), fetchWithTimeout(`https://api.github.com/users/${safeLogin}/repos?per_page=100&sort=updated`), @@ -173,6 +186,9 @@ function publicRepoFullName(env: Pick, owner async function fetchRepoStatsFromGitHub(env: Pick, repoFullName: string, nowMs: number): Promise { const [owner, repo] = repoFullName.split("/") as [string, string]; + // #regression-safe-propagation: same shared-public-token attribution gap as fetchPublicContributorProfile + // above, fixed the same way -- a rate-limited response here previously fell into key_scope="unknown". + const admissionKey: GitHubRateLimitAdmissionKey | undefined = env.GITHUB_PUBLIC_TOKEN ? githubRateLimitAdmissionKeyForPublicToken() : undefined; const response = await timeoutFetch(`https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}`, { headers: { accept: "application/vnd.github+json", @@ -180,6 +196,8 @@ async function fetchRepoStatsFromGitHub(env: Pick, r "x-github-api-version": "2022-11-28", ...(env.GITHUB_PUBLIC_TOKEN ? { authorization: `Bearer ${env.GITHUB_PUBLIC_TOKEN}` } : {}), }, + githubRateLimitAdmission: admissionKey !== undefined, + ...(admissionKey ? { githubRateLimitAdmissionKey: admissionKey } : {}), }); if (!response.ok) throw new Error(`github_repo_stats_unavailable:${response.status}`); const body = (await response.json()) as GitHubPublicRepoResponse; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9253c5fca6..d0e6e9a4a3 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -9322,6 +9322,24 @@ async function maybeApplyManifestPolicyGate( } } +/** Logs + audits a deliberate type-label no-op (#regression-safe-propagation): every reason this fires means + * "labels are left exactly as they are this pass," never "labels were cleared." Shared by every reason the + * type-label block below skips a pass -- the outer typeLabelsEnabled/gittensor_only gate, a contended + * per-PR actuation lock, and an inconclusive propagation recheck -- so all of them log/audit identically + * instead of duplicating the same two calls at each skip site. */ +async function logTypeLabelSkip(env: Env, repoFullName: string, pullNumber: number, reason: string): Promise { + console.log( + JSON.stringify({ event: "type_label_decision", repoFullName, pull: pullNumber, applied: false, reason }), + ); + await recordAuditEvent(env, { + eventType: "github_app.type_label_decision", + targetKey: `${repoFullName}#${pullNumber}`, + outcome: "denied", + detail: reason, + metadata: { labels: [], source: null }, + }).catch(() => undefined); +} + async function maybePublishPrPublicSurface( env: Env, installationId: number, @@ -9572,98 +9590,126 @@ async function maybePublishPrPublicSurface( decision.skipReason !== "miner_detection_unavailable" && decision.skipReason !== "not_official_gittensor_miner" ) { - try { - // Same reasoning as `typeLabelsEnabled` above: `settings.typeLabels` is optional only for - // RepositorySettings-fixture-construction backward compat -- getRepositorySettings always - // resolves it to a concrete, complete PrTypeLabelSet (parseTypeLabelSet never returns - // undefined), so the `?? DEFAULT_TYPE_LABELS` fallback is unreachable on this webhook- - // integration path. - /* v8 ignore next -- see the comment above */ - const typeLabels = settings.typeLabels ?? DEFAULT_TYPE_LABELS; - const propagation = settings.linkedIssueLabelPropagation; - // Caller-gated (mirrors shouldCollectLinkedIssueEvidence/resolveLinkedIssueHardRule's own - // cheap-check-before-fetch precedent): zero extra GitHub calls when propagation is off, which - // is the default -- a repo that never opts in pays nothing for this feature. - const linkedIssueLabels = - propagation?.enabled && pr.linkedIssues.length > 0 - ? await fetchLinkedIssueLabelsForPropagation({ + // Per-PR mutual exclusion (#regression-safe-propagation, mirrors the agent-maintenance claim at #2129 + // below in maybeRunAgentMaintenance): a merge fans out into a BURST of near-simultaneous webhook + // deliveries for the SAME PR -- the merge event itself, the linked issue's own auto-close, and even an + // echo of THIS block's own label writes a moment earlier -- so a webhook re-review and a sweep-driven + // agent-regate-pr job (or simply two overlapping webhook deliveries) can each reach this block + // concurrently, each with its own independently-timed live linked-issue fetch. Confirmed in production: + // a correct propagation_exclusive decision, followed within 30-90s by a second concurrent pass computing + // a DIFFERENT (wrong) verdict that then overwrote the first. A losing pass must defer to the next tick, + // never compute-and-act on a stale/racing verdict for a PR another pass is actively deciding for. + const typeLabelLock = await claimPrActuationLock(env, repoFullName, pr.number); + if (!typeLabelLock.acquired) { + await logTypeLabelSkip(env, repoFullName, pr.number, "lock_contended"); + } else { + try { + // Same reasoning as `typeLabelsEnabled` above: `settings.typeLabels` is optional only for + // RepositorySettings-fixture-construction backward compat -- getRepositorySettings always + // resolves it to a concrete, complete PrTypeLabelSet (parseTypeLabelSet never returns + // undefined), so the `?? DEFAULT_TYPE_LABELS` fallback is unreachable on this webhook- + // integration path. + /* v8 ignore next -- see the comment above */ + const typeLabels = settings.typeLabels ?? DEFAULT_TYPE_LABELS; + const propagation = settings.linkedIssueLabelPropagation; + // Caller-gated (mirrors shouldCollectLinkedIssueEvidence/resolveLinkedIssueHardRule's own + // cheap-check-before-fetch precedent): zero extra GitHub calls when propagation is off, which + // is the default -- a repo that never opts in pays nothing for this feature. + const propagationResult = + propagation?.enabled && pr.linkedIssues.length > 0 + ? await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName, + linkedIssues: pr.linkedIssues, + installationId, + prAuthorLogin: pr.authorLogin, + mappings: propagation.mappings, + // #4528: lets a closed linked issue still count when THIS PR's own merge is what closed it + // (the standard "Closes #N" auto-close), instead of losing propagation authority the instant + // the merge that's supposed to earn the label also closes its evidence. + prMergedAt: pr.mergedAt ?? null, + }) + : { labels: [], inconclusive: false }; + // #regression-safe-propagation: an INCONCLUSIVE recheck (the linked issue's facts or the + // maintainer-authored-issue permission check could not be verified this pass -- a transient GitHub + // fetch/rate-limit failure, never a confirmed "no") must NEVER be treated the same as a confirmed + // absence of propagation authority. Falling through to the title heuristic here would silently + // downgrade/remove a real, previously-applied propagation label the moment ANY transient hiccup hits + // this recheck -- exactly the bug #4528 was meant to close and didn't, because that fix only ever + // covered the CONFIRMED-closed-by-this-merge case, not an unrelated fetch failure. Leave existing + // labels untouched and defer; the next tick gets a fresh, hopefully-conclusive read. + if (propagationResult.labels.length === 0 && propagationResult.inconclusive) { + await logTypeLabelSkip(env, repoFullName, pr.number, "propagation_inconclusive"); + } else { + const decisionResult = resolvePrTypeLabel({ + title: pr.title, + linkedIssueLabels: propagationResult.labels, + labels: typeLabels, + propagation, + }); + for (const label of decisionResult.applyLabels) { + await ensurePullRequestLabel( env, + installationId, repoFullName, - linkedIssues: pr.linkedIssues, + pr.number, + label, + { createMissingLabel: true, mode }, + ); + } + for (const label of decisionResult.removeLabels) { + await removePullRequestLabel( + env, installationId, - prAuthorLogin: pr.authorLogin, - mappings: propagation.mappings, - // #4528: lets a closed linked issue still count when THIS PR's own merge is what closed it - // (the standard "Closes #N" auto-close), instead of losing propagation authority the instant - // the merge that's supposed to earn the label also closes its evidence. - prMergedAt: pr.mergedAt ?? null, - }) - : []; - const decisionResult = resolvePrTypeLabel({ - title: pr.title, - linkedIssueLabels, - labels: typeLabels, - propagation, - }); - for (const label of decisionResult.applyLabels) { - await ensurePullRequestLabel( - env, - installationId, - repoFullName, - pr.number, - label, - { createMissingLabel: true, mode }, - ); - } - for (const label of decisionResult.removeLabels) { - await removePullRequestLabel( - env, - installationId, - repoFullName, - pr.number, - label, - mode, + repoFullName, + pr.number, + label, + mode, + ); + } + console.log( + JSON.stringify({ + event: "type_label_decision", + repoFullName, + pull: pr.number, + applied: true, + labels: decisionResult.applyLabels, + source: decisionResult.source, + }), + ); + await recordAuditEvent(env, { + eventType: "github_app.type_label_decision", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + // `|| "none"` is unreachable: resolvePrTypeLabel's "title" source always resolves a non-empty + // label (deriveKindFromTitle only ever returns "bug"/"feature", and parseTypeLabelSet always + // falls back a built-in category to its default rather than an empty string), and its + // propagation sources only ever use a mapping's `prLabel`, which normalizeMapping drops + // entirely when empty -- applyLabels can never be [] here. + /* v8 ignore next */ + detail: `applied labels: ${decisionResult.applyLabels.join(", ") || "none"}`, + metadata: { labels: decisionResult.applyLabels, source: decisionResult.source }, + }).catch(() => undefined); + } + } catch (error) { + console.log( + JSON.stringify({ + event: "type_label_error", + repoFullName, + pull: pr.number, + message: errorMessage(error).slice(0, 150), + }), ); + await recordAuditEvent(env, { + eventType: "github_app.type_label_decision", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "error", + detail: errorMessage(error).slice(0, 150), + metadata: { labels: [], source: null }, + }).catch(() => undefined); + } finally { + await releasePrActuationLock(env, repoFullName, pr.number, typeLabelLock.ownerToken); } - console.log( - JSON.stringify({ - event: "type_label_decision", - repoFullName, - pull: pr.number, - applied: true, - labels: decisionResult.applyLabels, - source: decisionResult.source, - }), - ); - await recordAuditEvent(env, { - eventType: "github_app.type_label_decision", - targetKey: `${repoFullName}#${pr.number}`, - outcome: "completed", - // `|| "none"` is unreachable: resolvePrTypeLabel's "title" source always resolves a non-empty - // label (deriveKindFromTitle only ever returns "bug"/"feature", and parseTypeLabelSet always - // falls back a built-in category to its default rather than an empty string), and its - // propagation sources only ever use a mapping's `prLabel`, which normalizeMapping drops - // entirely when empty -- applyLabels can never be [] here. - /* v8 ignore next */ - detail: `applied labels: ${decisionResult.applyLabels.join(", ") || "none"}`, - metadata: { labels: decisionResult.applyLabels, source: decisionResult.source }, - }).catch(() => undefined); - } catch (error) { - console.log( - JSON.stringify({ - event: "type_label_error", - repoFullName, - pull: pr.number, - message: errorMessage(error).slice(0, 150), - }), - ); - await recordAuditEvent(env, { - eventType: "github_app.type_label_decision", - targetKey: `${repoFullName}#${pr.number}`, - outcome: "error", - detail: errorMessage(error).slice(0, 150), - metadata: { labels: [], source: null }, - }).catch(() => undefined); } } else { const skipReason = settings.agentPaused @@ -9671,22 +9717,7 @@ async function maybePublishPrPublicSurface( : decision.skipReason === "miner_detection_unavailable" || decision.skipReason === "not_official_gittensor_miner" ? decision.skipReason : "typeLabelsEnabled_false"; - console.log( - JSON.stringify({ - event: "type_label_decision", - repoFullName, - pull: pr.number, - applied: false, - reason: skipReason, - }), - ); - await recordAuditEvent(env, { - eventType: "github_app.type_label_decision", - targetKey: `${repoFullName}#${pr.number}`, - outcome: "denied", - detail: skipReason, - metadata: { labels: [], source: null }, - }).catch(() => undefined); + await logTypeLabelSkip(env, repoFullName, pr.number, skipReason); } // Respect the per-repo agent pause: suppress all public surface mutations (label, comment, context diff --git a/src/review/grounding-wire.ts b/src/review/grounding-wire.ts index 79f18729d8..c68e93b8da 100644 --- a/src/review/grounding-wire.ts +++ b/src/review/grounding-wire.ts @@ -12,7 +12,7 @@ // fail-safe: any missing CI data / fetch error degrades to "no grounding" and the review proceeds on the diff. import { createInstallationToken } from "../github/app"; -import { githubRateLimitAdmissionKeyForInstallation, timeoutFetch, type GitHubRateLimitAdmissionKey } from "../github/client"; +import { githubRateLimitAdmissionKeyForToken, timeoutFetch, type GitHubRateLimitAdmissionKey } from "../github/client"; import { getCachedGroundingFileContent, putCachedGroundingFileContent, recordAuditEvent } from "../db/repositories"; import type { CheckSummaryRecord, PullRequestFileRecord } from "../types"; import { repoParts } from "../utils/json"; @@ -126,14 +126,15 @@ function toGroundingFiles(files: PullRequestFileRecord[]): PullRequestFile[] { * treats null as "skip this file" and degrades to no-grounding when nothing is readable. */ export async function makeGithubFileFetcher(env: Env, repoFullName: string, installationId: number | null | undefined): Promise { - // Resolve the token once (best-effort): installation token > public token > none. + // Resolve the token once (best-effort): installation token > public token > none. `admissionKey` is derived + // from the FINAL token (#regression-safe-propagation), after the public-token fallback is applied -- computing + // it before that fallback (against the pre-fallback `installationId`-only branch) left every fallback call + // with `admissionKey: undefined` even though the actual token used (`GITHUB_PUBLIC_TOKEN`) has a perfectly + // nameable scope, silently dropping every such call into `key_scope="unknown"` on any rate-limited response. let token: string | undefined; - let admissionKey: GitHubRateLimitAdmissionKey | undefined; - if (installationId) { - token = await createInstallationToken(env, installationId).catch(() => undefined); - admissionKey = token !== undefined ? githubRateLimitAdmissionKeyForInstallation(installationId) : undefined; - } + if (installationId) token = await createInstallationToken(env, installationId).catch(() => undefined); token = token ?? env.GITHUB_PUBLIC_TOKEN; + const admissionKey: GitHubRateLimitAdmissionKey | undefined = githubRateLimitAdmissionKeyForToken(env, token, installationId); const { owner, name } = repoParts(repoFullName); return { async getFileContent(path: string, ref: string, maxChars = 24_001): Promise { diff --git a/src/review/linked-issue-label-propagation-fetch.ts b/src/review/linked-issue-label-propagation-fetch.ts index 380c6d07c5..6167a9bdf3 100644 --- a/src/review/linked-issue-label-propagation-fetch.ts +++ b/src/review/linked-issue-label-propagation-fetch.ts @@ -2,6 +2,7 @@ import { fetchLinkedIssueFacts, type LinkedIssueFactsFetch, type LinkedIssueFact import { createInstallationToken, getRepositoryCollaboratorPermission } from "../github/app"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { parseGitHubLoginList } from "../auth/security"; +import { errorMessage } from "../utils/json"; import type { LinkedIssueLabelPropagationMapping } from "../types"; // The GitHub-fetch orchestrator for linked-issue label propagation (#priority-linked-issue-gate), kept @@ -19,25 +20,52 @@ import type { LinkedIssueLabelPropagationMapping } from "../types"; // directly, without needing to trust every call site to have gone through the capped extractor first. const MAX_LINKED_ISSUES_TO_FETCH = 50; +/** Tri-state outcome of a maintainer-permission check (#regression-safe-propagation, mirrors + * {@link LinkedIssueFactsFetch}'s found/not_found/fetch_error split for the identical reason): a live + * GitHub collaborator-permission read can come back as a CONFIRMED "maintainer" or "not_maintainer" (a + * resolved response, even a 404 -- GitHub telling us plainly this login isn't a collaborator), or it can + * fail to resolve at all (network/5xx/secondary-rate-limit) -- "inconclusive". These must never be + * conflated: a transient fetch failure is NOT evidence the login lacks maintainer permission, and treating + * it as such is exactly how a momentary GitHub hiccup used to silently and permanently downgrade a + * correct `gittensor:feature`/`gittensor:priority` label to `gittensor:bug` (confirmed in production: 837 + * unattributed + 279 attributed secondary-rate-limit 403s in a single 90-minute window, directly + * overlapping observed downgrades). */ +type MaintainerCheckResult = "maintainer" | "not_maintainer" | "inconclusive"; + /** Whether `login` holds a maintainer-equivalent permission on `repoFullName` -- the literal repo owner, * a fleet-operator in the global `ADMIN_GITHUB_LOGINS` allowlist, or a live GitHub collaborator with * admin/maintain/write access (#priority-linked-issue-gate-ownership). Mirrors * `hasMaintainerOrOwnerPermission` in `src/queue/processors.ts` (kept as its own copy here rather than * imported, since that one is private to a file this module's header comment explicitly must NOT pull - * into its import graph -- see the file-level comment above). Fail-CLOSED: a collaborator-permission - * fetch error resolves to `null` inside `getRepositoryCollaboratorPermission` itself, which this treats - * the same as "not a maintainer" -- consistent with this whole file's bias toward denying an - * unverifiable trust claim rather than granting one. */ -async function isRepoMaintainerLogin(env: Env, installationId: number, repoFullName: string, login: string): Promise { + * into its import graph -- see the file-level comment above). A CONFIRMED answer (including a 404, + * `getRepositoryCollaboratorPermission`'s real "not a collaborator" signal) resolves deterministically; + * a thrown fetch error (network/5xx/rate-limit) is INCONCLUSIVE, not "not a maintainer" -- logged here + * (this call site was previously a silent `.catch(() => null)`, invisible in production telemetry even + * during a confirmed live rate-limit storm) and left for the caller to treat as unverifiable rather than + * a confirmed negative (#regression-safe-propagation). */ +async function isRepoMaintainerLogin(env: Env, installationId: number, repoFullName: string, login: string): Promise { // The ": \"\"" fallback is unreachable via the real webhook path: repoFullName is always the // "owner/repo"-formatted payload.repository.full_name, and the surrounding pipeline already requires a // repository match on that exact format before this function's caller runs (mirrors the identical // pattern + rationale in `hasMaintainerOrOwnerPermission`, `src/queue/processors.ts`). /* v8 ignore next */ const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")).toLowerCase() : ""; - if (login === repoOwner || parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(login)) return true; - const permission = await getRepositoryCollaboratorPermission(env, installationId, repoFullName, login).catch(() => null); - return permission != null && new Set(["admin", "maintain", "write"]).has(permission); + if (login === repoOwner || parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(login)) return "maintainer"; + let permission: Awaited>; + try { + permission = await getRepositoryCollaboratorPermission(env, installationId, repoFullName, login); + } catch (error) { + console.log( + JSON.stringify({ + event: "repo_maintainer_check_failed", + repoFullName, + login, + message: errorMessage(error).slice(0, 150), + }), + ); + return "inconclusive"; + } + return permission != null && new Set(["admin", "maintain", "write"]).has(permission) ? "maintainer" : "not_maintainer"; } /** True when the linked issue's authority for propagation can be trusted (#4528): it's still OPEN, or it @@ -53,6 +81,22 @@ function isLinkedIssueTrustworthy(facts: LinkedIssueFactsResult, prMergedAt: str return prMergedAt !== null && facts.closedAt !== null && facts.closedAt >= prMergedAt; } +/** {@link resolveIssueLabelsForPropagation}'s and {@link fetchLinkedIssueLabelsForPropagation}'s return shape + * (#regression-safe-propagation). `labels` is exactly what today's plain `string[]` used to be. `inconclusive` + * is the NEW signal: true when this issue's (or, aggregated, ANY linked issue's) propagation evidence could + * not be verified this pass -- a `fetch_error` reading the issue itself, or an errored (not merely negative) + * maintainer-permission check -- as opposed to a confirmed, deterministic "no" (issue genuinely not + * trustworthy right now, genuinely not authored/assigned to the PR author, genuinely not maintainer-owned). + * The caller (`src/queue/processors.ts`'s type-label block) must treat `inconclusive` + empty `labels` as + * "could not recheck this pass" and leave existing labels untouched, never as "propagation confirmed absent" -- + * that conflation is the exact mechanism that let a transient GitHub hiccup permanently strip a correct + * propagated label (#4528's fix closed the narrower "issue closed by this PR's own merge" race but never + * this one). */ +export type LinkedIssuePropagationLabels = { + labels: string[]; + inconclusive: boolean; +}; + /** Per-issue label resolution for {@link fetchLinkedIssueLabelsForPropagation}: a direct PR-author-is- * issue-author-or-assignee match unlocks EVERY label the issue carries (today's original behavior, * unchanged). Failing that, a mapping explicitly opted into `trustMaintainerAuthoredIssue` OR @@ -68,24 +112,52 @@ function isLinkedIssueTrustworthy(facts: LinkedIssueFactsResult, prMergedAt: str * maintainer-permission check (and its GitHub API call) entirely -- byte-identical to the pre-fix * behavior for any caller that hasn't opted in. Logs once per issue when the returned set is smaller * than what the issue actually carries, so a future "why didn't my PR inherit the label" report is - * diagnosable from structured logs instead of a source read. */ + * diagnosable from structured logs instead of a source read. + * + * Returns `inconclusive: true` (#regression-safe-propagation) ONLY for a genuinely unverifiable pass -- + * the issue fetch itself failed (`fetch_error`), or the maintainer-permission check errored -- never for a + * confirmed negative (issue not trustworthy, no ownership match, a resolved "not a collaborator" 404). A + * confirmed `not_found` (a proven-nonexistent issue) is likewise NOT inconclusive -- that is real, + * deterministic evidence, not a hiccup. */ async function resolveIssueLabelsForPropagation( args: { env: Env; repoFullName: string; installationId: number }, result: LinkedIssueFactsFetch, prAuthorLogin: string | undefined, relaxableLabels: ReadonlySet, prMergedAt: string | null, -): Promise { - if (result.status !== "found" || !isLinkedIssueTrustworthy(result.facts, prMergedAt) || !prAuthorLogin) return []; +): Promise { + if (result.status === "fetch_error") { + console.log( + JSON.stringify({ + event: "linked_issue_label_propagation_inconclusive", + repoFullName: args.repoFullName, + reason: "issue_fetch_error", + }), + ); + return { labels: [], inconclusive: true }; + } + if (result.status !== "found" || !isLinkedIssueTrustworthy(result.facts, prMergedAt) || !prAuthorLogin) return { labels: [], inconclusive: false }; const allLabels = result.facts.labels; const issueAuthorLogin = result.facts.authorLogin?.toLowerCase(); const assignees = result.facts.assignees.map((login) => login.toLowerCase()); - if (issueAuthorLogin === prAuthorLogin || assignees.includes(prAuthorLogin)) return allLabels; + if (issueAuthorLogin === prAuthorLogin || assignees.includes(prAuthorLogin)) return { labels: allLabels, inconclusive: false }; - const maintainerAuthored = - relaxableLabels.size > 0 && - !!issueAuthorLogin && - (await isRepoMaintainerLogin(args.env, args.installationId, args.repoFullName, issueAuthorLogin)); + const maintainerCheck: MaintainerCheckResult = + relaxableLabels.size > 0 && !!issueAuthorLogin + ? await isRepoMaintainerLogin(args.env, args.installationId, args.repoFullName, issueAuthorLogin) + : "not_maintainer"; + if (maintainerCheck === "inconclusive") { + console.log( + JSON.stringify({ + event: "linked_issue_label_propagation_inconclusive", + repoFullName: args.repoFullName, + issueNumber: result.facts.number, + reason: "maintainer_check_error", + }), + ); + return { labels: [], inconclusive: true }; + } + const maintainerAuthored = maintainerCheck === "maintainer"; const kept = maintainerAuthored ? allLabels.filter((label) => relaxableLabels.has(label.toLowerCase())) : []; if (kept.length < allLabels.length && allLabels.length > 0) { @@ -99,7 +171,7 @@ async function resolveIssueLabelsForPropagation( }), ); } - return kept; + return { labels: kept, inconclusive: false }; } /** FETCH every linked issue's labels (fail-open) and flatten into one label list for @@ -107,7 +179,7 @@ async function resolveIssueLabelsForPropagation( * closed no earlier than THIS PR's own merge (#4528, {@link isLinkedIssueTrustworthy}), can contribute * labels; closing-keyword text in a PR body is author-controlled and is not authority by itself. Mirrors * `resolveLinkedIssueHardRule`'s own fetch idiom (`src/review/linked-issue-hard-rules.ts`): a per-issue - * fetch failure contributes no labels rather than throwing, so if EVERY linked issue fails, the result is + * fetch failure contributes no labels rather than throwing, so if EVERY linked issue fails, `labels` is * `[]` — which can never match a mapping, meaning a sensitive label like `gittensor:priority` never applies * when its authority (the linked issue) cannot be verified. The bare `Promise.all` below is safe without a * per-item `.catch` because `fetchLinkedIssueFacts` (`src/github/backfill.ts`) never throws for a network, @@ -125,7 +197,14 @@ async function resolveIssueLabelsForPropagation( * either flag) reproduces today's strict author-or-assignee-only behavior exactly. * * `prMergedAt` (#4528) is this PR's own `merged_at`, or `null` while unmerged -- the caller's `pr.mergedAt` - * straight from the DB row, no extra fetch. */ + * straight from the DB row, no extra fetch. + * + * Returns {@link LinkedIssuePropagationLabels} (#regression-safe-propagation), NOT a bare `string[]`: + * `inconclusive` is true when ANY linked issue's resolution was inconclusive (fetch failure or an errored + * maintainer-permission check), aggregated across every linked issue with a plain OR -- deliberately + * coarse. A caller only needs to distinguish "confirmed: no propagation applies" from "could not fully + * verify this pass" when `labels` came back empty; when even one linked issue resolved with real labels, + * those labels are just as trustworthy as before regardless of a sibling issue's fetch trouble. */ export async function fetchLinkedIssueLabelsForPropagation(args: { env: Env; repoFullName: string; @@ -134,8 +213,8 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { prAuthorLogin: string | null | undefined; mappings?: readonly LinkedIssueLabelPropagationMapping[] | undefined; prMergedAt?: string | null | undefined; -}): Promise { - if (args.linkedIssues.length === 0) return []; +}): Promise { + if (args.linkedIssues.length === 0) return { labels: [], inconclusive: false }; const linkedIssues = args.linkedIssues.slice(0, MAX_LINKED_ISSUES_TO_FETCH); const token = (await createInstallationToken(args.env, args.installationId).catch( @@ -175,7 +254,7 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { ), ), ); - const perIssueLabels = await Promise.all( + const perIssueResults = await Promise.all( results.map((result) => resolveIssueLabelsForPropagation( { env: args.env, repoFullName: args.repoFullName, installationId: args.installationId }, @@ -186,5 +265,8 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { ), ), ); - return perIssueLabels.flat(); + return { + labels: perIssueResults.flatMap((result) => result.labels), + inconclusive: perIssueResults.some((result) => result.inconclusive), + }; } diff --git a/src/review/rag-index.ts b/src/review/rag-index.ts index d845f2e9c1..fc24a48464 100644 --- a/src/review/rag-index.ts +++ b/src/review/rag-index.ts @@ -26,7 +26,7 @@ // GitHub call, and does no adapter use — the deploy is byte-identical to today. import { createInstallationToken } from "../github/app"; -import { githubRateLimitAdmissionKeyForInstallation, timeoutFetch, type GitHubRateLimitAdmissionKey } from "../github/client"; +import { githubRateLimitAdmissionKeyForToken, timeoutFetch, type GitHubRateLimitAdmissionKey } from "../github/client"; import { incr } from "../selfhost/metrics"; import { isConfigFile, isDependencyManifestFile } from "../signals/path-matchers"; import { repoParts } from "../utils/json"; @@ -76,13 +76,17 @@ const UPSERT_BATCH = 50; const GITHUB_FETCH_TIMEOUT_MS = 10_000; /** Resolve the read token once for a repo: installation token (private-repo read) → public token → none. - * Best-effort — a token failure degrades to the next fallback, never throws. (Mirrors makeGithubFileFetcher.) */ + * Best-effort — a token failure degrades to the next fallback, never throws. (Mirrors makeGithubFileFetcher.) + * `admissionKey` is derived from the FINAL token (#regression-safe-propagation), not computed before the + * public-token fallback is applied -- deriving it early left every call on the fallback path (installation + * token absent or its mint failed) with `admissionKey: undefined` even though the actual token used + * (`GITHUB_PUBLIC_TOKEN`) has a perfectly nameable scope, silently dropping every such call into + * `key_scope="unknown"` on any rate-limited response instead of the real "public" key_scope bucket. */ async function resolveReadToken(env: Env, installationId: number | null | undefined): Promise<{ token: string | undefined; admissionKey?: GitHubRateLimitAdmissionKey | undefined }> { - if (installationId) { - const token = await createInstallationToken(env, installationId).catch(() => undefined); - if (token) return { token, admissionKey: githubRateLimitAdmissionKeyForInstallation(installationId) }; - } - return { token: env.GITHUB_PUBLIC_TOKEN }; + const token = installationId + ? ((await createInstallationToken(env, installationId).catch(() => undefined)) ?? env.GITHUB_PUBLIC_TOKEN) + : env.GITHUB_PUBLIC_TOKEN; + return { token, admissionKey: githubRateLimitAdmissionKeyForToken(env, token, installationId) }; } /** Shared GitHub headers for the read calls (raw media type returns file bodies directly). */ diff --git a/test/integration/api.test.ts b/test/integration/api.test.ts index 7433921b96..ba5466f9b5 100644 --- a/test/integration/api.test.ts +++ b/test/integration/api.test.ts @@ -155,6 +155,24 @@ describe("api routes", () => { expect(calls).toHaveLength(1); }); + it("REGRESSION (#regression-safe-propagation): serves public GitHub repo stats when GITHUB_PUBLIC_TOKEN is unset, still unauthenticated (self-host operators can run without it)", async () => { + const app = createApp(); + // No GITHUB_PUBLIC_TOKEN at all -- exercises fetchRepoStatsFromGitHub's admission-key "token absent" branch, + // distinct from the "token present" branch every other stats test in this file exercises. + const env = createTestEnv({}); + const calls: Array<{ url: string; authorization: string | null }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const authorization = new Headers(init?.headers).get("authorization"); + calls.push({ url: input.toString(), authorization }); + return Response.json({ full_name: "acme/anon", html_url: "https://github.com/acme/anon", stargazers_count: 2, forks_count: 0 }); + }); + + const response = await app.request("/v1/public/github/repos/acme/anon/stats", {}, env); + expect(response.status).toBe(200); + await expect(response.json()).resolves.toMatchObject({ repoFullName: "acme/anon", stargazers_count: 2, source: "github" }); + expect(calls).toEqual([{ url: "https://api.github.com/repos/acme/anon", authorization: null }]); + }); + it("serves public GitHub repo stats for any repo when PUBLIC_REPO_STATS_ALLOWLIST is unset (#4612)", async () => { const app = createApp(); // No PUBLIC_REPO_STATS_ALLOWLIST override: a self-hoster's own fork must work without code changes. diff --git a/test/unit/adapters.test.ts b/test/unit/adapters.test.ts index e246f96b5e..0d7b512f89 100644 --- a/test/unit/adapters.test.ts +++ b/test/unit/adapters.test.ts @@ -1,11 +1,13 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { normalizeGittBountySnapshot } from "../../src/bounties/ingest"; import { fetchPublicContributorProfile } from "../../src/github/public"; +import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { jsonString, normalizeRepoFullName, parseJson, repoParts } from "../../src/utils/json"; describe("small adapters and normalizers", () => { afterEach(() => { vi.unstubAllGlobals(); + resetMetrics(); }); it("keeps JSON helpers predictable on missing and malformed values", () => { @@ -192,4 +194,19 @@ describe("small adapters and normalizers", () => { await fetchPublicContributorProfile("dev"); expect(authHeaders).toEqual([null, null]); }); + + it("REGRESSION (#regression-safe-propagation): attributes a rate-limited public-profile lookup to key_scope=\"public-token\", not an unattributed \"unknown\" bucket", async () => { + vi.stubGlobal("fetch", async () => + Response.json({ message: "API rate limit exceeded" }, { status: 403, headers: { "retry-after": "0", "x-ratelimit-remaining": "0" } }), + ); + // Before the fix, this loop (the 500-login evidence batch's per-login profile lookup, the single highest- + // volume unattributed GitHub caller found in a fleet-wide audit) never opted into rate-limit admission at + // all, so every hit here silently fell into key_scope="unknown" -- indistinguishable from a genuinely + // mis-keyed opt-in, which is exactly what buried a confirmed secondary-rate-limit storm in production + // (invisible via structured logs, only found through this metric). + await fetchPublicContributorProfile("dev", { GITHUB_PUBLIC_TOKEN: "public-token" }); + const metrics = await renderMetrics(); + expect(metrics).toContain('gittensory_github_rest_rate_limit_responses_total{key_scope="public"'); + expect(metrics).not.toContain('gittensory_github_rest_rate_limit_responses_total{key_scope="unknown"'); + }); }); diff --git a/test/unit/github-client.test.ts b/test/unit/github-client.test.ts index 38dfa771ca..1b40ba2d27 100644 --- a/test/unit/github-client.test.ts +++ b/test/unit/github-client.test.ts @@ -1012,6 +1012,69 @@ describe("timeoutFetch", () => { expect(await secondResponse.text()).toBe("body-2"); }); + it("REGRESSION (#regression-safe-propagation): does not volatile-single-flight a bare linked-issue read, so two concurrent trust rechecks for the same issue never share one outcome", async () => { + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + const fetchId = (getFetches += 1); + return Response.json({ number: 42, state: fetchId === 1 ? "closed" : "open" }); + }); + + const url = "https://api.github.com/repos/o/r/issues/42"; + const first = timeoutFetch(url); + const second = timeoutFetch(url); + await Promise.resolve(); + // Two genuinely independent concurrent callers (e.g. a webhook re-review racing a sweep tick) must each get + // their OWN live read of the issue -- never one caller's snapshot (or a transient failure) silently reused + // as the other's answer, which is exactly how a momentary hiccup used to permanently strip a correctly + // propagated label. + expect(getFetches).toBe(2); + const [firstResponse, secondResponse] = await Promise.all([first, second]); + expect(firstResponse.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBeNull(); + expect(secondResponse.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBeNull(); + expect(await firstResponse.json()).toMatchObject({ state: "closed" }); + expect(await secondResponse.json()).toMatchObject({ state: "open" }); + }); + + it("REGRESSION (#regression-safe-propagation): does not volatile-single-flight a collaborator-permission check, so a transient error on one caller's read never becomes another concurrent caller's answer", async () => { + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + const fetchId = (getFetches += 1); + // The first concurrent caller hits a transient error; the second must NOT inherit that outcome. + if (fetchId === 1) return new Response("server error", { status: 500 }); + return Response.json({ permission: "write" }); + }); + + const url = "https://api.github.com/repos/o/r/collaborators/rando/permission"; + const first = timeoutFetch(url); + const second = timeoutFetch(url); + await Promise.resolve(); + expect(getFetches).toBe(2); + const [firstResponse, secondResponse] = await Promise.all([first, second]); + expect(firstResponse.status).toBe(500); + expect(secondResponse.status).toBe(200); + }); + + it("REGRESSION (#regression-safe-propagation): the new exclusion regexes are precise -- a bare collaborators list (no /permission) and an issue's comments sub-resource are still volatile-single-flighted", async () => { + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + const fetchId = (getFetches += 1); + return Response.json([{ id: fetchId }]); + }); + + // Neither URL matches the new /collaborators/{login}/permission or /issues/{n} exclusions -- both should + // still coalesce onto one in-flight request, same as before this fix. + const collaboratorsList = "https://api.github.com/repos/o/r/collaborators"; + const [first, second] = await Promise.all([timeoutFetch(collaboratorsList), timeoutFetch(collaboratorsList)]); + expect(getFetches).toBe(1); + expect(await first.json()).toEqual(await second.json()); + + getFetches = 0; + const issueComments = "https://api.github.com/repos/o/r/issues/42/comments"; + const [thirdCall, fourthCall] = await Promise.all([timeoutFetch(issueComments), timeoutFetch(issueComments)]); + expect(getFetches).toBe(1); + expect(await thirdCall.json()).toEqual(await fourthCall.json()); + }); + it("keeps volatile single-flight scoped by exact authorization identity", async () => { let releaseFetch!: () => void; const fetchGate = new Promise((resolve) => { diff --git a/test/unit/grounding-wiring.test.ts b/test/unit/grounding-wiring.test.ts index ad7b88e35e..3429bfc986 100644 --- a/test/unit/grounding-wiring.test.ts +++ b/test/unit/grounding-wiring.test.ts @@ -11,7 +11,11 @@ import { import { getCachedGroundingFileContent, putCachedGroundingFileContent, upsertCheckSummary, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import * as repositoriesModule from "../../src/db/repositories"; import * as githubApp from "../../src/github/app"; -import { githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation } from "../../src/github/client"; +import { + githubRateLimitAdmissionKeyForInstallation, + githubRateLimitAdmissionKeyForPublicToken, + latestGitHubRestRateLimitObservation, +} from "../../src/github/client"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import type { Advisory, CheckSummaryRecord, JsonValue, PullRequestFileRecord, RepositorySettings } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; @@ -727,6 +731,40 @@ describe("makeGithubFileFetcher (GitHub Contents-API-backed FileFetcher)", () => fetchSpy.mockRestore(); }); + it("REGRESSION (#regression-safe-propagation): attributes the public-token fallback (installation-token mint failed) to key_scope=\"public-token\", not a dropped/undefined admission key", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "ghp_public" }); + const key = githubRateLimitAdmissionKeyForPublicToken(); + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-06-24T12:00:00.000Z")); + const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async () => { + return new Response("body", { + status: 200, + headers: { + "x-ratelimit-resource": "core", + "x-ratelimit-remaining": "17", + "x-ratelimit-reset": String(Date.parse("2026-06-24T12:10:00.000Z") / 1000), + }, + }); + }); + + try { + // No GitHub App key is configured, so createInstallationToken rejects and the fetcher falls back to the + // public token -- before the fix, `admissionKey` was computed ONLY on the installationId branch (before + // the fallback reassigns `token`), so this exact path left admissionKey undefined despite the request + // actually authenticating with a perfectly nameable token. + const fetcher = await makeGithubFileFetcher(env, "acme/widgets", 12345); + expect(await fetcher.getFileContent("ok.ts", "sha7")).toBe("body"); + expect(latestGitHubRestRateLimitObservation(key)).toEqual({ + remaining: 17, + resetAt: "2026-06-24T12:10:00.000Z", + observedAtMs: Date.parse("2026-06-24T12:00:00.000Z"), + }); + } finally { + fetchSpy.mockRestore(); + vi.useRealTimers(); + } + }); + it("uses installation-token contents reads and records admission telemetry when token mint succeeds", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "ghp_public" }); const key = githubRateLimitAdmissionKeyForInstallation(12345); diff --git a/test/unit/linked-issue-label-propagation-fetch.test.ts b/test/unit/linked-issue-label-propagation-fetch.test.ts index 2c95f308c6..4bbf02a121 100644 --- a/test/unit/linked-issue-label-propagation-fetch.test.ts +++ b/test/unit/linked-issue-label-propagation-fetch.test.ts @@ -2,7 +2,10 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createTestEnv } from "../helpers/d1"; import * as appModule from "../../src/github/app"; import { clearInstallationTokenCacheForTest } from "../../src/github/app"; -import { fetchLinkedIssueLabelsForPropagation } from "../../src/review/linked-issue-label-propagation-fetch"; +import { + fetchLinkedIssueLabelsForPropagation, + type LinkedIssuePropagationLabels, +} from "../../src/review/linked-issue-label-propagation-fetch"; // `getRepositoryCollaboratorPermission` mints its own installation token internally with no fallback to // the public token, so a maintainer-authored-issue test that reaches it (i.e. isn't already short-circuited @@ -28,6 +31,13 @@ async function generatePrivateKeyPem(): Promise { return `${PEM_HEADER}\n${base64}\n${PEM_FOOTER}`; } +// #regression-safe-propagation: `fetchLinkedIssueLabelsForPropagation` returns `{labels, inconclusive}`, not a +// bare `string[]` -- `inconclusive` defaults false (a confirmed result) in every assertion below except the +// one test that simulates a genuinely unverifiable pass (a collaborator-permission check that errors). +function expectPropagation(result: LinkedIssuePropagationLabels, labels: string[], inconclusive = false): void { + expect(result).toEqual({ labels, inconclusive }); +} + describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", () => { afterEach(() => { vi.unstubAllGlobals(); @@ -56,7 +66,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "contrib", }); - expect(result).toEqual([]); + expectPropagation(result, []); expect(fetchSpy).not.toHaveBeenCalled(); }); @@ -81,10 +91,10 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "contrib", }); - expect(result).toEqual(["gittensor:priority", "help wanted"]); + expectPropagation(result, ["gittensor:priority", "help wanted"]); }); - it("surfaces only the successful issue's labels when one of several linked issues fails to fetch (partial fail-open)", async () => { + it("surfaces only the successful issue's labels when one of several linked issues fails to fetch, and flags the result inconclusive (#regression-safe-propagation)", async () => { stubFetch((url) => { if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); @@ -107,10 +117,13 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "contrib", }); - expect(result).toEqual(["gittensor:priority"]); + // A confirmed match from issue #1 is still trustworthy on its own merits -- but the sibling issue #2's + // fetch genuinely failed, so the aggregate is inconclusive too (the caller only acts on that flag when + // `labels` is ALSO empty, so a real match here is unaffected either way). + expectPropagation(result, ["gittensor:priority"], true); }); - it("returns [] when every linked issue fails to fetch (fully fail-open — never applies a sensitive label without a verified source)", async () => { + it("returns [] and flags inconclusive when every linked issue fails to fetch (#regression-safe-propagation: never a confirmed absence)", async () => { stubFetch((url) => { if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); @@ -124,7 +137,51 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "contrib", }); - expect(result).toEqual([]); + expectPropagation(result, [], true); + }); + + it("REGRESSION (#regression-safe-propagation): a CONFIRMED-nonexistent linked issue (404 with a proven installation token) is not inconclusive, unlike a genuine fetch error", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + // The linked issue itself 404s -- with a real installation token (proven access), this is CONFIRMED + // absence (GitHub telling us plainly this issue number doesn't exist), not a transient hiccup. + return new Response("not found", { status: 404 }); + }); + // A real signable key is required for createInstallationToken to actually mint (not silently fall back to + // undefined via its own .catch) -- without it, `token` would be undefined here too, and hasProvenAccess in + // fetchLinkedIssueFacts would be false, masking exactly the not_found/fetch_error distinction under test. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [999], + installationId: 123, + prAuthorLogin: "contrib", + }); + expectPropagation(result, [], false); + }); + + it("REGRESSION (#regression-safe-propagation): a confirmed-negative issue and an inconclusive issue aggregate to inconclusive, exercising the mixed (not just all-true/all-false) case", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/1")) + // Confirmed negative: found, open, but neither authored nor assigned to the PR author, no relaxable + // mapping configured -- a real, deterministic "no", never inconclusive. + return Response.json({ number: 1, state: "open", user: { login: "maintainer" }, assignees: [], labels: ["gittensor:priority"] }); + if (url.endsWith("/issues/2")) return new Response("server error", { status: 500 }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [1, 2], + installationId: 123, + prAuthorLogin: "contrib", + }); + // Issue #1 alone would resolve confirmed-empty (inconclusive: false); issue #2's fetch failure must still + // flip the AGGREGATE to inconclusive via .some(), proving the OR isn't accidentally an AND or a first-wins. + expectPropagation(result, [], true); }); it("falls back to the public token and still fails open (never throws) when the installation-token mint fails", async () => { @@ -149,7 +206,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "contrib", }); - expect(result).toEqual(["gittensor:priority"]); + expectPropagation(result, ["gittensor:priority"]); spy.mockRestore(); }); @@ -179,7 +236,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", }); expect(issueFetchCount).toBe(50); - expect(result).toEqual(Array(50).fill("gittensor:priority")); + expectPropagation(result, Array(50).fill("gittensor:priority")); }); it("ignores a priority label on an open linked issue when the PR author neither opened nor is assigned to it", async () => { @@ -204,7 +261,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "attacker", }); - expect(result).toEqual([]); + expectPropagation(result, []); }); it("ignores a priority label on a closed linked issue, even when the PR author is tied to it", async () => { @@ -228,7 +285,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "contrib", }); - expect(result).toEqual([]); + expectPropagation(result, []); }); describe("closed-by-own-merge trust (#4528 — merging a PR auto-closes its linked issue)", () => { @@ -254,7 +311,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", prMergedAt: "2026-07-09T22:15:13Z", }); - expect(result).toEqual(["gittensor:feature", "gittensor:priority"]); + expectPropagation(result, ["gittensor:feature", "gittensor:priority"]); }); it("does NOT propagate when the linked issue was already closed BEFORE this PR merged (anti-gaming: an unrelated, already-resolved issue can't be borrowed)", async () => { @@ -279,7 +336,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", prMergedAt: "2026-07-09T22:15:13Z", }); - expect(result).toEqual([]); + expectPropagation(result, []); }); it("does not propagate a closed issue missing closed_at even when prMergedAt is present (defensive: no provable closing-time relationship)", async () => { @@ -298,7 +355,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", prMergedAt: "2026-07-09T22:15:13Z", }); - expect(result).toEqual([]); + expectPropagation(result, []); }); }); @@ -323,7 +380,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: null, }); - expect(result).toEqual([]); + expectPropagation(result, []); }); it("propagates labels when the PR author is assigned to the open linked issue", async () => { @@ -348,7 +405,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "Contrib", }); - expect(result).toEqual(["gittensor:priority"]); + expectPropagation(result, ["gittensor:priority"]); }); describe("maintainer-authored-issue trust (#priority-linked-issue-gate-ownership)", () => { @@ -379,7 +436,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings: RELAXABLE_MAPPINGS, }); - expect(result).toEqual(["gittensor:feature"]); + expectPropagation(result, ["gittensor:feature"]); }); it("propagates a relaxable label from an issue authored by an ADMIN_GITHUB_LOGINS fleet-operator (not the literal repo owner)", async () => { @@ -398,7 +455,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings: RELAXABLE_MAPPINGS, }); - expect(result).toEqual(["gittensor:feature"]); + expectPropagation(result, ["gittensor:feature"]); }); it("propagates a relaxable label from an issue authored by a live write-collaborator (not the owner, not in the admin allowlist)", async () => { @@ -418,7 +475,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings: RELAXABLE_MAPPINGS, }); - expect(result).toEqual(["gittensor:feature"]); + expectPropagation(result, ["gittensor:feature"]); }); it("does not propagate a relaxable label when the issue author is a live collaborator with only read access", async () => { @@ -438,10 +495,10 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings: RELAXABLE_MAPPINGS, }); - expect(result).toEqual([]); + expectPropagation(result, []); }); - it("does not propagate a relaxable label when the collaborator-permission check errors (fails closed)", async () => { + it("does not propagate a relaxable label when the collaborator-permission check errors, and flags the result inconclusive rather than a confirmed absence (#regression-safe-propagation)", async () => { stubFetch((url) => { if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.endsWith("/issues/14")) @@ -458,7 +515,10 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings: RELAXABLE_MAPPINGS, }); - expect(result).toEqual([]); + // Before #regression-safe-propagation this and a confirmed "read-only collaborator" negative (the test + // above) were indistinguishable ([] either way) -- exactly the conflation that let a transient GitHub + // hiccup permanently strip a correct propagated label. Now the caller can tell them apart. + expectPropagation(result, [], true); }); it("does not propagate a relaxable label when the linked issue has no author (deleted/ghost account)", async () => { @@ -476,7 +536,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings: RELAXABLE_MAPPINGS, }); - expect(result).toEqual([]); + expectPropagation(result, []); }); it("does not propagate anything via maintainer-authored trust when no mapping opts in, even for the literal repo owner's own issue (byte-identical default)", async () => { @@ -496,7 +556,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "contrib", }); - expect(result).toEqual([]); + expectPropagation(result, []); // No mapping opted in, so relaxableLabels is empty and the collaborator-permission check must never fire. expect(fetchSpy.mock.calls.some(([input]) => input.toString().includes("/collaborators/"))).toBe(false); }); @@ -522,7 +582,8 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings, }); - expect(result.sort()).toEqual(["gittensor:bug", "gittensor:priority"]); + expect(result.labels.sort()).toEqual(["gittensor:bug", "gittensor:priority"]); + expect(result.inconclusive).toBe(false); }); it("does NOT propagate the reward label from a maintainer-authored issue when its mapping has not opted into trustMaintainerAuthoredIssueForReward (unchanged strict default)", async () => { @@ -545,7 +606,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings, }); - expect(result).toEqual(["gittensor:bug"]); + expectPropagation(result, ["gittensor:bug"]); }); it("REGRESSION: mixed-trust duplicate issue labels do not relax strict mappings through a shared label name", async () => { @@ -568,7 +629,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings, }); - expect(result).toEqual([]); + expectPropagation(result, []); }); it("still propagates the reward label via trustMaintainerAuthoredIssueForReward when the issue is authored by an ADMIN_GITHUB_LOGINS fleet-operator", async () => { @@ -588,7 +649,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings, }); - expect(result).toEqual(["gittensor:priority"]); + expectPropagation(result, ["gittensor:priority"]); }); it("does not propagate the reward label when the issue author is only a read-access collaborator (fails closed, same as trustMaintainerAuthoredIssue)", async () => { @@ -609,7 +670,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings, }); - expect(result).toEqual([]); + expectPropagation(result, []); }); }); @@ -629,7 +690,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( prAuthorLogin: "contrib", mappings: [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: true }], }); - expect(result).toEqual([]); + expectPropagation(result, []); }); }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index f5654cae01..e7d10f18f9 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -28668,7 +28668,12 @@ describe("queue processors", () => { expect(seen.removed).toEqual(["gittensor:bug"]); }); - it("fails open to the normal title-based label when the linked issue's fetch fails (#priority-linked-issue-gate)", async () => { + it("REGRESSION (#regression-safe-propagation, was: 'fails open to the normal title-based label'): skips the label decision entirely — never falls back to title — when the linked issue's fetch fails, leaving existing labels untouched", async () => { + // Before the fix, a fetch failure here fell through to the title guess and OVERWROTE whatever labels + // were already correct — the exact mechanism (an inconclusive recheck treated as a confirmed absence + // of propagation authority) that let a transient GitHub hiccup permanently strip a correctly propagated + // gittensor:feature/gittensor:priority label down to gittensor:bug (confirmed in production, PRs + // #4716/#4783 and 116 others in a 2-day sample). A fetch failure must now be a no-op, not a downgrade. const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); await upsertRepositorySettings(env, { @@ -28703,8 +28708,115 @@ describe("queue processors", () => { }); expect(seen.issueFetches).toBe(1); - expect(seen.posted).toEqual(["gittensor:bug"]); - expect(seen.removed.sort()).toEqual(["gittensor:feature", "gittensor:priority"]); + expect(seen.posted).toEqual([]); + expect(seen.removed).toEqual([]); + const events = await env.DB.prepare( + `select outcome, detail from audit_events where event_type = 'github_app.type_label_decision' and target_key = 'JSONbored/gittensory#221'`, + ).all(); + expect(events.results).toEqual([{ outcome: "denied", detail: "propagation_inconclusive" }]); + }); + + it("REGRESSION (#regression-safe-propagation): a second pass whose propagation recheck is inconclusive never clobbers a first pass's already-correct propagated labels", async () => { + // Reproduces the exact PR #4716/#4783 shape end-to-end: an EARLIER pass correctly propagates + // gittensor:feature/gittensor:priority from the linked issue, then a LATER pass (a webhook re-review, a + // sweep tick, or simply a second near-simultaneous delivery for the same merge) re-runs the same + // decision but this time the linked issue's fetch fails transiently. The later pass must leave the + // correct labels exactly as the first pass left them. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "widget", full_name: "acme/widget", private: false, owner: { login: "acme" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "acme/widget", + commentMode: "off", + publicSurface: "label_only", + autoLabelEnabled: true, + createMissingLabel: false, + checkRunMode: "off", + gateCheckMode: "enabled", reviewCheckMode: "required", + linkedIssueGateMode: "off", + aiReviewMode: "off", + linkedIssueLabelPropagation: { + enabled: true, + mode: "exclusive_type_label", + mappings: [{ issueLabel: "gittensor:feature", prLabel: "gittensor:feature", removeOtherTypeLabels: true }], + }, + }); + const seen = { posted: [] as string[], removed: [] as string[], issueFetches: 0 }; + let issueShouldFail = false; + stubPropagationFetch(4716, 2216, seen, () => + issueShouldFail + ? new Response("server error", { status: 500 }) + : Response.json({ number: 2216, state: "open", user: { login: "contributor" }, labels: ["gittensor:feature"] }), + ); + // Each pass uses a DIFFERENT action/head SHA so the second is a genuinely fresh re-evaluation, not a + // same-head no-op the surface-publish guard would short-circuit before ever reaching the label block. + const webhookPayload = (action: "opened" | "synchronize", headSha: string) => ({ + action, + installation: { id: 123, account: { login: "acme", id: 1, type: "User" as const } }, + repository: { name: "widget", full_name: "acme/widget", private: false, owner: { login: "acme" } }, + pull_request: { number: 4716, title: "fix: some bug", state: "open", user: { login: "contributor" }, author_association: "NONE" as const, head: { sha: headSha }, labels: [], body: "Closes #2216" }, + }); + + await processJob(env, { type: "github-webhook", deliveryId: "pass-1-correct", eventName: "pull_request", payload: webhookPayload("opened", "sha4716a") }); + expect(seen.posted).toEqual(["gittensor:feature"]); + expect(seen.removed.sort()).toEqual(["gittensor:bug", "gittensor:priority"]); + + issueShouldFail = true; + await processJob(env, { type: "github-webhook", deliveryId: "pass-2-inconclusive", eventName: "pull_request", payload: webhookPayload("synchronize", "sha4716b") }); + // No FURTHER posts/removes happened in pass 2 -- the correct labels from pass 1 are exactly as they were. + expect(seen.posted).toEqual(["gittensor:feature"]); + expect(seen.removed.sort()).toEqual(["gittensor:bug", "gittensor:priority"]); + }); + + it("REGRESSION (#regression-safe-propagation): a contended per-PR actuation lock skips the label decision entirely instead of racing the pass that already holds it", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "label_only", + autoLabelEnabled: true, + createMissingLabel: false, + checkRunMode: "off", + gateCheckMode: "enabled", reviewCheckMode: "required", + linkedIssueGateMode: "off", + aiReviewMode: "off", + linkedIssueLabelPropagation: { + enabled: true, + mode: "exclusive_type_label", + mappings: [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: true }], + }, + }); + const seen = { posted: [] as string[], removed: [] as string[], issueFetches: 0 }; + stubPropagationFetch(223, 1, seen, () => Response.json({ number: 1, state: "open", user: { login: "contributor" }, labels: ["gittensor:priority"] })); + + // Simulates a concurrent pass (a sibling webhook delivery, or the sweep) already holding this exact + // PR's actuation lock when this pass reaches the type-label block. + const held = await claimPrActuationLock(env, "JSONbored/gittensory", 223); + expect(held.acquired).toBe(true); + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "priority-propagation-lock-contended", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 223, title: "fix: some bug", state: "open", user: { login: "contributor" }, author_association: "NONE", head: { sha: "sha223" }, labels: [], body: "Fixes #1" }, + }, + }); + } finally { + await releasePrActuationLock(env, "JSONbored/gittensory", 223, held.ownerToken); + } + + // The fetch never even reaches the linked-issue check -- the lock is claimed BEFORE any propagation work. + expect(seen.issueFetches).toBe(0); + expect(seen.posted).toEqual([]); + expect(seen.removed).toEqual([]); + const events = await env.DB.prepare( + `select outcome, detail from audit_events where event_type = 'github_app.type_label_decision' and target_key = 'JSONbored/gittensory#223'`, + ).all(); + expect(events.results).toEqual([{ outcome: "denied", detail: "lock_contended" }]); }); it("never fetches a linked issue and keeps normal behavior when propagation is left at its default (disabled) (#priority-linked-issue-gate)", async () => { diff --git a/test/unit/rag-index.test.ts b/test/unit/rag-index.test.ts index 62690f69a5..cb4b1e7894 100644 --- a/test/unit/rag-index.test.ts +++ b/test/unit/rag-index.test.ts @@ -5,7 +5,11 @@ import { processJob, splitRepoForRag } from "../../src/queue/processors"; import { upsertRepositoryFromGitHub } from "../../src/db/repositories"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import * as githubApp from "../../src/github/app"; -import { githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation } from "../../src/github/client"; +import { + githubRateLimitAdmissionKeyForInstallation, + githubRateLimitAdmissionKeyForPublicToken, + latestGitHubRestRateLimitObservation, +} from "../../src/github/client"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { createTestEnv, TestD1Database } from "../helpers/d1"; @@ -244,6 +248,41 @@ describe("indexRepo: full repo index (tree → chunk → embed → upsert)", () } }); + it("REGRESSION (#regression-safe-propagation): attributes the public-token fallback read to key_scope=\"public-token\", not an undefined/unknown admission key", async () => { + const { env } = indexEnv(); + env.GITHUB_PUBLIC_TOKEN = "public-token-value"; + const key = githubRateLimitAdmissionKeyForPublicToken(); + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-06-24T12:00:00.000Z")); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const headers = { + "x-ratelimit-resource": "core", + "x-ratelimit-remaining": "44", + "x-ratelimit-reset": String(Date.parse("2026-06-24T12:10:00.000Z") / 1000), + }; + const url = String(input); + if (url.includes("/git/trees/")) return Response.json({ tree: [{ type: "blob", path: "src/a.ts", size: 20 }] }, { headers }); + if (url.includes("/contents/src/a.ts")) return new Response("export const a = 1;\n", { status: 200, headers }); + return new Response("missing", { status: 404, headers }); + }); + + try { + // REPO's own installationId is null -- resolveReadToken falls through to the public-token read. + await indexRepo(env, PROJECT, REPO); + // Before the fix, this admission-key path was left `undefined` on the public-token fallback (only the + // installation-token branch computed a key), so this observation was silently dropped instead of + // recorded under "public-token" -- indistinguishable from an unattributed "unknown" call on any + // rate-limited response. + expect(latestGitHubRestRateLimitObservation(key)).toEqual({ + remaining: 44, + resetAt: "2026-06-24T12:10:00.000Z", + observedAtMs: Date.parse("2026-06-24T12:00:00.000Z"), + }); + } finally { + vi.useRealTimers(); + } + }); + it("a storage error while listing stored paths is fail-safe (prunes nothing, still indexes) + surfaces it at ERROR for Sentry (#5)", async () => { const { env } = indexEnv(); const errSpy = vi.spyOn(console, "error").mockImplementation(() => {});