feat: persist and display model thinking traces (opt-in)

[lmqferreira]

v2 — addresses all 5 review items + self-found improvements

This update force-pushes a rebased + squashed v2 that addresses every concern from the previous review, plus a number of issues found while battle-testing on a live PiClaw instance (Donna, running this PR's code in production for ~24h with PICLAW_WEB_PERSIST_THINKING=1).

Rebased onto v2.6.2

Clean rebase, zero merge conflicts.

---

Review items — point-by-point response

1. ✅ Avoid content-based rediscovery of the stored message row

Fixed. storeAgentTurn() signature changed from boolean to number | null (returns the persisted message rowid). All 3 call sites in agent.ts updated. The previous maybePersistThinkingForStoredTurn() is renamed persistThinkingForRow(rowId) and now takes the rowid directly — the fragile content_blocks LIKE '%"thinking_ref"%' rediscovery query and the duplicated thread/no-thread branches are deleted.

Bonus: the LIKE was doing a full scan of messages for the chat on every terminal turn (no index can cover a LIKE '%...%' on JSON text). Removing it is also a per-turn O(N) perf win, meaningful on long-lived chats.

2. ✅ Validate /agent/thinking against the message table

Fixed. Endpoint now requires both message_id AND chat_jid. New getThinkingContentForChat(chatJid, messageId) runs a single JOIN that asserts:

• message exists in that chat_jid
• is_bot_message = 1
• content_blocks contains an actual thinking_ref block — checked via EXISTS (SELECT 1 FROM json_each(...) WHERE json_extract(value, '$.type') = 'thinking_ref') (not substring LIKE, which would be a confused-deputy primitive if any path ever lands the literal string "thinking_ref" in unrelated content)
• thinking_content row exists

Returns 404 uniformly for any failure — no enumeration oracle, no information leak about which prong failed. Returns 400 only for missing required params. Both UIs (classic + visual) now send chat_jid in the fetch URL.

Covered by 16 unit tests in runtime/test/web/thinking/thinking-endpoint.test.ts including a 6-case truth table over (correct chat / is bot / has ref block / has thinking row) and SQL-injection safety.

3. ✅ Cleanup / retention semantics

Fixed in 8 places — every code path that deletes from messages now also deletes the matching thinking_content rows:

Path	File
deleteMessageByRowId (UI single delete)	db/messages.ts — wrapped in db.transaction(...)
deleteThreadByRowId (UI thread delete)	db/messages.ts — wrapped in db.transaction(...)
rollbackChatRunWithError (cursor rollback on error)	db/chat-cursors.ts
rollbackInflightRun (crash recovery rollback)	db/chat-cursors.ts
permanentDeleteArchivedBranch (branch wipe)	db/chat-branches.ts
reapDreamArtifacts × 2 (stale dream sweep, full + exclude-one)	dream.ts
cleanupDreamChat (specific dream cleanup)	dream.ts

No FK CASCADE is possible: thinking_content.message_id is TEXT and messages has a composite PK (id, chat_jid), so SQLite refuses to declare a CASCADE FK on rowid. Manual cleanup matches the existing message_media pattern.

The atomic-transaction wrapping on the two UI-delete paths specifically prevents a rowid-reuse ghost: SQLite reuses the highest deleted rowid on next INSERT, so a crash between deleting thinking_content and messages could cause a future message to inherit ghost thinking. Wrapping in db.transaction(...) closes that window.

Retention purge: runtime/scripts/purge-thinking-content.ts provides a CLI (--all, --older-than-days N, --chat-jid JID) with safe dry-run-by-default, batched delete in a single transaction, optional --vacuum to reclaim space. An index idx_thinking_content_created_at makes time-based purges index-only.

Covered by 10 unit tests in runtime/test/web/thinking/thinking-cleanup.test.ts including an invariant test that runs 4 delete scenarios and asserts orphan count = 0 after each.

4. ✅ Privacy and storage documentation

Fixed. New docs/thinking-persistence.md (≈300 lines) covers:

• Quick start + both env knobs (PICLAW_WEB_PERSIST_THINKING, PICLAW_WEB_PERSIST_THINKING_MAX_CHARS)
• Schema: every column in thinking_content with type + meaning
• Summarized vs full thinking: Claude 4+ models return summarized thinking by default; the pi-ai provider sets thinking.display: "summarized". What gets persisted is already a condensed summary, not raw internal reasoning.
• Privacy & security: cites the EMNLP 2025 paper "Leaky Thoughts: Large Reasoning Models Are Not Private Thinkers" (arXiv:2506.15674) for the threat-model framing, lists mitigations in place (opt-in, cap, dream exclusion, endpoint scoping, single-user assumption), recommended user mitigations (don't sync messages.db across machines, periodic purge)
• Cleanup & retention: all 8 auto-cleanup paths listed, bulk purge script usage, raw SQL alternatives for hand purges, behavior when feature is disabled (capture vs display split)
• Operational notes: production storage growth from real data (0.28% of bot messages get thinking, ~3KB avg), retry/recovery handling, race-window safety, redacted thinking caveat
• API reference: GET /agent/thinking — required params, response shape, all 404 conditions

Linked from README.md Other-references section and docs/configuration.md env-var table.

5. ✅ Self-isolating test

Fixed. runtime/test/web/thinking/persist-thinking.test.ts now does:

// Side-effect import: forces PICLAW_DB_IN_MEMORY=1 and shared temp workspace
import "../../helpers.js";

which is the same pattern other tests in the repo use. Runs cleanly with bare bun test — no env var required. The 18-pass / 0-fail count from the maintainer's verification command is preserved.

---

Additional self-found improvements

These weren't in the review but surfaced during my audit / 60+ self-Q&A questions in the style of three expert-panel rounds:

#	Issue	Severity if shipped	Fix
X1	Content-LIKE rediscovery was also a perf bug (full table scan per turn)	🟡	Eliminated by R1 fix
X2	SSE broadcast happened before thinking_content INSERT → fast-click 404 race	🟡	Added onMessageStored callback that runs BEFORE broadcast
X3	text.slice(0, maxChars) could split UTF-16 surrogate pairs (emoji at boundary)	🟡	New safeTruncateUtf16 helper + 9 unit tests
X4	Pill used generic renderMarkdown while live streaming uses renderThinkingMarkdown (inconsistent: $VAR mangled as LaTeX in pill, fine in live)	🟡	Pill now uses renderThinkingMarkdown in both UIs
X5	No index on created_at for future retention queries	🟢	Added idx_thinking_content_created_at
X6	Maintainer asked for "how to purge" — no implementation, only docs	🟢	Added the CLI script
I1	Original PR cleaned 2 of 8 message-delete paths	🔴 (R3 scope)	All 8 paths covered
I6	Pending thinking accumulator survived agent retries → failed-attempt reasoning concatenated with successful-attempt reasoning, inflating duration_ms and confusing the trace	🟡	Discard on recovery_start event
Q19	Visual UI pill was missing chat_jid in fetch URL (broken by my own R2 fix)	🔴 (would 400 every click)	Added chatJid prop + getChatJid() fallback
Q21	SQLite rowid reuse + non-atomic delete pair → ghost thinking_content attaches to future messages	🟡	Atomic transactions in UI delete paths
Q3	created_at defaulted to datetime('now') (no T, no Z, no millis) — inconsistent with messages.timestamp which is ISO Z; would silently break lex-comparison if any future query joined them	🟢	Default now strftime('%Y-%m-%dT%H:%M:%fZ', 'now')
Q8	The LIKE '%"thinking_ref"%' was matching JSON text by substring — confused-deputy if any path lands the literal string in unrelated content_blocks	🟡	Replaced with json_each + json_extract predicate (see R2 above)
Q15+Q16	Pill was a <div onClick>, not keyboard-activatable, with a nested <button> (invalid HTML), no aria-expanded/aria-controls/aria-live — WCAG 2.1 SC 2.1.1 + 4.1.2 failures	🔴 (a11y AA)	Real <button> for the disclosure (sibling to the copy button, both in a flex .controls wrapper). Full ARIA: aria-expanded, aria-controls → stable detail id, descriptive aria-label. Detail region has role=region + aria-live=polite. CSS adds :focus-visible outline + button reset. Applied to both classic and visual UIs.

---

Test results

$ bun test runtime/test/web/thinking/ runtime/test/utils/safe-truncate.test.ts
  47 pass / 0 fail / 177 expect() calls / 4 files

$ PICLAW_DB_IN_MEMORY=1 bun test runtime/test/web/thinking/persist-thinking.test.ts runtime/test/web/service-worker.test.ts
  18 pass / 0 fail / 29 expect() calls / 2 files   ← maintainer's exact command

Breakdown of the 47 thinking tests:

• persist-thinking.test.ts — 12 (original config + CRUD)
• thinking-cleanup.test.ts — 10 (helpers + integration + orphan invariant)
• thinking-endpoint.test.ts — 16 (R2 truth table + IDOR + SQL injection)
• safe-truncate.test.ts — 9 (emoji boundaries + length invariants)

Plus the existing agent-message-store.test.ts (6 tests including 3 that needed expect(ok).toBe(true) → expect(typeof ok === "number" && ok > 0).toBe(true) for the new return-type contract).

---

Battle-tested live

The implementation has been running on a personal PiClaw instance (Donna, v2.6.2 base + this PR) since the start of development, with PICLAW_WEB_PERSIST_THINKING=1 active. Verified live:

• Pill renders + persists + reloads correctly in classic UI
• Same in visual UI (after Q19 fix)
• $PATH test confirmed renderThinkingMarkdown fix (no LaTeX mangling)
• Real deletion-cleanup: a tested message was deleted, thinking_content row gone, orphan count remained 0 (R3 validated in production)
• Existing 17-row thinking_content set from the old PR version survived the upgrade intact
• Atomic-transaction cleanup verified by orphan-count invariant test

[piclaw-bot]

Thanks for the PR. I reviewed this against current main; it is CI-green and auto-merges cleanly, but I do not think we should merge it as-is yet.

The feature direction is useful, and keeping it opt-in/default-off is the right starting point. However, this introduces durable storage and UI exposure for sensitive thinking/reasoning traces, so we need a stronger implementation boundary before merging.

Requested changes:

1. Avoid content-based rediscovery of the stored message row

   maybePersistThinkingForStoredTurn(...) currently stores the agent message and then re-finds the latest matching row by chat_jid, formatted content, optional thread_id, and content_blocks LIKE '%"thinking_ref"%'.

   That is fragile when two replies have identical content, retries produce the same text, or formatting changes. Please plumb the stored message rowid/id back from the message persistence path and call storeThinkingContent(...) with that direct identifier.

2. Validate /agent/thinking against the message table

   GET /agent/thinking?message_id=N currently returns persisted thinking by id only. Please verify the requested id corresponds to a real visible bot message containing a thinking_ref block, and ideally include/check chat_jid so the endpoint is scoped to the timeline context rather than being a raw thinking-content lookup.

3. Add cleanup / retention semantics

   thinking_content is durable and keyed separately from messages, with no cleanup path. Please add a cleanup story for message deletion and/or a retention/purge mechanism. If SQLite FK cascade is viable for this schema, use it; otherwise add code-level cleanup where messages are deleted.

4. Document the privacy and storage implications

   Please update docs for:

   • PICLAW_WEB_PERSIST_THINKING
   • PICLAW_WEB_PERSIST_THINKING_MAX_CHARS
   • default-off behavior
   • what is stored and where
   • privacy/security implications of persisting reasoning traces
   • how to purge/reset persisted thinking data

5. Make the new test self-isolating

   runtime/test/web/thinking/persist-thinking.test.ts passes with PICLAW_DB_IN_MEMORY=1, but when run directly without that env it hits the live DB safety guard. Please set up in-memory/test DB isolation inside the test or use the existing DB test helper pattern so it cannot attempt to open /workspace/.piclaw/store/messages.db.

Validation I ran:

# temporary merge against current main
Auto-merging runtime/src/channels/web/http/dispatch-agent.ts
Auto-merging runtime/src/core/config.ts
Automatic merge went well

PICLAW_DB_IN_MEMORY=1 bun test runtime/test/web/thinking/persist-thinking.test.ts runtime/test/web/service-worker.test.ts
→ 18 pass / 0 fail

UX/performance assessment:

• Default/off path should have minimal visible UX impact.
• Enabled path adds a useful collapsible/copyable thinking pill in Classic and Visual UI.
• Lazy-loading the full trace is a good performance choice.
• Storage growth needs a retention/cleanup plan: up to 100KB per persisted thinking-bearing assistant response by default.
• Main risk is privacy/safety of durable thinking traces and ensuring the lazy endpoint cannot retrieve traces detached from their message/chat context.

Once the items above are addressed, this should be much safer to merge.

[lmqferreira]

Rebased onto v2.6.2 (clean, zero conflicts). All changes from v2 remain — this is just a rebase to stay current with main.

Quick status recap since the v2 push:

• All 5 review items addressed — point-by-point response in the PR description above
• 47 thinking-specific tests pass (12 CRUD + 10 cleanup + 16 endpoint validation + 9 safe-truncate)
• CI-fast passes — 2962 pass, 4 fail (same 4 pre-existing failures as upstream main: 2 supervisor-config + 2 WebAuthn RP)
• Web bundle builds clean (classic + visual)
• Battle-tested on live instance (Donna) since initial development — orphan-count invariant holds, pills render correctly in both UIs

Additional improvements beyond the 5 requested items are documented in the PR description under "Additional self-found improvements" (X1–X6, I1, I6, Q3–Q21).

Ready for re-review whenever you have time.