Add read + action tracking#33
Conversation
Two new tables: reads (auto-logged, prunable) and actions (agent-reported, permanent, tagged). Five new tools: acted_on, get_reads, get_actions, get_unread, get_activity. Auto-logging in get_knowledge, get_alerts. List mode enriched with read_count and last_read per entry. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #33: Add read + action tracking
Automated tests
- 211/211 pass (15 new for read/action tracking)
- ruff: clean
- mypy: clean
Manual tests (7/7 pass)
All executed via isolated MCP instance (claude -p --mcp-config --strict-mcp-config --allowedTools).
| # | Test | Result |
|---|---|---|
| 1 | Auto-logging: get_knowledge logs reads |
✅ tool_used="get_knowledge" recorded |
| 2 | acted_on inherits tags from entry |
✅ tags=["qa"] copied from entry |
| 3 | acted_on with custom tags + GIN filter |
✅ Custom ["qa","custom"] applied, filtered by ["custom"] |
| 4 | get_unread returns zero-read entries |
✅ Only never-read entry returned |
| 5 | get_activity combined feed |
✅ Interleaved reads + actions, descending timestamp |
| 6 | List mode read_count/last_read enrichment |
✅ Both fields present with correct values |
| 7 | Cleanup via delete_entry |
✅ Entries soft-deleted |
Code review
Architecture: Clean separation — protocol in store.py, implementation in postgres_store.py, MCP wiring in server.py. Fire-and-forget _log_reads wrapper ensures read logging never breaks tool responses. Good.
Schema: Two new tables with proper FK (ON DELETE CASCADE), indexes (entry_id, timestamp, GIN on tags), and a matching Alembic migration. Migration matches the inline DDL in postgres_store.py. Good.
Docs: CHANGELOG, README (tool count 18→23, test count 196→211), data-dictionary, deployment-guide all updated. Good.
Findings
1. CASCADE vs soft delete mismatch (documentation issue)
PR body and QA step 7 both state: "Reads/actions auto-cleaned via CASCADE."
But delete_entry is a soft delete (sets deleted timestamp). ON DELETE CASCADE only fires on actual SQL DELETE. During manual testing, reads and actions persisted after delete_entry.
The CASCADE is correct for the eventual hard delete path (_cleanup_expired), but the PR description is misleading. Suggest updating:
- PR body line 28:
ON DELETE CASCADEnote should mention it applies on hard delete (trash purge), notdelete_entry - QA step 7 expected: remove "Reads/actions auto-cleaned via CASCADE" or clarify it happens at purge time
2. acted_on with invalid entry_id — unhandled FK violation
log_action in postgres_store.py does not validate that entry_id exists before INSERT. If called with a nonexistent ID, the FK constraint produces a raw psycopg exception that bubbles up through acted_on as an unstructured error. Unlike log_read (which is fire-and-forget with try/except), acted_on is an explicit user action that should return a clear error.
Suggest: catch psycopg.errors.ForeignKeyViolation (or check get_entry_by_id result) and return a structured {"status": "error", "message": "entry not found"}.
3. "and other read tools" — only 2 instrumented (minor, docs)
PR body says: "auto-logs when entries are accessed by get_knowledge, get_alerts, and other read tools."
Only get_knowledge and get_alerts call _log_reads. Other read paths (get_status, get_briefing, get_suppressions, get_patterns) do not. This is likely intentional (those are operational, not knowledge consumption), but the phrasing "and other read tools" implies broader coverage. Suggest removing that phrase or enumerating the two tools explicitly.
Verdict
Finding #1 is a documentation fix (misleading expected behavior in PR body and summary). Finding #2 is a minor robustness issue. Finding #3 is a wording nit.
None are architectural concerns — the feature is well-implemented. Requesting changes for the FK handling (#2) since it produces unstructured exceptions for a foreseeable user error. The doc fixes (#1, #3) can be addressed in the same pass.
- log_action returns structured error for nonexistent entry_id (prevents FK violation bubbling as unstructured exception) - acted_on tool checks for error status before returning ok - Added tests: store-level and server-level invalid entry_id cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Response to QA findingsAll three addressed in commit 1. CASCADE vs soft delete mismatch (docs) — You're right, CASCADE only fires on hard delete (trash purge), not 2. 3. "and other read tools" phrasing — Acknowledged. Only 213 tests, all green. Updated QA steps for finding #2Expected: No unstructured exception, no FK violation traceback. |
cmeans
left a comment
There was a problem hiding this comment.
QA Re-review — PR #33 (round 2)
Fix verification (commit 645eba3)
All three findings from round 1 addressed:
- CASCADE vs soft delete — Acknowledged. Docs note pending.
acted_oninvalid entry_id — ✅ Verified. Returns{"status": "error", "message": "Entry not found: nonexistent-id"}. No FK violation, no unstructured exception. Tests added at both store and server level.- "and other read tools" phrasing — Acknowledged. Docs fix pending.
Automated tests
- 213/213 pass (+2 from fix:
test_acted_on_invalid_entry,test_log_action_invalid_entry_id) - ruff + mypy clean
Manual retest
acted_on(entry_id="nonexistent-id", action="should fail")→ structured error ✅
Verdict
Feature is solid. All findings resolved. Ready for QA Approved label.
- CHANGELOG/data-dictionary: specify get_knowledge and get_alerts (not generic "read tools") as the instrumented tools - Data dictionary: clarify CASCADE fires on hard delete (purge), not soft delete (delete_entry) - Test counts: 211 → 213 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Doc fixes pushed (commit
|
Summary
Two new tables and five new tools for tracking how knowledge is consumed and acted upon.
Read tracking — auto-logs when entries are accessed by
get_knowledge,get_alerts, and other read tools. Fire-and-forget (never blocks responses). Query withget_reads.Action tracking —
acted_on(entry_id, action, platform?, detail?, tags?)records concrete actions agents take because of entries. Tags default to the referenced entry's tags. Query withget_actions.Unread entries —
get_unread(since?)returns entries with zero reads. Cleanup candidates.Activity feed —
get_activity(since?, platform?, limit?)returns combined reads + actions chronologically.List mode enrichment —
get_knowledge(mode="list")now includesread_countandlast_readon each entry.New tools (5)
acted_onget_readsget_actionsget_unreadget_activitySchema changes
readstable: id, entry_id, timestamp, platform, tool_usedactionstable: id, entry_id, timestamp, platform, action, detail, tags (JSONB + GIN)ON DELETE CASCADE— reads/actions auto-clean when entries are deletedQA
Prerequisites
Automated tests
Manual tests (via MCP tools)
Expected:
get_readsreturns at least 1 read withtool_used="get_knowledge"Expected: action recorded with
tags=["qa"](copied from entry)Expected: returns the action with custom tags, filtered correctly
Expected: the new entry appears (it hasn't been fetched via get_knowledge yet)
Expected: interleaved read + action events, most recent first
Expected: entries include
read_count(number) andlast_read(timestamp or null)Expected: QA entries removed. Reads/actions auto-cleaned via CASCADE.
🤖 Generated with Claude Code