[codex] Add MCP write audit log#2566
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds persistent MCP write audit logging: new record types, SQLite storage and query API, an internal-only RPC ChangesMCP Audit Logging System
Sequence Diagram(s)sequenceDiagram
participant Client
participant Protocol as MCP Protocol
participant CallTool as call_tool()
participant DispatchWrite as dispatch_write_tool()
participant MemPut as openhuman.memory_doc_put
participant AuditStore as Audit Store
Client->>Protocol: tools/call (write tool)
Protocol->>CallTool: call_tool(name, args, client_info)
CallTool->>CallTool: load_write_config()
CallTool->>CallTool: enforce_write_policy_for_config()
CallTool->>CallTool: inject source_type = client_info
CallTool->>DispatchWrite: dispatch_write_tool(audit_args, client_info, config)
DispatchWrite->>MemPut: invoke mapped RPC (e.g., memory_doc_put)
MemPut-->>DispatchWrite: response (success/failure)
DispatchWrite->>DispatchWrite: extract_document_id()
DispatchWrite->>AuditStore: record_write(NewMcpWriteRecord)
AuditStore-->>DispatchWrite: row id
DispatchWrite-->>CallTool: result
CallTool-->>Protocol: JSON response
Protocol-->>Client: tools/call result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/mcp_server/tools.rs (2)
1161-1177: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winComplete the write-path diagnostics and keep them at debug/trace.
The config-load failure is logged, but a policy denial in Lines 1173-1176 returns without any breadcrumb, and the per-request write diagnostics in Lines 1191-1218 are emitted at
info. Please add a denial-branch log and downgrade these request/success logs todebug!/trace!.As per coding guidelines: "Debug logging must follow these rules: default to verbose diagnostics on new/changed flows; log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors" and "Use
log/tracingatdebugortracelevel in Rust for development-oriented diagnostics."Also applies to: 1191-1218
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/mcp_server/tools.rs` around lines 1161 - 1177, Add missing denial breadcrumb and lower noisy info-level diagnostics to debug/trace: in enforce_write_policy, after calling SecurityPolicy::from_config(...).enforce_tool_operation(ToolOperation::Act, tool_name), log a denial branch (use log::debug! or log::trace!) that includes the tool_name and policy decision before returning ToolCallError::InvalidParams; also change the per-request write diagnostics around the later write-path (the info-level logs in the block referenced at lines ~1191-1218) to use log::debug! for entry/exit and success messages and log::trace! for more verbose state/step details so request and success breadcrumbs are recorded at development levels rather than info.
548-588:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAudit the rejected write-tool paths too.
Line 559, Line 581, and Line 586 can all fail before
dispatch_write_tool(...)runs, so malformed or policy-deniedmemory.store/memory.note/tree.tagcalls never reachmcp_writes. That leaves the new audit trail incomplete for failed write invocations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/mcp_server/tools.rs` around lines 548 - 588, The memory write branch ("memory.store" | "memory.note" | "tree.tag") currently can return early from reject_unexpected_arguments, enforce_write_policy, or validate_controller_params and skip the mcp_writes audit; update call_tool so that any error from reject_unexpected_arguments, enforce_write_policy, or validate_controller_params triggers a call to the existing mcp_writes audit path before returning the Err. Concretely, for the branch handling spec.name == "memory.store"|"memory.note"|"tree.tag", propagate errors from reject_unexpected_arguments, enforce_write_policy, and validate_controller_params via map_err or match handlers that first call mcp_writes (including spec.name, audit_arguments, client_info, params snapshot and the failure reason) and then return the original ToolCallError, ensuring dispatch_write_tool remains the path for successful calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/mcp_audit/mod.rs`:
- Around line 10-12: Add the two missing domain-level re-exports: export
schemas::all_controller_schemas under the alias all_mcp_audit_controller_schemas
and export controllers::all_registered_controllers under the alias
all_mcp_audit_registered_controllers so the module satisfies the domain-module
contract; i.e., add pub use schemas::all_controller_schemas as
all_mcp_audit_controller_schemas and pub use
controllers::all_registered_controllers as all_mcp_audit_registered_controllers
alongside the existing pub use lines (refer to the existing pub use
schemas::all_internal_controllers, store::list_writes/record_write, and
types::{McpWriteListQuery, McpWriteRecord, NewMcpWriteRecord} to place them
consistently).
In `@src/openhuman/mcp_audit/schemas.rs`:
- Around line 69-76: Add diagnostic logging to the handle_list RPC handler: log
an entry message at the start of handle_list, trace the result of
config_rpc::load_config_with_timeout (success or error), log a branch message
for serde_json::from_value parsing (include the raw params on entry and error
details on failure), emit a trace before calling store::list_writes and another
on its success with record counts or returned info, and ensure all error paths
convert/return errors after logging contextual error messages (include function
name, query, and config identifiers where available). Use the existing logging
facility consistent with other handlers so logs record entry/exit, branches,
external call boundaries, and error context for handle_list,
config_rpc::load_config_with_timeout,
serde_json::from_value::<McpWriteListQuery>, and store::list_writes.
- Around line 10-67: The module only exports all_internal_controllers but must
also expose the domain schema symbols expected by the contract: add and export
three items named schemas, all_controller_schemas, and
all_registered_controllers; implement schemas as a Vec<ControllerSchema> by
returning vec![schema()], implement all_controller_schemas as an alias or
wrapper returning the same vec (vec![schema()]) and implement
all_registered_controllers to return the existing all_internal_controllers() (or
forward to it), ensuring the names exactly match schemas,
all_controller_schemas, and all_registered_controllers so callers can find them.
In `@src/openhuman/mcp_audit/store.rs`:
- Around line 13-93: The record_write and list_writes functions lack the
required diagnostic logging; add debug/trace logs with a stable prefix like
"[mcp_audit]" at function entry and exit, log key correlation fields
(client_info, tool_name, timestamp_ms, and pagination/filter params such as
since_ms, limit, offset, client_filter, tool_filter, success_only) and include
contextual error details on DB failures (e.g., when serialize, prepare, execute,
query_map, or collect fail) while scrubbing PII (do not log full sensitive
args_summary or secret values); place logs inside record_write (around
serde_json::to_string and before/after the DB insert and on error) and inside
list_writes (log built SQL/filters applied, bound param summaries, before
executing stmt, after successful row count, and on any .context() error) using
the existing logging facility so the messages include the stable prefix and
correlation fields.
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 1265-1268: The audit_write function currently performs
mcp_audit::record_write(...) inline and blocks the response path; change it to
fire-and-forget the audit call so it runs in the background. Update audit_write
to spawn a background task (use tokio::task::spawn_blocking if record_write is
synchronous, or tokio::spawn if you can call an async variant) and move or clone
the Config and NewMcpWriteRecord into that task; inside the task call
mcp_audit::record_write and log any Err via log::warn! so failures remain
best-effort and do not block the write response. Ensure captured variables meet
'static requirements for the spawned task.
---
Outside diff comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 1161-1177: Add missing denial breadcrumb and lower noisy
info-level diagnostics to debug/trace: in enforce_write_policy, after calling
SecurityPolicy::from_config(...).enforce_tool_operation(ToolOperation::Act,
tool_name), log a denial branch (use log::debug! or log::trace!) that includes
the tool_name and policy decision before returning ToolCallError::InvalidParams;
also change the per-request write diagnostics around the later write-path (the
info-level logs in the block referenced at lines ~1191-1218) to use log::debug!
for entry/exit and success messages and log::trace! for more verbose state/step
details so request and success breadcrumbs are recorded at development levels
rather than info.
- Around line 548-588: The memory write branch ("memory.store" | "memory.note" |
"tree.tag") currently can return early from reject_unexpected_arguments,
enforce_write_policy, or validate_controller_params and skip the mcp_writes
audit; update call_tool so that any error from reject_unexpected_arguments,
enforce_write_policy, or validate_controller_params triggers a call to the
existing mcp_writes audit path before returning the Err. Concretely, for the
branch handling spec.name == "memory.store"|"memory.note"|"tree.tag", propagate
errors from reject_unexpected_arguments, enforce_write_policy, and
validate_controller_params via map_err or match handlers that first call
mcp_writes (including spec.name, audit_arguments, client_info, params snapshot
and the failure reason) and then return the original ToolCallError, ensuring
dispatch_write_tool remains the path for successful calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e476a831-11e0-4951-9e8e-ca479605003e
📒 Files selected for processing (10)
src/core/all.rssrc/core/all_tests.rssrc/openhuman/mcp_audit/mod.rssrc/openhuman/mcp_audit/schemas.rssrc/openhuman/mcp_audit/store.rssrc/openhuman/mcp_audit/types.rssrc/openhuman/mcp_server/protocol.rssrc/openhuman/mcp_server/tools.rssrc/openhuman/memory_tree/store.rssrc/openhuman/mod.rs
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/mcp_server/tools.rs (1)
1192-1204:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix return type mismatch in
load_write_config
TheOk(config)match arm currently returns a bareConfigwhile the function signature isResult<Config, ToolCallError>.Suggested fix
async fn load_write_config(tool_name: &str) -> Result<Config, ToolCallError> { match config_rpc::load_config_with_timeout().await { - Ok(config) => config, + Ok(config) => Ok(config), Err(err) => { log::warn!( "[mcp_server] enforce_write_policy config load failed tool={tool_name} error={err}" ); return Err(ToolCallError::Internal(format!( "failed to load config: {err}" ))); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/mcp_server/tools.rs` around lines 1192 - 1204, The match in load_write_config returns a bare Config on Ok instead of the expected Result; change the Ok arm to return Ok(config) so the function returns Result<Config, ToolCallError>, keeping the Err arm as-is (refer to load_write_config, config_rpc::load_config_with_timeout, and ToolCallError).
🧹 Nitpick comments (1)
src/openhuman/mcp_server/tools.rs (1)
1221-1498: 🏗️ Heavy liftExtract the new write-audit path into a smaller module.
This PR adds another large chunk of dispatch/audit logic to
mcp_server/tools.rs, which is already far beyond the repo’s size target. Moving the write-tool dispatch and audit helpers into a sibling module would make this flow easier to maintain and keep future MCP changes localized.As per coding guidelines: “File size should not exceed approximately 500 lines. When a module grows beyond this threshold, split it into smaller, more focused modules with clear responsibilities.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/mcp_server/tools.rs` around lines 1221 - 1498, Move the write dispatch and audit helpers into a new focused module to shrink tools.rs: extract the functions dispatch_write_tool, audit_write, audit_write_rejection, audit_write_rejection_without_config, is_write_tool, summarize_rejected_write_args, extract_document_id, summarize_write_args, first_chars, and now_ms into a new sibling module (e.g., mcp_server::write_dispatch), implement the same signatures, re-export or pub use the needed symbols, and update callers/imports in the codebase to reference the new module; ensure the new module imports Config, NewMcpWriteRecord, ToolCallError, Map, Value and other types used by these functions and that tokio/runtime and mcp_audit dependencies remain available, then run cargo build/tests to fix any missing imports or visibility issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 1192-1204: The match in load_write_config returns a bare Config on
Ok instead of the expected Result; change the Ok arm to return Ok(config) so the
function returns Result<Config, ToolCallError>, keeping the Err arm as-is (refer
to load_write_config, config_rpc::load_config_with_timeout, and ToolCallError).
---
Nitpick comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 1221-1498: Move the write dispatch and audit helpers into a new
focused module to shrink tools.rs: extract the functions dispatch_write_tool,
audit_write, audit_write_rejection, audit_write_rejection_without_config,
is_write_tool, summarize_rejected_write_args, extract_document_id,
summarize_write_args, first_chars, and now_ms into a new sibling module (e.g.,
mcp_server::write_dispatch), implement the same signatures, re-export or pub use
the needed symbols, and update callers/imports in the codebase to reference the
new module; ensure the new module imports Config, NewMcpWriteRecord,
ToolCallError, Map, Value and other types used by these functions and that
tokio/runtime and mcp_audit dependencies remain available, then run cargo
build/tests to fix any missing imports or visibility issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30fa063e-7625-4955-86c3-010d1d6616e7
📒 Files selected for processing (4)
src/openhuman/mcp_audit/mod.rssrc/openhuman/mcp_audit/schemas.rssrc/openhuman/mcp_audit/store.rssrc/openhuman/mcp_server/tools.rs
|
Review/CI triage note: this red run is a real compile failure, not the earlier checkout/GHCR infra class. The current first blocker is in |
|
Author of #2553 (the parallel draft for #2536) here — flagging coordination context, not pushing for territory. RFC design context (from the lazy-consensus comment on #2536):
So we're aligned on Q1–Q3. The split is Q4: my draft deliberately deferred the query RPC pending ratification (kept a One safety detail worth checking that bit me in #2553: let mut end = ERROR_MESSAGE_MAX_BYTES;
while end > 0 && !msg.is_char_boundary(end) {
end -= 1;
}
record.error_message = Some(msg[..end].to_string());Plus a regression test ( On the "writes vs write attempts" question @YOMXXX raised: I'd argue both should be recorded. Failure rows are the abuse-detection signal — a misbehaving MCP client repeatedly bouncing off the policy gate or Happy to close #2553 once @senamakel picks the canonical path. Either outcome works — flagging this so the design context from the RFC thread doesn't get lost in the handoff. |
|
@justinhsu1477 Agreed on both points, thanks for carrying the #2553/RFC context over here. I tightened this up in the latest commits:
I also fixed the Validation on this branch:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/mcp_audit/schemas.rs (2)
193-197:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest assertion assumes incorrect response structure.
This test expects
value.as_array()directly, but if the response is corrected to match the schema ({"records": [...]}), this will fail. Update to access the"records"field.Proposed fix (after handler is corrected)
let value = handle_list(Map::new()).await.expect("handle list"); - let records = value.as_array().expect("records array"); + let records = value["records"].as_array().expect("records array"); assert_eq!(records.len(), 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/mcp_audit/schemas.rs` around lines 193 - 197, The test incorrectly treats the handler response as a bare array; update the assertions to access the "records" field from the returned JSON object (i.e., call value["records"] or value.get("records") and then as_array()) before checking length and fields; adjust references to the existing handle_list await call and the local variables value and records so the test asserts on records[0]["tool_name"] and records[0]["client_info"] after extracting the records array.
131-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResponse structure does not match schema output definition.
The schema defines
outputswith a field named"records", implying the response should be{"records": [...]}. However,handle_listreturnsserde_json::to_value(records)directly, which produces just[...].This mismatch will cause clients relying on the schema contract to fail when accessing
response["records"].Proposed fix
let count = records.len(); - let value = serde_json::to_value(records).map_err(|err| { + let value = serde_json::to_value(serde_json::json!({ "records": records })).map_err(|err| { log::warn!("[mcp_audit] handle_list serialize response failed error={err}"); err.to_string() })?;Or alternatively:
+ let mut response = Map::new(); + response.insert( + "records".to_string(), + serde_json::to_value(records).map_err(|err| { + log::warn!("[mcp_audit] handle_list serialize response failed error={err}"); + err.to_string() + })?, + ); let count = records.len(); - let value = serde_json::to_value(records).map_err(|err| { - log::warn!("[mcp_audit] handle_list serialize response failed error={err}"); - err.to_string() - })?; log::debug!("[mcp_audit] handle_list exit records={count}"); - Ok(value) + Ok(Value::Object(response))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/mcp_audit/schemas.rs` around lines 131 - 137, handle_list is serializing and returning records directly (serde_json::to_value(records)) but the schema defines outputs with a top-level "records" field; change handle_list to wrap the vector into an object with the key "records" before serializing (e.g., create a serde-friendly map or struct containing records and use serde_json::to_value on that) so the returned value matches the schema, preserve the existing count/logging logic (records and count) and keep the same error handling around serde_json::to_value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/openhuman/mcp_audit/schemas.rs`:
- Around line 193-197: The test incorrectly treats the handler response as a
bare array; update the assertions to access the "records" field from the
returned JSON object (i.e., call value["records"] or value.get("records") and
then as_array()) before checking length and fields; adjust references to the
existing handle_list await call and the local variables value and records so the
test asserts on records[0]["tool_name"] and records[0]["client_info"] after
extracting the records array.
- Around line 131-137: handle_list is serializing and returning records directly
(serde_json::to_value(records)) but the schema defines outputs with a top-level
"records" field; change handle_list to wrap the vector into an object with the
key "records" before serializing (e.g., create a serde-friendly map or struct
containing records and use serde_json::to_value on that) so the returned value
matches the schema, preserve the existing count/logging logic (records and
count) and keep the same error handling around serde_json::to_value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2420efc9-4e5a-45af-ac8b-8d7feb285030
📒 Files selected for processing (3)
src/openhuman/mcp_audit/schemas.rssrc/openhuman/mcp_audit/store.rssrc/openhuman/mcp_server/tools.rs
…tch schema The ControllerSchema outputs define a "records" field, but handle_list was returning a bare JSON array. Clients relying on the schema contract would fail when accessing response["records"]. Updated the test assertion to match. Addresses @coderabbitai on schemas.rs:131-137 and schemas.rs:193-197.
| } | ||
| } | ||
|
|
||
| fn audit_write(config: &Config, record: NewMcpWriteRecord) { |
There was a problem hiding this comment.
Acknowledged nitpick: tools.rs is ~2750 lines — well beyond the ~500 line guideline. Extracting the write-dispatch and audit helpers (dispatch_write_tool, audit_write, audit_write_rejection, summarize_write_args, etc.) into a sibling module like mcp_server/write_dispatch.rs would be a solid follow-up. Deferring for a separate PR since it's a heavy refactor orthogonal to the audit feature.
4 participants
Summary
mcp_writestable to the memory-tree SQLite schemamemory.store,memory.note, andtree.tagopenhuman.mcp_audit_listRPC with limit, offset, since, client, tool, and success filtersMcpSession::source_type()into MCP write dispatch so audit rows and write provenance use the connected client labelCloses #2536.
Validation
cargo test mcp_audit --libcargo test summarize_write_args --libcargo test extract_document_id --libcargo test schema_for_rpc_method_finds_internal_mcp_audit_list --libcargo test rpc_method_from_parts_does_not_expose_internal_mcp_audit_list --libpnpm format:checkpnpm rust:checkpnpm typecheckpnpm lint(passes with existing warnings)pnpm --dir app run lint:commands-tokensgit diff --checkSummary by CodeRabbit
New Features
Tests