Add INTENTION entry type with time-based triggers#35
Conversation
New EntryType.INTENTION with lifecycle state machine (pending → fired → completed/snoozed/cancelled). Three new tools: remind, get_intentions, update_intention. Collator evaluates pending intentions in briefing — surfaces fired_intentions when deliver_at has passed. Recurrence field present but one-shot only for v1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- parse_iso: normalize 'Z' to '+00:00' for fromisoformat compatibility on Python 3.10 (3.11+ handles 'Z' natively) - Add test for get_intentions source filter and limit param Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #35: Add INTENTION entry type with time-based triggers
Automated tests
- 231/231 pass (PR says 230 — likely the second commit
ad47c19added one more) - ruff: clean
- mypy: clean
Manual tests (9/9 pass)
All executed via isolated MCP instance (claude -p --mcp-config --strict-mcp-config --allowedTools).
| # | Test | Result |
|---|---|---|
| 1 | Create time-based intention | ✅ state: "pending", ID returned |
| 2 | get_intentions(state="pending") |
✅ All fields present (goal, state, constraints, urgency, deliver_at) |
| 3 | Briefing fires past-due intention | ✅ attention_needed: true, fired_intentions with goal, summary "1 intention ready" |
| 4 | Complete intention | ✅ state: "completed" with reason |
| 5 | Verify state transition + changelog | ✅ state: completed, changelog records previous state |
| 6 | Future intention NOT fired | ✅ intentions_pending: 1, intentions_fired: 0, attention_needed: false |
| 7 | Invalid state transition | ✅ Structured error with valid states |
| 8 | List mode | ✅ Metadata only, no data field |
| 9 | Cleanup | ✅ Both entries trashed |
Code review
Architecture: Clean extension — EntryType.INTENTION + INTENTION_STATES in schema, 3 new store protocol methods, PostgresStore implementation uses JSONB data->>'state' queries (consistent with existing patterns), collator evaluates in generate_briefing(). Good.
Briefing integration: Fired intentions surface with attention_needed, summary, and suggested_mention. Evaluation field extended with intentions_pending and intentions_fired. Good.
Python 3.10 compat: ensure_dt now handles Z suffix — good catch (commit ad47c19).
Docs: CHANGELOG, README (tool count 23→26, test count 213→230), CLAUDE.md (8 entry types), data-dictionary, deployment-guide all updated. Good.
Findings
1. Double-counting: intentions_pending includes fired intentions
In the briefing, a past-due intention shows up in both intentions_fired (correctly) and intentions_pending (because it's still in pending state in the DB). Example from test 3:
"intentions_pending": 1,
"intentions_fired": 1These are the same entry. The collator calls get_fired_intentions() (returns pending entries with past deliver_at) and then get_intentions(state="pending") (returns ALL pending entries, including past-due ones). The fired count is a subset of pending.
This could confuse consumers. Options:
- Subtract fired from pending:
"intentions_pending": len(pending) - len(fired) - Or rename to
"intentions_total_pending"to clarify it includes fired
2. Changelog records new state_reason as old value
In update_intention_state (postgres_store.py:880-888):
entry.data["state_reason"] = reason # line 882 — sets new reason
...
changed["state_reason"] = entry.data.get("state_reason", "") # line 887 — reads it backThe changed dict is supposed to capture old values (as done elsewhere with update_entry). But state_reason is set to the new value on line 882, then read back on line 887. So the changelog records the new reason as if it were the old one. For the first transition (pending→completed), there was no old state_reason, so the changelog should record "" or omit it — but instead it records the new reason.
Fix: capture old_reason = entry.data.get("state_reason") before line 882, use that in changed.
3. List mode doesn't show goal or state for intentions (observation)
to_list_dict() populates description from data.description, but intentions use data.goal. Both entries show "description": "" in list mode. For intentions, the goal and current state are the most important metadata — list mode is less useful without them.
Suggest: override to_list_dict() for intentions, or have the get_intentions list mode add goal and state from data (similar to how get_knowledge list mode adds read_count).
4. No state transition validation (observation, non-blocker)
Any state can transition to any other state (e.g., completed→pending, cancelled→fired). The docstring describes a directed lifecycle pending → fired → completed/snoozed/cancelled but it isn't enforced. This is fine for v1 (flexibility is useful), but worth noting for future hardening.
Verdict
Finding #1 (double-counting) and #2 (changelog bug) are the substantive issues. #3 and #4 are observations for consideration.
The feature is well-implemented and all tests pass. Requesting changes for the double-counting (#1) and changelog bug (#2).
- Exclude fired intentions from pending count (were being double-counted) - Capture old state_reason before setting new one in changelog - Update test assertion for corrected pending count Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Response to QA findingsBoth substantive issues fixed in commit 1. Double-counting pending/fired — Fired intentions are now excluded from the pending count. 2. Changelog records new reason as old — 3. List mode + goal/state (observation) — Acknowledged. Will address in the upcoming SQL improvements PR alongside the 4. State transition validation (observation) — Agreed, fine for v1. Noted for future hardening. Updated QA steps for fixesFinding 1 — verify pending excludes fired: Expected: Finding 2 — verify changelog captures old reason: Then transition twice: Expected: changelog has 2 entries. Second entry's Cleanup: 231 tests, all green. |
cmeans
left a comment
There was a problem hiding this comment.
QA Re-review — PR #35 (round 2)
Fix verification (commit 574b0eb)
Both substantive findings addressed:
-
Double-counting pending/fired — ✅ Verified. Past-due intention now shows
intentions_fired: 1, intentions_pending: 0.fired_idsset correctly excludes fired from pending count. -
Changelog captures old reason — ✅ Verified. Two-transition test (pending→fired with "time trigger", fired→completed with "done"):
- Changelog entry 1:
changed.state: "pending"(no reason — correct) - Changelog entry 2:
changed.state: "fired",changed.state_reason: "time trigger"(old reason — correct) - Current:
state_reason: "done"(new reason — correct)
- Changelog entry 1:
Automated tests
- 231/231 pass
- ruff + mypy clean
Verdict
Both fixes clean and verified. Ready for QA Approved label.
Summary
New
EntryType.INTENTION— goals with constraints, evaluated when conditions align. This is the foundation for location-aware reminders, deferred triggers, and the decision-support features described in the vision document.What's included (v1):
deliver_attimestamp, evaluated in briefingremind,get_intentions,update_intentionattention_needed, summary, mentionintentions_pendingandintentions_firedcountsWhat's deferred:
New tools (3)
remindget_intentionsupdate_intentionQA
Prerequisites
Automated tests
Manual tests (via MCP tools)
Expected:
{"status": "ok", "id": "...", "state": "pending"}Expected: the milk intention appears with all fields
Expected:
attention_needed: true,fired_intentionscontains the milk goal,evaluation.intentions_fired >= 1, summary mentions "intention ready"Expected:
{"status": "ok", "state": "completed"}Expected: the milk intention with
state: completed, changelog shows transition from pendingExpected:
pending_intentions >= 1but nofired_intentionsfor this oneExpected:
{"status": "error", "message": "Invalid state: invalid_state..."}Expected: metadata only, no
datafield🤖 Generated with Claude Code