fix(alerts): reconcile adaptive CPU alert first-sample guard - #2
fix(alerts): reconcile adaptive CPU alert first-sample guard#2vortsghost2025 wants to merge 1 commit into
Conversation
Deployed hotfix reconciliation for SwarmMind adaptive CPU alerts. Changed files: - scripts/adaptive-cpu-alerts.js Live deployment status: deployed Live SHA256: - adaptive-cpu-alerts.js: eefbe801a80f9429c21238b4accaf7757b1c516bf6b0923b37cd09bd983655ee Rollback: remove scripts/adaptive-cpu-alerts.js or restore from origin/master.
📝 WalkthroughWalkthroughAdds ChangesAdaptive alerting
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
scripts/adaptive-cpu-alerts.js (2)
185-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why above-warning samples are excluded from the baseline.
The condition keeps incident traffic out of the baseline, which stops a sustained spike from raising thresholds and silencing alerts. That intent is not obvious from the code.
The same rule also creates a ratchet. If the workload rises permanently above
warningPct, no new sample ever enters the baseline. Thresholds stay at the old level and the alert fires until an operator clears the state file. A short comment records the tradeoff for the next reader.♻️ Proposed change
+ // Only sub-warning samples feed the baseline. This stops a sustained + // incident from inflating the baseline and silencing later alerts. + // Tradeoff: a permanent workload increase above warningPct never enters + // the baseline, so thresholds require a manual state reset to adapt. if (cpuPct < thresholds.warningPct) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/adaptive-cpu-alerts.js` around lines 185 - 193, Add a concise comment immediately above the cpuPct < thresholds.warningPct guard in the baseline sample collection logic explaining that above-warning samples are excluded to prevent sustained incident traffic from raising thresholds, while noting that permanently elevated workloads require clearing the state file to reset the baseline.
141-166: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPersist
_prevCpuand_prevWallMswith the Alert state.
loadState()restoressamples,alertCooldowns, and cumulative CPUs, but it does not populate_prevCpuor_prevWallMs. For reusable script invocations, the module starts every evaluation with these fields unset, soevaluate()treats each call as the first sample, skips all CPU threshold checks, and overwrites the persisted consecutive counters with0. Store and reload both fields so CPU sampling state survives restarts and stays independent of memory scope.[confirm_action]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/adaptive-cpu-alerts.js` around lines 141 - 166, Update the state persistence used by loadState() and the corresponding save logic to store and restore _prevCpu and _prevWallMs alongside the existing CPU sampling state. Ensure evaluate() receives these restored values before computing isFirstSample, while preserving first-sample initialization and existing memory-scope behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/adaptive-cpu-alerts.js`:
- Around line 302-318: Replace the inline memory threshold logic in
_checkThresholds with a call to _checkMemoryThreshold using the existing memRss
value, and append or merge its returned alerts into the current alerts array.
Preserve the existing evaluation order and cooldown behavior while removing the
duplicated comparison, severity, message, and cooldown code.
- Around line 27-30: Update the constructor initialization around stateDir and
statePath to ensure options.stateDir is always valid before calling path.join:
either apply an appropriate default directory or explicitly validate and reject
missing values, matching the existing options handling pattern.
- Around line 100-117: Update the threshold calculation in the function
containing the static return and adaptiveWarn/adaptiveCritical symbols: derive
static criticalPct using config.critical_multiplier_p95 instead of the hardcoded
5, and enforce criticalPct >= warningPct in both static and adaptive results,
while preserving the existing minimum floors and emergency ceiling.
- Around line 38-44: Validate the parsed state in the initialization flow before
using it: require samples to be an array whose entries contain numeric cpuPct
values, and require alertCooldowns to be an object; replace invalid values with
their empty defaults. Preserve valid persisted data and ensure the consecutive
CPU counters retain numeric defaults so _computeBaseline and _checkThresholds
cannot receive malformed state.
---
Nitpick comments:
In `@scripts/adaptive-cpu-alerts.js`:
- Around line 185-193: Add a concise comment immediately above the cpuPct <
thresholds.warningPct guard in the baseline sample collection logic explaining
that above-warning samples are excluded to prevent sustained incident traffic
from raising thresholds, while noting that permanently elevated workloads
require clearing the state file to reset the baseline.
- Around line 141-166: Update the state persistence used by loadState() and the
corresponding save logic to store and restore _prevCpu and _prevWallMs alongside
the existing CPU sampling state. Ensure evaluate() receives these restored
values before computing isFirstSample, while preserving first-sample
initialization and existing memory-scope behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f03e81e9-ec73-4819-b372-a1dd5bf0a18b
📒 Files selected for processing (1)
scripts/adaptive-cpu-alerts.js
| this.lane = options.lane || 'swarmmind'; | ||
| this.stateDir = options.stateDir; | ||
| this.config = Object.assign({}, DEFAULT_CONFIG, options.config || {}); | ||
| this.statePath = path.join(this.stateDir, 'adaptive-alert-state.json'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Give stateDir a default or validate it.
lane has a fallback, but stateDir does not. If a caller omits options.stateDir, path.join at Line 30 throws TypeError [ERR_INVALID_ARG_TYPE] during construction.
🛡️ Proposed fix
this.lane = options.lane || 'swarmmind';
- this.stateDir = options.stateDir;
+ this.stateDir = options.stateDir || path.join(process.cwd(), '.state', this.lane);
this.config = Object.assign({}, DEFAULT_CONFIG, options.config || {});
this.statePath = path.join(this.stateDir, 'adaptive-alert-state.json');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.lane = options.lane || 'swarmmind'; | |
| this.stateDir = options.stateDir; | |
| this.config = Object.assign({}, DEFAULT_CONFIG, options.config || {}); | |
| this.statePath = path.join(this.stateDir, 'adaptive-alert-state.json'); | |
| this.lane = options.lane || 'swarmmind'; | |
| this.stateDir = options.stateDir || path.join(process.cwd(), '.state', this.lane); | |
| this.config = Object.assign({}, DEFAULT_CONFIG, options.config || {}); | |
| this.statePath = path.join(this.stateDir, 'adaptive-alert-state.json'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/adaptive-cpu-alerts.js` around lines 27 - 30, Update the constructor
initialization around stateDir and statePath to ensure options.stateDir is
always valid before calling path.join: either apply an appropriate default
directory or explicitly validate and reject missing values, matching the
existing options handling pattern.
| if (fs.existsSync(this.statePath)) { | ||
| this._state = JSON.parse(fs.readFileSync(this.statePath, 'utf8')); | ||
| if (!this._state.samples) this._state.samples = []; | ||
| if (!this._state.alertCooldowns) this._state.alertCooldowns = {}; | ||
| if (this._state.consecutiveHighCpu === undefined) this._state.consecutiveHighCpu = 0; | ||
| if (this._state.consecutiveCriticalCpu === undefined) this._state.consecutiveCriticalCpu = 0; | ||
| if (this._state.consecutiveEmergencyCpu === undefined) this._state.consecutiveEmergencyCpu = 0; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate the shape of the persisted state, not just its presence.
The truthiness checks accept wrong types. If samples is an object or contains entries without a numeric cpuPct, _computeBaseline returns NaN for median, p95, and mad. Every threshold comparison in _checkThresholds then evaluates to false, and alerting stops without any error.
🛡️ Proposed fix
- if (!this._state.samples) this._state.samples = [];
- if (!this._state.alertCooldowns) this._state.alertCooldowns = {};
- if (this._state.consecutiveHighCpu === undefined) this._state.consecutiveHighCpu = 0;
- if (this._state.consecutiveCriticalCpu === undefined) this._state.consecutiveCriticalCpu = 0;
- if (this._state.consecutiveEmergencyCpu === undefined) this._state.consecutiveEmergencyCpu = 0;
+ if (!Array.isArray(this._state.samples)) this._state.samples = [];
+ this._state.samples = this._state.samples.filter(
+ s => s && Number.isFinite(s.cpuPct)
+ );
+ if (!this._state.alertCooldowns || typeof this._state.alertCooldowns !== 'object') {
+ this._state.alertCooldowns = {};
+ }
+ for (const k of ['consecutiveHighCpu', 'consecutiveCriticalCpu', 'consecutiveEmergencyCpu']) {
+ if (!Number.isFinite(this._state[k])) this._state[k] = 0;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (fs.existsSync(this.statePath)) { | |
| this._state = JSON.parse(fs.readFileSync(this.statePath, 'utf8')); | |
| if (!this._state.samples) this._state.samples = []; | |
| if (!this._state.alertCooldowns) this._state.alertCooldowns = {}; | |
| if (this._state.consecutiveHighCpu === undefined) this._state.consecutiveHighCpu = 0; | |
| if (this._state.consecutiveCriticalCpu === undefined) this._state.consecutiveCriticalCpu = 0; | |
| if (this._state.consecutiveEmergencyCpu === undefined) this._state.consecutiveEmergencyCpu = 0; | |
| if (fs.existsSync(this.statePath)) { | |
| this._state = JSON.parse(fs.readFileSync(this.statePath, 'utf8')); | |
| if (!Array.isArray(this._state.samples)) this._state.samples = []; | |
| this._state.samples = this._state.samples.filter( | |
| s => s && Number.isFinite(s.cpuPct) | |
| ); | |
| if (!this._state.alertCooldowns || typeof this._state.alertCooldowns !== 'object') { | |
| this._state.alertCooldowns = {}; | |
| } | |
| for (const k of ['consecutiveHighCpu', 'consecutiveCriticalCpu', 'consecutiveEmergencyCpu']) { | |
| if (!Number.isFinite(this._state[k])) this._state[k] = 0; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 38-38: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(this.statePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/adaptive-cpu-alerts.js` around lines 38 - 44, Validate the parsed
state in the initialization flow before using it: require samples to be an array
whose entries contain numeric cpuPct values, and require alertCooldowns to be an
object; replace invalid values with their empty defaults. Preserve valid
persisted data and ensure the consecutive CPU counters retain numeric defaults
so _computeBaseline and _checkThresholds cannot receive malformed state.
| return { | ||
| warningPct: this.config.static_floor_pct, | ||
| criticalPct: this.config.static_floor_pct * 5, | ||
| emergencyPct: this.config.emergency_hard_ceiling_pct, | ||
| mode: 'static', | ||
| baseline: null, | ||
| }; | ||
| } | ||
|
|
||
| const adaptiveWarn = Math.max( | ||
| this.config.static_floor_pct, | ||
| baseline.p95 * this.config.warning_multiplier_p95, | ||
| baseline.median + 3 * baseline.mad | ||
| ); | ||
| const adaptiveCritical = Math.max( | ||
| this.config.static_floor_pct * 2, | ||
| baseline.p95 * this.config.critical_multiplier_p95 | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Derive the static critical threshold from config and enforce threshold ordering.
Two related gaps in this function:
- Line 102 hardcodes the multiplier
5for the static mode critical threshold. The config already exposescritical_multiplier_p95(default5.0). If an operator tunes that value, static mode ignores it and adaptive mode honours it. - Nothing guarantees
criticalPct >= warningPct. The defaults keep the order correct, becausecritical_multiplier_p95(5.0) exceedswarning_multiplier_p95(2.5). If an operator lowerscritical_multiplier_p95belowwarning_multiplier_p95,criticalPctdrops belowwarningPct. Theelse if (cpuPct >= thresholds.criticalPct)branch at Line 250 is evaluated before the warning branch, so warning-level CPU is then reported asCRITICAL.
♻️ Proposed fix
if (!baseline) {
return {
warningPct: this.config.static_floor_pct,
- criticalPct: this.config.static_floor_pct * 5,
+ criticalPct: this.config.static_floor_pct * this.config.critical_multiplier_p95,
emergencyPct: this.config.emergency_hard_ceiling_pct,
mode: 'static',
baseline: null,
};
}
const adaptiveWarn = Math.max(
this.config.static_floor_pct,
baseline.p95 * this.config.warning_multiplier_p95,
baseline.median + 3 * baseline.mad
);
const adaptiveCritical = Math.max(
this.config.static_floor_pct * 2,
- baseline.p95 * this.config.critical_multiplier_p95
+ baseline.p95 * this.config.critical_multiplier_p95,
+ adaptiveWarn
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/adaptive-cpu-alerts.js` around lines 100 - 117, Update the threshold
calculation in the function containing the static return and
adaptiveWarn/adaptiveCritical symbols: derive static criticalPct using
config.critical_multiplier_p95 instead of the hardcoded 5, and enforce
criticalPct >= warningPct in both static and adaptive results, while preserving
the existing minimum floors and emergency ceiling.
| // Memory threshold check (always evaluated, not just first sample) | ||
| if (memRss > this.config.mem_threshold_bytes) { | ||
| const key = `mem-warning-${this.lane}`; | ||
| if (!this._isCooldownActive(key)) { | ||
| const memPct = (memRss / this.config.mem_threshold_bytes) * 100; | ||
| alerts.push({ | ||
| severity: memPct > 200 ? 'CRITICAL' : 'WARNING', | ||
| metric: 'memory', | ||
| value: memRss, | ||
| threshold: this.config.mem_threshold_bytes, | ||
| thresholdType: 'static', | ||
| mode: 'static', | ||
| message: `Memory RSS at ${(memRss / 1024 / 1024).toFixed(1)}MB exceeds threshold ${(this.config.mem_threshold_bytes / 1024 / 1024).toFixed(0)}MB`, | ||
| }); | ||
| this._setCooldown(key, this.config.mem_cooldown_seconds); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the duplicated memory check with a call to _checkMemoryThreshold.
This block duplicates _checkMemoryThreshold at lines 204-223 exactly: the same comparison, the same mem-warning-${this.lane} cooldown key, the same 200% CRITICAL cutoff, the same message template, and the same _setCooldown call. Any future change to the memory rule must be applied in both places, and the two copies will drift.
_checkThresholds already receives memRss. Delegating keeps behaviour identical, because both paths share one cooldown key and the method already returns an array.
♻️ Proposed refactor
- // Memory threshold check (always evaluated, not just first sample)
- if (memRss > this.config.mem_threshold_bytes) {
- const key = `mem-warning-${this.lane}`;
- if (!this._isCooldownActive(key)) {
- const memPct = (memRss / this.config.mem_threshold_bytes) * 100;
- alerts.push({
- severity: memPct > 200 ? 'CRITICAL' : 'WARNING',
- metric: 'memory',
- value: memRss,
- threshold: this.config.mem_threshold_bytes,
- thresholdType: 'static',
- mode: 'static',
- message: `Memory RSS at ${(memRss / 1024 / 1024).toFixed(1)}MB exceeds threshold ${(this.config.mem_threshold_bytes / 1024 / 1024).toFixed(0)}MB`,
- });
- this._setCooldown(key, this.config.mem_cooldown_seconds);
- }
- }
+ // Memory threshold check (always evaluated, not just first sample)
+ alerts.push(...this._checkMemoryThreshold(memRss));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Memory threshold check (always evaluated, not just first sample) | |
| if (memRss > this.config.mem_threshold_bytes) { | |
| const key = `mem-warning-${this.lane}`; | |
| if (!this._isCooldownActive(key)) { | |
| const memPct = (memRss / this.config.mem_threshold_bytes) * 100; | |
| alerts.push({ | |
| severity: memPct > 200 ? 'CRITICAL' : 'WARNING', | |
| metric: 'memory', | |
| value: memRss, | |
| threshold: this.config.mem_threshold_bytes, | |
| thresholdType: 'static', | |
| mode: 'static', | |
| message: `Memory RSS at ${(memRss / 1024 / 1024).toFixed(1)}MB exceeds threshold ${(this.config.mem_threshold_bytes / 1024 / 1024).toFixed(0)}MB`, | |
| }); | |
| this._setCooldown(key, this.config.mem_cooldown_seconds); | |
| } | |
| } | |
| // Memory threshold check (always evaluated, not just first sample) | |
| alerts.push(...this._checkMemoryThreshold(memRss)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/adaptive-cpu-alerts.js` around lines 302 - 318, Replace the inline
memory threshold logic in _checkThresholds with a call to _checkMemoryThreshold
using the existing memRss value, and append or merge its returned alerts into
the current alerts array. Preserve the existing evaluation order and cooldown
behavior while removing the duplicated comparison, severity, message, and
cooldown code.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 064fc11643
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const key = `mem-warning-${this.lane}`; | ||
| if (!this._isCooldownActive(key)) { |
There was a problem hiding this comment.
When RSS first exceeds 100 MiB and then rises above 200 MiB within the default 1,800-second cooldown, both severities use this same mem-warning-* key, so the initial WARNING suppresses the later CRITICAL alert and escalation. This is reproducible with two consecutive evaluate() calls crossing those thresholds; the second returns null, potentially hiding a rapidly worsening memory condition for 30 minutes. Use severity-specific cooldown keys or allow critical alerts to bypass a warning cooldown.
AGENTS.md reference: AGENTS.md:L330-L332
Useful? React with 👍 / 👎.
SupersededThis PR is preserved for audit history and must not be merged. It is superseded by draft replacement PR #3: The replacement branch This earlier PR uses branch Do not merge this PR. Use draft PR #3 as the clean reconciliation |
1 participant
Problem
Live adaptive-cpu-alerts.js was patched directly in the runtime repository without a corresponding git record.
Root Cause
Hotfix deployment bypassed the normal commit/push lifecycle.
Exact Changed Files
Tests
Live Deployment Status
Deployed to live runtime repository on 2026-08-04.
Live SHA256 Hashes
Rollback Notes
Remove scripts/adaptive-cpu-alerts.js or restore from origin/main.
Related PR
vortsghost2025/Archivist-Agent#2
Summary by CodeRabbit