feat(memory): temporal facts, knowledge history, cmd timeout + cancel fix

[chinkan]

Summary

• Knowledge history — SQLite UPDATE/DELETE triggers auto-archive prior values; knowledge_timeline / knowledge_as_of
• Temporal facts — (entity, relation, value, valid_from, valid_to); one active per pair; auto-close on new value
• MCP tools — add_fact, query_facts, close_fact, fact_history
• Shell timeout — sandbox.execute_timeout_secs default 90, 0 = off
• Cancel button fix — CallbackQuery no longer chat-serialized behind in-flight execute_command (root cause of dead Cancel)

Deferred

• Agentic RAG → feat: agentic RAG for search_memory #52
• Harness evolution → feat: harness evolution (SafeEvolve-inspired) #53

Docs

• CONTEXT.md memory glossary
• ADR 0004 temporal facts / knowledge history
• ADR 0005 callback concurrency

Test plan

• cargo test (incl. 6 new memory/knowledge unit tests)
• cargo clippy -- -D warnings
• Manual: run long sleep 120 → timeout ~90s
• Manual: Cancel button during running command → toast + kill
• Manual: remember A→B → fact_history(category,key) shows archive
• Manual: add_fact Nike then Adidas → query as_of past/present

[opencode-agent]

Both axes' key claims verified against source (idx_facts_active is non-unique at src/memory/mod.rs:364; pull-requests: read at .github/workflows/opencode-review.yml:13; button shown at src/command_tool.rs:105/113 before register at :122). Aggregated review:

Standards

SQL injection: none found. All new queries in knowledge.rs/mod.rs are parameterized via rusqlite::params!/bound placeholders; search_facts builds format!("%{query}%") but passes it as a bind param (safe; unescaped %/_ in user input are wildcards, cosmetic only). Triggers are static SQL.

Security (timeout/cancel): Default 90s cap tightens sandbox behavior; 0 = disable is documented in config.example.toml. kill_child SIGKILLs the process group on timeout — sound. Cancel path semantics preserved (ADR-0005 documents the distribution change).

Documented-standard issues

• CI action pinning (hard-ish): CLAUDE.md CI conventions use pinned tags (@stable, @v2); existing workflows use checkout@v4. New workflows use actions/checkout@v6 (inconsistent) and anomalyco/opencode/github@latest — an unpinned mutable tag, a supply-chain deviation from every existing workflow.
• Missing permission: opencode-review.yml posts PR reviews but grants only pull-requests: read / issues: read. If the action comments, it needs write.
• opencode.yml: no trailing newline at EOF.
• Tests: comply — #[cfg(test)] mod tests in knowledge.rs. CLAUDE.md's "No automated tests exist yet" is now stale; docs should be updated.
• default_execute_timeout_secs() follows the existing default_model() pattern — compliant.

Baseline smells (judgement calls)

• Duplicated Code: identical UPDATE facts SET valid_to = ?1 WHERE entity = ?2 AND relation = ?3 AND valid_to IS NULL in add_fact and close_fact; fact format string "{} —{}→ {} [{}..{}] conf={}" duplicated across query_facts and fact_history.
• Duplicated Code (CI): the two new workflows repeat the same checkout+run steps; consider a reusable workflow.
• Speculative Generality: search_facts has zero callers (no tool binding, no test); fact_history's four optional params with no required is a flag-arg two-tools-in-one shape.
• Primitive Obsession: change_type: String in KnowledgeVersion vs. the SQL CHECK enum. (Dates-as-String exempt: documented in CONTEXT.md.)
• Mysterious Name: // ponytail: LIKE, not FTS comment in search_facts.

Spec

(a) Missing / partial

1. knowledge_as_of unreachable by the agent — exists (src/memory/knowledge.rs:266) and is tested, but no MCP tool exposes it; fact_history only calls knowledge_timeline (src/memory_tools.rs:253). Spec line: "knowledge_timeline / knowledge_as_of".
2. "One active per pair" enforced only in Rust — idx_facts_active (src/memory/mod.rs:364) is a non-unique partial index; no UNIQUE constraint, so the invariant breaks for any other write path.
3. Timeout applies only to execute_command — supervisor's shell backend still runs unbounded Command::output() (src/supervisor/backend/shell.rs:61). Spec line: "Shell timeout".
4. ADR-0004 says "bi-temporal" but only valid-time is modelable — created_at is stored, never surfaced on Fact or used in queries.
5. ✓ Verified present: UPDATE and DELETE triggers, exact fact columns, auto-close, all four tools registered, CallbackQuery None key (src/platform/telegram.rs:293), exactly 6 new passing tests.

(b) Scope creep

• .github/workflows/opencode.yml + opencode-review.yml — not in the PR summary.
• search_facts() — not requested, no tool, no test = dead code.
• Cancel arm now breaks the loop and three UI-branch comments were deleted — churn not described.

(c) Implemented but looks wrong

1. Inverted windows on backfill: add_fact sets valid_to = new.valid_from unconditionally; if the new valid_from is older than the active fact's start, valid_to < valid_from makes the prior fact invisible at every as-of point.
2. Mixed precision string compares: changed_at uses datetime('now') (YYYY-MM-DD HH:MM:SS) while tools/ADR advertise 2024-09-01 granularity; h.changed_at <= as_of and valid_to > ?2 mis-order same-day values.
3. Cancel race: show_cancel_button (src/command_tool.rs:105/113) publishes the button before cancel_registry.register (:122) — now that callbacks run concurrently, early clicks hit "Command already finished" and the kill is lost.
4. Timeout discards output: result is only "⚠️ Command timed out after Ns"; buffered partial output is dropped, so the LLM gets no evidence.
5. Dedup short-circuit returns the existing id and silently ignores the new valid_from.

---

Summary: Standards — 6 findings, worst: unpinned @latest action + pull-requests: read-only permission in opencode-review.yml (the review workflow can't actually post reviews). Spec — 12 findings, worst: the cancel-button registration race (src/command_tool.rs:105 vs :122), which undermines the PR's headline Cancel fix under exactly the concurrency it just enabled.

opencode session  |  github run

[chinkan]

Addressed opencode review findings:

Finding	Fix
Cancel register-after-show race	register before show_cancel_button
Timeout drops output	append partial buffer to tool result
Inverted valid windows on backfill	historical insert closes at active start; active untouched
One-active only in Rust	UNIQUE partial index idx_facts_one_active
Date-only vs datetime compares	normalize YYYY-MM-DD → start/end of day
knowledge_as_of not exposed	fact_history(category,key,as_of=…)
Supervisor shell unbounded	tokio::time::timeout(job.timeout_secs)
Dead search_facts	removed
CI perms / checkout	pull-requests: write, issues: write, checkout@v4
ADR “bi-temporal”	corrected to valid-time

Skipped (YAGNI): pin @latest (opencode docs), reusable workflow extract, change_type enum.

[opencode-agent]

Both sub-agent reports verified against source. Spot-checks confirmed: cancel register (command_tool.rs:98) now precedes show_cancel_button (:111/:119); idx_facts_one_active is a proper partial UNIQUE index (memory/mod.rs:365); normalize_ts (knowledge.rs:486) is byte-identical to normalize_from (:469); execute_timeout_secs is plumbed only to main.rs:244/CommandTool — the supervisor still uses job.timeout_secs (default 600).

PR #54 Re-Review (post-fix, @ 4406413)

Standards

Prior-finding verification

#	Finding	Status
3	workflow perms pull-requests/issues: write	FIXED (opencode-review.yml:19-20)
4	checkout@v4	FIXED (both workflows)
5	dead search_facts	FIXED (absent)
6	SQL injection	PASS — all new queries parameterized via params![…]/?N; triggers interpolate only OLD./NEW. column refs, never user input
7	trailing newline / fmt	FIXED — cargo fmt --all --check clean
1a	Duplicate UPDATE facts SET valid_to…	PARTIAL — add_fact/close_fact no longer identical (close_fact adds AND valid_from <= ?1) but the WHERE entity=? AND relation=? AND valid_to IS NULL shape still repeats 3× with no shared helper
1b	Fact format string dup	PARTIAL — still near-duplicated across query_facts/fact_history
2	change_type: String vs SQL CHECK enum	NOT FIXED (knowledge.rs:27) — author marked YAGNI

Hard documented-standard violation

• CLAUDE.md:144 "No automated tests exist yet" is now false — knowledge.rs adds ~8 #[test]/#[tokio::test] cases. The PR should update this line (Testing section).

Baseline smells (judgement)

• Duplicated Code: normalize_ts (knowledge.rs:486) is byte-identical to normalize_from (:469) — one is redundant; collapse.
• Duplicated Code: two near-identical empty JobOutput { status: Failed, … } literals in shell.rs — a JobOutput::failed(errors) ctor would help.
• Repeated Switches: fact_history (memory_tools.rs) is a 3-level if let ladder over four optional args.

Spec

Prior-finding verification

#	Finding	Status
1	knowledge_as_of unreachable	FIXED — fact_history(as_of=…) dispatches to it
2	one-active per pair in DB	FIXED — partial UNIQUE index, NULL-dup rejected/closed-dup allowed
4	ADR "bi-temporal" wording	FIXED — ADR-0004 now "valid-time … created_at is audit only"
5	inverted backfill window	FIXED — historical row closes at active start, active untouched (tested)
6	mixed date/datetime precision	FIXED — date-only normalized to start/end-of-day
7	cancel register-after-show race	FIXED — register precedes show
8	timeout discarded partial output	FIXED — buffered output appended to result
3	supervisor shell timeout	PARTIAL
9	dedup ignores new valid_from	PARTIAL (intentional, documented+tested)

Worst — finding 3 (spec line: "sandbox.execute_timeout_secs default 90, 0 = off"): the timeout was added to shell.rs but wired to job.timeout_secs (hardcoded default 600, job.rs:89), not the execute_timeout_secs config key — which is plumbed only to the chat CommandTool. So execute_timeout_secs has no effect on the supervisor, and 0 = off is unconfigurable there. Secondary: tokio::time::timeout around Command::output() has no process-group kill (unlike command_tool.rs), so the child can survive the timeout. This is a fix that addresses the letter of the prior finding but not the spec's intent.

Minor gaps

• is_date_only/normalize leaves T-separated ISO (2025-06-01T12:00) unnormalized → mis-compares vs space format (finding 6 residual).
• CREATE UNIQUE INDEX (no IF NOT EXISTS-safe dedup migration) will abort startup on a dev DB that has duplicate active rows from the branch's earlier non-unique index. Main is unaffected (unreleased); flag for anyone who ran intermediate commits.

Scope creep (unchanged)

• .github/workflows/opencode.yml + opencode-review.yml are unrelated to the memory/timeout/cancel spec.

---

Summary: Standards — 3 open (1 hard: stale CLAUDE.md test claim; worst: duplicated normalize_ts/normalize_from + 3×-repeated SQL). Spec — 7 of 9 prior findings fully fixed; worst: supervisor timeout wired to hardcoded job.timeout_secs, not execute_timeout_secs, so the spec's config knob does nothing on that path.

opencode session  |  github run