From 1b0875caa1865c03e2e2c4c5c60d1b0b4155c953 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 31 Jul 2026 14:27:38 +0800 Subject: [PATCH] fix(engine): normalize actionClass once and read policies/buckets own-property in write-rate-limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit writeRateLimitRepoKey trimmed actionClass for the per-repo key, but evaluateWriteRateLimit, recordWriteRateLimitAllowed, recordWriteRateLimitDenied and clearWriteRateLimitBackoff used the RAW value for the global bucket key and both policy lookups. So " open_pr " resolved the per-repo key trimmed while the global bucket and both policies fell through to PERMISSIVE_CONFIG's 1_000_000 ceiling, and recordWriteRateLimitAllowed keyed the global bucket under the raw string — permanently diverging a decision from the state written for it. Separately, policyFor and the bucket reads used bare table[actionClass], so an actionClass naming an Object.prototype member ("constructor"/"toString") resolved an inherited value (a fabricated limit:0) instead of the intended fallback. Normalize actionClass exactly once at the top of each function and use it for every lookup, and read policies/buckets via an ownValue(Object.hasOwn) helper. writeRateLimitRepoKey's output, DEFAULT_WRITE_RATE_LIMIT_POLICIES, PERMISSIVE_CONFIG's fallback role, and the verdict shape are all unchanged. Closes #9999 --- .../src/governor/write-rate-limit.ts | 38 +++++++++++++------ .../test/write-rate-limit-enforcement.test.ts | 36 ++++++++++++++++++ 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/packages/loopover-engine/src/governor/write-rate-limit.ts b/packages/loopover-engine/src/governor/write-rate-limit.ts index 62fea91a28..d598bc8953 100644 --- a/packages/loopover-engine/src/governor/write-rate-limit.ts +++ b/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(table: Record, 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,9 +188,10 @@ 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 }; } @@ -184,9 +199,10 @@ export function recordWriteRateLimitDenied( /** 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 }; diff --git a/packages/loopover-engine/test/write-rate-limit-enforcement.test.ts b/packages/loopover-engine/test/write-rate-limit-enforcement.test.ts index 1131fbea57..c178329da2 100644 --- a/packages/loopover-engine/test/write-rate-limit-enforcement.test.ts +++ b/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); +});