Fix #746: Correct inverted type 2 normal restoral conditions - #796
Merged
TheWitness merged 2 commits intoSep 20, 2026
Merged
TheWitness merged 2 commits into
TheWitness merged 2 commits into
Conversation
The type 2 (time-based) restoral logic in thold_check_threshold() used '<' instead of '>=' for the fail count comparisons: - Warning restoral: $warning_failures < $warning_trigger - Alert restoral: $failures < $trigger This inversion caused restorals to fire for thresholds that were never triggered (count below trigger), while silently suppressing restorals for thresholds that were actually triggered (count at or above trigger). Changed '<' to '>=' for both comparisons, matching the correct pattern used in the type 0 restoral logic which checks: $thold_data['thold_warning_fail_count'] >= $warning_trigger
somethingwithproof
approved these changes
Aug 17, 2026
somethingwithproof
previously requested changes
Aug 17, 2026
somethingwithproof
left a comment
somethingwithproof
left a comment
Member
There was a problem hiding this comment.
The inverted restoral comparisons appear to be the intended fix, but this branch includes the three unrelated #789 threshold_value edits and has no behavioral coverage. Please isolate the two restoral-condition changes, add Cacti-Composer/Pest cases for never-triggered, warning-triggered, and alert-triggered type-2 thresholds returning to normal, and add the CHANGELOG entry. The integration matrix is red.
Member
|
All four checks fail before this plugin loads because the branch’s older workflow tests unpinned Cacti develop, which crashes in core with an undefined __() call. Maintainer edits are disabled, so I cannot apply the pinned-Cacti and bounded-retry workflow fix. Please enable maintainer edits or update/rebase the branch. |
xmacan
approved these changes
Sep 15, 2026
Member
|
The CI failures here are not caused by this PR. All four integration test legs fail identically with: This happens during That upstream branch has since moved forward and no longer contains the problematic early call. Since these workflow runs are more than 30 days old, GitHub won't allow a direct rerun, so I'm closing and reopening this PR to trigger a fresh CI run against the current |
TheWitness
dismissed
somethingwithproof’s stale review
September 20, 2026 01:13
I'm going to be removing the integration tests. They will be restored in the near future across all plugins.
TheWitness
added a commit
that referenced
this pull request
Sep 20, 2026
Merging develop brought in #796's fix for thold_check_threshold()'s type-2 (time-based) restoral logic (`<` -> `>=` for the warning/alert fail-count comparisons). This test harness always stubs the log-derived fail count to 0, and the two restoral tests here relied on the default trigger of 1, so under the corrected operator (0 >= 1 is false) neither restoral branch fired anymore and both tests broke. Override time_fail_trigger/time_warning_fail_trigger to 0 in both tests, representing a threshold with no minimum streak requirement, so the harness's stubbed zero count still satisfies the corrected `>=` check and the tests exercise real restoral behavior again instead of the inverted bug.
TheWitness
added a commit
that referenced
this pull request
Sep 21, 2026
…vr4v-qvqm-gm9j) (#807) * security: replace eval() in RPN math binary ops with safe dispatch (S1523) Replace the eval()-based binary arithmetic evaluation at line 378 with a new thold_rpn_math_binary() helper function that uses a switch statement for +, -, *, /, %, ^ operators. The operator was already whitelisted via in_array(, , true) and operands validated with is_numeric(), but removing eval() eliminates the code injection risk entirely (CWE-95). Addresses SonarQube finding: php:S1523 at thold_functions.php:378 * ci: retry transient dependency downloads * test(rpn): cover safe binary dispatch * fix: restore SQL binding, trigger-command quoting, and RPN safety fixes Same set of pre-existing gaps as develop (covered by tests but never implemented in thold_functions.php), plus one bug specific to this PR: - get_allowed_thresholds()/get_allowed_threshold_logs(): bind graph_id and caller-supplied params instead of interpolating them into the SQL text, and use the *_prepared fetchers. - thold_replace_threshold_tags(): add a \ flag that quotes device/threshold free-text values with cacti_escapeshellarg() when the substituted text is a trigger command. - thold_command_execution(): pass shell=true for the three trigger commands, and fix a topic-string typo ('thold' vs 'thold_cmd') that silently suppressed exit-status logging for inline command runs. - thold_expression_math_rpn(): remove a stray break that exited the operator switch before pushing the 0/0 result, flag modulo-by-zero instead of letting PHP throw DivisionByZeroError, reject non-numeric unary operands before eval(), and flag SQRT/LOG results that are NAN or INF instead of pushing them onto the stack (unary math here is still eval()-based; unrelated to this PR's binary-math dispatcher). - thold_rpn_math_binary(): this PR's new dispatcher mapped '^' to pow(), silently changing it from the pre-existing bitwise XOR behaviour (eval('... ^ ...') always computed XOR). Restored XOR to avoid breaking existing user thresholds, and corrected this PR's own TholdRpnBinaryMathTest 'power' dataset, which had asserted the incorrect pow() behaviour. * fix(tests): stop includes/arrays.php loading from silently no-op'ing thold_functions.php includes includes/arrays.php with a plain include() (not include_once()) from several of its own functions (e.g. thold_log()), keyed off \['base_path']. Once any test in the shared Pest process exercised one of those call sites, PHP's include-once registry considered the file already included: a later thold_test_load() (require_once) on the same resolved path silently no-op'd and never (re)published \ to \, depending on which order the test files happened to run in. This intermittently broke every test that reads \ (TholdReplaceThresholdTagsTest's THOLDTYPE substitution) as well as tests several call frames downstream of thold_log()/thold_check_threshold() (NotificationEmailDeduplicationTest, ThresholdTimeBasedCharacterizationTest), depending on file execution order. Add thold_test_load_always()/loadPluginSourceAlways(), which use a plain require() instead of require_once(), for plugin files that only assign file-scope variables (no function/class declarations) and are therefore safe to load more than once. Switch the four test classes that load includes/arrays.php to the new helper. * debug: temporary CI diagnostics for notification/restoral failures * Revert "debug: temporary CI diagnostics for notification/restoral failures" This reverts commit 21fbd16. * debug: temporary CI diagnostics for notification/restoral failures * test: fix restoral tests for the corrected type-2 restoral operator Merging develop brought in #796's fix for thold_check_threshold()'s type-2 (time-based) restoral logic (`<` -> `>=` for the warning/alert fail-count comparisons). This test harness always stubs the log-derived fail count to 0, and the two restoral tests here relied on the default trigger of 1, so under the corrected operator (0 >= 1 is false) neither restoral branch fired anymore and both tests broke. Override time_fail_trigger/time_warning_fail_trigger to 0 in both tests, representing a threshold with no minimum streak requirement, so the harness's stubbed zero count still satisfies the corrected `>=` check and the tests exercise real restoral behavior again instead of the inverted bug. --------- Co-authored-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #746
In
thold_functions.php, the type 2 (time-based) normal restoral logic inthold_check_threshold()(lines ~3443 and ~3512) used<instead of>=for the fail count comparisons:Impact
The
<vs>=inversion caused:Fix
Changed
<to>=for both$warning_failuresand$failurescomparisons in the type 2 restoral block:This matches the correct pattern used in the type 0 restoral logic (line ~2685), which checks:
Testing
php -l thold_functions.php— no syntax errors.