Skip to content

orb(gate): three gateMode() call sites skip the ?? default its fail-closed branch assumes, so the score-signal recorder no longer mirrors the evaluator #10015

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

#9167 made gateMode() fail CLOSED on an unrecognized value (src/rules/advisory.ts:1582-1594):

// #9167: fail CLOSED on a value that isn't one of the three real modes, matching the rest of this
// codebase's fail-closed defaults -- every legitimate caller already supplies its own `?? "advisory"`
// default before reaching here (see every `gateMode(policy.xGateMode ?? "advisory")` call site above), so
// this branch is only ever reached for a truly malformed value …
function gateMode(value: GateRuleMode | null | undefined): GateRuleMode {
  if (value === "off" || value === "block" || value === "advisory") return value;
  return "block";
}

The claim "every legitimate caller already supplies its own ?? "advisory" default before reaching here" is
not true of three call sites in the same file, and every GateCheckPolicy mode field is declared optional
(slopGateMode?: GateRuleMode | undefined, src/rules/advisory.ts:87; qualityGateMode?: …, :61), so
undefined is a typed, legal input rather than a malformed one:

  • src/rules/advisory.ts:1505const slopMode = gateMode(effective.slopGateMode);
  • src/rules/advisory.ts:1525const qualityMode = gateMode(effective.qualityGateMode);
  • src/rules/advisory.ts:1549if (gateMode(policy.qualityGateMode) === "off") return null;

The consequential one is the slop line, because it is documented as an exact mirror of the evaluator
(src/rules/advisory.ts:1479-1483):

 * Record a fired signal for each score gate that actually EVALUATED its score this pass (#8223) -- the
 * same filter the pure evaluation applies: slop evaluates only in `block` mode with a non-null risk
 * (mirrors {@link buildSlopGateBlocker}); …

The evaluator it claims to mirror supplies the default (src/rules/advisory.ts:1567-1568):

function buildSlopGateBlocker(policy: GateCheckPolicy): AdvisoryFinding | null {
  if (gateMode(policy.slopGateMode ?? "advisory") !== "block") return null;

So with slopGateMode absent and a non-null slopRisk:

  • buildSlopGateBlocker resolves advisory ⇒ evaluates nothing, pushes no blocker.
  • recordGateScoreSignals resolves block ⇒ writes a slop_gate_score RuleFiredEvent whose
    rawSignal reads … (mode block) for a threshold comparison the gate never made.

Those events are corpus input, not telemetry: GATE_SCORE_SIGNAL_CODES puts slop_gate_score into
CONFIGURED_GATE_BLOCKER_SIGNAL_CODES (src/rules/advisory.ts:208, :236) precisely so a human reversal
can be paired against it, and DEFAULT_SLOP_BLOCK_THRESHOLD's doc comment (:1563-1565) names this corpus
as the anchor for the loosenable-knob registry. Fabricated block-mode fires for repos that never enabled the
slop gate contaminate exactly that evidence.

This is also a silent flip introduced by #9167: before it, gateMode(undefined) returned "advisory", so
this call site recorded nothing.

test/unit/configured-gate-blocker-signals.test.ts:296-360 covers recordGateScoreSignals only with an
explicit slopGateMode / qualityGateMode in every case, so the absent-mode divergence is untested.

Requirements

  • recordGateScoreSignals must resolve the slop mode with the SAME default buildSlopGateBlocker uses:
    gateMode(effective.slopGateMode ?? "advisory").
  • recordGateScoreSignals must resolve the quality mode with the SAME default buildQualityGateWarning
    uses, and buildQualityGateWarning itself must use an explicit default rather than a bare
    gateMode(policy.qualityGateMode). The default chosen for quality must be "advisory" so the two stay
    identical and so an absent qualityGateMode keeps its pre-#9167 resolution (a bare gateMode(undefined)
    today resolves block, which is not off and therefore already behaves like advisory for both of these
    call sites — the explicit default makes that intent legible instead of incidental).
  • gateMode's doc comment must be corrected: it must no longer assert that every caller supplies a ??
    default, or it must be accurate once the three call sites do.
  • Behaviour that must NOT change: gateMode's fail-closed return "block" for a genuinely malformed value;
    DEFAULT_SLOP_BLOCK_THRESHOLD (60); applyMergeReadinessGate's ?? "off" composite default and its
    override of slopGateMode; the slopRisk !== null / readinessScore !== null / qualityMin !== null
    guards; the outcome strings (above_threshold / below_threshold /
    at_or_above_threshold); normalizeScore; and the .catch(() => undefined) best-effort posture of both
    writes.

⚠️ Required pattern: buildSlopGateBlocker (src/rules/advisory.ts:1567-1572) is the reference — the
recorder must resolve its mode through the identical expression. What does NOT satisfy this issue: changing
gateMode's fallback back to "advisory" (that reverts #9167's fail-closed decision); making
GateCheckPolicy's mode fields non-optional (a repo-wide type change with blast radius across
src/queue/gate-checks.ts, src/review/decision-replay.ts and every test fixture); extracting a new
resolveSlopMode() helper used by only one of the two sites; a test-only PR.

Deliverables

  • src/rules/advisory.ts: recordGateScoreSignals resolves slopMode via
    gateMode(effective.slopGateMode ?? "advisory") and qualityMode via
    gateMode(effective.qualityGateMode ?? "advisory").
  • src/rules/advisory.ts: buildQualityGateWarning resolves its mode via
    gateMode(policy.qualityGateMode ?? "advisory").
  • src/rules/advisory.ts: gateMode's #9167 doc comment no longer makes a claim about its callers
    that the file contradicts.
  • A test in test/unit/configured-gate-blocker-signals.test.ts asserting
    recordGateScoreSignals(env, { slopRisk: 72, slopGateMinScore: 60 }, "owner/repo", 7) (no
    slopGateMode) records NO slop_gate_score event, and that
    evaluateGateCheck(advisoryWithNoFindings, { slopRisk: 72, slopGateMinScore: 60 }) likewise produces
    no slop_risk_above_threshold blocker — the two sides agreeing is the point.
  • A test in test/unit/configured-gate-blocker-signals.test.ts asserting the composite path still
    records: { mergeReadinessGateMode: "block", slopRisk: 72, slopGateMinScore: 60 } (no explicit
    slopGateMode) still writes a slop_gate_score event, because applyMergeReadinessGate promotes it.
  • A test in test/unit/configured-gate-blocker-signals.test.ts asserting
    recordGateScoreSignals(env, { readinessScore: 40, qualityGateMinScore: 70 }, "owner/repo", 7) (no
    qualityGateMode) still records a quality_gate_score event with outcome: "below_threshold"
    the quality leg's behaviour is unchanged by this fix.
  • A regression test at test/unit/configured-gate-blocker-signals.test.ts named for this bug (e.g.
    "REGRESSION: an absent slopGateMode records nothing, matching buildSlopGateBlocker").

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
fixing the slop line but leaving the two quality call sites bare, or fixing the code without correcting
gateMode's doc comment — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; src/rules/advisory.ts is measured and
gated. Each edited line introduces a ?? with two arms — an absent mode and an explicit mode — and all three
need both arms tested. The surrounding conditions must also keep both arms covered:
slopMode === "block" && slopRisk !== null (each conjunct false at least once, plus the true case),
qualityMode !== "off" && readinessScore !== null && qualityMin !== null likewise, and
buildQualityGateWarning's score === null || minScore === null || score >= minScore (each disjunct).
src/rules/advisory.ts is the host half of the hand-maintained gate-decision twin pair
(scripts/check-engine-parity.ts); confirm npm run test:ci's parity check stays green — the functions
touched here are not in GATE_DECISION_CORE_MARKERS, so no engine-side change should be required.

Expected Outcome

The score-signal recorder resolves its gate modes exactly as the pure evaluator does, so the calibration
corpus stops receiving slop_gate_score fires labeled block for repos whose slop gate never evaluated, and
gateMode's doc comment stops asserting a caller contract the file does not keep.

Links & Resources

  • src/rules/advisory.ts:1582-1594gateMode and the #9167 caller claim
  • src/rules/advisory.ts:1489-1546recordGateScoreSignals and its "mirrors buildSlopGateBlocker" contract
  • src/rules/advisory.ts:1548-1580buildQualityGateWarning / buildSlopGateBlocker
  • src/rules/advisory.ts:199-237GATE_SCORE_SIGNAL_CODES inside CONFIGURED_GATE_BLOCKER_SIGNAL_CODES
  • src/rules/advisory.ts:1615-1624applyMergeReadinessGate, the composite promotion
  • test/unit/configured-gate-blocker-signals.test.ts:296-360 — the existing, mode-always-explicit tests
  • orb(config): mergeReadiness:advisory silently demotes three explicitly-blocking gates at once, and unknown gate modes coerce toward merge #9167 — the fail-closed change whose caller assumption this corrects

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions