fix(engine): normalize actionClass once and read policies/buckets own-property in write-rate-limit

packages/loopover-engine/src/governor/write-rate-limit.ts

@@ -65,13 +65,20 @@ export function writeRateLimitRepoKey(actionClass: string, repoFullName: string)
   return `${actionClass.trim()}:${repoFullName.trim().toLowerCase()}`;
 }
 
+/** Own-property lookup: `policies`/`buckets` are plain object records, so a bare `table[key]` walks the
+ *  prototype chain — an `actionClass` naming an `Object.prototype` member ("constructor"/"toString"/"valueOf")
+ *  would resolve an inherited value instead of the intended fallback. Read own properties only (#9999). */
+function ownValue<T>(table: Record<string, T>, key: string): T | undefined {
+  return Object.hasOwn(table, key) ? table[key] : undefined;
+}
+
 function policyFor(
   policies: WriteRateLimitPolicies,
   actionClass: string,
   scope: "global" | "perRepo",
 ): LocalRateLimitConfig {
   const table = scope === "global" ? policies.global : policies.perRepo;
-  return table[actionClass] ?? PERMISSIVE_CONFIG;
+  return ownValue(table, actionClass) ?? PERMISSIVE_CONFIG;
 }
 
 function emptyBucket(nowMs: number): LocalRateBucket {
@@ -109,13 +116,17 @@ export function evaluateWriteRateLimit(input: {
   const policies = input.policies ?? DEFAULT_WRITE_RATE_LIMIT_POLICIES;
   const randomFn = input.randomFn ?? (() => 0.5);
   const nowMs = Number.isFinite(input.nowMs) ? input.nowMs : 0;
-  const repoKey = writeRateLimitRepoKey(input.actionClass, input.repoFullName);
+  // Normalize the action class ONCE, then key the global bucket, the per-repo key, and both policy lookups
+  // off the same value — a stray-whitespace actionClass must not resolve the per-repo key trimmed while the
+  // global bucket and both policies fall through to PERMISSIVE_CONFIG on the raw value (#9999).
+  const actionClass = input.actionClass.trim();
+  const repoKey = writeRateLimitRepoKey(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");
+  const globalBucket = ownValue(input.buckets.global, actionClass) ?? emptyBucket(nowMs);
+  const perRepoBucket = ownValue(input.buckets.perRepo, repoKey) ?? emptyBucket(nowMs);
+  const globalConfig = policyFor(policies, actionClass, "global");
+  const perRepoConfig = policyFor(policies, actionClass, "perRepo");
 
   const global = evaluateLocalRateLimit(globalBucket, globalConfig, nowMs);
   const perRepo = evaluateLocalRateLimit(perRepoBucket, perRepoConfig, nowMs);
@@ -149,16 +160,19 @@ export function evaluateWriteRateLimit(input: {
 /** Record a permitted write against both bucket scopes. */
 export function recordWriteRateLimitAllowed(
   buckets: WriteRateLimitBucketStore,
-  actionClass: string,
+  actionClassInput: string,
   repoFullName: string,
   nowMs: number,
   policies: WriteRateLimitPolicies = DEFAULT_WRITE_RATE_LIMIT_POLICIES,
 ): WriteRateLimitBucketStore {
+  // Normalize ONCE and key the global bucket by the same value evaluateWriteRateLimit reads it with, so a
+  // decision and the state written for it can never address different buckets (#9999).
+  const actionClass = actionClassInput.trim();
   const repoKey = writeRateLimitRepoKey(actionClass, repoFullName);
   const globalConfig = policyFor(policies, actionClass, "global");
   const perRepoConfig = policyFor(policies, actionClass, "perRepo");
-  const globalBucket = buckets.global[actionClass] ?? emptyBucket(nowMs);
-  const perRepoBucket = buckets.perRepo[repoKey] ?? emptyBucket(nowMs);
+  const globalBucket = ownValue(buckets.global, actionClass) ?? emptyBucket(nowMs);
+  const perRepoBucket = ownValue(buckets.perRepo, repoKey) ?? emptyBucket(nowMs);
   return {
     global: {
       ...buckets.global,
@@ -174,19 +188,21 @@ export function recordWriteRateLimitAllowed(
 /** Bump the jitter backoff attempt after a throttled write (does not mutate rate buckets). */
 export function recordWriteRateLimitDenied(
   backoffAttempts: WriteRateLimitBackoffStore,
-  actionClass: string,
+  actionClassInput: string,
   repoFullName: string,
 ): WriteRateLimitBackoffStore {
+  const actionClass = actionClassInput.trim();
   const key = writeRateLimitRepoKey(actionClass, repoFullName);
   return { ...backoffAttempts, [key]: (backoffAttempts[key] ?? 0) + 1 };
 }
 
 /** Clear backoff attempts after a successful write. */
 export function clearWriteRateLimitBackoff(
   backoffAttempts: WriteRateLimitBackoffStore,
-  actionClass: string,
+  actionClassInput: string,
   repoFullName: string,
 ): WriteRateLimitBackoffStore {
+  const actionClass = actionClassInput.trim();
   const key = writeRateLimitRepoKey(actionClass, repoFullName);
   if (!(key in backoffAttempts)) return backoffAttempts;
   const next = { ...backoffAttempts };

packages/loopover-engine/test/write-rate-limit-enforcement.test.ts

@@ -3,8 +3,10 @@ import { test } from "node:test";
 
 import {
   buildWriteRateLimitGovernorLedgerEvent,
+  clearWriteRateLimitBackoff,
   evaluateWriteRateLimit,
   recordWriteRateLimitAllowed,
+  recordWriteRateLimitDenied,
 } from "../dist/index.js";
 
 test("barrel: the public entrypoint re-exports write-rate-limit enforcement (#2344)", () => {
@@ -47,3 +49,37 @@ test("evaluateWriteRateLimit: global and per-repo buckets both gate a write", ()
   assert.equal(blocked.allowed, false);
   assert.equal(blocked.blockedBy, "global");
 });
+
+test("REGRESSION (#9999): actionClass is normalized once and looked up own-property across every function", () => {
+  // (1) a whitespace-padded actionClass resolves the SAME global/per-repo policy limits as the trimmed form,
+  //     instead of falling through both scopes to PERMISSIVE_CONFIG's 1_000_000 ceiling.
+  const padded = evaluateWriteRateLimit({ actionClass: " open_pr ", repoFullName: "o/r", buckets: { global: {}, perRepo: {} }, backoffAttempts: {}, nowMs: 0 });
+  const trimmed = evaluateWriteRateLimit({ actionClass: "open_pr", repoFullName: "o/r", buckets: { global: {}, perRepo: {} }, backoffAttempts: {}, nowMs: 0 });
+  assert.equal(padded.global.limit, 30);
+  assert.equal(padded.perRepo.limit, 3);
+  assert.equal(padded.global.limit, trimmed.global.limit);
+  assert.equal(padded.perRepo.limit, trimmed.perRepo.limit);
+
+  // (2) recordWriteRateLimitAllowed keys the global bucket under the trimmed class, so a later evaluate with
+  //     the trimmed class observes the recorded count (a decision and its recorded state hit the same bucket).
+  const afterRecord = recordWriteRateLimitAllowed({ global: {}, perRepo: {} }, " open_pr ", "o/r", 0);
+  assert.equal(Object.hasOwn(afterRecord.global, "open_pr"), true);
+  assert.equal(Object.hasOwn(afterRecord.global, " open_pr "), false);
+  const seen = evaluateWriteRateLimit({ actionClass: "open_pr", repoFullName: "o/r", buckets: afterRecord, backoffAttempts: {}, nowMs: 0 });
+  assert.equal(seen.global.remaining, 28); // 30 limit, 1 already recorded, this eval consumes one more view → 28
+
+  // (3) an actionClass naming an Object.prototype member resolves the intended fallback (PERMISSIVE_CONFIG +
+  //     empty bucket), not an inherited member — today it returns allowed:false with a fabricated limit:0.
+  const proto = evaluateWriteRateLimit({ actionClass: "constructor", repoFullName: "o/r", buckets: { global: {}, perRepo: {} }, backoffAttempts: {}, nowMs: 0 });
+  assert.equal(proto.allowed, true);
+  assert.equal(proto.reason, "under_limit");
+  assert.equal(proto.global.limit, 1_000_000);
+  assert.equal(proto.perRepo.limit, 1_000_000);
+
+  // (4) recordWriteRateLimitDenied and clearWriteRateLimitBackoff normalize identically, so a backoff recorded
+  //     with the padded class is cleared by the trimmed one.
+  const backoff = recordWriteRateLimitDenied({}, " open_pr ", "o/r");
+  assert.equal(backoff["open_pr:o/r"], 1);
+  const cleared = clearWriteRateLimitBackoff(backoff, "open_pr", "o/r");
+  assert.equal(Object.hasOwn(cleared, "open_pr:o/r"), false);
+});