Skip to content

engine(governor): evaluateWriteRateLimit normalizes actionClass for the per-repo key but not for the policy or global-bucket lookup #9999

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

writeRateLimitRepoKey normalizes actionClass before building the per-repo bucket key.
packages/loopover-engine/src/governor/write-rate-limit.ts:64:

export function writeRateLimitRepoKey(actionClass: string, repoFullName: string): string {
  return `${actionClass.trim()}:${repoFullName.trim().toLowerCase()}`;
}

Every other use of actionClass in the same function body uses the RAW value.
packages/loopover-engine/src/governor/write-rate-limit.ts:108:

  const repoKey = writeRateLimitRepoKey(input.actionClass, input.repoFullName);
  const backoffAttempt = input.backoffAttempts[repoKey] ?? 0;

  const globalBucket = input.buckets.global[input.actionClass] ?? emptyBucket(nowMs);
  const perRepoBucket = input.buckets.perRepo[repoKey] ?? emptyBucket(nowMs);
  const globalConfig = policyFor(policies, input.actionClass, "global");
  const perRepoConfig = policyFor(policies, input.actionClass, "perRepo");

policyFor (packages/loopover-engine/src/governor/write-rate-limit.ts:67) resolves
table[actionClass] ?? PERMISSIVE_CONFIG, where PERMISSIVE_CONFIG is { limit: 1_000_000, windowMs: 60_000 }
(packages/loopover-engine/src/governor/write-rate-limit.ts:17). So one un-normalized character in
actionClass"open_pr " — resolves NEITHER the global nor the per-repo policy, and the write is
evaluated against a one-million-per-minute effective ceiling on both scopes, while its per-repo bucket is
still recorded under the trimmed key open_pr:owner/repo. The global bucket is meanwhile stored under the
raw key: recordWriteRateLimitAllowed
(packages/loopover-engine/src/governor/write-rate-limit.ts:146) has the identical split
(buckets.global[actionClass] raw, buckets.perRepo[repoKey] trimmed), so the two scopes' bookkeeping
diverges permanently for that action class.

A second, related lookup hazard sits in the same two helpers: policies.global, policies.perRepo,
buckets.global and buckets.perRepo are plain object records, so table[actionClass] walks the
prototype chain. actionClass === "constructor" (or "toString", "valueOf") resolves an inherited
Object.prototype member instead of undefined, and that member is then read as a
LocalRateLimitConfig / LocalRateBucket. This one happens to fail CLOSED — evaluateLocalRateLimit
normalizes the missing .limit to 0 (packages/loopover-engine/src/governor/rate-limit.ts:58), which
denies — but it denies for the wrong reason, with a LocalRateLimitDecision whose limit and resetAtMs
are fabricated from a function object rather than from any configured policy.

Both are latent today: the action classes come from LocalWriteActionSpec's fixed literal set, so no
current caller passes a padded or prototype-named value. That is precisely why the fix belongs in the
module — this is the shared engine both ORB and AMS build on, and the module's own contract
(packages/loopover-engine/src/governor/write-rate-limit.ts:1) is that it "maintains bucket math only"
for callers that own persistence, i.e. it must be correct for whatever string a caller hands it.

Requirements

  • evaluateWriteRateLimit, recordWriteRateLimitAllowed, recordWriteRateLimitDenied and
    clearWriteRateLimitBackoff must each normalize actionClass exactly ONCE, at the top of the function,
    and use that single normalized value for the global bucket key, the per-repo key, and both policy
    lookups. The normalization must be the same one writeRateLimitRepoKey already applies (.trim()).
  • writeRateLimitRepoKey must keep its current output for every input — it is exported and callers build
    keys with it.
  • policyFor and both bucket lookups must use own-property access (Object.hasOwn(table, key) ? table[key] : undefined,
    or a Map/null-prototype record), so an actionClass that names an Object.prototype member resolves to
    the intended fallback rather than an inherited member.
  • recordWriteRateLimitAllowed must key the GLOBAL bucket by the same normalized action class
    evaluateWriteRateLimit reads it with, so a decision and the state written for that decision can never
    address different buckets.
  • What must NOT change: DEFAULT_WRITE_RATE_LIMIT_POLICIES, PERMISSIVE_CONFIG's role as the
    unconfigured-action-class fallback, incrementBucket's window arithmetic, the blockedBy precedence
    (global when the global scope blocked, else per_repo), retryAfterMs's
    Math.max(windowWait, jitterWait) composition, and buildWriteRateLimitGovernorLedgerEvent's payload
    shape. Every existing assertion in
    packages/loopover-engine/test/write-rate-limit-enforcement.test.ts must keep passing unmodified.

⚠️ Required pattern: mirror the single-normalization discipline writeRateLimitRepoKey
(packages/loopover-engine/src/governor/write-rate-limit.ts:64) already applies — normalize once, then use
the normalized value everywhere in the body. What does NOT satisfy this issue: (a) making the callers in
packages/loopover-engine/src/governor/chokepoint.ts trim before calling, which leaves the module wrong
for every other consumer; (b) adding a runtime validation that throws on an unrecognized action class —
PERMISSIVE_CONFIG is the documented fallback for a class with no configured ceiling and must remain
non-throwing; (c) switching the whole policy/bucket model to Map as a repo-wide refactor of
WriteRateLimitPolicies / WriteRateLimitBucketStore, which changes the exported type surface every
caller persists against; a local own-property guard is sufficient; (d) a test-only PR.

Deliverables

  • packages/loopover-engine/src/governor/write-rate-limit.ts
    evaluateWriteRateLimit({ actionClass: " open_pr ", repoFullName: "o/r", buckets: { global: {}, perRepo: {} }, backoffAttempts: {}, nowMs: 0 })
    resolves the SAME global.limit (30) and perRepo.limit (3) as actionClass: "open_pr" does, where
    today both are 1_000_000.
  • packages/loopover-engine/src/governor/write-rate-limit.ts
    recordWriteRateLimitAllowed(buckets, " open_pr ", "o/r", 0) writes the global bucket under the key
    "open_pr" (not " open_pr "), so a following evaluateWriteRateLimit with "open_pr" observes the
    recorded count.
  • packages/loopover-engine/src/governor/write-rate-limit.ts
    evaluateWriteRateLimit({ actionClass: "constructor", ... }) resolves PERMISSIVE_CONFIG for both
    scopes and an empty bucket, and returns allowed: true with reason: "under_limit"; today it returns
    allowed: false with a fabricated limit: 0.
  • recordWriteRateLimitDenied and clearWriteRateLimitBackoff normalize actionClass identically, so
    a backoff recorded with " open_pr " is cleared by "open_pr".
  • A regression test at packages/loopover-engine/test/write-rate-limit-enforcement.test.ts named for
    this bug, covering all four bullets above.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that trims actionClass in evaluateWriteRateLimit but leaves recordWriteRateLimitAllowed's global
bucket keyed on the raw value, so a decision and its recorded state address different buckets — 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 — the touched path
packages/loopover-engine/src/governor/write-rate-limit.ts IS measured.

Branches touched, each needing BOTH arms tested: policyFor's new own-property guard (configured action
class → the configured policy; absent or prototype-named → PERMISSIVE_CONFIG); the
scope === "global" ? policies.global : policies.perRepo selector (both scopes); the
?? emptyBucket(nowMs) fallbacks for the global and per-repo buckets (present bucket and absent bucket, for
each); and the existing global.allowed && perRepo.allowed split, whose blockedBy arms (global and
per_repo) must both still be asserted after the policy-lookup change.

Engine lines are credited by two uploads whose hits are unioned — add the test to
packages/loopover-engine/test/** as well as any root test/** coverage, or the patch gate can still fail.

Expected Outcome

An action class is normalized once per call and every bucket key and policy lookup inside the write
rate-limiter addresses the same entry, so a decision can never be evaluated against a
one-million-per-minute permissive fallback while its state is recorded under a different key, and an action
class that collides with an Object.prototype member resolves the intended fallback instead of reading a
function object as a rate-limit config.

Links & Resources

  • packages/loopover-engine/src/governor/write-rate-limit.ts:64writeRateLimitRepoKey, the one place that normalizes
  • packages/loopover-engine/src/governor/write-rate-limit.ts:67policyFor's prototype-chain lookup and permissive fallback
  • packages/loopover-engine/src/governor/write-rate-limit.ts:108 — the raw-vs-trimmed split inside evaluateWriteRateLimit
  • packages/loopover-engine/src/governor/write-rate-limit.ts:146 — the same split inside recordWriteRateLimitAllowed
  • packages/loopover-engine/src/governor/rate-limit.ts:58 — where a missing .limit normalizes to 0
  • packages/loopover-engine/test/write-rate-limit-enforcement.test.ts — the suite to extend

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