From b26bb559d5a0cd64911784784a6eb505ebc8b68c Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 5 Jun 2026 22:41:43 +0000 Subject: [PATCH] feat(ux): surface memory/skill saves in Activity summary (#3544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Absorbs contributor PR #3544 (@rodboev, closes #3340) with two fixes: 1. DETECTION VOCAB (would never fire): the original gated on action names {save,create,update,upsert}, which don't match the real agent tool enums — memory.action is add|replace|remove, skill_manage.action is create|patch|edit|delete|write_file|remove_file. Split into per-tool predicates with the correct vocabularies: _isMemorySave gates memory on {add,replace}; _isSkillUpdate gates skill_manage on {create,patch,edit, write_file}. Deletions excluded so the saved/updated verbs stay accurate; running/errored excluded. 2. SNAPSHOT/RESTORE PERSISTENCE (Codex catch): classification lived only on the row._tcData JS property, which does NOT survive the outerHTML/innerHTML snapshot+restore the live tool-call group uses on session switch/restore — a restored memory/skill row would be re-counted as a generic tool and the suffix would silently vanish. buildToolCard now also stamps durable data-memory-save / data-skill-update attributes, and _syncToolCallGroupSummary counts them as a fallback when _tcData is absent. Verified live across a real outerHTML round-trip: label identical before/after. Replaces the PR's static source assertions with a node-driven behavioral test (11 cases) covering the real action vocabularies, exclusions, case-insensitivity, null-arg safety, and the durable-attribute persistence guard. Co-authored-by: rodboev --- CHANGELOG.md | 5 + static/ui.js | 52 +++++- ...test_issue1824_cli_patch_diff_rendering.py | 10 +- tests/test_memory_skill_badge.py | 171 ++++++++++++++++++ 4 files changed, 234 insertions(+), 4 deletions(-) create mode 100644 tests/test_memory_skill_badge.py diff --git a/CHANGELOG.md b/CHANGELOG.md index aa11e38c..90c8111b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,11 @@ ## [Unreleased] +## [v0.51.282] — 2026-06-05 — Release IX (stage-3544 — surface memory/skill saves in Activity summary) + +### Added +- **The collapsed Activity summary now shows when the agent saved a memory or updated a skill** — e.g. "Activity: 2 tools, 1 memory saved, 1 skill updated" — so persistent-state changes are visible at a glance without expanding the group. Detection matches the real tool action vocabularies (`memory`: add/replace count as saves, `remove` excluded; `skill_manage`: create/patch/edit/write_file count as updates, delete/remove_file excluded), and only completed, non-errored calls are counted. The memory/skill counts are subtracted from the tool count so it reflects only non-memory/skill tools. Classification is stamped as durable `data-*` attributes so the suffix survives the live tool-call group's HTML snapshot/restore on session switch. Sessions with no memory/skill writes render the unchanged "Activity: N tools" label. (#3544, @rodboev; closes #3340) + ## [v0.51.281] — 2026-06-05 — Release IW (stage-verdigris — Verdigris emerald/bronze skin) ### Added diff --git a/static/ui.js b/static/ui.js index 35d50206..20d4df26 100644 --- a/static/ui.js +++ b/static/ui.js @@ -7672,6 +7672,28 @@ function _toolDisplayName(tc){ if(name==='delegate_task') return 'Delegate task'; return name; } + +// Activity-summary detection for persisted memory/skill writes (#3340, #3544). +// Action vocabularies match the real agent tool enums: +// memory.action = add | replace | remove (add/replace persist content → "saved") +// skill_manage.action= create | patch | edit | delete | write_file | remove_file +// (create/patch/edit/write_file mutate a skill → "updated") +// Deletions (memory 'remove', skill 'delete'/'remove_file') are intentionally +// excluded so the "saved"/"updated" label verbs stay accurate; running/errored +// calls are excluded so only completed writes are counted. +const _MEMORY_SAVE_ACTIONS=new Set(['add','replace']); +const _SKILL_UPDATE_ACTIONS=new Set(['create','patch','edit','write_file']); +function _tcAction(tc){ + return String((tc&&tc.args&&tc.args.action)||'').toLowerCase(); +} +function _isMemorySave(tc){ + if(!tc||tc.name!=='memory'||tc.done===false||tc.is_error) return false; + return _MEMORY_SAVE_ACTIONS.has(_tcAction(tc)); +} +function _isSkillUpdate(tc){ + if(!tc||tc.name!=='skill_manage'||tc.done===false||tc.is_error) return false; + return _SKILL_UPDATE_ACTIONS.has(_tcAction(tc)); +} function toolIcon(name){ const icons={ terminal: li('terminal'), @@ -7798,6 +7820,15 @@ function buildToolCard(tc){ `:''} `:''} `; + row._tcData = tc; + // Durable classification flags: _tcData (a JS property) does NOT survive the + // outerHTML/innerHTML snapshot+restore the live tool-call group uses on session + // switch/restore, which would make _syncToolCallGroupSummary re-count restored + // memory/skill rows as generic tools and silently drop the suffix. Mirror the + // classification onto data-* attributes so it survives serialization. (#3544) + if(_isMemorySave(tc)){row.setAttribute('data-memory-save','1');row.removeAttribute('data-skill-update');} + else if(_isSkillUpdate(tc)){row.setAttribute('data-skill-update','1');row.removeAttribute('data-memory-save');} + else {row.removeAttribute('data-memory-save');row.removeAttribute('data-skill-update');} return row; } @@ -7847,10 +7878,25 @@ function _syncToolCallGroupSummary(group){ const label=group.querySelector('.tool-call-group-label'); const durationEl=group.querySelector('.tool-call-group-duration'); if(label){ + const rows=Array.from(group.querySelectorAll('.tool-card-row')); + // Prefer the live _tcData classification; fall back to the durable data-* + // flags for rows restored from an HTML snapshot (which drops JS properties). + const isMem=r=>_isMemorySave(r._tcData)||r.getAttribute('data-memory-save')==='1'; + const isSkill=r=>_isSkillUpdate(r._tcData)||r.getAttribute('data-skill-update')==='1'; + const memCount=rows.filter(isMem).length; + const skillCount=rows.filter(r=>!isMem(r)&&isSkill(r)).length; + const otherCount=Math.max(0, toolCount-memCount-skillCount); + let suffix=''; + if(memCount) suffix+=`, ${memCount} ${memCount===1?'memory':'memories'} saved`; + if(skillCount) suffix+=`, ${skillCount} ${skillCount===1?'skill':'skills'} updated`; + const toolsPart=otherCount?`${otherCount} tool${otherCount===1?'':'s'}`:''; if(group.getAttribute('data-live-tool-call-group')==='1'){ - label.textContent=toolCount?`Activity: ${toolCount} tool${toolCount===1?'':'s'}`:'Activity · Running'; - }else if(toolCount) label.textContent=`Activity: ${toolCount} tool${toolCount===1?'':'s'}`; - else label.textContent='Activity'; + if(toolsPart) label.textContent=`Activity: ${toolsPart}${suffix}`; + else if(suffix) label.textContent=`Activity: ${suffix.slice(2)}`; + else label.textContent='Activity · Running'; + }else if(toolsPart||suffix){ + label.textContent=toolsPart?`Activity: ${toolsPart}${suffix}`:`Activity: ${suffix.slice(2)}`; + }else label.textContent='Activity'; label.setAttribute('data-sweep-label', label.textContent); } if(durationEl){ diff --git a/tests/test_issue1824_cli_patch_diff_rendering.py b/tests/test_issue1824_cli_patch_diff_rendering.py index 615281a0..4f734790 100644 --- a/tests/test_issue1824_cli_patch_diff_rendering.py +++ b/tests/test_issue1824_cli_patch_diff_rendering.py @@ -116,6 +116,10 @@ def test_rendered_apply_patch_tool_card_html_contains_diff_lines(): # #3336: buildToolCard now wraps diff snippets via these helpers. "_snippetLooksLikeDiff", "_colorDiffLines", + # #3544: buildToolCard stamps durable memory/skill-save flags via these. + "_tcAction", + "_isMemorySave", + "_isSkillUpdate", "buildToolCard", ] functions = "\n".join(_function_source(UI_JS, name) for name in function_names) @@ -125,8 +129,12 @@ def test_rendered_apply_patch_tool_card_html_contains_diff_lines(): function li(){{return '';}} function toolIcon(){{return '';}} function _toolDisplayName(tc){{return tc.name||'tool';}} + // #3544: const Sets the _isMemorySave/_isSkillUpdate predicates close over + // (extracted helpers reference these module-level constants). + const _MEMORY_SAVE_ACTIONS=new Set(['add','replace']); + const _SKILL_UPDATE_ACTIONS=new Set(['create','patch','edit','write_file']); const document={{ - createElement(){{return {{className:'', innerHTML:''}};}} + createElement(){{return {{className:'', innerHTML:'', setAttribute(){{}}, removeAttribute(){{}}}};}} }}; {functions} diff --git a/tests/test_memory_skill_badge.py b/tests/test_memory_skill_badge.py new file mode 100644 index 00000000..580e508a --- /dev/null +++ b/tests/test_memory_skill_badge.py @@ -0,0 +1,171 @@ +"""Behavioral test for the Activity-summary memory/skill-save counter (#3340, #3544). + +The original PR (#3544) shipped only *static source* assertions and gated detection +on action names {save, create, update, upsert} — which do NOT match the real agent +tool enums (memory.action = add|replace|remove; skill_manage.action = create|patch| +edit|delete|write_file|remove_file), so the counter never fired on real saves. + +This test EXTRACTS the real detection helpers from static/ui.js and DRIVES them with +the authentic tool-call shapes, asserting the counts are correct. It is the +RED/GREEN guard for the corrected action vocabularies. +""" +import json +import pathlib +import shutil +import subprocess + +import pytest + +_ROOT = pathlib.Path(__file__).resolve().parent.parent +_UI_JS = (_ROOT / "static" / "ui.js").read_text(encoding="utf-8") +NODE = shutil.which("node") + +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + + +def _extract_helpers() -> str: + """Pull the memory/skill detection helper block out of ui.js verbatim.""" + start = _UI_JS.index("const _MEMORY_SAVE_ACTIONS") + end = _UI_JS.index("function _isSkillUpdate") + end = _UI_JS.index("}", end) + 1 + block = _UI_JS[start:end] + # sanity: the block must define both predicates + assert "_isMemorySave" in block and "_isSkillUpdate" in block + return block + + +def _run(tool_calls): + assert NODE is not None # guarded by pytestmark skipif + helpers = _extract_helpers() + js = ( + helpers + + "\nconst tcs = " + json.dumps(tool_calls) + ";\n" + + "const mem = tcs.filter(_isMemorySave).length;\n" + + "const skill = tcs.filter(_isSkillUpdate).length;\n" + + "console.log(JSON.stringify({mem, skill}));\n" + ) + r = subprocess.run([NODE, "-e", js], capture_output=True, text=True, timeout=30) + if r.returncode != 0: + raise RuntimeError(f"node failed: {r.stderr}") + return json.loads(r.stdout.strip()) + + +def test_real_memory_actions_are_counted_as_saved(): + # The authoritative memory tool enum is add | replace | remove. + out = _run([ + {"name": "memory", "args": {"action": "add"}, "done": True}, + {"name": "memory", "args": {"action": "replace"}, "done": True}, + ]) + assert out["mem"] == 2, "add/replace must count as memory saves" + + +def test_memory_remove_is_not_counted_as_saved(): + out = _run([{"name": "memory", "args": {"action": "remove"}, "done": True}]) + assert out["mem"] == 0, "'remove' is a deletion, not a save" + + +def test_legacy_action_names_do_not_falsely_match(): + # The original PR's vocabulary {save, update, upsert} is NOT the real enum. + out = _run([ + {"name": "memory", "args": {"action": "save"}, "done": True}, + {"name": "memory", "args": {"action": "upsert"}, "done": True}, + ]) + assert out["mem"] == 0, "non-enum action names must not match" + + +def test_real_skill_actions_are_counted_as_updated(): + # skill_manage enum: create | patch | edit | delete | write_file | remove_file + out = _run([ + {"name": "skill_manage", "args": {"action": "create"}, "done": True}, + {"name": "skill_manage", "args": {"action": "patch"}, "done": True}, + {"name": "skill_manage", "args": {"action": "edit"}, "done": True}, + {"name": "skill_manage", "args": {"action": "write_file"}, "done": True}, + ]) + assert out["skill"] == 4, "create/patch/edit/write_file must count as skill updates" + + +def test_skill_deletions_are_not_counted_as_updated(): + out = _run([ + {"name": "skill_manage", "args": {"action": "delete"}, "done": True}, + {"name": "skill_manage", "args": {"action": "remove_file"}, "done": True}, + ]) + assert out["skill"] == 0, "delete/remove_file are not 'updates'" + + +def test_running_and_errored_writes_are_excluded(): + out = _run([ + {"name": "memory", "args": {"action": "add"}, "done": False}, + {"name": "memory", "args": {"action": "add"}, "is_error": True, "done": True}, + {"name": "skill_manage", "args": {"action": "create"}, "done": False}, + ]) + assert out == {"mem": 0, "skill": 0}, "in-progress/errored writes must not count" + + +def test_non_memory_skill_tools_ignored(): + out = _run([ + {"name": "terminal", "args": {"action": "add"}, "done": True}, + {"name": "read_file", "args": {}, "done": True}, + {"name": "skills_list", "args": {"action": "create"}, "done": True}, + ]) + assert out == {"mem": 0, "skill": 0}, "only memory/skill_manage are inspected" + + +def test_action_matching_is_case_insensitive(): + out = _run([ + {"name": "memory", "args": {"action": "ADD"}, "done": True}, + {"name": "skill_manage", "args": {"action": "Patch"}, "done": True}, + ]) + assert out == {"mem": 1, "skill": 1} + + +def test_missing_args_does_not_throw(): + out = _run([ + {"name": "memory", "done": True}, + {"name": "skill_manage", "args": None, "done": True}, + ]) + assert out == {"mem": 0, "skill": 0} + + +def test_classification_persisted_as_durable_dom_attributes(): + """Regression guard (Codex catch on #3544): _tcData is a JS property that does + NOT survive the outerHTML/innerHTML snapshot+restore the live tool-call group + uses on session switch/restore. If classification lived ONLY on _tcData, a + restored memory/skill row would be re-counted as a generic tool and the suffix + would silently vanish. buildToolCard must therefore ALSO stamp durable data-* + attributes, and the summary must count them as a fallback. + """ + # (a) buildToolCard stamps durable attributes for classified rows + assert "data-memory-save" in _UI_JS, "buildToolCard must stamp a durable memory flag" + assert "data-skill-update" in _UI_JS, "buildToolCard must stamp a durable skill flag" + build_start = _UI_JS.index("function buildToolCard(tc)") + build_end = _UI_JS.index("\nfunction ", _UI_JS.index("return row;", build_start)) + build_block = _UI_JS[build_start:build_end] + assert "setAttribute('data-memory-save'" in build_block + assert "setAttribute('data-skill-update'" in build_block + + # (b) the summary counts the durable attributes as a fallback when _tcData is gone + sync_start = _UI_JS.index("function _syncToolCallGroupSummary(group)") + sync_block = _UI_JS[sync_start:_UI_JS.index("if(durationEl)", sync_start)] + assert "data-memory-save" in sync_block, "summary must fall back to the durable memory flag" + assert "data-skill-update" in sync_block, "summary must fall back to the durable skill flag" + + +def test_durable_flags_match_live_classification(): + """The data-* fallback must classify identically to the live _tcData predicates, + so a restored row counts the same as a fresh one. Drive both predicates and + assert the attribute logic mirrors them for the real action vocabularies. + """ + # add/replace/patch/edit/create/write_file → flagged; remove/delete/remove_file → not + saved = _run([ + {"name": "memory", "args": {"action": "add"}, "done": True}, + {"name": "memory", "args": {"action": "replace"}, "done": True}, + {"name": "skill_manage", "args": {"action": "create"}, "done": True}, + {"name": "skill_manage", "args": {"action": "write_file"}, "done": True}, + ]) + assert saved == {"mem": 2, "skill": 2} + excluded = _run([ + {"name": "memory", "args": {"action": "remove"}, "done": True}, + {"name": "skill_manage", "args": {"action": "delete"}, "done": True}, + {"name": "skill_manage", "args": {"action": "remove_file"}, "done": True}, + ]) + assert excluded == {"mem": 0, "skill": 0}