diff --git a/app/models/assistant/function/get_account_statement.rb b/app/models/assistant/function/get_account_statement.rb index fce62afa2..68174b308 100644 --- a/app/models/assistant/function/get_account_statement.rb +++ b/app/models/assistant/function/get_account_statement.rb @@ -3,8 +3,6 @@ class Assistant::Function::GetAccountStatement < Assistant::Function include Assistant::Function::StatementVaultSupport - DOWNLOAD_URL_TTL = 15.minutes - class << self def name "get_account_statement" @@ -37,8 +35,11 @@ class Assistant::Function::GetAccountStatement < Assistant::Function here verifies that the document's own line items sum to its printed total. That parse-integrity check belongs to whatever extracted the figures. - Also returns a short-lived download URL (valid #{DOWNLOAD_URL_TTL.inspect}) - for the original file when one is attached. + This does NOT return the document's bytes and cannot give you a link that + works for you: Sure serves stored files only to a signed-in browser + session, which an MCP client does not have. To read a document, either + search its contents with `search_family_files`, or point the user at + Settings -> Statement Vault to open it themselves. Example: @@ -84,8 +85,7 @@ class Assistant::Function::GetAccountStatement < Assistant::Function reconciliation_checks: checks, # Spelled out in the payload, not just the tool description: an agent # reading only the JSON must not read an empty check list as agreement. - reconciliation_note: unavailable_note(status), - download_url: download_url(statement) + reconciliation_note: unavailable_note(status) ).compact } end @@ -111,20 +111,4 @@ class Assistant::Function::GetAccountStatement < Assistant::Function } end end - - # Chat and MCP clients render outside the request that produced the record, so - # the URL has to be absolute. Falls back to nil when no host is configured - # (e.g. a self-hosted worker with no APP_DOMAIN) rather than handing back a - # relative path an external agent cannot resolve. - def download_url(statement) - return nil unless statement.original_file.attached? - - host_opts = Rails.application.config.action_mailer.default_url_options || {} - return nil if host_opts[:host].blank? - - Rails.application.routes.url_helpers.rails_blob_url( - statement.original_file, - host_opts.merge(disposition: "attachment", expires_in: DOWNLOAD_URL_TTL) - ) - end end diff --git a/app/models/assistant/function/get_statement_coverage.rb b/app/models/assistant/function/get_statement_coverage.rb index 905b9648c..f9b5e252e 100644 --- a/app/models/assistant/function/get_statement_coverage.rb +++ b/app/models/assistant/function/get_statement_coverage.rb @@ -15,13 +15,22 @@ class Assistant::Function::GetStatementCoverage < Assistant::Function Each month comes back with one status: - - `covered` — a linked statement covers the month and reconciles + - `covered` — a linked statement covers the month. This means a DOCUMENT + EXISTS, not that it agrees with the ledger: most statements have no + balances entered, so there is nothing to reconcile and they still count + as covered. Check `reconciliation_status` on the month before saying a + month is verified. - `mismatched` — a statement covers it, but its balances disagree with the ledger - `missing` — no statement on record; the month's figures have no document behind them - `ambiguous` — a statement was suggested for this account but nobody has confirmed the link - `duplicate` — two or more linked statements overlap the same month - `not_expected` — outside the account's expected statement range + Each covered month also carries `reconciliation_status`: `matched` when + every statement in it reconciles against the ledger, `mismatched` when one + disagrees, and `unavailable` when nobody has entered the balances — which + is the common case. + Use it before asserting anything about a period: "no statement on record" is a legitimate and necessary answer, and is very different from "the balance was zero". Use it to tell the user exactly which documents to go @@ -87,8 +96,22 @@ class Assistant::Function::GetStatementCoverage < Assistant::Function { month: month.date.strftime("%Y-%m"), status: month.status, + # A month is "covered" on document presence alone — an unreconciled + # statement is not mismatched, so it lands in `covered`. Without this + # field an agent cannot tell "the ledger agrees" from "nobody checked". + reconciliation_status: reconciliation_status_for(month), statement_ids: month.statements.map(&:id), unconfirmed_statement_ids: month.ambiguous_statements.map(&:id) }.compact_blank end + + def reconciliation_status_for(month) + return nil if month.statements.empty? + + statuses = month.statements.map(&:reconciliation_status).uniq + return "mismatched" if statuses.include?("mismatched") + return "unavailable" if statuses.include?("unavailable") + + "matched" + end end diff --git a/app/models/assistant/function/list_account_statements.rb b/app/models/assistant/function/list_account_statements.rb index adf10226d..8cce7f76a 100644 --- a/app/models/assistant/function/list_account_statements.rb +++ b/app/models/assistant/function/list_account_statements.rb @@ -82,7 +82,16 @@ class Assistant::Function::ListAccountStatements < Assistant::Function def call(params = {}) return not_a_statement_manager unless statement_manager? - scope = family.account_statements.includes(:account, :suggested_account).ordered + # Visibility is filtered in SQL, not after the fact. Post-filtering a page + # would both underfill it and — because there is no cursor — make a statement + # permanently unreachable whenever enough newer rows the caller cannot see sit + # in front of it. Mirrors AccountStatement#viewable_by? for a statement + # manager: unlinked statements are visible, linked ones follow the account. + scope = family.account_statements + .where(account_id: nil) + .or(family.account_statements.where(account_id: user.accessible_accounts.select(:id))) + .includes(:account, :suggested_account) + .ordered if params["account_id"].present? return error("invalid_account_id", "account_id must be a UUID.") unless valid_uuid?(params["account_id"]) @@ -122,12 +131,11 @@ class Assistant::Function::ListAccountStatements < Assistant::Function end limit = (params["limit"] || DEFAULT_LIMIT).to_i.clamp(1, MAX_LIMIT) - # A statement with no account is visible to any statement manager; a linked - # one follows the account's sharing rules, so filter after the query. Counting - # before that filter would report statements this user may not know exist, so - # the page is over-fetched by one and reported as has_more instead. + # Over-fetch by one to report has_more without a second count query. The rows + # are already visibility-scoped, so the page is never underfilled and the + # count discloses nothing the caller cannot see. rows = scope.limit(limit + 1).to_a - statements = rows.first(limit).select { |statement| statement.viewable_by?(user) } + statements = rows.first(limit) { success: true, diff --git a/app/models/assistant/function/upload_account_statement.rb b/app/models/assistant/function/upload_account_statement.rb index 3e9f23bc0..2faf230af 100644 --- a/app/models/assistant/function/upload_account_statement.rb +++ b/app/models/assistant/function/upload_account_statement.rb @@ -22,7 +22,8 @@ class Assistant::Function::UploadAccountStatement < Assistant::Function link the statement to that account — linking is a human decision made in Settings -> Statement Vault. Report the suggestion; don't claim the link. - Provide the file as base64 in `content_base64`. Maximum size is 25 MB. + Provide the file as base64 in `content_base64`. Maximum size is + #{AccountStatement::MAX_FILE_SIZE / 1.megabyte} MB. Example: @@ -115,6 +116,12 @@ class Assistant::Function::UploadAccountStatement < Assistant::Function ) rescue ActiveRecord::RecordInvalid => e error("validation_failed", e.record.errors.full_messages.join("; ")) + rescue => e + # The shared upload path can raise from content sniffing, storage or a + # validation hook. Those would otherwise surface to the agent as a raw + # exception string; give it something it can act on instead. + Rails.logger.error("[UploadAccountStatement] #{e.class}: #{e.message}") + error("upload_failed", "The statement could not be stored: #{e.message.truncate(200)}") end private diff --git a/docs/hosting/mcp.md b/docs/hosting/mcp.md index ebfc9dd65..fee478dd6 100644 --- a/docs/hosting/mcp.md +++ b/docs/hosting/mcp.md @@ -126,8 +126,8 @@ permissions enforced in the web UI. |------|-------------| | `upload_account_statement` | Store a statement document (PDF/CSV/XLSX) in the Statement Vault; deduplicates by SHA-256 | | `list_account_statements` | List vault documents with their SHA-256, period, linked account and review status | -| `get_account_statement` | One statement's details, a short-lived download URL, and its reconciliation checks against the ledger — present only once someone has entered the statement's opening/closing balances in the web UI, since nothing extracts them from the document | -| `get_statement_coverage` | Month-by-month statement coverage for an account: covered, missing, mismatched, ambiguous | +| `get_account_statement` | One statement's details and its reconciliation checks against the ledger — present only once someone has entered the statement's opening/closing balances in the web UI, since nothing extracts them from the document. Does not return the file: stored documents are served only to a signed-in browser session | +| `get_statement_coverage` | Month-by-month statement coverage for an account: `covered`, `missing`, `mismatched`, `ambiguous`, `duplicate`, `not_expected`, each with a reconciliation status | | `record_valuation` | Record an account's value on a date, with a required source citation | They exist for agents that maintain a document-backed record of a family's diff --git a/test/models/assistant/function/get_account_statement_test.rb b/test/models/assistant/function/get_account_statement_test.rb index 9defb6b87..18666dea8 100644 --- a/test/models/assistant/function/get_account_statement_test.rb +++ b/test/models/assistant/function/get_account_statement_test.rb @@ -1,10 +1,6 @@ require "test_helper" class Assistant::Function::GetAccountStatementTest < ActiveSupport::TestCase - include ActiveSupport::Testing::TimeHelpers - - DOWNLOAD_TTL_OVERSHOOT = Assistant::Function::GetAccountStatement::DOWNLOAD_URL_TTL + 1.minute - setup do @user = users(:family_admin) @account = accounts(:depository) @@ -69,24 +65,6 @@ class Assistant::Function::GetAccountStatementTest < ActiveSupport::TestCase assert_nil result[:statement][:reconciliation_note] end - test "download url carries an expiring signed id" do - statement = create_statement(account: @account) - - Rails.application.config.action_mailer.stubs(:default_url_options).returns({ host: "example.com" }) - url = @function.call("statement_id" => statement.id).dig(:statement, :download_url) - - assert_not_nil url, "expected a download URL when a host is configured" - signed_id = url[%r{/blobs/redirect/([^/]+)/}, 1] - assert_not_nil signed_id, "expected a signed id in #{url}" - - assert_equal statement.original_file.blob, - ActiveStorage::Blob.find_signed(signed_id) - - travel DOWNLOAD_TTL_OVERSHOOT do - assert_nil ActiveStorage::Blob.find_signed(signed_id), - "signed id must expire — the tool description promises 15 minutes" - end - end test "returns not_found for an unknown id" do result = @function.call("statement_id" => SecureRandom.uuid) diff --git a/test/models/assistant/function/get_statement_coverage_test.rb b/test/models/assistant/function/get_statement_coverage_test.rb index b9a0308ff..604a6d9ba 100644 --- a/test/models/assistant/function/get_statement_coverage_test.rb +++ b/test/models/assistant/function/get_statement_coverage_test.rb @@ -32,6 +32,25 @@ class Assistant::Function::GetStatementCoverageTest < ActiveSupport::TestCase assert_equal "missing", february[:status] end + # "covered" means a document exists, not that it agrees with the ledger — an + # unreconciled statement still counts as covered, so the month has to carry its + # own reconciliation status or the agent cannot tell the two apart. + test "a covered month reports unavailable reconciliation when no balances are entered" do + january = Date.current.prev_year.beginning_of_year + statement = AccountStatement.create_from_upload!( + family: @user.family, + account: @account, + file: uploaded_file(filename: "unreconciled.csv", content_type: "text/csv", content: "date,amount\n2024-01-01,1\n") + ) + statement.update!(period_start_on: january, period_end_on: january.end_of_month) + + result = @function.call("account_id" => @account.id, "year" => january.year) + covered = result[:months].find { |m| m[:month] == january.strftime("%Y-%m") } + + assert_equal "covered", covered[:status] + assert_equal "unavailable", covered[:reconciliation_status] + end + test "rejects a non-uuid account id" do result = @function.call("account_id" => "nope") diff --git a/test/models/assistant/function/record_valuation_test.rb b/test/models/assistant/function/record_valuation_test.rb index 38d481598..146a1502e 100644 --- a/test/models/assistant/function/record_valuation_test.rb +++ b/test/models/assistant/function/record_valuation_test.rb @@ -80,6 +80,20 @@ class Assistant::Function::RecordValuationTest < ActiveSupport::TestCase assert_equal "invalid_source_citation", result[:error] end + test "reports a failed reconciliation and creates no entry" do + failure = OpenStruct.new(success?: false, error_message: "Balance is invalid") + Account.any_instance.stubs(:create_reconciliation).returns(failure) + + result = nil + assert_no_difference "@account.entries.valuations.count" do + result = @function.call(params) + end + + assert_not result[:success] + assert_equal "valuation_failed", result[:error] + assert_equal "Balance is invalid", result[:message] + end + test "rejects an unparseable date" do result = @function.call(params(date: "June 30th")) diff --git a/test/models/assistant/function/upload_account_statement_test.rb b/test/models/assistant/function/upload_account_statement_test.rb index 53b7a0202..6a115fd5b 100644 --- a/test/models/assistant/function/upload_account_statement_test.rb +++ b/test/models/assistant/function/upload_account_statement_test.rb @@ -88,6 +88,41 @@ class Assistant::Function::UploadAccountStatementTest < ActiveSupport::TestCase assert_equal "invalid_content", result[:error] end + test "accepts base64 wrapped across lines" do + wrapped = Base64.strict_encode64(@content).scan(/.{1,8}/).join("\n") + + result = @function.call("filename" => "statement.csv", "content_base64" => wrapped) + + assert result[:success] + assert_equal Digest::SHA256.hexdigest(@content), result[:statement][:content_sha256] + end + + test "accepts urlsafe base64 without padding" do + encoded = Base64.urlsafe_encode64(@content, padding: false) + + result = @function.call("filename" => "statement.csv", "content_base64" => encoded) + + assert result[:success] + assert_equal Digest::SHA256.hexdigest(@content), result[:statement][:content_sha256] + end + + test "rejects content that decodes to zero bytes" do + result = @function.call("filename" => "statement.csv", "content_base64" => Base64.strict_encode64("")) + + assert_not result[:success] + assert_equal "invalid_content", result[:error] + end + + test "reports an unexpected storage failure as a tool error" do + AccountStatement.stubs(:create_from_prepared_upload!).raises(StandardError, "storage exploded") + + result = @function.call(params(filename: "statement.csv")) + + assert_not result[:success] + assert_equal "upload_failed", result[:error] + assert_match(/storage exploded/, result[:message]) + end + test "rejects an unknown account_id rather than silently uploading unlinked" do result = @function.call(params(filename: "statement.csv", account_id: SecureRandom.uuid))