From 1f7cb870bb79e79a8dcf8536dd518597024af2b2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:25:29 -0700 Subject: [PATCH 1/5] fix(review): reconcile decision-ledger content against record_json (#9078) verifyDecisionLedger walked decision_ledger's hash chain but never joined decision_records to recompute contentDigest(JSON.parse(record_json)) against the digest each row committed to -- the reconciliation its own doc comment promised. A rewrite of record_json (or a hand-deletion of the row) passed verification clean as long as the chain itself stayed self-consistent. Adds two distinct break kinds: content_mismatch (record_json no longer hashes to what the chain committed to) and missing_record (the chain references a decision_records id that no longer exists). Both batch-fetch against the ledger window already being verified, so this stays a single extra query rather than N+1. Also stops persistDecisionRecord from conflating "the record failed to persist at all" with "the record persisted but its ledger append failed" -- the latter is now its own dedicated console.error alarm (forwarded to Sentry like other operator-facing anomaly signals) instead of falling through to the generic swallowed warn. --- src/review/decision-record.ts | 81 ++++++++++++++++++++++++---- test/unit/decision-record.test.ts | 88 ++++++++++++++++++++++++++++++- 2 files changed, 158 insertions(+), 11 deletions(-) diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index f408f12e4..78bb9bf36 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -193,16 +193,36 @@ export async function persistDecisionRecord(env: Env, record: DecisionRecord, re ) .bind(id, record.repoFullName.slice(0, 200), record.pullNumber, record.headSha, record.action, record.reasonCode.slice(0, 200), recordDigest, canonicalJson(record), record.decidedAt) .run(); - // #8837: every write appends a chain row — including a supersession's OWN new row, so re-decisions - // are visible history rather than silent replacement. - await appendDecisionLedger(env, id, recordDigest); - return id; } catch (error) { if (attempt >= attempts) throw error; // A concurrent supersession at the exact same (repo, pull, head) raced the count-then-insert above and // collided on the PK — re-count and retry with the next revision id (mirrors appendDecisionLedger's // own PK-collision retry for the ledger tip immediately above). + continue; } + // The record row landed. #9078: a failure chaining it must NOT be conflated with the persist failure + // handled below (nothing landed at all) — the record already exists, so this is its own distinct + // failure mode ("an unchained record", per this module's header) and deserves its own dedicated, + // non-swallowed alarm rather than the same generic console.warn every other persist failure gets. + // verifyDecisionLedger's `missing_record`/`short_tail` reconciliation is what catches this after the + // fact; this alarm is what makes it visible the moment it happens, instead of only on the next verify. + try { + // #8837: every write appends a chain row — including a supersession's OWN new row, so re-decisions + // are visible history rather than silent replacement. + await appendDecisionLedger(env, id, recordDigest); + } catch (error) { + console.error( + JSON.stringify({ + level: "error", + event: "decision_ledger_append_failed", + target: `${record.repoFullName}#${record.pullNumber}`, + recordId: id, + message: errorMessage(error).slice(0, 160), + at: nowIso(), + }), + ); + } + return id; } } catch (error) { console.warn(JSON.stringify({ event: "decision_record_persist_error", target: `${record.repoFullName}#${record.pullNumber}`, message: errorMessage(error).slice(0, 160) })); @@ -334,7 +354,18 @@ export type LedgerBreak = | { kind: "row_hash_mismatch"; atSeq: number } // #9122: a self-consistent chain that stops short of every decision_records row it should account for — see // the reconciliation check at the end of verifyDecisionLedger below for exactly what this catches and why. - | { kind: "short_tail"; atSeq: number }; + | { kind: "short_tail"; atSeq: number } + // #9078: the ledger's OWN committed digest for this row disagrees with a digest freshly recomputed from the + // record's current record_json — content tampering (or an operational bug) that rewrote decision_records + // without touching the chain. Comparing against the ledger's row.recordDigest — not the equally-tamperable + // decision_records.record_digest column — is what makes this a real check: a forger would have to also + // recompute every row_hash after this one to make a tampered record_json look consistent, which + // row_hash_mismatch would already catch. + | { kind: "content_mismatch"; atSeq: number; recordId: string } + // #9078: a ledger row commits to a decision_records id that no longer has a row at all — the one preimage + // the chain vouched for is simply gone (a direct-DB deletion, or some other operation none of the + // gap/predecessor/hash checks above can see, since those only ever compare ledger rows against each other). + | { kind: "missing_record"; atSeq: number; recordId: string }; /** #9122: the exact shape a scheduled external-anchoring job (git-commit checkpoint, transparency log, or an * on-chain commitment — the actual publishing mechanism is a genuinely open infra/protocol decision tracked @@ -348,11 +379,16 @@ export function buildLedgerAnchorPayload(tip: { seq: number; rowHash: string }, /** * Verify a window of the chain, resumable via `afterSeq` (0 = genesis). Reports the FIRST break with its - * class — a gap, a broken predecessor link, a rewritten row, or a short tail (see below) — and the cursor for - * the next window. Always returns the CURRENT global tip (`tipSeq`/`tipHash`) and total row count, regardless - * of where this window's pagination stopped, so a third-party checkpoint-keeper can compare it against - * whatever tip it last observed (#9122 — the exact shape a future external-anchoring job would need). Pure - * read; safe on a public route (hashes and ids only, no record contents). + * class — a gap, a broken predecessor link, a rewritten row, a short tail, a content mismatch, or a missing + * record (see below) — and the cursor for the next window. Always returns the CURRENT global tip + * (`tipSeq`/`tipHash`) and total row count, regardless of where this window's pagination stopped, so a + * third-party checkpoint-keeper can compare it against whatever tip it last observed (#9122 — the exact shape + * a future external-anchoring job would need). #9078: also reconciles each row against `decision_records` — + * recomputing `contentDigest(JSON.parse(record_json))` and comparing it to the digest the chain itself + * committed to, so a rewrite of `record_json` that left `decision_records.record_digest` untouched (or vice + * versa) is caught here instead of only being provable by an external challenger who happens to still have the + * original preimage. Read-only; safe on a public route — it reads `record_json` to recompute a digest but + * never RETURNS record contents, only the break kind, sequence, and (public, already-exposed) record id. */ export async function verifyDecisionLedger( env: Env, @@ -379,6 +415,18 @@ export async function verifyDecisionLedger( ) .bind(afterSeq, bounded) .all<{ seq: number; recordId: string; recordDigest: string; prevHash: string; rowHash: string; createdAt: string }>(); + // #9078: batch-fetch every decision_records row this window's chain rows reference, ONE query rather than + // one per row, so the content reconciliation below is a map lookup instead of an N+1 query per verify call. + const recordIds = [...new Set(results.map((row) => row.recordId))]; + const recordsById = new Map(); + if (recordIds.length > 0) { + const { results: recordRows } = await env.DB.prepare( + `SELECT id, record_digest AS recordDigest, record_json AS recordJson FROM decision_records WHERE id IN (${recordIds.map(() => "?").join(",")})`, + ) + .bind(...recordIds) + .all<{ id: string; recordDigest: string; recordJson: string }>(); + for (const recordRow of recordRows) recordsById.set(recordRow.id, { recordDigest: recordRow.recordDigest, recordJson: recordRow.recordJson }); + } let checked = 0; // Tracks the created_at of the last row this call actually verified clean — the anchor the tail-truncation // reconciliation below compares decision_records against. Seeded from `prior` (the checkpoint we resumed @@ -389,6 +437,19 @@ export async function verifyDecisionLedger( if (row.prevHash !== prevHash) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "predecessor_mismatch", atSeq: row.seq } }; const recomputed = await ledgerRowHash(prevHash, { seq: row.seq, recordId: row.recordId, recordDigest: row.recordDigest, createdAt: row.createdAt }); if (recomputed !== row.rowHash) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "row_hash_mismatch", atSeq: row.seq } }; + // #9078: the promised record/ledger reconciliation — a row_hash chained cleanly can still commit to a + // digest whose CONTENT has since been rewritten (or whose preimage is simply gone). Neither is visible to + // the chain-only checks above, since those only ever compare ledger rows against each other. + const storedRecord = recordsById.get(row.recordId); + if (!storedRecord) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "missing_record", atSeq: row.seq, recordId: row.recordId } }; + let recomputedContentDigest: string | null = null; + try { + recomputedContentDigest = await contentDigest(JSON.parse(storedRecord.recordJson)); + } catch { + // Unparseable record_json is itself proof the content no longer matches whatever the chain committed to. + recomputedContentDigest = null; + } + if (recomputedContentDigest !== row.recordDigest) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "content_mismatch", atSeq: row.seq, recordId: row.recordId } }; prevHash = row.rowHash; lastVerifiedCreatedAt = row.createdAt; expectedSeq = row.seq + 1; diff --git a/test/unit/decision-record.test.ts b/test/unit/decision-record.test.ts index c45bdc454..d8e42666d 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -136,6 +136,38 @@ describe("buildDecisionRecord / persistDecisionRecord", () => { expect([idOne, idTwo, idThree]).toEqual([`record:o/r#7@abc1234def`, `record:o/r#7@abc1234def:rev2`, `record:o/r#7@abc1234def:rev3`]); }); + it("#9078: a ledger-append failure after a successful record insert raises a dedicated alarm (not a swallowed warn) and still returns the id", async () => { + const env = createTestEnv(); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const realPrepare = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => { + if (sql.includes("INSERT INTO decision_ledger")) { + return { + bind: () => ({ + run: async () => { + throw new Error("ledger down"); + }, + }), + } as never; + } + return realPrepare(sql); + }); + const { record, recordDigest } = await buildDecisionRecord(recordInput()); + const id = await persistDecisionRecord(env, record, recordDigest); + // The record row itself still landed -- an append failure must not be conflated with a persist failure. + expect(id).toBe(`record:o/r#7@abc1234def`); + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("decision_ledger_append_failed")); + expect(warnSpy).not.toHaveBeenCalled(); + const row = await env.DB.prepare("SELECT id FROM decision_records WHERE id = ?").bind(id).first(); + expect(row).toBeTruthy(); + const ledgerRow = await env.DB.prepare("SELECT seq FROM decision_ledger").first(); + // Unchained -- exactly the state verifyDecisionLedger's `missing_record` reconciliation now catches. + expect(ledgerRow ?? null).toBeNull(); + vi.restoreAllMocks(); + }); + it("a persist failure is swallowed (legibility must never break finalization) and resolves null", async () => { const env = createTestEnv(); const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); @@ -417,7 +449,61 @@ describe("decision ledger (#8837)", () => { it("a concurrent append races on the PK and retries with a re-read predecessor — both rows land, chain intact", async () => { const env = createTestEnv(); - await Promise.all([appendDecisionLedger(env, "record:a", "1".repeat(64)), appendDecisionLedger(env, "record:b", "2".repeat(64))]); + // #9078: verifyDecisionLedger now reconciles every chain row against a real decision_records preimage, so + // this test (which is really about the ledger's own PK-collision race, not persistDecisionRecord's insert + // path) needs real backing rows instead of the bare synthetic digests it used before that reconciliation + // existed. + const a = await buildDecisionRecord(recordInput({ pullNumber: 201 })); + const b = await buildDecisionRecord(recordInput({ pullNumber: 202 })); + for (const [id, built] of [ + ["record:a", a], + ["record:b", b], + ] as const) { + await env.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + ) + .bind(id, built.record.repoFullName, built.record.pullNumber, built.record.headSha, built.record.action, built.record.reasonCode, built.recordDigest, canonicalJson(built.record), built.record.decidedAt) + .run(); + } + await Promise.all([appendDecisionLedger(env, "record:a", a.recordDigest), appendDecisionLedger(env, "record:b", b.recordDigest)]); + const verified = await verifyDecisionLedger(env); + expect(verified).toMatchObject({ ok: true, checked: 2 }); + }); + + it("#9078: a rewritten record_json is caught as a content mismatch even when the record_digest column is untouched", async () => { + const env = createTestEnv(); + await persist(env, 1); + await env.DB.prepare("UPDATE decision_records SET record_json = ? WHERE pull_number = 1") + .bind(JSON.stringify({ tampered: true })) + .run(); + const verified = await verifyDecisionLedger(env); + expect(verified.ok).toBe(false); + expect(verified.break).toMatchObject({ kind: "content_mismatch", atSeq: 1, recordId: "record:o/r#1@abc1234def" }); + }); + + it("#9078: unreadable record_json is treated as a content mismatch rather than throwing", async () => { + const env = createTestEnv(); + await persist(env, 1); + await env.DB.prepare("UPDATE decision_records SET record_json = '{not json' WHERE pull_number = 1").run(); + const verified = await verifyDecisionLedger(env); + expect(verified.ok).toBe(false); + expect(verified.break).toMatchObject({ kind: "content_mismatch" }); + }); + + it("#9078: a ledger row whose decision_records preimage was deleted outright is reported as a missing record, distinct from a content mismatch", async () => { + const env = createTestEnv(); + await persist(env, 1); + await env.DB.prepare("DELETE FROM decision_records WHERE pull_number = 1").run(); + const verified = await verifyDecisionLedger(env); + expect(verified.ok).toBe(false); + expect(verified.break).toMatchObject({ kind: "missing_record", recordId: "record:o/r#1@abc1234def" }); + }); + + it("#9078: an untampered chain still verifies clean through the new content reconciliation", async () => { + const env = createTestEnv(); + await persist(env, 1); + await persist(env, 2); const verified = await verifyDecisionLedger(env); expect(verified).toMatchObject({ ok: true, checked: 2 }); }); From 60f5b5c9b9860e74ed6df0cda0841c4aa7be8635 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:44:56 -0700 Subject: [PATCH 2/5] fix(gittensor): match miner identity on the immutable githubId, not login (#9079) fetchOfficialGittensorMiner matched a PR/issue author against the upstream /miners list purely by lowercased githubUsername. A GitHub login is renameable and a freed one is reclaimable by anyone, so if the upstream list still carries a miner's old username (its refresh cadence is upstream's, not ours), whoever claims that freed login inherits confirmed_miner -- a genuinely elevated role (command self-retrigger, the miner-scoped open-PR cap, the unlinked-issue-guardrail velocity exception). fetchOfficialGittensorMiner/fetchGittensorContributorSnapshot now accept an optional githubId and match on it FIRST when the caller has one, never falling back to a username match once an id was supplied. Threaded through getCachedOfficialMinerDetection's callers via PullRequestRecord/ IssueRecord.authorGithubId (captured from the webhook's user.id since #9125) and unlinked-issue-guardrail's/miner-detection-cache's own isConfirmedOfficialMiner. Callers with no stored id yet keep the prior login-only behavior unchanged (a strict narrowing, never a widening, of the old always-username match). Left the bulk fetchOfficialGittensorMinerLogins cohort-classification read and a couple of advisory-only lookups (reputation-wire's install-wide reputation widening, the MCP/decision-pack snapshot tools that only ever receive a login as their input) on login-only matching -- none of them grant an elevated role, and re-scoping every caller's own input contract was out of scope for this fix. --- src/gittensor/api.ts | 28 ++++++++++++++--- src/gittensor/miner-detection-cache.ts | 8 +++-- src/queue/processors.ts | 42 ++++++++++++++++++-------- src/review/unlinked-issue-guardrail.ts | 14 ++++++--- test/unit/gittensor-api.test.ts | 37 +++++++++++++++++++++++ 5 files changed, 106 insertions(+), 23 deletions(-) diff --git a/src/gittensor/api.ts b/src/gittensor/api.ts index 14fc80ccb..ebe338499 100644 --- a/src/gittensor/api.ts +++ b/src/gittensor/api.ts @@ -163,9 +163,9 @@ export type GittensorContributorSnapshot = { issueLabels: string[]; }; -export async function fetchGittensorContributorSnapshot(login: string): Promise { +export async function fetchGittensorContributorSnapshot(login: string, githubId?: number | string | null): Promise { try { - const detection = await fetchOfficialGittensorMiner(login); + const detection = await fetchOfficialGittensorMiner(login, githubId); return detection.status === "confirmed" ? detection.snapshot : null; } catch { /* v8 ignore next -- fetchOfficialGittensorMiner converts network failures into an unavailable status; this is a last-resort guard. */ @@ -178,11 +178,31 @@ export type OfficialGittensorMinerDetection = | { status: "not_found" } | { status: "unavailable"; error: string }; -export async function fetchOfficialGittensorMiner(login: string): Promise { +/** + * #9079: `login` alone used to be the ENTIRE match key against the upstream `/miners` list — but a GitHub + * login is renameable, and a freed username is reclaimable by anyone the instant its prior owner renames or + * deletes their account. If the upstream list still carries the old username (refresh cadence is upstream's, + * not ours), a squatter who claims it inherits `confirmed_miner` — a genuinely elevated role (command + * self-retrigger, the miner-scoped open-PR cap, the unlinked-issue-guardrail velocity exception). `githubId` + * is GitHub's own immutable numeric user id (present on every webhook payload's `user.id`, threaded through as + * `PullRequestRecord`/`IssueRecord.authorGithubId` since #9125) and CANNOT be reassigned by a rename or a + * reclaimed login, so it is checked FIRST whenever the caller has one. `login` is now purely a display/lookup + * fallback: still used verbatim when a caller genuinely has no stored id yet (a PR/issue predating #9125, or + * a call site not yet threading it through), which is a strict narrowing of the old always-username-only + * behavior, never a widening of it. + */ +export async function fetchOfficialGittensorMiner(login: string, githubId?: number | string | null): Promise { try { const miners = await fetchJson(`${GITTENSOR_API_BASE}/miners`); + const normalizedTargetGithubId = githubId === null || githubId === undefined ? null : String(githubId); const normalizedLogin = login.toLowerCase(); - const miner = miners.find((candidate) => candidate.githubUsername?.toLowerCase() === normalizedLogin); + const miner = normalizedTargetGithubId + ? (miners.find((candidate) => candidate.githubId === normalizedTargetGithubId) ?? + // A caller-supplied id that the upstream list doesn't (yet) carry is NOT "confirmed via username" -- + // that would reopen exactly the login-only hole this id-first match exists to close. Only fall back to + // a username match when the id itself was never known at all. + undefined) + : miners.find((candidate) => candidate.githubUsername?.toLowerCase() === normalizedLogin); if (!miner?.githubId || !miner.githubUsername) return { status: "not_found" }; return { status: "confirmed", snapshot: await buildGittensorContributorSnapshot({ ...miner, githubId: miner.githubId, githubUsername: miner.githubUsername }) }; } catch (error) { diff --git a/src/gittensor/miner-detection-cache.ts b/src/gittensor/miner-detection-cache.ts index 60ca21691..4f469748b 100644 --- a/src/gittensor/miner-detection-cache.ts +++ b/src/gittensor/miner-detection-cache.ts @@ -11,13 +11,15 @@ import { fetchOfficialGittensorMiner } from "./api"; const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000; const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000; -/** Fail-safe: any lookup failure resolves to "not a confirmed miner," never the reverse. */ -export async function isConfirmedOfficialMiner(env: Env, login: string): Promise { +/** Fail-safe: any lookup failure resolves to "not a confirmed miner," never the reverse. #9079: matches on + * `githubId` (the author's immutable numeric id) when the caller has one -- `login` is only ever the + * fallback/cache key, since a GitHub username is renameable and a freed one is reclaimable by anyone. */ +export async function isConfirmedOfficialMiner(env: Env, login: string, githubId?: number | null): Promise { const cached = await getFreshOfficialMinerDetection(env, login).catch(() => null); if (cached) return cached.status === "confirmed"; // fetchOfficialGittensorMiner already converts every failure into a returned {status: "unavailable"} // value rather than rejecting -- nothing to catch here. - const detection = await fetchOfficialGittensorMiner(login); + const detection = await fetchOfficialGittensorMiner(login, githubId); // A cache-write failure must never block the caller from using the freshly-fetched (just uncached) // detection -- worst case, the next call re-fetches instead of hitting the cache. const cacheable = await upsertOfficialMinerDetection( diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d6a324de5..3bd34e1c0 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2358,6 +2358,8 @@ async function finalizePolicyCloseDisposition( pullNumber: number; headSha: string | null | undefined; authorLogin?: string | null | undefined; + /** #9079: the author's immutable numeric GitHub user id, preferred over authorLogin for miner matching. */ + authorGithubId?: number | null | undefined; deliveryId: string; planned: PlannedAgentAction[]; settings: Awaited>; @@ -2378,7 +2380,7 @@ async function finalizePolicyCloseDisposition( await getCachedOfficialMinerDetection(env, args.authorLogin, { targetKey: `${args.repoFullName}#${args.pullNumber}`, deliveryId: args.deliveryId, - }) + }, args.authorGithubId) ).status === "confirmed" : false; const breakerOnPlan = applyPrecisionBreakers( @@ -2997,7 +2999,7 @@ async function resolveContributorCapMatch( const officialMiner = await getCachedOfficialMinerDetection(env, pr.authorLogin, { targetKey: `${repoFullName}#${pr.number}`, deliveryId, - }); + }, pr.authorGithubId); const globalCap = officialMiner.status === "confirmed" ? prGlobalCapForMiner : prGlobalCapForHuman; if (globalCap !== null) { const globalOpenCount = await verifiedGlobalOpenItemCount(env, installationId, pr.authorLogin, { @@ -3611,7 +3613,7 @@ async function runAgentMaintenancePlanAndExecute( await getCachedOfficialMinerDetection(env, pr.authorLogin, { targetKey: `${repoFullName}#${pr.number}`, deliveryId, - }) + }, pr.authorGithubId) ).status === "confirmed" : false; // #7986: a cheap, cron-refreshed single-row read (readUntrustworthyRuleCodes) — never a fresh aggregate @@ -6147,7 +6149,7 @@ async function maybeCloseIssueOverContributorCap( const officialMiner = await getCachedOfficialMinerDetection(env, authorLogin, { targetKey: `${repoFullName}#${issue.number}`, deliveryId, - }); + }, issue.authorGithubId); const globalCap = officialMiner.status === "confirmed" ? globalCapForMiner : globalCapForHuman; if (globalCap !== null) { const globalOpenCount = await verifiedGlobalOpenItemCount(env, installationId, authorLogin, { @@ -9714,7 +9716,7 @@ async function maybePublishPrPublicSurface( official = await getCachedOfficialMinerDetection(env, author, { targetKey: `${repoFullName}#${pr.number}`, deliveryId: webhook.deliveryId, - }); + }, pr.authorGithubId); if (requireOfficialMiner && official.status === "unavailable") { await auditPrVisibilitySkip( env, @@ -10376,7 +10378,7 @@ async function maybePublishPrPublicSurface( official = await getCachedOfficialMinerDetection(env, author, { targetKey: `${repoFullName}#${pr.number}`, deliveryId: webhook.deliveryId, - }); + }, pr.authorGithubId); } // Resolve the author's confirmed-Gittensor status. It feeds on-chain SCORING and the public surface, but @@ -13557,7 +13559,7 @@ async function runE2eTestGenerationAndDeliver( let commitOutcome: E2eTestGenCommitOutcome | undefined; if (testSource && deliveryMode === "commit") { const minerDetection = args.pr.authorLogin - ? await getCachedOfficialMinerDetection(env, args.pr.authorLogin, { targetKey: args.targetKey, deliveryId: args.deliveryId }) + ? await getCachedOfficialMinerDetection(env, args.pr.authorLogin, { targetKey: args.targetKey, deliveryId: args.deliveryId }, args.pr.authorGithubId) : ({ status: "not_found" } as const); if (minerDetection.status === "confirmed") { commitOutcome = { status: "blocked" }; @@ -14339,6 +14341,10 @@ async function authorizePrActionActor(args: { ); const pullRequestAuthor = args.pr.authorLogin ?? args.issue.user?.login ?? null; + // #9079: mirrors the pullRequestAuthor fallback above -- prefer the cached PR row's immutable id, falling + // back to the webhook issue payload's own `user.id` only when the PR row's login itself was unavailable + // (the exact case where pullRequestAuthor also fell back to the webhook login). + const pullRequestAuthorGithubId = args.pr.authorLogin ? args.pr.authorGithubId : (args.issue.user?.id ?? null); const official = args.needsMinerDetection && pullRequestAuthor && @@ -14352,7 +14358,7 @@ async function authorizePrActionActor(args: { ? await getCachedOfficialMinerDetection(args.env, pullRequestAuthor, { targetKey: `${args.repoFullName}#${args.issue.number}`, deliveryId: args.deliveryId, - }) + }, pullRequestAuthorGithubId) : undefined; const authorization = isAuthorizedCommandActor({ commandName: args.commandName, @@ -14670,6 +14676,7 @@ async function maybeThrottleReviewNagPing( pullNumber: pr.number, headSha: pr.headSha, authorLogin: pr.authorLogin, + authorGithubId: pr.authorGithubId, deliveryId, planned, settings, @@ -14879,6 +14886,7 @@ async function maybeThrottleMonitoredMentions( pullNumber: pr.number, headSha: pr.headSha, authorLogin: pr.authorLogin, + authorGithubId: pr.authorGithubId, deliveryId, planned, settings, @@ -15230,6 +15238,9 @@ async function maybeProcessLoopOverMentionCommand( const pullRequestAuthor = cachedPullRequest?.authorLogin ?? issue.user?.login ?? null; + // #9079: mirrors the pullRequestAuthor fallback above -- prefer the cached PR row's immutable id, falling + // back to the webhook issue payload's own `user.id` only when the cached row's login itself was unavailable. + const pullRequestAuthorGithubId = cachedPullRequest?.authorLogin ? cachedPullRequest.authorGithubId : (issue.user?.id ?? null); // Intent-classification router (#4596): an unrecognized-verb mention with real trailing text (e.g. "why is // this stuck?") gets ONE chance to be re-routed to an existing Q&A command. Because the classifier is a @@ -15255,7 +15266,7 @@ async function maybeProcessLoopOverMentionCommand( ? await getCachedOfficialMinerDetection(env, pullRequestAuthor, { targetKey: `${repoFullName}#${issue.number}`, deliveryId, - }) + }, pullRequestAuthorGithubId) : undefined; let interpretedFrom: { question: string; matchedCommand: LoopOverMentionCommandName } | undefined; if (command.name === "help" && command.unrecognizedText && settings.advisoryAiRouting?.intentRouting === true) { @@ -15318,7 +15329,7 @@ async function maybeProcessLoopOverMentionCommand( official = await getCachedOfficialMinerDetection(env, pullRequestAuthor, { targetKey: `${repoFullName}#${issue.number}`, deliveryId, - }); + }, pullRequestAuthorGithubId); } const authorization = isAuthorizedCommandActor({ commandName: command.name, @@ -15862,12 +15873,14 @@ async function maybeProcessAgentCommandFeedbackReaction( } const pullRequestAuthor = cachedPullRequest?.authorLogin ?? issue.user?.login ?? null; + // #9079: mirrors the pullRequestAuthor fallback above -- see the identical derivation earlier in this file. + const pullRequestAuthorGithubId = cachedPullRequest?.authorLogin ? cachedPullRequest.authorGithubId : (issue.user?.id ?? null); const official = pullRequestAuthor && actor.toLowerCase() === pullRequestAuthor.toLowerCase() ? await getCachedOfficialMinerDetection(env, actor, { targetKey, deliveryId, - }) + }, pullRequestAuthorGithubId) : undefined; // #8682: feedback votes must be authorized against the SAME command policy that authorized the answer, // not isAuthorizedCommandActor's `"preflight"` default. Load live settings so commandAuthorization / @@ -16014,6 +16027,11 @@ async function getCachedOfficialMinerDetection( env: Env, login: string, context: { targetKey: string; deliveryId: string }, + // #9079: the author's immutable numeric GitHub user id (PullRequestRecord/IssueRecord.authorGithubId), + // when the caller has one. Preferred over `login` for the actual upstream match (fetchOfficialGittensorMiner + // checks id first) -- `login` remains the cache key/audit-log identity and the fallback match key for a + // caller that genuinely doesn't have a stored id yet. + githubId?: number | null, ): Promise { const cached = await getFreshOfficialMinerDetection(env, login); if (cached) { @@ -16035,7 +16053,7 @@ async function getCachedOfficialMinerDetection( context, "miss", ); - const detection = await fetchOfficialGittensorMiner(login); + const detection = await fetchOfficialGittensorMiner(login, githubId); const cacheableDetection = await upsertOfficialMinerDetection( env, login, diff --git a/src/review/unlinked-issue-guardrail.ts b/src/review/unlinked-issue-guardrail.ts index 902aadfac..8979ca7cf 100644 --- a/src/review/unlinked-issue-guardrail.ts +++ b/src/review/unlinked-issue-guardrail.ts @@ -137,13 +137,14 @@ async function recordUnlinkedIssueVerifyUsage(env: Env, repoFullName: string, pu /** Minimal cached miner-identity check, deliberately independent of processors.ts's getCachedOfficialMinerDetection * (same cache table and TTLs, no audit-log side effect -- this call site doesn't need one). Fail-safe: any - * lookup failure resolves to "not a confirmed miner," never the reverse. */ -async function isConfirmedOfficialMiner(env: Env, login: string): Promise { + * lookup failure resolves to "not a confirmed miner," never the reverse. #9079: matches on `githubId` (the + * author's immutable numeric id) when the caller has one -- `login` is only ever the fallback/cache key. */ +async function isConfirmedOfficialMiner(env: Env, login: string, githubId?: number | null): Promise { const cached = await getFreshOfficialMinerDetection(env, login).catch(() => null); if (cached) return cached.status === "confirmed"; // fetchOfficialGittensorMiner already converts every failure into a returned {status: "unavailable"} // value rather than rejecting -- nothing to catch here. - const detection = await fetchOfficialGittensorMiner(login); + const detection = await fetchOfficialGittensorMiner(login, githubId); // A cache-write failure must never block the caller from using the freshly-fetched (just uncached) // detection -- worst case, the next call re-fetches instead of hitting the cache. const cacheable = await upsertOfficialMinerDetection( @@ -172,6 +173,11 @@ export type ResolveUnlinkedIssueMatchDispositionInput = { * correlated across PRs, so repeat-detection is skipped entirely and a confirmed match always holds * (fail-safe: never escalate to a close on an unidentifiable author). */ prAuthorLogin: string | null | undefined; + /** #9079: the author's immutable numeric GitHub user id (`PullRequestRecord.authorGithubId`), when the + * caller has it. Preferred over `prAuthorLogin` for the velocity exception's miner-identity check below -- + * a login is renameable and a freed one is reclaimable, so matching a `confirmed_miner`-gated exception on + * username alone would let a squatter who claims a former miner's old login inherit it. */ + prAuthorGithubId?: number | null | undefined; }; function unlinkedIssueMatchTargetKey(repoFullName: string, pullNumber: number): string { @@ -271,7 +277,7 @@ export async function resolveUnlinkedIssueMatchDisposition(env: Env, input: Reso const gapMs = Date.now() - new Date(priorMatchIso).getTime(); // #4512 velocity exception: gated on CONFIRMED miner identity, not on speed alone -- an unverified // account repeating this fast is the MORE suspicious case, not less, and still escalates to close. - const velocityExceptionApplies = gapMs >= 0 && gapMs < VELOCITY_EXCEPTION_MAX_GAP_MS && (await isConfirmedOfficialMiner(env, authorLogin).catch(() => false)); + const velocityExceptionApplies = gapMs >= 0 && gapMs < VELOCITY_EXCEPTION_MAX_GAP_MS && (await isConfirmedOfficialMiner(env, authorLogin, input.prAuthorGithubId).catch(() => false)); if (!velocityExceptionApplies) { return { kind: "close", diff --git a/test/unit/gittensor-api.test.ts b/test/unit/gittensor-api.test.ts index ce279a9af..41264c262 100644 --- a/test/unit/gittensor-api.test.ts +++ b/test/unit/gittensor-api.test.ts @@ -195,6 +195,43 @@ describe("Gittensor API contributor snapshots", () => { await expect(fetchOfficialGittensorMiner("12345")).resolves.toEqual({ status: "not_found" }); }); + describe("#9079: matches on the immutable githubId when the caller has one, not the renameable login", () => { + it("confirms by githubId even when the caller-supplied login no longer matches the upstream username", async () => { + // The registered miner's CURRENT upstream username is "renamed-miner" -- the caller only knows the OLD + // login it observed on a stale/cached PR row. Login-only matching would report not_found here; the id + // is what actually identifies the same account across a rename. + vi.stubGlobal("fetch", async () => Response.json([{ githubUsername: "renamed-miner", githubId: "49853598" }])); + await expect(fetchOfficialGittensorMiner("stale-old-login", "49853598")).resolves.toMatchObject({ status: "confirmed", snapshot: { githubId: "49853598", githubUsername: "renamed-miner" } }); + }); + + it("REFUSES to fall back to a username match when a supplied githubId doesn't match anyone -- the exact exploit this closes", async () => { + // An attacker reclaims a miner's freed login. The upstream list may still carry an entry for that exact + // username (from before the rename/deletion, e.g. under a DIFFERENT numeric id, or simply not carry the + // caller's id at all) -- but the caller here is asking about a SPECIFIC github user id that is not a + // registered miner. Matching the login instead would hand the attacker the real miner's confirmed status. + vi.stubGlobal("fetch", async () => Response.json([{ githubUsername: "attacker-claimed-login", githubId: "1111" }])); + await expect(fetchOfficialGittensorMiner("attacker-claimed-login", "9999")).resolves.toEqual({ status: "not_found" }); + }); + + it("falls back to a login match only when the caller has no githubId at all (unchanged pre-#9079 behavior)", async () => { + vi.stubGlobal("fetch", async () => Response.json([{ githubUsername: "jsonbored", githubId: "49853598" }])); + await expect(fetchOfficialGittensorMiner("jsonbored")).resolves.toMatchObject({ status: "confirmed", snapshot: { githubId: "49853598" } }); + await expect(fetchOfficialGittensorMiner("jsonbored", null)).resolves.toMatchObject({ status: "confirmed", snapshot: { githubId: "49853598" } }); + await expect(fetchOfficialGittensorMiner("jsonbored", undefined)).resolves.toMatchObject({ status: "confirmed", snapshot: { githubId: "49853598" } }); + }); + + it("accepts a numeric githubId (not just a string) and matches it against the upstream string field", async () => { + vi.stubGlobal("fetch", async () => Response.json([{ githubUsername: "jsonbored", githubId: "49853598" }])); + await expect(fetchOfficialGittensorMiner("jsonbored", 49853598)).resolves.toMatchObject({ status: "confirmed" }); + }); + + it("threads githubId through fetchGittensorContributorSnapshot the same way", async () => { + vi.stubGlobal("fetch", async () => Response.json([{ githubUsername: "renamed-miner", githubId: "49853598" }])); + await expect(fetchGittensorContributorSnapshot("stale-old-login", "49853598")).resolves.toMatchObject({ githubId: "49853598" }); + await expect(fetchGittensorContributorSnapshot("attacker-claimed-login", "9999")).resolves.toBeNull(); + }); + }); + it("classifies official miner detection without a complete identity and handles non-Error failures", async () => { vi.stubGlobal("fetch", async () => Response.json([{ githubUsername: "jsonbored" }, { githubId: "49853598" }])); await expect(fetchOfficialGittensorMiner("jsonbored")).resolves.toEqual({ status: "not_found" }); From b2e07ca1a464baf66d4e756731e3e83d62a9a8dd Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:08:10 -0700 Subject: [PATCH 3/5] fix(bounties): close the bounty import audit gap and id-change wedge (#9080) The bounty mirror import route collected every lifecycle event into one array and persisted them all only after the whole loop finished, so a mid-loop failure left every bounty upserted so far already advanced with zero lifecycle rows recorded for any of them -- unrecoverable once the next import's diff runs against the now-updated rows. Each item is now upserted and its own lifecycle event written in the same step, wrapped in its own try/catch, so one bad row can no longer take the rest of the batch down with it. upsertBounty's insert only handled a conflict on the id primary key, but the table also has a UNIQUE(repo_full_name, issue_number) index. Upstream re-issuing a bounty on the same issue under a new id hit that second, unhandled unique index and threw, 500ing the import and wedging every subsequent import on the same row permanently. A second chained onConflictDoUpdate (SQLite's native multi-target upsert syntax) resolves both conflicts to an update; the surviving row always keeps whichever id it already has, so lifecycle events already keyed to it are never orphaned by a later re-issue. classifyBountyLifecycle's status regexes were unanchored substring matches, so a base word buried mid-token false-positived: "unclaimed" and "not_completed" both classified as completed, "avoid" classified as cancelled, "renewed" classified as active. Anchoring each alternative on a leading word boundary keeps every genuine prefix match (cancelled, completed, rewarded, ...) while no longer matching those buried substrings. fundingStatus compared the amount against exactly one hardcoded zero-string literal ("0.0000"), so any other zero-ish spelling upstream might send ("0", "0.0", "0.00") reported as funded. Parsing to a number and requiring it to be finite and positive treats every zero-ish spelling the same, string or numeric, and degrades a malformed amount to target-only/unknown instead of reporting funded. Closes #9080 --- .../loopover-engine/src/signals/engine.ts | 36 ++++++++-- src/api/routes.ts | 52 +++++++++----- src/db/repositories.ts | 44 ++++++------ test/integration/routes-errors.test.ts | 70 +++++++++++++++++++ test/unit/db-persistence.test.ts | 64 +++++++++++++++++ test/unit/signals.test.ts | 17 +++++ 6 files changed, 238 insertions(+), 45 deletions(-) diff --git a/packages/loopover-engine/src/signals/engine.ts b/packages/loopover-engine/src/signals/engine.ts index 8ca9e5932..9c8df56fa 100644 --- a/packages/loopover-engine/src/signals/engine.ts +++ b/packages/loopover-engine/src/signals/engine.ts @@ -3829,15 +3829,29 @@ export function indexBountiesByIssue(bounties: BountyRecord[]): Map 0 ? "funded" : target ? "target_only" : "unknown"; const source = buildBountySourceContext(bounty); const linkedPrs = buildBountyLinkedPrs(issue, pullRequests, recentMergedPullRequests); const findings: SignalFinding[] = []; diff --git a/src/api/routes.ts b/src/api/routes.ts index cbc57dc9c..98c5a862f 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -339,7 +339,6 @@ import { } from "../signals/registration-readiness"; import { fileUpstreamDriftIssues, loadUpstreamStatus, refreshUpstreamDrift, registryHyperparameterDriftWarningsForRepo, resolveAutoFileDriftIssuesManifestOverride } from "../upstream/ruleset"; import type { - BountyLifecycleEventRecord, ControlPanelRoleName, ContributorEvidenceRecord, DataQuality, @@ -5091,24 +5090,45 @@ export function createApp() { app.post("/v1/internal/bounties/import", async (c) => { const body = await c.req.json().catch(() => null); const bounties = normalizeGittBountySnapshot(body); - const events: BountyLifecycleEventRecord[] = []; + let imported = 0; + let lifecycleEvents = 0; for (const bounty of bounties) { - const existing = await getBounty(c.env, bounty.id); - await upsertBounty(c.env, bounty); - if (!existing || existing.status !== bounty.status) { - events.push({ - id: crypto.randomUUID(), - bountyId: bounty.id, - repoFullName: bounty.repoFullName, - issueNumber: bounty.issueNumber, - status: bounty.status, - payload: { previousStatus: existing?.status ?? null, source: "gitt_import" }, - generatedAt: nowIso(), - }); + // #9080: the lifecycle event for THIS bounty is now written in the SAME step as its own upsert, and + // each item is individually try/caught -- previously every event was collected into one array and + // persisted only AFTER the whole loop finished, so a mid-loop failure (a bad row further down the + // batch) left every bounty upserted so far already advanced in `bounties` with ZERO lifecycle rows + // recorded for any of them, and the transition is unrecoverable once the next import's diff runs + // against the now-already-updated row (the same "state advanced, audit trail didn't" shape as #8997). + // A single bad item can no longer take the rest of the batch down with it, either. + try { + const existing = await getBounty(c.env, bounty.id); + await upsertBounty(c.env, bounty); + imported += 1; + if (!existing || existing.status !== bounty.status) { + await persistBountyLifecycleEvent(c.env, { + id: crypto.randomUUID(), + bountyId: bounty.id, + repoFullName: bounty.repoFullName, + issueNumber: bounty.issueNumber, + status: bounty.status, + payload: { previousStatus: existing?.status ?? null, source: "gitt_import" }, + generatedAt: nowIso(), + }); + lifecycleEvents += 1; + } + } catch (error) { + console.warn( + JSON.stringify({ + event: "bounty_import_item_failed", + bountyId: bounty.id, + repoFullName: bounty.repoFullName, + issueNumber: bounty.issueNumber, + message: errorMessage(error).slice(0, 160), + }), + ); } } - await Promise.all(events.map((event) => persistBountyLifecycleEvent(c.env, event))); - return c.json({ ok: true, imported: bounties.length, lifecycleEvents: events.length }); + return c.json({ ok: true, imported, lifecycleEvents }); }); app.post("/v1/internal/queue-intelligence", async (c) => { diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 64893348b..bf4afb011 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -5211,30 +5211,30 @@ export async function getBounty(env: Env, id: string): Promise { const db = getDb(env.DB); + const set = { + repoFullName: bounty.repoFullName, + issueNumber: bounty.issueNumber, + status: bounty.status, + amountText: bounty.amountText, + sourceUrl: bounty.sourceUrl, + payloadJson: jsonString(bounty.payload), + updatedAt: nowIso(), + }; await db .insert(bounties) - .values({ - id: bounty.id, - repoFullName: bounty.repoFullName, - issueNumber: bounty.issueNumber, - status: bounty.status, - amountText: bounty.amountText, - sourceUrl: bounty.sourceUrl, - payloadJson: jsonString(bounty.payload), - updatedAt: nowIso(), - }) - .onConflictDoUpdate({ - target: bounties.id, - set: { - repoFullName: bounty.repoFullName, - issueNumber: bounty.issueNumber, - status: bounty.status, - amountText: bounty.amountText, - sourceUrl: bounty.sourceUrl, - payloadJson: jsonString(bounty.payload), - updatedAt: nowIso(), - }, - }); + .values({ id: bounty.id, ...set }) + // #9080: TWO independent unique constraints can each conflict with this insert -- the primary key + // `id` (a re-import of the SAME upstream bounty) and `(repo_full_name, issue_number)` + // (`bounties_repo_issue_unique`, upstream re-issuing a bounty on the SAME issue under a NEW id, e.g. + // after cancelling and refunding the old one). Before this, only `id` was handled: a re-issued bounty + // conflicted on the unhandled unique index, the insert THREW, and — since nothing here caught it — + // the whole import batch 500'd and never advanced past that row on any subsequent import either + // (a permanent wedge). Chaining a second onConflictDoUpdate matches SQLite's own native multi-target + // upsert syntax, so both conflicts resolve to an update instead of one of them throwing. Neither + // clause touches `id`: the surviving row always keeps whichever id it already has, so + // bounty_lifecycle_events rows already keyed to it are never orphaned by a later re-issue. + .onConflictDoUpdate({ target: bounties.id, set }) + .onConflictDoUpdate({ target: [bounties.repoFullName, bounties.issueNumber], set }); } export async function persistAdvisory(env: Env, advisory: Advisory): Promise { diff --git a/test/integration/routes-errors.test.ts b/test/integration/routes-errors.test.ts index abcbb2fce..0537a8bb7 100644 --- a/test/integration/routes-errors.test.ts +++ b/test/integration/routes-errors.test.ts @@ -1275,6 +1275,38 @@ describe("api route guards and error branches", () => { expect(clearNoBody.status).toBe(200); await expect(clearNoBody.json()).resolves.toMatchObject({ cleared: true }); }); + + it("isolates a single bad row in a bounty import batch instead of losing the whole batch (#9080)", async () => { + const app = createApp(); + const env = withBountyUpsertFailureForIssueNumber(createTestEnv(), 4242); + + const imported = await app.request( + "/v1/internal/bounties/import", + { + method: "POST", + headers: internalHeaders(env), + body: JSON.stringify({ + success: true, + issue_count: 2, + issues: [ + { id: 10, repository_full_name: "entrius/allways-ui", issue_number: 4242, status: "Open", bounty_alpha: "5.0000" }, + { id: 11, repository_full_name: "entrius/allways-ui", issue_number: 20, status: "Open", bounty_alpha: "5.0000" }, + ], + }), + }, + env, + ); + + // Before #9080 a single bad row's exception propagated out of the route handler (500) and no + // lifecycle events were ever persisted for ANY item, since every event was collected into one array + // and written only after the whole loop finished. Now the good row still lands and gets counted. + expect(imported.status).toBe(200); + await expect(imported.json()).resolves.toMatchObject({ imported: 1, lifecycleEvents: 1 }); + + const bounties = await app.request("/v1/bounties", { headers: apiHeaders(env) }, env); + const bountyList = (await bounties.json()) as Array<{ id: string; issueNumber: number }>; + expect(bountyList.map((bounty) => bounty.issueNumber)).toEqual([20]); + }); }); async function seedRegisteredInstalledRepo(env: Env, installationId: number, owner: string, name: string): Promise { @@ -1359,6 +1391,44 @@ function withProductUsageInsertFailure(env: Env): Env { }; } +// #9080: simulates ONE bad row deep in a `/v1/internal/bounties/import` batch (a transient D1 write +// failure, a constraint violation the caller couldn't anticipate, etc.) by throwing only when the +// upsert's own bound issue_number matches the target -- every other row in the same batch must still +// land, and its own lifecycle event must still be written, instead of one bad item taking the whole +// batch down (the pre-#9080 shape, since events were only ever persisted AFTER the whole loop finished). +function withBountyUpsertFailureForIssueNumber(env: Env, issueNumber: number): Env { + const db = env.DB as unknown as { prepare(sql: string): { bind(...values: unknown[]): unknown }; batch(statements: unknown[]): Promise }; + return { + ...env, + DB: { + prepare(sql: string) { + const statement = db.prepare(sql); + // Only the targeted INSERT is instrumented -- every other query (session lookups, the SELECT + // half of the upsert's own getBounty read, lifecycle-event inserts, ...) passes through to the + // real statement untouched, so it keeps whatever methods (e.g. `.raw()`) drizzle's D1 driver + // expects on it. + if (!sql.includes('insert into "bounties"')) return statement; + return { + bind(...values: unknown[]) { + const bound = statement.bind(...values) as { run(): Promise }; + if (values.includes(issueNumber)) { + return { + run() { + throw new Error(`simulated bounty upsert failure for issue ${issueNumber}`); + }, + }; + } + return bound; + }, + }; + }, + batch(statements: unknown[]) { + return db.batch.call(db, statements); + }, + } as unknown as D1Database, + }; +} + function firstCookiePair(header: string, name?: string): string { const cookies = header.split(/,(?=\s*[^;,]+=)/).map((part) => part.trim()); const cookie = name ? cookies.find((part) => part.startsWith(`${name}=`)) : cookies[0]; diff --git a/test/unit/db-persistence.test.ts b/test/unit/db-persistence.test.ts index 564314db9..90e914e3d 100644 --- a/test/unit/db-persistence.test.ts +++ b/test/unit/db-persistence.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { getActiveReviewStartedAt, + getBounty, getContributorScoringProfile, getOpenUpstreamDriftReportByFingerprint, hasActiveReviewForHeadSha, @@ -17,6 +18,7 @@ import { startActiveReviewTracking, terminalizeActiveReviewTracking, updateUpstreamDriftReportIssue, + upsertBounty, upsertContributorRepoStat, upsertContributorScoringProfile, upsertPullRequestFile, @@ -79,6 +81,68 @@ describe("database persistence helpers", () => { await expect(env.DB.prepare("select count(*) as count from bounty_lifecycle_events").first<{ count: number }>()).resolves.toMatchObject({ count: 1 }); }); + describe("upsertBounty (#9080)", () => { + it("updates the existing row in place on a same-id re-import", async () => { + const env = createTestEnv(); + await upsertBounty(env, { + id: "bounty-1", + repoFullName: "JSONbored/loopover", + issueNumber: 7, + status: "Open", + amountText: "5.0000", + payload: { bounty_alpha: "5.0000" }, + }); + await upsertBounty(env, { + id: "bounty-1", + repoFullName: "JSONbored/loopover", + issueNumber: 7, + status: "Completed", + amountText: "5.0000", + payload: { bounty_alpha: "5.0000" }, + }); + + expect(await getBounty(env, "bounty-1")).toMatchObject({ id: "bounty-1", status: "Completed" }); + await expect(env.DB.prepare("select count(*) as count from bounties").first<{ count: number }>()).resolves.toMatchObject({ count: 1 }); + }); + + it("reconciles onto the existing (repoFullName, issueNumber) row instead of throwing when upstream re-issues a bounty under a new id (#9080)", async () => { + const env = createTestEnv(); + await upsertBounty(env, { + id: "bounty-old", + repoFullName: "JSONbored/loopover", + issueNumber: 7, + status: "Cancelled", + amountText: "0.0000", + payload: { bounty_alpha: "0.0000" }, + }); + + // Upstream re-issues the bounty on the SAME issue under a brand-new id -- before #9080 this threw + // (unhandled conflict on the (repo_full_name, issue_number) unique index), 500ing the whole import + // batch and wedging every subsequent import on the same row. + await expect( + upsertBounty(env, { + id: "bounty-new", + repoFullName: "JSONbored/loopover", + issueNumber: 7, + status: "Open", + amountText: "12.0000", + payload: { bounty_alpha: "12.0000" }, + }), + ).resolves.toBeUndefined(); + + // Exactly one row survives for that (repo, issue) -- the id column is never part of the update, so + // the surviving row keeps its ORIGINAL id (bounty_lifecycle_events rows already keyed to it are not + // orphaned), but every other column reconciles onto the newly-imported values. + await expect(env.DB.prepare("select count(*) as count from bounties").first<{ count: number }>()).resolves.toMatchObject({ count: 1 }); + expect(await getBounty(env, "bounty-old")).toMatchObject({ + id: "bounty-old", + status: "Open", + amountText: "12.0000", + }); + expect(await getBounty(env, "bounty-new")).toBeNull(); + }); + }); + it("returns an empty array when no totals snapshots exist", async () => { const env = createTestEnv(); expect(await listLatestRepoGithubTotalsSnapshots(env)).toEqual([]); diff --git a/test/unit/signals.test.ts b/test/unit/signals.test.ts index e5b40757f..b9e54a6d3 100644 --- a/test/unit/signals.test.ts +++ b/test/unit/signals.test.ts @@ -914,6 +914,14 @@ describe("world-class backend signals", () => { // Empty/whitespace status is a sparse-cache fallback that cannot be classified. expect(classifyBountyLifecycle({ ...base, id: "blank", status: " " }, openIssue)).toBe("unknown"); + // #9080: the lifecycle regexes were unanchored substring matches, so a base word buried mid-token + // false-positived. A leading `\b` fixes this without losing the genuine prefix matches above + // (cancelled/completed/rewarded/etc. all still match -- nothing but a word start precedes them). + expect(classifyBountyLifecycle({ ...base, id: "unclaimed", status: "Unclaimed" }, openIssue)).not.toBe("completed"); + expect(classifyBountyLifecycle({ ...base, id: "not-completed", status: "not_completed" }, openIssue)).not.toBe("completed"); + expect(classifyBountyLifecycle({ ...base, id: "avoid", status: "Avoid" }, openIssue)).not.toBe("cancelled"); + expect(classifyBountyLifecycle({ ...base, id: "renewed", status: "Renewed" }, openIssue)).not.toBe("active"); + expect(buildBountyAdvisory(historical, repo, openIssue).findings.map((finding) => finding.code)).toContain("historical_bounty"); expect(buildBountyAdvisory(completed, repo, openIssue).findings.map((finding) => finding.code)).toContain("completed_bounty"); expect(buildBountyAdvisory(cancelled, repo, openIssue).findings.map((finding) => finding.code)).toContain("cancelled_bounty"); @@ -922,6 +930,15 @@ describe("world-class backend signals", () => { expect(buildBountyAdvisory(stale, repo, openIssue).isActiveOpportunity).toBe(false); expect(buildBountyAdvisory({ ...base, id: "target-only", status: "Open", payload: { target_bounty: 1, bounty_amount: "0.0000" } }, repo, openIssue).fundingStatus).toBe("target_only"); expect(buildBountyAdvisory({ ...base, id: "unknown-funding", status: "Open", payload: {} }, repo, openIssue).fundingStatus).toBe("unknown"); + // #9080: the old guard only excluded the ONE hardcoded literal "0.0000" -- any other zero-ish string + // (upstream sending "0", "0.0", or "0.00") was a non-empty, truthy string that reported "funded". + // Number(amount) > 0 treats every zero-ish spelling the same regardless of exact formatting. + expect(buildBountyAdvisory({ ...base, id: "zero-string", status: "Open", payload: { bounty_amount: "0" } }, repo, openIssue).fundingStatus).toBe("unknown"); + expect(buildBountyAdvisory({ ...base, id: "zero-point-oh", status: "Open", payload: { bounty_amount: "0.0" } }, repo, openIssue).fundingStatus).toBe("unknown"); + expect(buildBountyAdvisory({ ...base, id: "zero-point-oh-oh", status: "Open", payload: { bounty_amount: "0.00", target_bounty: 5 } }, repo, openIssue).fundingStatus).toBe("target_only"); + // A malformed/non-numeric amount is not a payout figure at all; degrade to target-only/unknown rather + // than reporting funded off a string that happened to be truthy. + expect(buildBountyAdvisory({ ...base, id: "non-numeric-amount", status: "Open", payload: { bounty_amount: "TBD" } }, repo, openIssue).fundingStatus).toBe("unknown"); const stalePreflight = buildPreflightResult({ repoFullName: repo.fullName, title: "Fix cache", body: "Fixes #7" }, repo, [openIssue], [], [stale]); const ambiguousPreflight = buildPreflightResult({ repoFullName: repo.fullName, title: "Fix cache", body: "Fixes #7" }, repo, [openIssue], [], [ambiguousStatus]); From 3769d8cc0ed3b9ea0389072226d5b5c8266db6a8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:28:17 -0700 Subject: [PATCH 4/5] fix(gittensor): gate reward-label maintainer-trust propagation on a satisfaction verdict (#9077) isLinkedIssueTrustworthy trusts every OPEN linked issue unconditionally, and trustMaintainerAuthoredIssueForReward's entire unlock was "the linked issue's author independently checks out as a repo maintainer" -- nothing verified the PR actually addressed the issue it cited. A contributor could get a reward label (e.g. gittensor:priority) applied by citing any open, maintainer- authored, reward-labeled issue, whether or not their PR touched it. A label reached only via that relaxed maintainer-authored path is now additionally required to have a PUBLISHED "addressed" linked-issue- satisfaction verdict for the exact (repo, PR, issue) triple, read via getLatestPublishedLinkedIssueSatisfaction rather than spending a fresh model call. This doesn't touch the direct author/assignee path (already strict) or a non-reward trustMaintainerAuthoredIssue mapping (bug/feature mirroring, lower stakes). A repo that hasn't opted into linkedIssueSatisfactionGateMode never persists a verdict at all, so the reward relaxation simply never fires for it -- a fail-safe degrade, not a silent bypass. Closes #9077 --- .../linked-issue-label-propagation-fetch.ts | 61 +++++++++++- src/types.ts | 12 ++- ...nked-issue-label-propagation-fetch.test.ts | 97 ++++++++++++++++++- 3 files changed, 165 insertions(+), 5 deletions(-) diff --git a/src/review/linked-issue-label-propagation-fetch.ts b/src/review/linked-issue-label-propagation-fetch.ts index aa5cb6f23..e82b23e6b 100644 --- a/src/review/linked-issue-label-propagation-fetch.ts +++ b/src/review/linked-issue-label-propagation-fetch.ts @@ -8,6 +8,7 @@ import { import { createInstallationToken, getRepositoryCollaboratorPermission } from "../github/app"; import { githubRateLimitAdmissionKeyForToken, type GitHubRateLimitAdmissionKey } from "../github/client"; import { isPerRepoAdminModeEnabled, parseGitHubLoginList } from "../auth/security"; +import { getLatestPublishedLinkedIssueSatisfaction } from "../db/repositories"; import { errorMessage } from "../utils/json"; import type { LinkedIssueLabelPropagationMapping } from "../types"; @@ -135,6 +136,7 @@ async function resolveIssueLabelsForPropagation( result: LinkedIssueFactsFetch, prAuthorLogin: string | undefined, relaxableLabels: ReadonlySet, + rewardTrustLabels: ReadonlySet, prMergedAt: string | null, // #9161: issueLabels named by at least one ADDITIVE mapping (`removeOtherTypeLabels !== true` -- see // pr-type-label.ts's exclusive/additive split, e.g. `gittensor:priority` composing alongside bug/feature). @@ -243,7 +245,9 @@ async function resolveIssueLabelsForPropagation( } const maintainerAuthored = maintainerCheck === "maintainer"; const relaxedRewardLabels = maintainerAuthored ? rewardCandidateLabels.filter((label) => relaxableLabels.has(label.toLowerCase())) : []; - const kept = [...directTypeLabels, ...relaxedRewardLabels]; + // `let`, not `const`: #9077's satisfaction-verdict check below (rewardCandidates/rewardTrustLabels) can + // further filter an unconfirmed reward-flagged label back out of this set. + let kept = [...directTypeLabels, ...relaxedRewardLabels]; if (kept.length < allLabels.length && allLabels.length > 0) { console.log( @@ -260,6 +264,43 @@ async function resolveIssueLabelsForPropagation( }), ); } + + // #9077: the maintainer-authored relaxation above accepts "a maintainer opened this issue" as the ENTIRE + // unlock for a `trustMaintainerAuthoredIssueForReward` label -- unlike the direct author/assignee match + // above (return-early, genuine involvement), a contributor here did nothing but CITE an issue they had no + // part in. Nothing upstream of this point verifies the PR actually addresses it: `isLinkedIssueTrustworthy` + // trusts every OPEN issue unconditionally, and an OPEN maintainer-authored `gittensor:priority` issue is + // exactly what this mapping exists to match. A reward-flagged label reached only via this relaxed path is + // therefore additionally gated on a PUBLISHED linked-issue-satisfaction verdict of "addressed" for this + // exact (repo, PR, issue) -- the same AI-backed assessment the `linkedIssueSatisfactionGateMode` gate + // already computes and persists (`src/services/linked-issue-satisfaction.ts`), read here rather than + // re-run, so this never spends a second model call. A repo that has not opted into that gate mode (the + // "off" default) never persists a verdict at all, so the reward relaxation simply never fires for it -- + // a fail-safe degrade, not a silent bypass: "no evidence this PR addresses the issue" must never be + // conflated with "confirmed addressed." This check does NOT apply to a non-reward relaxed label (plain + // `trustMaintainerAuthoredIssue`, e.g. bug/feature mirroring) or to the direct-ownership path above -- + // both keep their existing behavior unchanged. + const rewardCandidates = kept.filter((label) => rewardTrustLabels.has(label.toLowerCase())); + if (rewardCandidates.length > 0) { + const verdict = + args.prNumber !== undefined + ? await getLatestPublishedLinkedIssueSatisfaction(args.env, args.repoFullName, args.prNumber, result.facts.number).catch(() => null) + : null; + const satisfactionConfirmed = verdict?.status === "ok" && verdict.result?.status === "addressed"; + if (!satisfactionConfirmed) { + console.log( + JSON.stringify({ + event: "linked_issue_reward_label_requires_satisfaction", + repoFullName: args.repoFullName, + issueNumber: result.facts.number, + droppedCount: rewardCandidates.length, + }), + ); + const rewardCandidateSet = new Set(rewardCandidates); + kept = kept.filter((label) => !rewardCandidateSet.has(label)); + } + } + return { labels: kept, inconclusive: false }; } @@ -283,7 +324,12 @@ async function resolveIssueLabelsForPropagation( * `mappings` (optional, #priority-linked-issue-gate-ownership) is the propagation config's own mapping * list, used ONLY to know which `issueLabel`s are allowed to unlock via `resolveIssueLabelsForPropagation`'s * relaxed maintainer-authored-issue path (either trust flag) -- omitting it (or a mapping never setting - * either flag) reproduces today's strict author-or-assignee-only behavior exactly. + * either flag) reproduces today's strict author-or-assignee-only behavior exactly. #9077: a label reached + * ONLY via that relaxed path AND flagged `trustMaintainerAuthoredIssueForReward` is additionally required + * to have a PUBLISHED "addressed" linked-issue-satisfaction verdict for this exact (repo, PR, issue) before + * it is kept -- citing an untouched maintainer-authored issue is no longer sufficient by itself for a + * reward-bearing label. See `resolveIssueLabelsForPropagation`'s own comment on that check for the repo- + * opt-in dependency this introduces (`linkedIssueSatisfactionGateMode` must not be `"off"`). * * `prMergedAt` (#4528) is this PR's own `merged_at`, or `null` while unmerged -- the caller's `pr.mergedAt` * straight from the DB row (or webhook payload), no extra fetch in the common case. @@ -347,6 +393,16 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { const rewardLabels = new Set( [...mappingsByIssueLabel.entries()].filter(([, mappings]) => mappings.some((mapping) => mapping.removeOtherTypeLabels !== true)).map(([issueLabel]) => issueLabel), ); + // #9077: a STRICTER subset of `relaxableLabels` -- any issueLabel where AT LEAST ONE of its mappings + // opted into the reward-specific flag, not just the generic maintainer-trust one. Checked with `.some` + // rather than `.every` (unlike `relaxableLabels` above) so a repo cannot dodge the extra satisfaction- + // verdict gate below by pairing a reward-flagged mapping with a non-reward one under the same issueLabel + // string -- any reward intent on a label is enough to require the stronger evidence bar. + const rewardTrustLabels = new Set( + [...mappingsByIssueLabel.entries()] + .filter(([, mappings]) => mappings.some((mapping) => mapping.trustMaintainerAuthoredIssueForReward === true)) + .map(([issueLabel]) => issueLabel), + ); const results = await Promise.all( linkedIssues.map((issueNumber) => fetchLinkedIssueFacts( @@ -365,6 +421,7 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { result, prAuthorLogin, relaxableLabels, + rewardTrustLabels, prMergedAt, rewardLabels, ), diff --git a/src/types.ts b/src/types.ts index 84abec9a1..9cfb94b37 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1609,7 +1609,17 @@ export type LinkedIssueLabelPropagationMapping = { * issue) as sufficient for the reward label too, when a repo's operator has decided that's the intended * workflow (e.g. a maintainer hand-labels an issue `gittensor:priority` specifically to attract ANY * contributor to pick it up, per the label's own "reserved for outstanding work" framing -- the hand-picking - * already happened at issue-labeling time, not gated on which contributor later closes it). */ + * already happened at issue-labeling time, not gated on which contributor later closes it). + * + * #9077: this flag alone is NOT sufficient once relaxed -- a label unlocked via this path additionally + * requires a PUBLISHED linked-issue-satisfaction verdict of "addressed" for the exact (repo, PR, issue) + * triple (`review/linked-issue-label-propagation-fetch.ts`'s `resolveIssueLabelsForPropagation`), since + * "a maintainer opened this issue" was previously the ENTIRE evidence bar: any open, maintainer-authored, + * reward-labeled issue could be cited by a PR that never touched it. This makes the reward relaxation + * depend on also setting `linkedIssueSatisfactionGateMode` to something other than `"off"` -- a repo + * that opts into this flag without also enabling that gate mode will never see the reward label + * propagate via this path (a fail-safe degrade: no verdict published is treated as no evidence, never + * as a confirmed pass). */ trustMaintainerAuthoredIssueForReward?: boolean | undefined; }; diff --git a/test/unit/linked-issue-label-propagation-fetch.test.ts b/test/unit/linked-issue-label-propagation-fetch.test.ts index 51235128f..ebde5aaf1 100644 --- a/test/unit/linked-issue-label-propagation-fetch.test.ts +++ b/test/unit/linked-issue-label-propagation-fetch.test.ts @@ -2,12 +2,26 @@ 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 { putCachedLinkedIssueSatisfaction } from "../../src/db/repositories"; import { fetchLinkedIssueLabelsForPropagation, type LinkedIssuePropagationLabels, } from "../../src/review/linked-issue-label-propagation-fetch"; import { generatePrivateKeyPem } from "../helpers/github-app-key"; +// #9077: a `trustMaintainerAuthoredIssueForReward` label additionally requires a PUBLISHED "addressed" +// linked-issue-satisfaction verdict for the exact (repo, PR, issue) triple before it is kept -- see +// `resolveIssueLabelsForPropagation`'s own comment on that check. This persists one directly (mirroring +// exactly what `putCachedLinkedIssueSatisfaction` callers in `src/queue/processors.ts` write for a real +// "ok" + "addressed" assessment), so a test can grant the gate without spinning up a real model call. +async function publishAddressedSatisfactionVerdict(env: Env, repoFullName: string, pullNumber: number, issueNumber: number): Promise { + await putCachedLinkedIssueSatisfaction(env, repoFullName, pullNumber, "test-head-sha", issueNumber, "test-fingerprint", { + status: "ok", + result: { status: "addressed", rationale: "test fixture: this PR addresses the linked issue.", confidence: 0.9 }, + estimatedNeurons: 10, + }); +} + // `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 // by a literal-owner or ADMIN_GITHUB_LOGINS match) needs a real signable key or the mint throws before ever @@ -791,7 +805,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( }); describe("reward-label maintainer trust (#priority-reward-maintainer-trust)", () => { - it("REGRESSION (metagraphed PR #4554 / issue #3947 shape): a reward mapping with trustMaintainerAuthoredIssueForReward propagates alongside a routine trusted label from the SAME maintainer-authored issue, for a contributor who is neither its author nor assignee", async () => { + it("REGRESSION (metagraphed PR #4554 / issue #3947 shape): a reward mapping with trustMaintainerAuthoredIssueForReward propagates alongside a routine trusted label from the SAME maintainer-authored issue, for a contributor who is neither its author nor assignee, once a satisfaction verdict confirms the PR addresses it (#9077)", async () => { const mappings = [ { issueLabel: "gittensor:bug", prLabel: "gittensor:bug", removeOtherTypeLabels: true, trustMaintainerAuthoredIssue: true }, { issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false, trustMaintainerAuthoredIssueForReward: true }, @@ -803,11 +817,13 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( return new Response("not found", { status: 404 }); }); const env = createTestEnv({}); + await publishAddressedSatisfactionVerdict(env, "owner/repo", 4554, 3947); const result = await fetchLinkedIssueLabelsForPropagation({ env, repoFullName: "owner/repo", linkedIssues: [3947], installationId: 123, + prNumber: 4554, prAuthorLogin: "contrib", mappings, }); @@ -815,6 +831,60 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( expect(result.inconclusive).toBe(false); }); + it("#9077 REGRESSION GUARD: the SAME reward mapping/maintainer-authored-issue shape as the PR #4554 test above no longer propagates the reward label when NO satisfaction verdict has been published yet -- citing an untouched maintainer-authored issue is not, by itself, sufficient evidence", async () => { + const mappings = [ + { issueLabel: "gittensor:bug", prLabel: "gittensor:bug", removeOtherTypeLabels: true, trustMaintainerAuthoredIssue: true }, + { issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false, trustMaintainerAuthoredIssueForReward: true }, + ]; + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/3947")) + return Response.json({ number: 3947, state: "open", user: { login: "owner" }, assignees: [], labels: ["gittensor:bug", "gittensor:priority"] }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + // No publishAddressedSatisfactionVerdict call this time -- no verdict published at all. + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [3947], + installationId: 123, + prNumber: 4554, + prAuthorLogin: "contrib", + mappings, + }); + // The non-reward mapping (bug, trustMaintainerAuthoredIssue) is unaffected -- only the reward label + // is withheld. + expect(result.labels).toEqual(["gittensor:bug"]); + expect(result.inconclusive).toBe(false); + }); + + it("#9077: still withholds the reward label when a satisfaction verdict WAS published but reads 'unaddressed' or 'partial', not just when none exists at all", async () => { + const mappings = [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false, trustMaintainerAuthoredIssueForReward: true }]; + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/3952")) + return Response.json({ number: 3952, state: "open", user: { login: "owner" }, assignees: [], labels: ["gittensor:priority"] }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + await putCachedLinkedIssueSatisfaction(env, "owner/repo", 4555, "test-head-sha", 3952, "test-fingerprint", { + status: "ok", + result: { status: "partial", rationale: "test fixture: only partially addresses the issue.", confidence: 0.9 }, + estimatedNeurons: 10, + }); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [3952], + installationId: 123, + prNumber: 4555, + prAuthorLogin: "contrib", + mappings, + }); + expectPropagation(result, []); + }); + it("does NOT propagate the reward label from a maintainer-authored issue when its mapping has not opted into trustMaintainerAuthoredIssueForReward (unchanged strict default)", async () => { const mappings = [ { issueLabel: "gittensor:bug", prLabel: "gittensor:bug", removeOtherTypeLabels: true, trustMaintainerAuthoredIssue: true }, @@ -861,7 +931,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( expectPropagation(result, []); }); - it("still propagates the reward label via trustMaintainerAuthoredIssueForReward when the issue is authored by an ADMIN_GITHUB_LOGINS fleet-operator", async () => { + it("still propagates the reward label via trustMaintainerAuthoredIssueForReward when the issue is authored by an ADMIN_GITHUB_LOGINS fleet-operator, once a satisfaction verdict confirms the PR addresses it (#9077)", async () => { const mappings = [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false, trustMaintainerAuthoredIssueForReward: true }]; stubFetch((url) => { if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); @@ -870,17 +940,40 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( return new Response("not found", { status: 404 }); }); const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "fleetop" }); + await publishAddressedSatisfactionVerdict(env, "owner/repo", 4949, 3949); const result = await fetchLinkedIssueLabelsForPropagation({ env, repoFullName: "owner/repo", linkedIssues: [3949], installationId: 123, + prNumber: 4949, prAuthorLogin: "contrib", mappings, }); expectPropagation(result, ["gittensor:priority"]); }); + it("#9077: withholds the reward label from an ADMIN_GITHUB_LOGINS fleet-operator's own issue too when no satisfaction verdict has been published (the maintainer-trust relaxation alone is not enough)", async () => { + const mappings = [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false, trustMaintainerAuthoredIssueForReward: true }]; + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/3949")) + return Response.json({ number: 3949, state: "open", user: { login: "fleetop" }, assignees: [], labels: ["gittensor:priority"] }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "fleetop" }); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [3949], + installationId: 123, + prNumber: 4949, + prAuthorLogin: "contrib", + mappings, + }); + expectPropagation(result, []); + }); + it("does not propagate the reward label when the issue author is only a read-access collaborator (fails closed, same as trustMaintainerAuthoredIssue)", async () => { const mappings = [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false, trustMaintainerAuthoredIssueForReward: true }]; stubFetch((url) => { From c5d38aa4a962dbd5d6d4b02fa3df45b59ec828e0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:28:33 -0700 Subject: [PATCH 5/5] fix(gittensor): stop guessing a reward-bearing type label from title text once propagation is enabled (#9077) resolvePrTypeLabel fell through to deriveKindFromTitle (pure PR-title regex matching, entirely author-controlled) whenever linked-issue label propagation was enabled but produced no exclusive/additive match for a pass -- silently reintroducing the exact author-controlled-free-text classification a repo opted OUT of by turning propagation on. A contributor could pick their own gittensor:bug/gittensor:feature multiplier by title wording alone, on any repo running propagation, whenever the linked issue simply hadn't been triaged yet. resolvePrTypeLabel now has a distinct "propagation_unmatched" outcome for that case: no label is applied, and removeLabels still clears every configured category (so a stale prior label doesn't look like a confirmed classification). This only changes behavior for a repo that has explicitly enabled linkedIssueLabelPropagation -- the shipped default (propagation disabled, the overwhelming majority of installs) is completely unchanged, still title-derived exactly as before. This does NOT eliminate title-derived classification for the shipped default itself -- doing so would silently disable type labels for every repo that hasn't configured propagation with mappings covering bug/feature, which is a larger design decision (a real diff/changed-path classifier, or a default-behavior migration) better suited to its own follow-up given this issue's own "Long Term Features" milestone. See the PR description for the full disposition. --- .../src/settings/pr-type-label.ts | 29 ++++++++--- src/queue/processors.ts | 14 +++--- test/unit/pr-type-label-engine.test.ts | 48 +++++++++++++++---- test/unit/pr-type-label.test.ts | 48 +++++++++++++++---- 4 files changed, 107 insertions(+), 32 deletions(-) diff --git a/packages/loopover-engine/src/settings/pr-type-label.ts b/packages/loopover-engine/src/settings/pr-type-label.ts index 3ced5e21b..0813b92b5 100644 --- a/packages/loopover-engine/src/settings/pr-type-label.ts +++ b/packages/loopover-engine/src/settings/pr-type-label.ts @@ -108,7 +108,7 @@ export function normalizeTypeLabelSet(input: unknown, warnings: string[]): PrTyp export type PrTypeLabelDecision = { applyLabels: string[]; removeLabels: string[]; - source: "propagation_exclusive" | "propagation_additive" | "title"; + source: "propagation_exclusive" | "propagation_additive" | "title" | "propagation_unmatched"; }; /** @@ -125,11 +125,18 @@ export type PrTypeLabelDecision = { * - `removeOtherTypeLabels: false` (additive) — the mapped label is applied ALONGSIDE the * normal title-based bug/feature label, which is left untouched (e.g. a generic * `customer:vip` → `triage:vip` triage marker that has nothing to do with bug/feature/priority). - * 2. Otherwise, feature (feat/feature) / bug (everything else) by the conventional-commit title prefix - * -- ONLY when `labels` actually has a name registered for that built-in category; a configured set - * that omits `bug`/`feature` entirely (a self-hoster who only wants custom, propagation-driven - * categories, or an explicit `typeLabels: {}` resolved to zero categories) applies nothing for that - * branch rather than inventing a label name (#label-modularity). + * 2. When propagation is enabled but produced NEITHER an exclusive nor an additive match for this pass + * (the linked issue hasn't been triaged with a mapped label yet, or carries none), no label is applied + * at all (`source: "propagation_unmatched"`) -- #9077: falling back to a title guess here would silently + * reintroduce the exact author-controlled-free-text classification the repo opted OUT of by turning + * propagation on in the first place. This is a REPO-level opt-in consequence only: it never fires for a + * repo that hasn't configured `linkedIssueLabelPropagation.enabled` (see case 3 below), so it changes + * nothing for the shipped default. + * 3. Otherwise (propagation disabled/unconfigured), feature (feat/feature) / bug (everything else) by the + * conventional-commit title prefix -- ONLY when `labels` actually has a name registered for that + * built-in category; a configured set that omits `bug`/`feature` entirely (a self-hoster who only + * wants custom, propagation-driven categories, or an explicit `typeLabels: {}` resolved to zero + * categories) applies nothing for that branch rather than inventing a label name (#label-modularity). * `removeLabels` is always "every member of the configured type-label set that isn't one of * `applyLabels`" — generic and total over however many categories are configured, and safe even if a * misconfigured additive mapping's `prLabel` happens to collide with a type-label-set name (it is @@ -177,6 +184,16 @@ export function resolvePrTypeLabel(input: { const applyLabels = [exclusiveMatch ? exclusiveMatch.prLabel : titleLabel, ...additiveMatches.map((mapping) => mapping.prLabel)]; return decide(applyLabels, exclusiveMatch ? "propagation_exclusive" : "propagation_additive"); } + // #9077: this repo has opted OUT of title-derived classification for its reward-bearing categories by + // turning propagation on -- neither an exclusive nor an additive mapping matched this pass (the linked + // issue has no mapped label yet, or none at all), so there is no confirmed evidence to classify from. + // Falling through to `deriveKindFromTitle` here would defeat the entire point of enabling propagation: + // a contributor's own title wording would silently become the reward-multiplier source again, exactly + // the author-controlled-free-text problem propagation exists to remove. No type label is applied instead + // -- `removeLabels` below still clears every configured category, since "unclassified" must never leave + // a STALE label (e.g. from before the linked issue's labels changed, or before propagation was enabled) + // sitting on the PR looking like a confirmed classification. + return decide([], "propagation_unmatched"); } return decide([titleLabel], "title"); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3bd34e1c0..1e4002e3f 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -9893,14 +9893,12 @@ async function maybePublishPrPublicSurface( 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 a built-in category always - // falls back to its default rather than an empty string -- either DEFAULT_TYPE_LABELS directly - // for the config-as-code-only DB base (#6443), or normalizeTypeLabelSet's own fallback for a - // manifest override), 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 */ + // `|| "none"` IS reachable as of #9077: a `propagation_unmatched` decision (propagation enabled, + // but no exclusive/additive mapping matched this pass) deliberately resolves `applyLabels` to + // `[]` rather than falling back to a title guess -- every OTHER source still always resolves a + // non-empty label (deriveKindFromTitle only ever returns "bug"/"feature", and a built-in category + // always falls back to its default rather than an empty string; a matched propagation source + // only ever uses a mapping's `prLabel`, which normalizeMapping drops entirely when empty). detail: `applied labels: ${decisionResult.applyLabels.join(", ") || "none"}`, metadata: { labels: decisionResult.applyLabels, source: decisionResult.source }, }).catch(() => undefined); diff --git a/test/unit/pr-type-label-engine.test.ts b/test/unit/pr-type-label-engine.test.ts index 4480f83c3..8aff09aa7 100644 --- a/test/unit/pr-type-label-engine.test.ts +++ b/test/unit/pr-type-label-engine.test.ts @@ -49,14 +49,14 @@ describe("resolvePrTypeLabel (#priority-linked-issue-gate)", () => { expect(result).toEqual({ applyLabels: ["gittensor:priority"], removeLabels: [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature], source: "propagation_exclusive" }); }); - it("never invents priority: falls through to the title-based label when no linked issue carries the configured issue label", () => { + it("never invents priority, and (#9077) no longer falls through to a title guess either, when no linked issue carries the configured issue label", () => { const result = resolvePrTypeLabel({ title: "fix: y", linkedIssueLabels: ["unrelated-label"], propagation: propagation({ mappings: [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: true }] }), }); - expect(result.applyLabels).toEqual([DEFAULT_TYPE_LABELS.bug]); - expect(result.source).toBe("title"); + expect(result.applyLabels).toEqual([]); + expect(result.source).toBe("propagation_unmatched"); }); it("never invents priority: falls through to title-based even with matching linked-issue labels when propagation is disabled", () => { @@ -100,19 +100,49 @@ describe("resolvePrTypeLabel (#priority-linked-issue-gate)", () => { expect(result.source).toBe("propagation_additive"); }); - it("does not crash on an empty mappings array and falls through to title-based", () => { + it("does not crash on an empty mappings array; withholds a label instead of falling through to title (#9077)", () => { + // Propagation is enabled (the repo's own opt-out of title-derived reward classification) but there is + // nothing to match against at all -- falling through to the title here would silently reintroduce the + // exact author-controlled classification propagation exists to remove. const result = resolvePrTypeLabel({ title: "feat: add provider fallback", linkedIssueLabels: ["anything"], propagation: propagation({ mappings: [] }) }); - expect(result.applyLabels).toEqual([DEFAULT_TYPE_LABELS.feature]); - expect(result.source).toBe("title"); + expect(result).toEqual({ applyLabels: [], removeLabels: [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature, DEFAULT_TYPE_LABELS.priority], source: "propagation_unmatched" }); }); - it("does not crash when linkedIssueLabels is omitted entirely (propagation enabled with mappings configured)", () => { + it("does not crash when linkedIssueLabels is omitted entirely (propagation enabled with mappings configured); withholds a label (#9077)", () => { const result = resolvePrTypeLabel({ title: "feat: add provider fallback", propagation: propagation({ mappings: [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: true }] }), }); - expect(result.applyLabels).toEqual([DEFAULT_TYPE_LABELS.feature]); - expect(result.source).toBe("title"); + expect(result).toEqual({ applyLabels: [], removeLabels: [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature, DEFAULT_TYPE_LABELS.priority], source: "propagation_unmatched" }); + }); + + describe("propagation enabled but unmatched withholds the label entirely (#9077)", () => { + it("a linked issue carrying none of the configured issueLabels applies nothing and clears every configured category", () => { + const result = resolvePrTypeLabel({ + title: "feat: add provider fallback", // title alone would say "feature" -- must NOT be used as a fallback guess + linkedIssueLabels: ["unrelated-label"], + propagation: propagation({ + mappings: [ + { issueLabel: "gittensor:bug", prLabel: "gittensor:bug", removeOtherTypeLabels: true }, + { issueLabel: "gittensor:feature", prLabel: "gittensor:feature", removeOtherTypeLabels: true }, + { issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false }, + ], + }), + }); + expect(result).toEqual({ applyLabels: [], removeLabels: [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature, DEFAULT_TYPE_LABELS.priority], source: "propagation_unmatched" }); + }); + + it("clears a stale previously-applied label rather than leaving it looking like a confirmed classification", () => { + // Only bug/feature are configured (no priority category at all) -- removeLabels must still be exactly + // that configured subset, never the full built-in triad. + const result = resolvePrTypeLabel({ + title: "fix: y", + linkedIssueLabels: [], + labels: { bug: "gittensor:bug", feature: "gittensor:feature" }, + propagation: propagation({ mappings: [{ issueLabel: "gittensor:bug", prLabel: "gittensor:bug", removeOtherTypeLabels: true }] }), + }); + expect(result).toEqual({ applyLabels: [], removeLabels: ["gittensor:bug", "gittensor:feature"], source: "propagation_unmatched" }); + }); }); it("resolves the LAST matching exclusive mapping (highest declared precedence) when multiple linked-issue labels are present (#5385)", () => { diff --git a/test/unit/pr-type-label.test.ts b/test/unit/pr-type-label.test.ts index fc305dd41..f06f29751 100644 --- a/test/unit/pr-type-label.test.ts +++ b/test/unit/pr-type-label.test.ts @@ -42,14 +42,14 @@ describe("resolvePrTypeLabel (#priority-linked-issue-gate)", () => { expect(result).toEqual({ applyLabels: ["gittensor:priority"], removeLabels: [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature], source: "propagation_exclusive" }); }); - it("never invents priority: falls through to the title-based label when no linked issue carries the configured issue label", () => { + it("never invents priority, and (#9077) no longer falls through to a title guess either, when no linked issue carries the configured issue label", () => { const result = resolvePrTypeLabel({ title: "fix: y", linkedIssueLabels: ["unrelated-label"], propagation: propagation({ mappings: [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: true }] }), }); - expect(result.applyLabels).toEqual([DEFAULT_TYPE_LABELS.bug]); - expect(result.source).toBe("title"); + expect(result.applyLabels).toEqual([]); + expect(result.source).toBe("propagation_unmatched"); }); it("never invents priority: falls through to title-based even with matching linked-issue labels when propagation is disabled", () => { @@ -93,19 +93,49 @@ describe("resolvePrTypeLabel (#priority-linked-issue-gate)", () => { expect(result.source).toBe("propagation_additive"); }); - it("does not crash on an empty mappings array and falls through to title-based", () => { + it("does not crash on an empty mappings array; withholds a label instead of falling through to title (#9077)", () => { + // Propagation is enabled (the repo's own opt-out of title-derived reward classification) but there is + // nothing to match against at all -- falling through to the title here would silently reintroduce the + // exact author-controlled classification propagation exists to remove. const result = resolvePrTypeLabel({ title: "feat: add provider fallback", linkedIssueLabels: ["anything"], propagation: propagation({ mappings: [] }) }); - expect(result.applyLabels).toEqual([DEFAULT_TYPE_LABELS.feature]); - expect(result.source).toBe("title"); + expect(result).toEqual({ applyLabels: [], removeLabels: [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature, DEFAULT_TYPE_LABELS.priority], source: "propagation_unmatched" }); }); - it("does not crash when linkedIssueLabels is omitted entirely (propagation enabled with mappings configured)", () => { + it("does not crash when linkedIssueLabels is omitted entirely (propagation enabled with mappings configured); withholds a label (#9077)", () => { const result = resolvePrTypeLabel({ title: "feat: add provider fallback", propagation: propagation({ mappings: [{ issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: true }] }), }); - expect(result.applyLabels).toEqual([DEFAULT_TYPE_LABELS.feature]); - expect(result.source).toBe("title"); + expect(result).toEqual({ applyLabels: [], removeLabels: [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature, DEFAULT_TYPE_LABELS.priority], source: "propagation_unmatched" }); + }); + + describe("propagation enabled but unmatched withholds the label entirely (#9077)", () => { + it("a linked issue carrying none of the configured issueLabels applies nothing and clears every configured category", () => { + const result = resolvePrTypeLabel({ + title: "feat: add provider fallback", // title alone would say "feature" -- must NOT be used as a fallback guess + linkedIssueLabels: ["unrelated-label"], + propagation: propagation({ + mappings: [ + { issueLabel: "gittensor:bug", prLabel: "gittensor:bug", removeOtherTypeLabels: true }, + { issueLabel: "gittensor:feature", prLabel: "gittensor:feature", removeOtherTypeLabels: true }, + { issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false }, + ], + }), + }); + expect(result).toEqual({ applyLabels: [], removeLabels: [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature, DEFAULT_TYPE_LABELS.priority], source: "propagation_unmatched" }); + }); + + it("clears a stale previously-applied label rather than leaving it looking like a confirmed classification", () => { + // Only bug/feature are configured (no priority category at all) -- removeLabels must still be exactly + // that configured subset, never the full built-in triad. + const result = resolvePrTypeLabel({ + title: "fix: y", + linkedIssueLabels: [], + labels: { bug: "gittensor:bug", feature: "gittensor:feature" }, + propagation: propagation({ mappings: [{ issueLabel: "gittensor:bug", prLabel: "gittensor:bug", removeOtherTypeLabels: true }] }), + }); + expect(result).toEqual({ applyLabels: [], removeLabels: ["gittensor:bug", "gittensor:feature"], source: "propagation_unmatched" }); + }); }); it("resolves the LAST matching exclusive mapping (highest declared precedence) when multiple linked-issue labels are present (#5385)", () => {