fix(review): never let an inconclusive linked-issue recheck downgrade a propagated label (#4816)

* fix(review): never let an inconclusive linked-issue recheck downgrade a propagated label

A merge-time (or later sweep/webhook) recheck of a PR's linked-issue label
propagation could not distinguish "the recheck genuinely couldn't be
verified this pass" (a transient GitHub fetch/rate-limit failure) from "the
issue confirms no propagation applies" -- both fell through to the blunt
title heuristic and silently overwrote whatever correct label a prior pass
had already applied. Confirmed live: 118 PRs mislabeled across gittensory
and metagraphed in a 2-day sample, including contributor-reported PRs #4716
and #4783.

- linked-issue-label-propagation-fetch.ts: fetchLinkedIssueLabelsForPropagation
  now returns {labels, inconclusive} instead of a bare label list;
  isRepoMaintainerLogin distinguishes a confirmed permission result from an
  errored (inconclusive) one, and logs the error instead of swallowing it.
- processors.ts: the type-label block skips the mutation entirely (leaving
  existing labels untouched) when the recheck was inconclusive rather than
  confirmed-negative, and now claims the existing per-PR actuation lock so
  two concurrent passes for the same PR can no longer race each other here.
- client.ts: excludes the two trust-deciding endpoints (linked-issue reads,
  collaborator-permission checks) from cross-caller request coalescing, so
  one caller's transient failure can no longer become a different
  concurrent caller's answer.
- public.ts / rag-index.ts / grounding-wire.ts: three GitHub callers were
  computing their rate-limit admission key before a token fallback was
  applied (or never attributing one at all for a high-volume caller),
  which is what buried the live incident in an unattributed metric bucket.

Full regression coverage added, including an end-to-end reproduction of
the exact PR #4716/#4783 race in queue.test.ts.

* fix(review): follow the reviewCheckMode migration in the new type-label tests

Rebased onto main's gateCheckMode-deprecation migration (#4618); the two new
regression tests added alongside it still need reviewCheckMode set directly,
matching every sibling test in the same describe block.

Author: JSONbored

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,

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<En
     // loop doesn't exhaust it and silently degrade (mirrors fetchPublicRepoStats) (#790).
     ...(env?.GITHUB_PUBLIC_TOKEN ? { authorization: `Bearer ${env.GITHUB_PUBLIC_TOKEN}` } : {}),
   };
+  // #regression-safe-propagation: this is the single highest-volume unattributed GitHub caller found in a
+  // fleet-wide audit -- the 500-login evidence batch (processors.ts) calls this once per login, each call
+  // issuing up to 6 raw GETs, all on the SAME shared env.GITHUB_PUBLIC_TOKEN -- up to ~3000 requests/batch with
+  // no rate-limit admission tracking at all, so a 403 anywhere in that batch silently fell into
+  // `key_scope="unknown"` instead of the real "public" key_scope bucket (confirmed live: 837 scheduled + 279
+  // exhausted secondary-rate-limit 403s in 90 minutes in production). Opting in costs nothing (no extra call)
+  // and makes this loop's real impact on the shared public-token rate-limit bucket finally visible.
+  const admissionKey: GitHubRateLimitAdmissionKey | undefined = env?.GITHUB_PUBLIC_TOKEN ? githubRateLimitAdmissionKeyForPublicToken() : undefined;
   try {
     const fetchWithTimeout = (url: string): Promise<Response> =>
-      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,13 +186,18 @@ function publicRepoFullName(env: Pick<Env, "PUBLIC_REPO_STATS_ALLOWLIST">, owner
 
 async function fetchRepoStatsFromGitHub(env: Pick<Env, "GITHUB_PUBLIC_TOKEN">, repoFullName: string, nowMs: number): Promise<PublicRepoStats> {
   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",
       "user-agent": "gittensory/0.1",
       "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;

src/queue/processors.ts

@@ -9378,6 +9378,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<void> {
+  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,
@@ -9628,121 +9646,134 @@ 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
       ? "agent_paused"
       : 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