Skip to content

Commit 7cdd1e7

Browse files
m4r1kclaude
andcommitted
fix: address PR #42 review findings
Addresses CodeRabbit's 5 findings (cubic.dev was clean): 1. docs/changelog.md — clarify the live TUI events cap is now min(30, visible_panel_rows), not a flat 30, since the second commit made it height-bound. 2. docs/configuration.md — make all four example commands use the same `sudo eneru ...` invocation form. Earlier draft mixed the wrapper-path form with the bare command; maintainer prefers the bare form (PATH-resolved) consistently. 3. src/eneru/tui.py — three-tier trim instead of two: POWER_EVENTS always survive; LIFECYCLE_EVENTS fill next; chatter rounds out only if room remains. In --verbose mode the previous code let chatter evict lifecycle rows when the cap was hit; the new tier matches the user-facing priority hierarchy. 4. tests/test_tui.py — rename single-letter loop var `l` to `line` in five spots (Ruff E741). 5. tests/e2e/groups/stats.sh — wrap the `--length 5` invocation in `if cmd; then ... fi`. Under `set -e` the previous bare `[ $? -ne 0 ]` branch would never run because the script aborts on the first non-zero exit, masking any regression with no log dump. Plus one new unit test (`test_tiered_trim_lifecycle_outranks_chatter`) covering the new 3-tier semantics: 1 power + 30 chatter + 4 lifecycle at cap=7 produces 1 power + 4 lifecycle + 2 chatter, NOT 1 power + 6 chatter. 982 unit tests pass (was 981). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent c571b82 commit 7cdd1e7

5 files changed

Lines changed: 78 additions & 22 deletions

File tree

docs/changelog.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,9 @@ Bug-fix release. Drop-in upgrade.
3535
### Changed
3636
- `--time` and `<T>` apply to the graph only. Use `--length` for events.
3737
- Events panel defaults to priority-only.
38-
- Live TUI events cap raised from 8 to 30 rows.
38+
- Live TUI events cap raised (now `min(30, visible_panel_rows)` so
39+
power events stay inside the visible window on smaller terminals;
40+
`<M>` still expands to 500 rows for full scrollable history).
3941

4042
### Migration notes
4143
- Scripts grepping `--events-only` output for low-priority event

docs/configuration.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -442,5 +442,5 @@ Example package commands:
442442
sudo eneru validate --config /etc/ups-monitor/config.yaml
443443
sudo eneru run --dry-run --config /etc/ups-monitor/config.yaml
444444
sudo eneru monitor --once --events-only --config /etc/ups-monitor/config.yaml
445-
sudo /opt/ups-monitor/eneru.py monitor --once --events-only --verbose --length 100 --config /etc/ups-monitor/config.yaml
445+
sudo eneru monitor --once --events-only --verbose --length 100 --config /etc/ups-monitor/config.yaml
446446
```

src/eneru/tui.py

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -429,11 +429,11 @@ def query_events_for_display(
429429
430430
``max_events=None`` (or 0) disables the row cap entirely.
431431
432-
Trim is **tiered**: when capping, every row in :data:`POWER_EVENTS`
433-
is preserved, then remaining slots are filled with the most-recent
434-
:data:`LIFECYCLE_EVENTS` and other priority rows. Daemon noise can
435-
therefore never push power events off-screen, even when the cap is
436-
20 and the most recent 50 events are all daemon restarts.
432+
Trim is **3-tier**: POWER_EVENTS always survive the cap; remaining
433+
slots are filled first with most-recent LIFECYCLE_EVENTS, then with
434+
chatter (only reachable when ``priority_only=False`` widens the
435+
upstream filter). Power events are never evicted; lifecycle context
436+
outranks voltage-flap chatter when the cap is hit in verbose mode.
437437
"""
438438
multi_ups = config.multi_ups
439439
rows: List[tuple] = [] # (ts, label, event_type, detail)
@@ -470,22 +470,33 @@ def query_events_for_display(
470470

471471
rows.sort(key=lambda r: r[0])
472472

473-
# Tiered trim: power events always survive the cap. ``max_events``
474-
# in (None, 0) means no cap.
473+
# Three-tier trim: POWER_EVENTS always survive; if room left,
474+
# LIFECYCLE_EVENTS fill next; only then chatter (other rows that
475+
# made it through ``priority_only=False``). This matches the
476+
# priority hierarchy users care about: power transitions are
477+
# never evicted, daemon-lifecycle context is preferred over
478+
# voltage-flap chatter, and chatter rounds out the cap when
479+
# ``--verbose`` is on. ``max_events`` in (None, 0) means no cap.
475480
if max_events and len(rows) > max_events:
476481
power = [r for r in rows if r[2] in POWER_EVENTS]
477-
non_power = [r for r in rows if r[2] not in POWER_EVENTS]
482+
lifecycle = [r for r in rows if r[2] in LIFECYCLE_EVENTS]
483+
chatter = [r for r in rows
484+
if r[2] not in POWER_EVENTS and r[2] not in LIFECYCLE_EVENTS]
478485
if len(power) >= max_events:
479486
# Pathological: more power events than the cap. Show the
480487
# most-recent N -- still better than evicting them.
481488
kept = power[-max_events:]
482489
else:
483490
remaining = max_events - len(power)
484-
kept_non_power = non_power[-remaining:]
491+
# Lifecycle fills first; chatter only if room remains.
492+
kept_lifecycle = lifecycle[-remaining:]
493+
remaining -= len(kept_lifecycle)
494+
kept_chatter = chatter[-remaining:] if remaining > 0 else []
485495
# Re-sort by ts so the panel never displays out-of-time-order
486496
# rows after the tier merge. Python's sort is stable, so
487-
# equal-timestamp rows preserve insertion order (power first).
488-
kept = sorted(power + kept_non_power, key=lambda r: r[0])
497+
# equal-timestamp rows preserve insertion order.
498+
kept = sorted(power + kept_lifecycle + kept_chatter,
499+
key=lambda r: r[0])
489500
rows = kept
490501

491502
return [_format_event_line(ts, label, etype, detail, multi_ups)

tests/e2e/groups/stats.sh

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -264,8 +264,12 @@ echo " PASS: priority-only filter hides chatter, keeps DAEMON_START"
264264
# --length 5 caps output. There's only one daemon-lifecycle event in the
265265
# stats DB at this point (from the daemon's own DAEMON_START), so we
266266
# can't easily count to 5 here -- just confirm --length runs cleanly.
267-
eneru monitor --once --events-only --length 5 --config "$E2E_DIR/config-e2e-stats.yaml" > /tmp/test31-length.log 2>&1
268-
if [ $? -ne 0 ]; then
267+
# Wrap the invocation in the `if` directly: under `set -e` a separate
268+
# `[ $? -ne 0 ]` branch would never run because the script aborts on
269+
# the first non-zero exit, masking any regression with no log dump.
270+
if ! eneru monitor --once --events-only --length 5 \
271+
--config "$E2E_DIR/config-e2e-stats.yaml" \
272+
> /tmp/test31-length.log 2>&1; then
269273
echo "FAIL: --length 5 returned non-zero"
270274
cat /tmp/test31-length.log
271275
exit 1

tests/test_tui.py

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1239,16 +1239,16 @@ def test_tiered_trim_preserves_power_events(self, tmp_path):
12391239

12401240
lines = query_events_for_display(config, max_events=20)
12411241
# All 3 power-event rows MUST survive the cap.
1242-
assert any("ON_BATTERY: real outage" in l for l in lines), (
1242+
assert any("ON_BATTERY: real outage" in line for line in lines), (
12431243
f"ON_BATTERY pushed off by daemon noise -- tiered trim regressed. "
12441244
f"Got: {lines}"
12451245
)
1246-
assert any("POWER_RESTORED: outage 10m" in l for l in lines)
1247-
assert any("EMERGENCY_SHUTDOWN_INITIATED" in l for l in lines)
1246+
assert any("POWER_RESTORED: outage 10m" in line for line in lines)
1247+
assert any("EMERGENCY_SHUTDOWN_INITIATED" in line for line in lines)
12481248
# Result respects the cap.
12491249
assert len(lines) == 20
12501250
# Remaining 17 slots are most-recent daemon rows.
1251-
daemon_lines = [l for l in lines if "DAEMON_RESTARTED" in l]
1251+
daemon_lines = [line for line in lines if "DAEMON_RESTARTED" in line]
12521252
assert len(daemon_lines) == 17
12531253

12541254
@pytest.mark.unit
@@ -1271,9 +1271,48 @@ def test_tiered_trim_pathological_more_power_than_cap(self, tmp_path):
12711271
lines = query_events_for_display(config, max_events=10)
12721272
# 10 most-recent ON_BATTERY rows; daemon entirely evicted.
12731273
assert len(lines) == 10
1274-
assert all("ON_BATTERY" in l for l in lines)
1275-
assert any("outage-49" in l for l in lines) # latest power event
1276-
assert any("outage-40" in l for l in lines) # 10th-latest
1274+
assert all("ON_BATTERY" in line for line in lines)
1275+
assert any("outage-49" in line for line in lines) # latest power event
1276+
assert any("outage-40" in line for line in lines) # 10th-latest
1277+
1278+
@pytest.mark.unit
1279+
def test_tiered_trim_lifecycle_outranks_chatter(self, tmp_path):
1280+
"""5.2.2 (CodeRabbit refinement): in --verbose mode (priority_only
1281+
=False), chatter rows reach the trim layer alongside lifecycle
1282+
rows. Lifecycle is more important than per-condition chatter --
1283+
when the cap evicts non-power rows, lifecycle should survive
1284+
first, then chatter fills any leftover slots."""
1285+
from eneru.tui import query_events_for_display
1286+
import time as _time
1287+
config = _events_config(tmp_path)
1288+
now = int(_time.time())
1289+
# 1 power event + 30 chatter rows + 4 lifecycle rows.
1290+
# Cap = 7. Result must be: 1 power + 4 lifecycle + 2 chatter,
1291+
# NOT: 1 power + 6 chatter (chatter evicting lifecycle).
1292+
events = [(now - 5000, "ON_BATTERY", "outage")]
1293+
for i in range(30):
1294+
events.append(
1295+
(now - 1000 + i, "VOLTAGE_FLAP_SUPPRESSED", f"flap-{i}")
1296+
)
1297+
for i in range(4):
1298+
events.append(
1299+
(now - 100 + i * 10, "DAEMON_RESTARTED", f"d-{i}")
1300+
)
1301+
_seed_events(config, config.ups_groups[0], events)
1302+
1303+
lines = query_events_for_display(
1304+
config, max_events=7, priority_only=False,
1305+
)
1306+
assert len(lines) == 7
1307+
assert sum("ON_BATTERY" in line for line in lines) == 1, (
1308+
f"power event must survive: {lines}"
1309+
)
1310+
assert sum("DAEMON_RESTARTED" in line for line in lines) == 4, (
1311+
f"all 4 lifecycle rows must outrank chatter; got: {lines}"
1312+
)
1313+
assert sum("VOLTAGE_FLAP_SUPPRESSED" in line for line in lines) == 2, (
1314+
f"chatter fills only the 2 leftover slots; got: {lines}"
1315+
)
12771316

12781317

12791318
class TestRobustBounds:

0 commit comments

Comments
 (0)