feat(review): scope the live auto-tune breaker to miner-originated PRs independently (#2352) - #5061
Merged
JSONbored merged 1 commit intoJul 11, 2026
Conversation
…s independently 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.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
1 similar comment
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5061 +/- ##
=======================================
Coverage 94.13% 94.13%
=======================================
Files 465 465
Lines 39542 39554 +12
Branches 14431 14433 +2
=======================================
+ Hits 37223 37235 +12
Misses 1664 1664
Partials 655 655
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2352.
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.On the issue's own premise: #2352 asks to scope via
computeGateEval's existingsourcefilter, but that doesn't hold up --sourceonly ever distinguishes WHICH REVIEW ENGINE decided (currently always'gittensory-native'in live use), not who authored the PR. I flagged this explicitly to the user before writing any code (a factually-incorrect issue premise requiring live D1 schema surgery on a table with a deliberate privacy design felt like it warranted confirmation, not a unilateral call) and got the go-ahead to build it correctly. The real signal --confirmedContributor, an official-Gittensor-miner check via the live subnet API -- was already computed inprocessors.tsright wheregate_decisionrows get written, just never threaded through.What changed
migrations/0144: addsreview_audit.miner_authored-- a coarse, non-identifying boolean CATEGORY (not a login), deliberately preservingreview_audit's own design ("no actor-identifying data", since it feeds the anonymized cross-instance export -- see migration 0049's own comment). It reveals no more than "was this PR's author, at decision time, a confirmed official Gittensor miner."parity-wire.ts:recordNativeGateDecisiontakes an optionalminerAuthoredfield, written alongside the existing (unchanged)sourcecolumn.processors.ts: threads the already-in-scopeconfirmedContributorthrough at the onegate_decisionwrite site (3 lines).parity.ts:computeGateEvalgains an optionalminerOnlyfilter, orthogonal tosource(both AND together when both are set). Omitted -- every pre-maintainer: wire calibration accuracy into the live auto-tune circuit-breaker for miner-originated PRs #2352 caller -- is byte-identical to before.outcomes-wire.ts:runSelfTuneBreakernow runs a SECOND, miner-scopedcomputeGateEvalpass alongside the existing one, then re-keys its rows with a:minersuffix before running them through the exact sameapplyAutoTune/applyCloseAutoTune/maybeAutoClear*primitives used for the existing pass. Every one of those (pluscreateFlagStoreandlistEngagedProjectScopes) is already fully generic over an opaqueprojectstring, so the distinct flag scope (holdonly:<project>:miner) falls out naturally with zero changes toauto-tune.tsitself -- it stays the "byte-faithful port" its own doc comment says it is. Refactored the existing engage+log+autoclear sequence into a sharedrunBreakerPassForReporthelper, run once per scope, so the human/mixed pass's behavior is preserved byte-for-byte (identical event names, identical log shape) while the miner pass reuses the identical logic under a"miner_"event prefix.Important nuance discovered while writing tests: the existing/unscoped pass is NOT disjoint from miner-authored data -- it has no
miner_authoredfilter, so it counts every prediction for a project regardless of authorship. This preserves that pass's existing meaning (overall accuracy, unchanged, per the issue's own "preserve every existing invariant" deliverable) but means a project's miner-authored rows are counted in BOTH the mixed population and the miner-only subset. My regression tests account for this (see the doc comment at the top of the new testdescribeblock inoutcomes-wire.test.ts).Validation
npm run typecheck-- clean.npm run db:migrations:check-- 147 migrations OK, contiguous through 0144.npm run db:schema-drift:check-- clean (review_auditis a raw-SQL-only allowlisted table, not Drizzle-managed, so noschema.tschange needed).parity.test.ts+parity-wire.test.ts+outcomes-wire.test.ts; 308/308 across the broader affected suite (addsqueue-lifecycle-guards.test.ts).parity.ts/parity-wire.ts's only gaps are incomputeGateParity/computeParityReadiness(functions I never touched, already marked/* v8 ignore next */).outcomes-wire.tshas one pre-existing gap (the outer catch block) -- confirmed pre-existing, not introduced by this PR, by stashing my changes and re-measuring against origin/main's version: branch coverage was 89.42% both before and after (identical), proving my diff adds zero new gaps.Test plan
Per the issue's own explicit deliverable:
recordNativeGateDecisioncorrectly recordsminer_authoredend-to-end through a realprocessJobwebhook, for both a confirmed-miner author and a non-confirmed one.