Skip to content

feat(review): discount reverted/reopened outcomes in the gate-eval fold - #4169

Merged
JSONbored merged 1 commit into
mainfrom
feat/value-weighted-gate-eval-2348
Jul 8, 2026
Merged

feat(review): discount reverted/reopened outcomes in the gate-eval fold#4169
JSONbored merged 1 commit into
mainfrom
feat/value-weighted-gate-eval-2348

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • computeGateEval's mergePrecision/closePrecision treats every PR as equally weighted — a miner (or the fleet) could game the accuracy number by producing high volumes of barely-passing PRs that later get reverted, since a later revert never affects the "was this prediction correct" measurement.
  • Adds weightedMergeConfirmed/weightedCloseConfirmed/weightedMergePrecision/weightedClosePrecision to GateEvalRow, additive alongside the existing raw fields — no existing field's meaning or value changes for any current consumer.
  • A merge/close whose target is later marked reversal_reverted/reversal_reopened (the existing signal recordReversalSignals in src/review/outcomes-wire.ts already writes) gets its credit toward the weighted-CONFIRMED bucket discounted by REVERSAL_DISCOUNT_WEIGHT — a new, hardcoded, documented module constant (currently 0, full discount), not read from env/config, per the issue's explicit "not silently tunable at runtime" requirement.
  • The denominator (wouldMerge/wouldClose — how many merge/close predictions were made) is unchanged; only the credit for an outcome that didn't hold up is discounted. This means weightedMergePrecision <= mergePrecision always, and weightedClosePrecision <= closePrecision always.
  • SQL: added a rev CTE (SELECT DISTINCT target_id FROM review_audit WHERE event_type IN ('reversal_reverted', 'reversal_reopened')) LEFT JOINed into the existing gd ⨝ po query, with reversed added as an extra GROUP BY dimension so the JS fold can apply the discount per-cell.

Scope

Validation

  • git diff --check
  • npm run typecheck
  • npm run docs:drift-check — unaffected (no *GateMode/flag/command surface touched; this is a scoring-definition constant, not a config field).
  • npx vitest run test/unit/parity.test.ts — 36/36 passing (30 pre-existing + 6 new). All 30 pre-existing tests pass unchanged — old test fixtures have no reversed field at all, so c.reversed is undefined (falsy) and every existing fixture naturally falls through to unweighted behavior, proving byte-identical backward compatibility rather than just asserting it.
  • New tests cover: a zero-reversal fixture producing weighted precision identical to raw precision (explicit backward-compat proof); a reversed merge being discounted while wouldMerge/mergeConfirmed/mergePrecision stay untouched; the same for a reversed (reopened) close; a reversed mergeFalse/closeFalse cell contributing nothing to either weighted bucket (reversal only ever discounts an already-CONFIRMED credit, never creates negative credit); weighted precisions staying null (not 0/0) with no merge/close predictions at all; and a pinned-value test on REVERSAL_DISCOUNT_WEIGHT itself so a future silent formula change shows up as a failing test, not a silent behavior shift.
  • Scoped coverage on src/review/parity.ts: 100% lines, 99.07% statements, 97.61% branches — the two remaining "uncovered" spots (lines with pre-existing /* v8 ignore next */ pragmas) are inside computeGateParity, a function this PR does not touch (confirmed via git diff — zero changes in that function's body), so they don't affect this PR's patch coverage.
  • npm run test:workers, build:mcp, test:mcp-pack, ui:openapi:check, ui:lint, ui:typecheck, ui:build, npm audit — not re-run locally (no worker/MCP/OpenAPI/UI-component code touched); CI runs them authoritatively.

Safety

  • No secrets, wallet/hotkey/trust-score/reward data anywhere.
  • No behavior change for any EXISTING consumer — the raw fields are computed identically to before, and nothing yet reads the new weighted fields (see Notes).
  • No UI changes — no UI Evidence section needed.
  • No docs/changelog changes needed.

Notes

  • Deliberately out of scope, per the issue's own framing: re-verifying src/review/auto-tune.ts's planAutoTune/planCloseAutoTune against the new weighted fields. They continue reading the raw mergePrecision/closePrecision fields unchanged in this PR — switching the auto-tune circuit-breaker to the weighted definition is a deliberate, separate maintainer decision (the issue explicitly calls out that downstream consumers "must be re-verified against the new definition before cutover"), not something to fold silently into the fold-logic change itself.
  • Rollout note (the issue's 4th deliverable): "accuracy" as reported by any existing dashboard/report consuming computeGateEval (e.g. src/services/review-recap.ts) keeps its historical (raw, unweighted) meaning unless a future change explicitly switches it to read weightedMergePrecision/weightedClosePrecision instead. This PR does not flip that switch anywhere.
  • REVERSAL_DISCOUNT_WEIGHT = 0 (full discount) was chosen over a partial-credit value as the most defensible, auditable default: it makes gaming via later-reverted volume completely unprofitable rather than merely more expensive, with no magic partial-credit number to justify. The constant's own doc comment notes how to change it (bump the value + comment, never make it runtime-configurable) if the maintainer later wants partial credit instead.

computeGateEval's mergePrecision/closePrecision treats every PR as equally
weighted, so a miner (or the fleet) could game the accuracy number with
high volumes of barely-passing PRs that later get reverted. Adds
weightedMergeConfirmed/weightedCloseConfirmed alongside the existing raw
counts, discounting a merge/close later marked reversal_reverted/
reversal_reopened by REVERSAL_DISCOUNT_WEIGHT (a hardcoded, documented
constant -- not runtime-tunable, per the issue's auditability requirement).
The denominator (wouldMerge/wouldClose) is unchanged; only the credit for
an outcome that didn't hold up is discounted. Raw fields are byte-identical
to today for every existing consumer -- this is purely additive.

Does not change what auto-tune.ts reads; that re-verification is explicit
follow-up work per the issue, not bundled here.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.74%. Comparing base (8fe552b) to head (b96c568).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4169   +/-   ##
=======================================
  Coverage   93.73%   93.74%           
=======================================
  Files         387      387           
  Lines       36304    36312    +8     
  Branches    13298    13301    +3     
=======================================
+ Hits        34031    34039    +8     
  Misses       1617     1617           
  Partials      656      656           
Files with missing lines Coverage Δ
src/review/parity.ts 98.01% <100.00%> (+0.17%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 8, 2026
@loopover-orb

loopover-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-08 09:18:20 UTC

2 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This PR adds additive, backward-compatible weighted-precision fields to computeGateEval that discount a merge/close's credit when the target was later reverted or reopened, via a new `rev` CTE LEFT JOINed into the existing gate-eval query and a hardcoded REVERSAL_DISCOUNT_WEIGHT constant. The fold logic is correct: raw buckets (mergeConfirmed/closeConfirmed/wouldMerge/wouldClose) are untouched, and weightedN is 0 only when reversed=1 given the current weight of 0, so weightedMergePrecision <= mergePrecision holds as documented. Tests cover backward-compat (no `reversed` field), the discount path for both merge and close, the false-bucket non-contribution case, and the null-denominator case, all exercising the real fold path against mocked D1 results shaped like the actual SQL output.

Nits — 5 non-blocking
  • With REVERSAL_DISCOUNT_WEIGHT hardcoded at 0, expressions like `6 + 4 * REVERSAL_DISCOUNT_WEIGHT` in test/unit/parity.test.ts always reduce to `6` regardless of whether the fold multiplies, adds, or otherwise mishandles the discount — the tests currently can't distinguish `c.n * REVERSAL_DISCOUNT_WEIGHT` from any other formula that also yields 0; consider also asserting on a locally-overridden weight or asserting the exact multiplication expression via a non-zero test constant to keep the assertions meaningful if REVERSAL_DISCOUNT_WEIGHT is later bumped.
  • The doc comment on REVERSAL_DISCOUNT_WEIGHT (src/review/parity.ts:85) claims weightedMergePrecision/weightedClosePrecision can 'only ever be <= the raw precision, never higher,' but nothing in the code enforces the constant stays within [0,1] — if a future edit bumps it above 1, that invariant silently breaks with no test or assertion catching it.
  • I can't verify from the provided context that `recordReversalSignals` in src/review/outcomes-wire.ts actually writes `reversal_reverted`/`reversal_reopened` event_type rows against `target_id` matching the `gd`/`po` join key used here — worth confirming the event_type strings and target_id shape line up exactly with what the new `rev` CTE queries.
  • The `rev` CTE plus its LEFT JOIN and extra `reversed` GROUP BY dimension could alternatively be expressed as a correlated `EXISTS` subquery in the SELECT list, avoiding the extra JOIN and GROUP BY cardinality increase — minor style/perf preference, not a real regression at current data volumes.
  • Consider adding one test where REVERSAL_DISCOUNT_WEIGHT is temporarily monkey-patched or the fold logic is exercised with a non-zero simulated weight, so the weighting formula itself (not just its zero-collapsed output) is verified.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 52 registered-repo PR(s), 43 merged, 486 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 52 PR(s), 486 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 52 PR(s), 486 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 8, 2026
@JSONbored
JSONbored merged commit ab13a1b into main Jul 8, 2026
10 checks passed
@JSONbored
JSONbored deleted the feat/value-weighted-gate-eval-2348 branch July 8, 2026 09:26
JSONbored added a commit that referenced this pull request Jul 11, 2026
#2348) (#5088)

planAutoTune/shouldAutoClear and their close-side mirrors now engage and
clear on GateEvalRow's reversal-discounted weightedMergePrecision /
weightedClosePrecision (already computed by parity.ts's computeGateEval,
PR #4169) instead of the raw precision fields. A high volume of later-
reverted merges can no longer keep the raw number artificially healthy
to dodge the breaker. The raw fields are preserved on AutoTuneAction /
CloseAutoTuneAction for log continuity and surfaced alongside the
weighted number in the alert message. computeTuningRecommendations (the
separate advisory-only tuning surface) is unchanged.

evalRowFromCalibration's synthetic rows (selftune-wire.ts, sourced from
agent_recommendation_outcomes) carry no reversal signal, so their
weighted fields mirror the raw ones by construction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

1 participant