feat(mcp): add mcp_writes audit log table (draft, RFC #2536)

[justinhsu1477]

Draft scaffold for the MCP write audit log proposed in #2536 (Phase 3 RFC #2269 Q4).

Opening as draft now to claim territory and give @senamakel a concrete artifact to react to alongside the four design questions on #2536. The implementation here follows the lazy-consensus defaults I posted in that thread — happy to revise if the consensus lands elsewhere.

What this PR does

Replaces the tracing::info!-only audit trail for the three MCP write tools (memory.store, memory.note, tree.tag) with a persistent SQLite-backed mcp_writes table colocated in the existing memory-tree DB.

Schema (in tree::store::SCHEMA)

CREATE TABLE IF NOT EXISTS mcp_writes (
    id                  INTEGER PRIMARY KEY AUTOINCREMENT,
    timestamp_ms        INTEGER NOT NULL,
    client_source_type  TEXT NOT NULL,   -- e.g. "mcp:claude-desktop"
    tool_name           TEXT NOT NULL,   -- "memory.store" | "memory.note" | "tree.tag"
    args_summary        TEXT,            -- slim per-tool identifier JSON
    resulting_chunk_id  TEXT,            -- document_id on success, NULL on failure
    success             INTEGER NOT NULL,
    error_message       TEXT             -- ≤ 1 KiB, NULL on success
);
CREATE INDEX IF NOT EXISTS idx_mcp_writes_timestamp ON mcp_writes (timestamp_ms DESC);
CREATE INDEX IF NOT EXISTS idx_mcp_writes_client    ON mcp_writes (client_source_type);
CREATE INDEX IF NOT EXISTS idx_mcp_writes_tool      ON mcp_writes (tool_name);

Additive CREATE TABLE IF NOT EXISTS — applied on first connection through the existing with_connection cache. No separate migration step.

Wiring

• dispatch_write_tool now records both successful and failed dispatches. Failed audit inserts are logged at warn! and never propagate — write availability is not degraded by the audit subsystem.
• New tools::call_tool_with_session(name, args, client_source_type) threads McpSession::source_type() (from @YOMXXX-G's feat(mcp): capture client provenance in stdio sessions #2332) through so per-client provenance (mcp:claude-desktop / mcp:cursor / fallback mcp) lands in the row.
• tools::call_tool kept as a back-compat wrapper for any callers we haven't threaded yet.

args_summary — identifying metadata only

Per-tool slim extractor stores just enough to recognise a write — never the document content itself (that already lives in the memory tree via doc_put, so duplicating here would bloat the table for no information gain):

Tool	Fields
memory.store	{title (≤128 chars), namespace, tag_count}
memory.note	{chunk_id, note_text_length}
tree.tag	{chunk_id, tags}

Safety details

• error_message capped at 1 KiB, truncated on a UTF-8 char boundary (so multibyte chars never get sliced mid-codepoint).
• SQLite calls go through tokio::task::spawn_blocking — no blocking the async runtime.

Mapping to RFC #2536 design questions

Following the lazy-consensus defaults I posted in #2536:

Q	Default	This PR
Q1 — Storage location	A colocated in memory-tree DB	Implemented
Q2 — Failure-mode coupling	A best-effort, never fail the write	Implemented (warn-and-swallow)
Q3 — Schema shape	A start with the 7 columns RFC enumerates	Implemented
Q4 — Query surface	Deferred	recent() helper is #[cfg(test)] only; graduates to pub + an openhuman.mcp_audit_list RPC in a follow-up PR once the surface is ratified

If any of these need to flip, I'd rather revise here in this draft than ship a v1 we have to walk back.

Out of scope (follow-ups)

• openhuman.mcp_audit_list query RPC
• MCP-client-side exposure of the audit log
• UI surface for browsing audit history
• Retention / pruning policy

Test plan

• 6 new unit tests in memory::mcp_audit::tests:
  • Round-trip insert + readback
  • Failure rows preserve error_message
  • DESC ordering by timestamp
  • limit honoured
  • Oversize error_message truncated to 1 KiB
  • Multibyte UTF-8 truncation lands on a char boundary
• cargo test --lib mcp_server — all 78 existing tests still pass
• cargo fmt --check clean
• cargo check --lib clean (only pre-existing warnings, none in new code)

Why draft

I want @senamakel to see the shape of the wiring before I close out the lazy-consensus window on #2536. If a different consensus emerges on Q1/Q2/Q3, the changes here are localised enough to pivot without rewriting everything. Marking ready-for-review once the RFC questions settle (or after enough silent agreement on the issue thread).

Closes part of #2269 (Phase 3 Q4). Refs #2536.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9c931d1-b7f6-429e-a50a-08dc82200e37

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[YOMXXX]

Review/CI triage note for the draft: the current failures are not CI infrastructure. The branch is still using the pre-consolidation memory tree path (crate::openhuman::memory::tree), but current main moved that code to crate::openhuman::memory_tree; Rust Core / Tauri checks fail on that unresolved import before the tests can validate the audit behavior. There are also E0282 type-inference errors later in the same run.\n\nGiven #2566 is now a non-draft implementation for the same #2536 scope, reviewer load would be lower if we pick one canonical PR: either rebase/fix this draft onto current memory_tree and continue here, or mark this as superseded by #2566 and move any design deltas there.

[justinhsu1477]

Thanks for the triage @YOMXXX — both points were dead-on.

The path issue: you're right — my force-push yesterday landed an interim version of the commit where the memory_tree rename was on my disk but never made it into the actual commit (I edited the file after git rebase resolved cleanly and lost track of whether git add had run before git push --force-with-lease). My local cargo check / cargo test were green because they read the on-disk file; CI was the source of truth and was correctly failing.

Just amended + force-pushed the fix (1203aac6):

• Rebased onto current upstream/main (picks up the further memory_tree/store → memory_store::chunks::store::with_connection move from test(memory): extend memory coverage across retrieval and tooling #2574 / test(memory): share workspace env lock #2582).
• Fixed the import to the new path.
• Added a ::<()> turbofish on the with_connection call site — the new signature is stricter about inferring T when the closure returns bare Ok(()), which is where the E0282 was coming from.

Local verification on the rebased tree:

• cargo check --lib — clean (only pre-existing warnings).
• cargo test --lib memory::mcp_audit — 6/6 pass.
• cargo fmt --check — clean.

CI should reflect the same shortly.

On #2566 / consolidation: agreed reviewer load is the bigger concern here than authorship. My #2553 was intentionally a V1 scaffold (Q4 query RPC explicitly deferred per the lazy-consensus comment on #2536), so the two PRs aren't quite the same merge candidate — #2566 takes Q4=A as a unilateral call and ships the mcp_audit_list controller in v1, where my draft waited on ratification. If senamakel's preference is to land Q1–Q4 all-at-once, #2566 is the canonical merge candidate and I'll close this; if he'd rather walk the four design Qs through ratification first and land them in slices, this draft is the slice for Q1–Q3.

Either way I'll defer to his call rather than ask reviewers to compare the two cold. Will post a coordination note on #2566 with the RFC design context so it doesn't get lost if that one becomes canonical.