Skip to content

fix(achievements): persist lifetime counters to prevent regression after prune - #28713

Closed
ygd58 wants to merge 2 commits into
NousResearch:mainfrom
ygd58:fix/achievements-lifetime-counter-persist-v2
Closed

ygd58 wants to merge 2 commits into
NousResearch:mainfrom
ygd58:fix/achievements-lifetime-counter-persist-v2

Conversation

@ygd58

@ygd58 ygd58 commented May 19, 2026

Copy link
Copy Markdown
Contributor

Problem

When sessions.auto_prune deletes old rows from state.db, achievement lifetime counters regress because they are recomputed only from remaining sessions (issue #28661).

Fix

Merge scanned aggregate counters with persisted lifetime_counters in state.json using max(). Once a counter reaches a value it never goes backward — pruning state.db cannot reduce lifetime achievements.

Fixes #28661

…ter prune

When sessions.auto_prune deletes old rows from state.db, achievement
lifetime counters regress because they are recomputed only from the
remaining sessions (issue NousResearch#28661).

Fix: merge scanned aggregate counters with persisted lifetime_counters
in state.json using max(). Once a counter reaches a value it never
goes backward — pruning state.db cannot reduce lifetime achievements.

Fixes NousResearch#28661
@supplefrog

Copy link
Copy Markdown
Contributor

I checked the duplicate delta from #50882. The direct missing piece here is regression coverage for the prune case; the implementation in this PR already passes the targeted achievements tests with that coverage added.

I couldn't push directly to the fork branch from this account, so here is an apply-ready patch:

From 4a5dae945fd5c56c4eb587a291460b51952d2e1e Mon Sep 17 00:00:00 2001
From: supplefrog <78985073+supplefrog@users.noreply.github.com>
Date: Tue, 23 Jun 2026 03:31:11 +0530
Subject: [PATCH] test(achievements): cover lifetime counter pruning

---
 tests/plugins/test_achievements_plugin.py | 34 +++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tests/plugins/test_achievements_plugin.py b/tests/plugins/test_achievements_plugin.py
index 2d908b3d4..9932cfd30 100644
--- a/tests/plugins/test_achievements_plugin.py
+++ b/tests/plugins/test_achievements_plugin.py
@@ -375,3 +375,37 @@ def test_partial_snapshots_do_not_persist_unlock_timestamps(plugin_api):
         "partial scans must not record unlock timestamps — a later session "
         "could change whether the badge deserves to be unlocked yet"
     )
+
+
+def test_full_scans_persist_lifetime_counters_monotonically(plugin_api):
+    """Lifetime achievement progress must not regress when old sessions are pruned."""
+    plugin_api.save_state({"unlocks": {}})
+
+    high_scan = {
+        "sessions": [{"session_id": "old", "title": "old", "tool_call_count": 5000}],
+        "aggregate": {
+            "session_count": 500,
+            "total_tool_calls": 5000,
+            "max_tool_calls_in_session": 200,
+        },
+        "scan_meta": {"mode": "full"},
+    }
+    high = plugin_api._compute_from_scan(high_scan, is_partial=False)
+
+    assert high["aggregate"]["total_tool_calls"] == 5000
+    assert plugin_api.load_state()["lifetime_counters"]["total_tool_calls"] == 5000
+
+    low_scan = {
+        "sessions": [{"session_id": "new", "title": "new", "tool_call_count": 1}],
+        "aggregate": {
+            "session_count": 1,
+            "total_tool_calls": 1,
+            "max_tool_calls_in_session": 1,
+        },
+        "scan_meta": {"mode": "full"},
+    }
+    low = plugin_api._compute_from_scan(low_scan, is_partial=False)
+
+    assert low["aggregate"]["total_tool_calls"] == 5000
+    assert low["aggregate"]["session_count"] == 500
+    assert plugin_api.load_state()["lifetime_counters"]["total_tool_calls"] == 5000
-- 
2.53.0.windows.2

Verified locally on this PR branch:

uv run pytest tests/plugins/test_achievements_plugin.py -q
# 10 passed

After this patch lands, a normal synchronize/re-run should also pick up the current supply-chain workflow from main; the existing supply-chain failure appears to be stale workflow noise, not a finding in this PR's one-file achievements diff.

Adds regression coverage for the prune scenario: a full scan with high
counts persists lifetime_counters, then a subsequent full scan with
lower counts (simulating sessions.auto_prune deleting old rows) must
not regress the persisted lifetime values.

Patch contributed by @supplefrog in review of NousResearch#28713.

Verified: 10/10 tests pass.
@ygd58

ygd58 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @supplefrog -- applied your patch cleanly. All 10 tests pass including the new test_full_scans_persist_lifetime_counters_monotonically.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused regression fix. The current-main premise is confirmed: scan_sessions() reads only live SessionDB rows and recomputes the aggregate from them (plugins/hermes-achievements/dashboard/plugin_api.py:604,662).

Problems

  • The added max() merge at plugins/hermes-achievements/dashboard/plugin_api.py:810 prevents a decrease, but it also discards activity after pruning. With a persisted total_tool_calls of 5000, a post-prune scan containing one newly retained call still reports 5000 rather than 5001.
  • The scanner rewrites its checkpoint using only current sessions (plugins/hermes-achievements/dashboard/plugin_api.py:607,641,651-655), so it cannot reconstruct those omitted increments after older rows disappear.

Suggested changes

  • Preserve cumulative per-session contributions (or an equivalent durable ledger) so new live sessions can advance lifetime totals after pruning.
  • Extend tests/plugins/test_achievements_plugin.py with a prune-then-new-activity case that asserts the persisted lifetime counter increases.

Automated hermes-sweeper review.

scanned_val = aggregate.get(key, 0)
persisted_val = persisted_counters.get(key, 0)
try:
merged[key] = max(int(scanned_val or 0), int(persisted_val or 0))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This high-water mark prevents regression, but it also loses post-prune increments: a persisted total of 5000 plus a scan containing one new tool call remains 5000. Please use a durable cumulative contribution ledger (or equivalent) and add a prune-then-new-activity regression case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The author opened #63736 with a per-session ledger approach. I reviewed that replacement and found its first version filtered raw checkpoint stats by total_/max_, so real scans would persist nothing; its test fabricated aggregate keys and bypassed scan_sessions(). I posted a cherry-pickable correction and real end-to-end prune/append coverage here: #63736 (comment). #63736 should supersede this max()-only PR once corrected.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
@ygd58 ygd58 closed this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hermes-achievements lifetime counters regress after sessions.auto_prune prunes state.db

4 participants