Skip to content

Commit 3832ee3

Browse files
authored
feat(review): scope the live auto-tune breaker to miner-originated PRs independently (#5061)
Extends the existing self-tightening precision circuit-breaker (src/review/auto-tune.ts) so a miner fleet's own self-review accuracy trips the SAME safety breaker independently of the maintainer's overall (mixed) review-stack accuracy, without ever blanket-penalizing human-submitted PRs to the same repo. Issue #2352's own premise (scope via computeGateEval's existing `source` filter) does not hold: `source` only ever distinguishes WHICH REVIEW ENGINE decided (currently always 'gittensory-native'), not who authored the PR. The real signal -- `confirmedContributor`, an official-Gittensor-miner check via the live subnet API -- was already computed in processors.ts right where gate_decision rows get written, but never threaded through. - migrations/0144: adds `review_audit.miner_authored` -- a coarse, non-identifying boolean category (NOT a login), preserving review_audit's own deliberate "no actor-identifying data" design (it feeds the anonymized cross-instance export). - parity-wire.ts: recordNativeGateDecision takes an optional `minerAuthored` field, written alongside the existing (unchanged) `source` column. - processors.ts: threads the already-in-scope `confirmedContributor` through at the one gate_decision write site. - parity.ts: computeGateEval gains an optional `minerOnly` filter, orthogonal to `source`. Omitted (every pre-#2352 caller) is byte-identical to before. - outcomes-wire.ts: runSelfTuneBreaker now runs a SECOND, miner-scoped computeGateEval pass alongside the existing one, then re-keys its rows with a `:miner` suffix before running them through the SAME applyAutoTune/applyCloseAutoTune/maybeAutoClear* primitives used for the existing pass. Every one of those (plus createFlagStore and listEngagedProjectScopes) is already fully generic over an opaque `project` string, so the distinct flag scope (holdonly:<project>:miner) falls out naturally with ZERO changes to auto-tune.ts itself. Refactored the existing engage+log+autoclear sequence into a shared runBreakerPassForReport helper, run once per scope, so the human/mixed pass's behavior is preserved byte-for-byte (same event names, same log shape) while the miner pass reuses the identical logic under a "miner_" event prefix. IMPORTANT, discovered while testing: the existing/unscoped pass is NOT disjoint from miner-authored data -- it has no miner_authored filter, so it counts every prediction for a project, miner-authored or not. This preserves that pass's existing meaning (overall accuracy, unchanged) but means a project's miner-authored rows are counted in BOTH the mixed population and the miner-only subset. Regression-tested per the issue's own deliverable: the miner-scoped breaker fires independently of the existing one in both directions, the CLOSE-side mirror does too, and clearing one flag (via cooldown + precision recovery) never clears the other -- both merge- and close-side, both directions.
1 parent b1b57c7 commit 3832ee3

8 files changed

Lines changed: 412 additions & 66 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
-- #2352: extend the live auto-tune circuit-breaker (src/review/auto-tune.ts) to consider miner-originated PRs
2+
-- as a distinguishable population, so a miner fleet's own self-review accuracy trips the SAME safety breaker
3+
-- independently of the maintainer's overall (mixed) review-stack accuracy.
4+
--
5+
-- review_audit (migration 0049) deliberately carries NO actor-identifying data (it feeds the anonymized
6+
-- cross-instance export in src/selfhost/orb-collector.ts -- see that migration's own "Privacy: ... No actor
7+
-- logins" comment). This column preserves that: it is a coarse, non-identifying miner/non-miner CATEGORY, not
8+
-- a login -- it reveals no more than "was this PR's author, at decision time, a confirmed official Gittensor
9+
-- miner" (src/queue/processors.ts's `confirmedContributor`, itself an aggregate boolean from the live
10+
-- Gittensor subnet API, not a stored identity).
11+
--
12+
-- Written by src/review/parity-wire.ts's recordNativeGateDecision alongside the existing `source` column (that
13+
-- write is UNCHANGED -- this is a strictly additive column, so every existing read of review_audit, including
14+
-- computeGateEval's current unscoped/source-scoped passes, is byte-identical unless it opts into the new
15+
-- `minerOnly` filter). Read by src/review/parity.ts's computeGateEval when its new `minerOnly` option is set.
16+
ALTER TABLE review_audit ADD COLUMN miner_authored INTEGER NOT NULL DEFAULT 0;

src/queue/processors.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9200,6 +9200,9 @@ async function maybePublishPrPublicSurface(
92009200
headSha: pr.headSha,
92019201
conclusion: gateEvaluation.conclusion,
92029202
reasonCode,
9203+
// #2352: lets the live auto-tune breaker (src/review/outcomes-wire.ts's runSelfTuneBreaker) scope a
9204+
// SEPARATE precision read to miner-originated PRs, independently of the maintainer's overall accuracy.
9205+
minerAuthored: confirmedContributor,
92039206
});
92049207
// #2349 (PR 1): additive per-contributor calibration data, mirroring recordNativeGateDecision's own
92059208
// action derivation above so both writers agree on whether this conclusion is a comparable decision --

src/review/outcomes-wire.ts

Lines changed: 118 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,91 @@ const BREAKER_EVAL_WINDOW_DAYS = 90;
513513
* break the cron). With no pr_outcome history the eval reads neutral → nothing engages → byte-identical. The
514514
* close breaker is INERT until selftune is enabled AND close-outcome data is present, exactly like its merge twin.
515515
*/
516+
// #2352: the flag-scope suffix that makes a miner-originated project's breaker flags (holdonly:<project>:miner
517+
// / closehold:<project>:miner) DISTINCT from the same project's human/mixed-population flags. Every downstream
518+
// primitive that keys on `project` -- applyAutoTune/applyCloseAutoTune/maybeAutoClear* (auto-tune.ts),
519+
// createFlagStore, listEngagedProjectScopes -- is already fully generic over that opaque string, so re-keying
520+
// a report's rows with this suffix is the ENTIRE mechanism; none of those primitives needed to change.
521+
const MINER_BREAKER_SCOPE_SUFFIX = ":miner";
522+
523+
function minerBreakerScope(project: string): string {
524+
return `${project}${MINER_BREAKER_SCOPE_SUFFIX}`;
525+
}
526+
527+
/** Run the full engage + auto-clear sequence for one {@link GateEvalReport} (either the plain project-keyed
528+
* report or a miner-rescoped one). `eventPrefix` namespaces the emitted log events (`""` for the existing
529+
* human/mixed pass, `"miner_"` for the #2352 miner-scoped pass) so an operator can tell which population
530+
* triggered a given line. `engagedHoldonly`/`engagedClosehold` are this SAME scope's already-engaged flags
531+
* (the caller pre-splits {@link listEngagedProjectScopes}'s result by scope) -- passing the WRONG scope's
532+
* engaged list here would auto-clear a flag using the other population's precision, which is exactly the
533+
* cross-scope leak #2352 exists to prevent. */
534+
async function runBreakerPassForReport(
535+
flags: FlagStore,
536+
report: GateEvalReport,
537+
engagedHoldonly: readonly string[],
538+
engagedClosehold: readonly string[],
539+
nowMs: number,
540+
eventPrefix: string,
541+
): Promise<void> {
542+
const engaged = await applyAutoTune(flags, report);
543+
for (const action of engaged) {
544+
console.error(
545+
JSON.stringify({
546+
level: "error",
547+
event: `${eventPrefix}breaker_engaged`,
548+
project: action.project,
549+
mergePrecision: action.mergePrecision,
550+
decided: action.decided,
551+
floor: AUTOTUNE_MERGE_PRECISION_FLOOR,
552+
}),
553+
);
554+
}
555+
// CLOSE-side breaker: engage closehold for any repo whose close precision dropped below the floor.
556+
const closeEngaged = await applyCloseAutoTune(flags, report);
557+
for (const action of closeEngaged) {
558+
console.error(
559+
JSON.stringify({
560+
level: "error",
561+
event: `${eventPrefix}close_breaker_engaged`,
562+
project: action.project,
563+
closePrecision: action.closePrecision,
564+
decided: action.decided,
565+
floor: AUTOTUNE_CLOSE_PRECISION_FLOOR,
566+
}),
567+
);
568+
}
569+
// OBSERVABILITY: a single summary line of the engaged close-hold backlog so a human can see, at a glance,
570+
// how many (and which) repos are currently holding would-closes for review. Only emitted when ≥1 engaged.
571+
if (closeEngaged.length > 0) {
572+
console.error(
573+
JSON.stringify({
574+
level: "error",
575+
event: `${eventPrefix}closehold_backlog`,
576+
count: closeEngaged.length,
577+
projects: closeEngaged.map((a) => a.project),
578+
}),
579+
);
580+
}
581+
// Auto-clear any auto-engaged breaker (merge AND close) that has cooled down + recovered. Candidates are the
582+
// UNION of report.rows (projects with a fresh decided sample) and every project currently holding a
583+
// per-project flag IN THIS SCOPE (#autoclear-deadlock) — a project whose breaker is suppressing 100% of its
584+
// merges/closes stops producing new decided samples for THAT action class and can drop out of report.rows
585+
// entirely, which would otherwise strand its flag engaged forever regardless of how long the cooldown has
586+
// elapsed.
587+
const mergeClearCandidates = new Set([...report.rows.map((row) => row.project), ...engagedHoldonly]);
588+
const closeClearCandidates = new Set([...report.rows.map((row) => row.project), ...engagedClosehold]);
589+
for (const project of mergeClearCandidates) {
590+
if (await maybeAutoClearHoldOnly(flags, report, project, nowMs)) {
591+
console.log(JSON.stringify({ event: `${eventPrefix}breaker_auto_cleared`, project }));
592+
}
593+
}
594+
for (const project of closeClearCandidates) {
595+
if (await maybeAutoClearCloseHoldOnly(flags, report, project, nowMs)) {
596+
console.log(JSON.stringify({ event: `${eventPrefix}close_breaker_auto_cleared`, project }));
597+
}
598+
}
599+
}
600+
516601
export async function runSelfTuneBreaker(env: Env): Promise<void> {
517602
try {
518603
const nowMs = Date.now();
@@ -521,64 +606,41 @@ export async function runSelfTuneBreaker(env: Env): Promise<void> {
521606
nowMs,
522607
source: GITTENSORY_NATIVE_SOURCE,
523608
});
609+
// #2352: a SEPARATE, miner-scoped pass so a miner fleet's own self-review accuracy trips the SAME breaker
610+
// independently of the maintainer's overall (mixed) accuracy. Re-keying every row's `project` with the
611+
// `:miner` suffix (see MINER_BREAKER_SCOPE_SUFFIX's own doc comment) is what makes every downstream
612+
// primitive naturally produce a DISTINCT flag, with zero changes to auto-tune.ts itself.
613+
const minerReportRaw = await computeGateEval(env, {
614+
days: BREAKER_EVAL_WINDOW_DAYS,
615+
nowMs,
616+
source: GITTENSORY_NATIVE_SOURCE,
617+
minerOnly: true,
618+
});
619+
const minerReport: GateEvalReport = {
620+
hasSignal: minerReportRaw.hasSignal,
621+
rows: minerReportRaw.rows.map((row) => ({ ...row, project: minerBreakerScope(row.project) })),
622+
};
623+
524624
const flags = createFlagStore(env);
525-
const engaged = await applyAutoTune(flags, report);
526-
for (const action of engaged) {
527-
console.error(
528-
JSON.stringify({
529-
level: "error",
530-
event: "breaker_engaged",
531-
project: action.project,
532-
mergePrecision: action.mergePrecision,
533-
decided: action.decided,
534-
floor: AUTOTUNE_MERGE_PRECISION_FLOOR,
535-
}),
536-
);
537-
}
538-
// CLOSE-side breaker: engage closehold for any repo whose close precision dropped below the floor.
539-
const closeEngaged = await applyCloseAutoTune(flags, report);
540-
for (const action of closeEngaged) {
541-
console.error(
542-
JSON.stringify({
543-
level: "error",
544-
event: "close_breaker_engaged",
545-
project: action.project,
546-
closePrecision: action.closePrecision,
547-
decided: action.decided,
548-
floor: AUTOTUNE_CLOSE_PRECISION_FLOOR,
549-
}),
550-
);
551-
}
552-
// OBSERVABILITY: a single summary line of the engaged close-hold backlog so a human can see, at a glance,
553-
// how many (and which) repos are currently holding would-closes for review. Only emitted when ≥1 engaged.
554-
if (closeEngaged.length > 0) {
555-
console.error(
556-
JSON.stringify({
557-
level: "error",
558-
event: "closehold_backlog",
559-
count: closeEngaged.length,
560-
projects: closeEngaged.map((a) => a.project),
561-
}),
562-
);
563-
}
564-
// Auto-clear any auto-engaged breaker (merge AND close) that has cooled down + recovered. Candidates are the
565-
// UNION of report.rows (projects with a fresh decided sample) and every project currently holding a
566-
// per-project flag (#autoclear-deadlock) — a project whose breaker is suppressing 100% of its merges/closes
567-
// stops producing new decided samples for THAT action class and can drop out of report.rows entirely, which
568-
// would otherwise strand its flag engaged forever regardless of how long the cooldown has elapsed.
569625
const engagedScopes = await listEngagedProjectScopes(env);
570-
const mergeClearCandidates = new Set([...report.rows.map((row) => row.project), ...engagedScopes.holdonly]);
571-
const closeClearCandidates = new Set([...report.rows.map((row) => row.project), ...engagedScopes.closehold]);
572-
for (const project of mergeClearCandidates) {
573-
if (await maybeAutoClearHoldOnly(flags, report, project, nowMs)) {
574-
console.log(JSON.stringify({ event: "breaker_auto_cleared", project }));
575-
}
576-
}
577-
for (const project of closeClearCandidates) {
578-
if (await maybeAutoClearCloseHoldOnly(flags, report, project, nowMs)) {
579-
console.log(JSON.stringify({ event: "close_breaker_auto_cleared", project }));
580-
}
581-
}
626+
const isMinerScope = (project: string): boolean => project.endsWith(MINER_BREAKER_SCOPE_SUFFIX);
627+
628+
await runBreakerPassForReport(
629+
flags,
630+
report,
631+
engagedScopes.holdonly.filter((project) => !isMinerScope(project)),
632+
engagedScopes.closehold.filter((project) => !isMinerScope(project)),
633+
nowMs,
634+
"",
635+
);
636+
await runBreakerPassForReport(
637+
flags,
638+
minerReport,
639+
engagedScopes.holdonly.filter(isMinerScope),
640+
engagedScopes.closehold.filter(isMinerScope),
641+
nowMs,
642+
"miner_",
643+
);
582644
} catch (error) {
583645
console.warn(
584646
JSON.stringify({

src/review/parity-wire.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,19 @@ type ParityRecorderEnv = {
137137
*/
138138
export async function recordNativeGateDecision(
139139
env: ParityRecorderEnv,
140-
input: { project: string; pullNumber: number; headSha: string | null | undefined; conclusion: GateCheckConclusion; reasonCode?: string | null | undefined; action?: GateAction | undefined },
140+
input: {
141+
project: string;
142+
pullNumber: number;
143+
headSha: string | null | undefined;
144+
conclusion: GateCheckConclusion;
145+
reasonCode?: string | null | undefined;
146+
action?: GateAction | undefined;
147+
/** #2352: true when the PR's author is a confirmed official Gittensor miner (processors.ts's
148+
* `confirmedContributor`) at decision time. A coarse, non-identifying category -- NOT a login -- so this
149+
* stays within review_audit's own "no actor-identifying data" design (see migration 0144's own comment).
150+
* Omitted defaults to `false` (not miner-originated), matching every pre-#2352 caller unchanged. */
151+
minerAuthored?: boolean | undefined;
152+
},
141153
): Promise<void> {
142154
// Self-hosted instances always record (their own local DB; exportOrbBatch needs this data). The cloud
143155
// worker keeps the exact flag-gated, byte-identical-when-off contract.
@@ -148,16 +160,17 @@ export async function recordNativeGateDecision(
148160
const project = input.project.slice(0, 200);
149161
const targetId = `${project}#${input.pullNumber}`;
150162
const summary = input.reasonCode ? input.reasonCode.slice(0, 200) : null;
163+
const minerAuthored = input.minerAuthored === true ? 1 : 0;
151164
try {
152165
// Deterministic id per (source, project, pr, sha): a re-run at the SAME commit REPLACES its prior decision
153166
// (the latest finalize wins), while a new commit gets its own row. event_type/source default in the schema
154167
// but are written explicitly for clarity.
155168
await env.DB.prepare(
156-
`INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, created_at)
157-
VALUES (?, ?, ?, 'gate_decision', ?, ?, ?, ?, ?)
158-
ON CONFLICT(id) DO UPDATE SET decision = excluded.decision, summary = excluded.summary, created_at = excluded.created_at`,
169+
`INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, miner_authored, created_at)
170+
VALUES (?, ?, ?, 'gate_decision', ?, ?, ?, ?, ?, ?)
171+
ON CONFLICT(id) DO UPDATE SET decision = excluded.decision, summary = excluded.summary, miner_authored = excluded.miner_authored, created_at = excluded.created_at`,
159172
)
160-
.bind(`gate:${GITTENSORY_NATIVE_SOURCE}:${targetId}@${input.headSha}`, project, targetId, action, GITTENSORY_NATIVE_SOURCE, input.headSha, summary, nowIso())
173+
.bind(`gate:${GITTENSORY_NATIVE_SOURCE}:${targetId}@${input.headSha}`, project, targetId, action, GITTENSORY_NATIVE_SOURCE, input.headSha, summary, minerAuthored, nowIso())
161174
.run();
162175
} catch (error) {
163176
// Telemetry must never break finalization.

src/review/parity.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,17 +84,22 @@ export const REVERSAL_DISCOUNT_WEIGHT = 0;
8484
* pr_outcome (ground truth) is the human's realized merge/close, so it is NOT source-scoped — both
8585
* systems are graded against the same answer key. Also LEFT JOINs a reversal existence check (#2348) so the
8686
* fold below can additionally compute weightedMergeConfirmed/weightedCloseConfirmed alongside the existing
87-
* raw counts — see REVERSAL_DISCOUNT_WEIGHT's doc comment for the formula. */
88-
export async function computeGateEval(env: Env, opts: { days: number; nowMs: number; source?: string }): Promise<GateEvalReport> {
87+
* raw counts — see REVERSAL_DISCOUNT_WEIGHT's doc comment for the formula.
88+
* `minerOnly` (#2352) additionally scopes the PREDICTION side to rows recorded with `miner_authored = 1`
89+
* (migration 0144) — orthogonal to `source`: both filters AND together when both are set. Ground truth stays
90+
* unscoped either way (same answer key). Omitted (the default, and every pre-#2352 caller) is byte-identical
91+
* to before this option existed. */
92+
export async function computeGateEval(env: Env, opts: { days: number; nowMs: number; source?: string; minerOnly?: boolean }): Promise<GateEvalReport> {
8993
const days = Number.isFinite(opts.days) && opts.days > 0 ? Math.min(opts.days, 730) : 90;
9094
const fromIso = new Date(opts.nowMs - days * 86_400_000).toISOString().slice(0, 10);
9195
// SQLite "bare column with MAX()" picks the column from the max-created_at row → the LATEST decision /
9296
// outcome per target (a reopened+reclosed PR keeps its final state).
9397
const sourceFilter = opts.source ? "AND source = ?" : "";
98+
const minerFilter = opts.minerOnly ? "AND miner_authored = 1" : "";
9499
const sql = `
95100
WITH gd AS (
96101
SELECT target_id, project, decision AS pred, MAX(created_at) AS t
97-
FROM review_audit WHERE event_type = 'gate_decision' AND decision IS NOT NULL AND created_at >= ? ${sourceFilter}
102+
FROM review_audit WHERE event_type = 'gate_decision' AND decision IS NOT NULL AND created_at >= ? ${sourceFilter} ${minerFilter}
98103
GROUP BY target_id
99104
),
100105
po AS (

0 commit comments

Comments
 (0)