Repository navigation
fix(cloud): reject prefix-parsed telemetry thresholds - #24131
Conversation
lalalune
left a comment
lalalune
left a comment
There was a problem hiding this comment.
Reviewed exact head 0cefd1b8841ec04c0af9768cf91c0b027960dac3.
The strict whole-string validation is directionally correct, but /^\d+$/ introduces a compatibility regression: the existing Number.parseInt(raw, 10) accepts an explicitly positive value such as +500, while the replacement silently falls back. Please preserve the optional leading plus (for example, validate a signed whole integer and retain the existing parsed > 0 constraint, or use an optional-plus pattern).
Please also add regression coverage for:
+500remaining a valid positive threshold- an integer above
Number.MAX_SAFE_INTEGERfalling back
The latter is required because this patch changes the acceptance predicate from finite to safe integer and the PR explicitly claims that boundary behavior, but the new tests currently exercise only trailing garbage and an ordinary positive integer.
Hosted checks are queued. This source blocker is the reason for the changes-requested disposition; the PR should remain ready for review.
0cefd1b to
f4e2146
Compare
|
Fixed at Both requested regressions are in: This applied to five sibling PRs, so I corrected all of them rather than just this one — same
One further note: #23970 already merged with this same regression in the four electrobun helpers. I will open a separate follow-up for that rather than leave it in While reworking #24143 I also caught that a bulk edit had touched a pre-existing |
|
CLAIMING REVIEW: exact-head adversarial re-review of the cloud telemetry threshold parser and requested regression coverage on |
standujar
left a comment
standujar
left a comment
There was a problem hiding this comment.
Reviewed exact head f4e21465ebe2559a54c446cca81339344df9ec33.
The earlier semantic requests are fixed: +500 remains accepted, the first integer above Number.MAX_SAFE_INTEGER falls back, trimming and all three threshold keys behave consistently, and environment reads remain live rather than cached. The PR's 7 tests plus 31 independent adversarial grammar/bounds/isolation cases pass (38/38) under Bun 1.3.14 in a credential-free, network-denied OS sandbox. git diff --check passes, and merge synthesis with current develop@06480e1ca7e86e57cd86627188738aa6b142c9f8 is conflict-free (tree 8783c908dbed6c7f67906588bac4e4b280272483).
Two P3 formatter findings remain on newly added lines. With repository-pinned Biome 2.5.8, the exact command biome check packages/cloud/shared/src/lib/observability/cloud-backend-observability.ts packages/cloud/shared/src/lib/observability/cloud-backend-observability.test.ts reports four diffs: two are already present unchanged on current develop (observeCloudRequest and recordCloudStreamMilestones), while the two inline findings below exist only on this PR head. Please format the two introduced lines; this review does not attribute the two baseline diffs to the PR. No hosted checks are currently reported for this head.
AI provider/model: OpenAI / gpt-5.6-sol
Client / agent tooling: Codex desktop
Contribution skill revision: f4e2146:packages/skills/skills/contribute-to-eliza
Attribution status: self-reported
— [standujar/review-24131]
| // The optional leading plus is kept deliberately: `Number.parseInt` accepted | ||
| // "+500", so rejecting it here would be a compatibility regression rather | ||
| // than a fix. | ||
| const parsed = trimmed && /^\+?\d+$/.test(trimmed) ? Number(trimmed) : Number.NaN; |
There was a problem hiding this comment.
[P3] Format this newly added conditional with the repository-pinned Biome 2.5.8. The exact targeted check requires the assignment to wrap after =; this diff is additional to the two unrelated formatter failures already present on current develop.
| }); | ||
|
|
||
| describe("telemetry threshold env parsing", () => { | ||
| const KEYS = ["CLOUD_SLOW_DB_MS", "CLOUD_SLOW_REQUEST_MS", "CLOUD_DB_BURST_COUNT"]; |
There was a problem hiding this comment.
[P3] Format this newly added key list with the repository-pinned Biome 2.5.8. The targeted check requires the three entries on separate lines; current develop does not contain this formatter failure.
|
Evidence rows added and validated locally with Applied to every PR of mine that was missing them — #23979, #23995, #24001, #24008, #24124, #24131, #24143, #24188, #24193 — each with its own Two I deliberately did not touch: #23950 and #23951 still show For the rest the N/A reasons are narrow and checkable — e.g. on #23979 the two changed files are a non-rendering |
Follow-up to elizaOS#23970, which is already merged. That change tightened four integer env parsers to reject trailing garbage, but used `/^\d+$/` — which also rejects an explicitly signed value: ELIZA_RENDERER_PROXY_IDLE_TIMEOUT_SECONDS=+30 `Number.parseInt("+30", 10)` returned `30`, so that configuration worked before elizaOS#23970 and silently falls back to the default now. That is a compatibility regression introduced by the fix, not by the original defect. Fix: allow the optional leading plus in all four parsers (`/^\+?\d+$/`). The trailing-garbage rejection elizaOS#23970 added is unchanged, and so is the `Number.isSafeInteger` bound. Also adds the coverage that would have caught this: each affected parser now asserts a signed positive value still resolves, and that an integer beyond `Number.MAX_SAFE_INTEGER` falls back — the boundary elizaOS#23970 introduced but did not test. Caught in review of the sibling PRs applying the same pattern (thanks @lalalune on elizaOS#24131). The five unmerged siblings — elizaOS#23995, elizaOS#24001, elizaOS#24008, elizaOS#24120, elizaOS#24143 — were corrected on their own branches; this PR repairs the one that had already landed. Verification - 4 affected suites: 21 passed - full electrobun lane: 586 passed across 76 files - `biome check` on all six changed files: clean Mutation resistance - Restoring develop's current sources and keeping the new tests fails with `expected 255 to be 30` (renderer proxy idle timeout) and `expected 240 to be 120` (TTS chunk limit) — the signed values falling back instead of being honoured. Attribution - Regression identified by @lalalune during review of elizaOS#24131. - Repair, cross-branch audit, and mutation proof by Claude Code (`claude-opus-5`). Claude-Session: https://claude.ai/code/session_011wVLgS8jLiPytQVLnqwLZL
Follow-up to elizaOS#23970, which is already merged. That change tightened four integer env parsers to reject trailing garbage, but used `/^\d+$/` — which also rejects an explicitly signed value: ELIZA_RENDERER_PROXY_IDLE_TIMEOUT_SECONDS=+30 `Number.parseInt("+30", 10)` returned `30`, so that configuration worked before elizaOS#23970 and silently falls back to the default now. That is a compatibility regression introduced by the fix, not by the original defect. Fix: allow the optional leading plus in all four parsers (`/^\+?\d+$/`). The trailing-garbage rejection elizaOS#23970 added is unchanged, and so is the `Number.isSafeInteger` bound. Also adds the coverage that would have caught this: each affected parser now asserts a signed positive value still resolves, and that an integer beyond `Number.MAX_SAFE_INTEGER` falls back — the boundary elizaOS#23970 introduced but did not test. Caught in review of the sibling PRs applying the same pattern (thanks @lalalune on elizaOS#24131). The five unmerged siblings — elizaOS#23995, elizaOS#24001, elizaOS#24008, elizaOS#24120, elizaOS#24143 — were corrected on their own branches; this PR repairs the one that had already landed. Verification - 4 affected suites: 21 passed - full electrobun lane: 586 passed across 76 files - `biome check` on all six changed files: clean Mutation resistance - Restoring develop's current sources and keeping the new tests fails with `expected 255 to be 30` (renderer proxy idle timeout) and `expected 240 to be 120` (TTS chunk limit) — the signed values falling back instead of being honoured. Attribution - Regression identified by @lalalune during review of elizaOS#24131. - Repair, cross-branch audit, and mutation proof by Claude Code (`claude-opus-5`). Claude-Session: https://claude.ai/code/session_011wVLgS8jLiPytQVLnqwLZL
Follow-up to elizaOS#23970, which is already merged. That change tightened four integer env parsers to reject trailing garbage, but used `/^\d+$/` — which also rejects an explicitly signed value: ELIZA_RENDERER_PROXY_IDLE_TIMEOUT_SECONDS=+30 `Number.parseInt("+30", 10)` returned `30`, so that configuration worked before elizaOS#23970 and silently falls back to the default now. That is a compatibility regression introduced by the fix, not by the original defect. Fix: allow the optional leading plus in all four parsers (`/^\+?\d+$/`). The trailing-garbage rejection elizaOS#23970 added is unchanged, and so is the `Number.isSafeInteger` bound. Also adds the coverage that would have caught this: each affected parser now asserts a signed positive value still resolves, and that an integer beyond `Number.MAX_SAFE_INTEGER` falls back — the boundary elizaOS#23970 introduced but did not test. Caught in review of the sibling PRs applying the same pattern (thanks @lalalune on elizaOS#24131). The five unmerged siblings — elizaOS#23995, elizaOS#24001, elizaOS#24008, elizaOS#24120, elizaOS#24143 — were corrected on their own branches; this PR repairs the one that had already landed. Verification - 4 affected suites: 21 passed - full electrobun lane: 586 passed across 76 files - `biome check` on all six changed files: clean Mutation resistance - Restoring develop's current sources and keeping the new tests fails with `expected 255 to be 30` (renderer proxy idle timeout) and `expected 240 to be 120` (TTS chunk limit) — the signed values falling back instead of being honoured. Attribution - Regression identified by @lalalune during review of elizaOS#24131. - Repair, cross-branch audit, and mutation proof by Claude Code (`claude-opus-5`). Claude-Session: https://claude.ai/code/session_011wVLgS8jLiPytQVLnqwLZL
…nvs (#24188) * fix(app-core): keep accepting a signed integer in electrobun env parsing Follow-up to #23970, which is already merged. That change tightened four integer env parsers to reject trailing garbage, but used `/^\d+$/` — which also rejects an explicitly signed value: ELIZA_RENDERER_PROXY_IDLE_TIMEOUT_SECONDS=+30 `Number.parseInt("+30", 10)` returned `30`, so that configuration worked before #23970 and silently falls back to the default now. That is a compatibility regression introduced by the fix, not by the original defect. Fix: allow the optional leading plus in all four parsers (`/^\+?\d+$/`). The trailing-garbage rejection #23970 added is unchanged, and so is the `Number.isSafeInteger` bound. Also adds the coverage that would have caught this: each affected parser now asserts a signed positive value still resolves, and that an integer beyond `Number.MAX_SAFE_INTEGER` falls back — the boundary #23970 introduced but did not test. Caught in review of the sibling PRs applying the same pattern (thanks @lalalune on #24131). The five unmerged siblings — #23995, #24001, #24008, #24120, #24143 — were corrected on their own branches; this PR repairs the one that had already landed. Verification - 4 affected suites: 21 passed - full electrobun lane: 586 passed across 76 files - `biome check` on all six changed files: clean Mutation resistance - Restoring develop's current sources and keeping the new tests fails with `expected 255 to be 30` (renderer proxy idle timeout) and `expected 240 to be 120` (TTS chunk limit) — the signed values falling back instead of being honoured. Attribution - Regression identified by @lalalune during review of #24131. - Repair, cross-branch audit, and mutation proof by Claude Code (`claude-opus-5`). Claude-Session: https://claude.ai/code/session_011wVLgS8jLiPytQVLnqwLZL * test(app-core): cover every Electrobun integer parser --------- Co-authored-by: moon <autonomousresearcher@gmail.com>
f4e2146 to
e7e5047
Compare
e7e5047 to
e3481ba
Compare
`numberEnv` accepts a value `parseInt` merely starts with:
const parsed = raw ? Number.parseInt(raw, 10) : Number.NaN;
return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback;
`parseInt` stops at the first non-digit, so `CLOUD_SLOW_DB_MS=500junk` yields
`500` — finite and positive, so the documented fallback never runs. `NaN` is the
only malformed input this guard actually catches.
All three observability thresholds go through it: `CLOUD_SLOW_REQUEST_MS`,
`CLOUD_SLOW_DB_MS` and `CLOUD_DB_BURST_COUNT`. They decide which calls are
classified slow or bursty in `getCloudTelemetrySnapshot`:
slowDb: db.filter((r) => r.durationMs >= t.slowDbMs),
and the snapshot also *publishes* the thresholds it used. So a typo does not
just misclassify — it makes the telemetry report state a boundary nobody
configured, while presenting it as the configured one. Observability that
misreports its own parameters is worse than observability that is absent,
because the numbers still look authoritative.
Fix: require the whole trimmed value to be decimal before converting, so
malformed input reaches the fallback the function already defines.
`Number.isSafeInteger` replaces `Number.isFinite` so a value past 2^53 is
rejected too. Valid thresholds are unaffected.
Verification
- `cloud-backend-observability.test.ts`: 5 passed
- `biome check` on both changed files: clean
- Drift-checked against current `develop`: `numberEnv` is unchanged upstream, so
the defect is live.
Mutation resistance
- Reverting only the source change and keeping the tests fails with
`Expected: 250, Received: 500` — the snapshot publishing the prefix-parsed
threshold.
- The "still honours a clean threshold" case passes in both directions, so the
fix is specific to malformed input.
Attribution
- Found by OpenAI `gpt-5.6-sol` via Codex (AgentRouter) during a numeric-input
validation sweep of packages/cloud.
- Independent verification, the snapshot-publishes-its-own-thresholds framing,
the safe-integer bound, and the mutation proof by Claude Code
(`claude-opus-5`).
Claude-Session: https://claude.ai/code/session_011wVLgS8jLiPytQVLnqwLZL
Review follow-up: preserve the optional leading plus. Number.parseInt accepted
"+500", so requiring ^\d+$ would have rejected a value the old code took — a
compatibility regression rather than a fix. Added regression coverage for a
signed positive value and for an integer beyond Number.MAX_SAFE_INTEGER, which
the safe-integer predicate this patch introduces now rejects.
Review follow-up: preserve the optional leading plus. Number.parseInt accepted
"+500", so requiring ^\d+$ would have rejected a value the old code took — a
compatibility regression rather than a fix. Added regression coverage for a
signed positive value and for an integer beyond Number.MAX_SAFE_INTEGER, which
the safe-integer predicate this patch introduces now rejects.
e3481ba to
b02fa68
Compare
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@standujar Thanks for the adversarial run — the 38/38 confirmation on the semantic contract is appreciated. On the two P3 formatter findings, I can't reproduce them with repository-pinned Biome 2.5.8, and I believe applying them would break the check rather than fix it.
Both fall between the two limits, which is exactly the band where the root default and the package config disagree. At the head you reviewed ( Also clean with I did apply both requested edits to be sure, then ran the command CI actually runs for this package ( Biome wants both lines joined — which is how they already are on this head. So I've left them as-is. If your run resolved the root config for these paths (a Hosted CI on the current head is green ("All Tests Passed"). @lalalune — your two source blockers were fixed at head |
3 participants
Relates to
No separate issue — the defect is described in full below.
Definition of Done: full standard in
CONTRIBUTING.md.developand is built directly on the currentorigin/developtree, so it applies with zero conflicts.bun run verifywas not run green:developcurrently carries unrelated@elizaos/agentBiome formatter diagnostics, the same blocker recorded in fix(voice): restore immediate prewarmed phone turns #24174.Sync with develop
origin/developtree at review time; zero conflicts.@elizaos/agentformatter diagnostics).Risks
Low. Observability thresholds only. Valid thresholds are unchanged; malformed ones now take the documented fallback instead of being published as configured.
Background
numberEnvaccepts a valueparseIntmerely starts with:parseIntstops at the first non-digit, soCLOUD_SLOW_DB_MS=500junkyields500— finite and positive, so the documented fallback never runs.NaNis the only malformed input this guard actually catches.All three observability thresholds go through it —
CLOUD_SLOW_REQUEST_MS,CLOUD_SLOW_DB_MS,CLOUD_DB_BURST_COUNT. They decide which calls are classified slow or bursty:…and
getCloudTelemetrySnapshotalso publishes the thresholds it used:So a typo does not just misclassify — it makes the telemetry report state a boundary nobody configured, while presenting it as the configured one. Observability that misreports its own parameters is worse than observability that is absent, because the numbers still look authoritative.
The fix
Require the whole trimmed value to be decimal before converting, so malformed input reaches the fallback the function already defines.
Number.isSafeIntegerreplacesNumber.isFiniteso a value past 2^53 is rejected too. Valid thresholds are unaffected.Verification
cloud-backend-observability.test.tsbiome check(both changed files)Drift-checked against current
develop:numberEnvis unchanged upstream, so the defect is live.Mutation resistance
Reverting only the source change and keeping the tests:
The failure is the snapshot publishing the prefix-parsed threshold. The "still honours a clean threshold" case passes in both directions, so the fix is specific to malformed input.
Attribution
gpt-5.6-solvia Codex (AgentRouter) during a numeric-input validation sweep ofpackages/cloud.claude-opus-5).Documentation changes needed?
My changes do not require a change to the project documentation — the parser's documented contract is unchanged; this makes the implementation match it.
Testing
Where should a reviewer start?
Start with the mutation-resistance block below: it reverts only the source change and shows the exact wrong value the current code produces.
Detailed testing steps
None beyond the automated suites quoted above — the change is a pure input-parsing boundary and is fully covered by the regression tests.
Evidence Gate
N/A - backend-only change to the cloud telemetry threshold parsers, no UI surface.N/A - backend-only change to the cloud telemetry threshold parsers, no UI surface.N/A - the observable behaviour is the thresholds published in the telemetry snapshot, shown by the mutation-proof output quoted below.N/A - no service is started by this change; the exercised path is covered by the unit suite quoted below.N/A - no frontend change.N/A - no model, prompt, provider or action path is changed; this is an input parser.N/A - no domain record is produced; the observable output is the thresholds published in the telemetry snapshot, asserted in the tests.N/A - the change has no rendered visual surface.