From 5c2c38e37ef87070f037fd6b14b703a552a5beca Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Thu, 30 Jul 2026 01:20:06 +0000
Subject: [PATCH] Address review feedback on the vault MCP tools
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two non-blocking items from the review pass:

Document why get_statement_coverage reads through accessible_by rather than
writable_by. It reports which documents exist and writes nothing, so read
access is the right bar — and tightening it would hide coverage gaps from
people who can already see the figures those gaps sit behind. The comment
exists so a future refactor doesn't "fix" it.

Close the acknowledged verification gap with tests rather than a one-off
manual check. The review noted that nothing proved a real vault payload
serializes cleanly out through tools/call — vault responses are richer than
the other tools' output, with nested account hashes, decimal balances, dates
and a compacted hash. Two integration tests now drive the real /mcp endpoint
end to end against a real AccountStatement: one listing it, one uploading
bytes and reading back the SHA-256. Permanent regression coverage instead of
a smoke test someone has to remember to repeat.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JFDp9HhXDeswadu4cxFojn
---
 .../function/get_statement_coverage.rb        |  5 ++
 test/controllers/mcp_controller_test.rb       | 55 +++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/app/models/assistant/function/get_statement_coverage.rb b/app/models/assistant/function/get_statement_coverage.rb
index 832c56381..905b9648c 100644
--- a/app/models/assistant/function/get_statement_coverage.rb
+++ b/app/models/assistant/function/get_statement_coverage.rb
@@ -62,6 +62,11 @@ class Assistant::Function::GetStatementCoverage < Assistant::Function
     account_id = params["account_id"].to_s
     return error("invalid_account_id", "account_id must be a UUID.") unless valid_uuid?(account_id)
 
+    # accessible_by, not writable_by, is deliberate: this reports which documents
+    # exist for an account and writes nothing. Someone with read access to an
+    # account is entitled to know which of its statements are on file. Do not
+    # "tighten" this to writable_by — that would hide gaps from the people who
+    # can see the figures those gaps sit behind.
     account = family.accounts.accessible_by(user).find_by(id: account_id)
     return error("account_not_found", "No accessible account found with that ID.") unless account
 
diff --git a/test/controllers/mcp_controller_test.rb b/test/controllers/mcp_controller_test.rb
index 0f2713b74..a01627435 100644
--- a/test/controllers/mcp_controller_test.rb
+++ b/test/controllers/mcp_controller_test.rb
@@ -332,6 +332,61 @@ class McpControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
+  # A vault payload is richer than the other tools' output — nested account hashes,
+  # BigDecimal balances, dates, a `.compact`ed hash — so these exercise a real
+  # record all the way out through tools/call's JSON envelope, rather than
+  # trusting that the unit-tested return value serializes cleanly.
+  test "tools/call round-trips a real vault payload through list_account_statements" do
+    @user.update!(preferences: (@user.preferences || {}).merge("preview_features_enabled" => true))
+    statement = AccountStatement.create_from_upload!(
+      family: @user.family,
+      account: accounts(:depository),
+      file: uploaded_file(filename: "statement.csv", content_type: "text/csv", content: "date,amount\n2024-01-01,1\n")
+    )
+
+    with_mcp_env do
+      post "/mcp", params: jsonrpc_request("tools/call", {
+        name: "list_account_statements",
+        arguments: {}
+      }).to_json, headers: mcp_headers(@token)
+
+      assert_response :ok
+      result = JSON.parse(response.body)["result"]
+      assert_not result["isError"], "vault payload should not surface as a tool error"
+
+      inner = JSON.parse(result["content"][0]["text"])
+      assert inner["success"]
+
+      payload = inner["statements"].find { |s| s["id"] == statement.id }
+      assert_not_nil payload, "expected the created statement in the response"
+      assert_equal statement.content_sha256, payload["content_sha256"]
+      assert_equal accounts(:depository).id, payload.dig("account", "id")
+    end
+  end
+
+  test "tools/call round-trips an upload through upload_account_statement" do
+    @user.update!(preferences: (@user.preferences || {}).merge("preview_features_enabled" => true))
+    content = "date,amount\n2024-02-01,7\n"
+
+    with_mcp_env do
+      assert_difference "AccountStatement.count", 1 do
+        post "/mcp", params: jsonrpc_request("tools/call", {
+          name: "upload_account_statement",
+          arguments: { filename: "uploaded.csv", content_base64: Base64.strict_encode64(content) }
+        }).to_json, headers: mcp_headers(@token)
+      end
+
+      assert_response :ok
+      result = JSON.parse(response.body)["result"]
+      assert_not result["isError"]
+
+      inner = JSON.parse(result["content"][0]["text"])
+      assert inner["success"]
+      assert_not inner["duplicate"]
+      assert_equal Digest::SHA256.hexdigest(content), inner.dig("statement", "content_sha256")
+    end
+  end
+
   test "tools/call wraps function errors as isError response" do
     with_mcp_env do
       # Force a function error by stubbing