Skip to content

AGENT_ASK_RATE_LIMIT_PER_HOUR=0 resolves to 30, bypassing its own Math.max(1) floor #985

Description

@lilyshen0722

Summary

AGENT_ASK_RATE_LIMIT_PER_HOUR=0 resolves to 30, not to the intended floor of 1. Every other out-of-range value clamps; zero alone jumps to the default.

backend/services/agentAskService.ts:33-36:

const ASK_RATE_LIMIT_PER_HOUR = Math.max(
  1,
  Number.parseInt(process.env.AGENT_ASK_RATE_LIMIT_PER_HOUR || '', 10) || 30,
);

The Math.max(1, …) is clearly deliberate — an operator may not disable the ask limiter entirely, and the minimum is 1 ask/hr. But || 30 runs first, so a parsed 0 never reaches the clamp.

Measured

env value    resolves to
---------    -----------
(unset)      30
""           30
"0"          30     ← asks for the minimum, gets the default
"1"           1
"5"           5
"-5"          1     ← clamps correctly
"abc"        30     ← silent
"0.5"        30     ← parseInt → 0 → same path as "0"

Why it matters

The failure is quiet and it inverts the operator's intent: someone turning the limiter down to its most restrictive setting gets the most permissive one instead, with nothing logged. -5 clamps to 1 as designed, so the inconsistency is specific to values that parseInt renders as 0.

Low severity — it needs an operator to set the variable, and the outcome is "the default stays in force" rather than a bypass. But 0 is the natural thing to type when you want the floor, and this is the one input where the floor doesn't apply.

Suggested fix

Move the fallback so only unparseable input takes the default, and let the clamp own the range:

const parsed = Number.parseInt(process.env.AGENT_ASK_RATE_LIMIT_PER_HOUR ?? '', 10);
const ASK_RATE_LIMIT_PER_HOUR = Math.max(1, Number.isFinite(parsed) ? parsed : 30);

That gives 0 → 1, -5 → 1, abc → 30, unset → 30. Worth a warn on the unparseable branch too, so a typo isn't silent.

Context

Surfaced because #981 cites this constant as its model for env-tunable limiter config. #981 makes the opposite choice deliberately — 0 is meaningful there (cap: 0 refuses every agent-triggered turn, addressedGrace: 0 restores pre-#973 behaviour) and its resolver validates on a single path with a loud warning. That divergence is correct; this issue is only about the sibling's own zero-handling being inconsistent with its own floor.

Not verified: whether anyone sets this variable in any environment today. I checked the code path, not the deployed config.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions