diff --git a/app/models/assistant/function/get_account_statement.rb b/app/models/assistant/function/get_account_statement.rb index a6a7f3846..fce62afa2 100644 --- a/app/models/assistant/function/get_account_statement.rb +++ b/app/models/assistant/function/get_account_statement.rb @@ -13,18 +13,29 @@ class Assistant::Function::GetAccountStatement < Assistant::Function def description <<~INSTRUCTIONS Fetch one Statement Vault document by ID: its identity (SHA-256, filename, - period), the balances read off the document, and the reconciliation checks - comparing those balances against the ledger. + period), any balances recorded for it, and reconciliation checks against + the ledger. - The reconciliation checks are the trustworthy part. Each compares a figure - printed on the statement against the account's recorded balance: + IMPORTANT — reconciliation is usually empty, and its absence means nothing. + The checks only exist once a human has typed the statement's opening and + closing balances into the Statement Vault UI. Nothing extracts them from + the document, so a statement you uploaded through `upload_account_statement` + comes back with `reconciliation_checks: []` and + `reconciliation_status: "unavailable"`. That is "nobody has entered the + figures", NOT "the document agrees with the ledger". Never report an + unreconciled statement as verified. + + When the balances have been entered, each check compares that figure + against the account's recorded balance: - `opening_balance` / `closing_balance` — the statement's figure vs the ledger's - `period_movement` — the change across the period, both sides - Each check reports `matched` or `mismatched` (tolerance: 0.01). A mismatch - means the ledger and the document disagree — report it, and do not paper - over it by adjusting the number to fit. + Each reports `matched` or `mismatched` (tolerance: 0.01). A mismatch means + the ledger and the document disagree — report it, and do not paper over it + by adjusting the number to fit. Note this is ledger agreement only: nothing + 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. @@ -63,17 +74,32 @@ class Assistant::Function::GetAccountStatement < Assistant::Function return error("not_found", "No statement found with that ID that this user can view.") end + checks = reconciliation_payload(statement) + status = statement.reconciliation_status + { success: true, statement: statement_payload(statement).merge( - reconciliation_status: statement.reconciliation_status, - reconciliation_checks: reconciliation_payload(statement), + reconciliation_status: status, + 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) ).compact } end private + def unavailable_note(status) + return nil unless status == "unavailable" + + "No reconciliation has been performed: this statement has no opening/closing " \ + "balances recorded, and nothing extracts them from the document. Someone must " \ + "enter them in the Statement Vault UI. This is not evidence that the statement " \ + "agrees with the ledger." + end + def reconciliation_payload(statement) statement.reconciliation_checks.map do |check| { diff --git a/app/models/assistant/function/list_account_statements.rb b/app/models/assistant/function/list_account_statements.rb index 125eef6b8..adf10226d 100644 --- a/app/models/assistant/function/list_account_statements.rb +++ b/app/models/assistant/function/list_account_statements.rb @@ -27,12 +27,16 @@ class Assistant::Function::ListAccountStatements < Assistant::Function uploaded documents use `search_family_files`; to fetch one statement's reconciliation figures and a download link use `get_account_statement`. + There is no cursor or offset. `has_more: true` means the result was + truncated — raise `limit` (up to #{MAX_LIMIT}) or narrow the filters to see + the rest; paging forward is not possible. + Example: ``` list_account_statements({ review_status: "unmatched", - period_start_on_or_after: "2026-01-01" + overlapping_from: "2026-01-01" }) ``` INSTRUCTIONS @@ -57,15 +61,15 @@ class Assistant::Function::ListAccountStatements < Assistant::Function }, content_sha256: { type: "string", - description: "Look up a specific document by the SHA-256 of its contents. Use this to check whether a file is already archived." + description: "Look up a specific document by the SHA-256 of its contents (hex; case-insensitive). Use this to check whether a file is already archived." }, - period_start_on_or_after: { + overlapping_from: { type: "string", - description: "ISO 8601 date (YYYY-MM-DD). Only statements whose period ends on or after this date." + description: "ISO 8601 date (YYYY-MM-DD). Only statements whose period overlaps this date or later, i.e. whose period ENDS on or after it." }, - period_end_on_or_before: { + overlapping_until: { type: "string", - description: "ISO 8601 date (YYYY-MM-DD). Only statements whose period starts on or before this date." + description: "ISO 8601 date (YYYY-MM-DD). Only statements whose period overlaps this date or earlier, i.e. whose period STARTS on or before it." }, limit: { type: "integer", @@ -95,18 +99,24 @@ class Assistant::Function::ListAccountStatements < Assistant::Function scope = scope.where(review_status: status) end - scope = scope.where(content_sha256: params["content_sha256"].to_s.strip) if params["content_sha256"].present? + # Downcased because the column is constrained to lowercase hex + # (chk_account_statements_content_sha256), so uppercase input would not merely + # be unlikely to match — it could never match, and the agent would read the + # empty result as "not archived" and upload a duplicate. + if params["content_sha256"].present? + scope = scope.where(content_sha256: params["content_sha256"].to_s.strip.downcase) + end - if params["period_start_on_or_after"].present? - date = parse_date(params["period_start_on_or_after"]) - return error("invalid_date", "period_start_on_or_after must be an ISO 8601 date (YYYY-MM-DD).") unless date + if params["overlapping_from"].present? + date = parse_date(params["overlapping_from"]) + return error("invalid_date", "overlapping_from must be an ISO 8601 date (YYYY-MM-DD).") unless date scope = scope.where("period_end_on >= ?", date) end - if params["period_end_on_or_before"].present? - date = parse_date(params["period_end_on_or_before"]) - return error("invalid_date", "period_end_on_or_before must be an ISO 8601 date (YYYY-MM-DD).") unless date + if params["overlapping_until"].present? + date = parse_date(params["overlapping_until"]) + return error("invalid_date", "overlapping_until must be an ISO 8601 date (YYYY-MM-DD).") unless date scope = scope.where("period_start_on <= ?", date) end diff --git a/app/models/assistant/function/record_valuation.rb b/app/models/assistant/function/record_valuation.rb index 02e4b5f08..2c976755e 100644 --- a/app/models/assistant/function/record_valuation.rb +++ b/app/models/assistant/function/record_valuation.rb @@ -34,7 +34,9 @@ class Assistant::Function::RecordValuation < Assistant::Function this function — ask the user for the source. A value with no provenance is worse than a missing value, because it looks authoritative. - Recording a valuation on a date that already has one replaces it. + Recording a valuation on a date that already has one replaces it. The + citation is stored in the entry's notes; any note a person wrote there is + preserved and the new citation appended below it. Example: @@ -90,6 +92,12 @@ class Assistant::Function::RecordValuation < 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) + # Unlike the Statement Vault tools, this one deliberately does NOT check + # AccountStatement.statement_manager?. That role governs the document archive; + # writing a value to an account is governed by the account ACL, and + # writable_by is the same scope the human-facing api/v1/valuations endpoint + # uses. Do not "tighten" this by adding the vault role — the two permissions + # answer different questions. account = family.accounts.writable_by(user).find_by(id: account_id) return error("account_not_found", "No account found with that ID that this user can write to.") unless account @@ -109,7 +117,7 @@ class Assistant::Function::RecordValuation < Assistant::Function end entry = account.entries.valuations.find_by!(date: date) - entry.update!(notes: citation.to_s) + entry.update!(notes: merged_notes(entry.notes, citation)) end unless entry @@ -137,6 +145,24 @@ class Assistant::Function::RecordValuation < Assistant::Function end private + # Re-recording a date replaces the valuation, and the citation lives in the + # entry's notes — where a human may also have written something. Nothing is + # ever removed: the note is kept and the citation appended, because we cannot + # tell a tool-written line from a human one (almost any prose is a valid + # ungraded citation) and guessing wrong would destroy the only copy. + # + # Re-recording with the same citation is a no-op, so the common case does not + # grow the note. A genuinely different citation is appended, which is the + # right outcome for a provenance trail: it records that the cited basis for + # this date changed. + def merged_notes(existing, citation) + existing = existing.to_s.strip + return citation.to_s if existing.blank? + return existing if existing.lines.any? { |line| line.strip == citation.to_s } + + [ existing, citation.to_s ].join("\n\n") + end + def parse_date(value) return nil if value.blank? diff --git a/app/models/assistant/function/upload_account_statement.rb b/app/models/assistant/function/upload_account_statement.rb index dd7b6b422..3e9f23bc0 100644 --- a/app/models/assistant/function/upload_account_statement.rb +++ b/app/models/assistant/function/upload_account_statement.rb @@ -138,14 +138,18 @@ class Assistant::Function::UploadAccountStatement < Assistant::Function end end - # Whitespace is stripped first because agents routinely wrap long base64 - # across lines, but decoding stays strict after that: Base64.decode64 quietly - # discards characters it doesn't understand, which would archive corrupted - # bytes under a hash that looks perfectly legitimate. + # Whitespace is stripped because agents routinely wrap long base64 across + # lines, and the urlsafe alphabet is translated to the standard one because + # they sometimes emit it. Decoding stays strict after that: Base64.decode64 + # quietly discards characters it doesn't understand, which would archive + # corrupted bytes under a hash that looks perfectly legitimate. def decode_content(value) return nil if value.blank? - Base64.strict_decode64(value.to_s.gsub(/\s+/, "")) + normalized = value.to_s.gsub(/\s+/, "").tr("-_", "+/") + normalized += "=" * ((4 - normalized.length % 4) % 4) + + Base64.strict_decode64(normalized) rescue ArgumentError nil end diff --git a/app/models/provenance/citation.rb b/app/models/provenance/citation.rb index ad35563a8..b8df6d39f 100644 --- a/app/models/provenance/citation.rb +++ b/app/models/provenance/citation.rb @@ -18,7 +18,13 @@ module Provenance MIN_TEXT_LENGTH = 3 MAX_LENGTH = 500 - FORMAT = /\A(?estimated:\s)?(?.+?)(?:\s\(grade:\s(?[ABC])\))?\z/ + # Spacing around the grade is tolerated (\s? before the paren, \s* after the + # colon) so the suffix is recognised wherever GRADE_SUFFIX recognises it. The + # two patterns must stay in sync: if FORMAT is the stricter of the pair, a + # citation like "Doc (grade:A)" passes the suffix pre-check and then fails to + # match here, folding the grade into the citation text and yielding an + # ungraded source — silently losing the reliability the caller supplied. + FORMAT = /\A(?estimated:\s)?(?.+?)(?:\s?\(grade:\s*(?[ABC])\))?\z/ # Matched separately so "(grade: D)" fails loudly instead of folding into the # citation text and passing as an ungraded — but plausible-looking — source. GRADE_SUFFIX = /\(grade:\s*(?[^)]*)\)\s*\z/ diff --git a/docs/hosting/mcp.md b/docs/hosting/mcp.md index 7acc55964..ccff5eff2 100644 --- a/docs/hosting/mcp.md +++ b/docs/hosting/mcp.md @@ -126,7 +126,7 @@ 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, its reconciliation checks against the ledger, and a short-lived download URL | +| `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 | | `record_valuation` | Record an account's value on a date, with a required source citation | diff --git a/test/models/assistant/function/get_account_statement_test.rb b/test/models/assistant/function/get_account_statement_test.rb index c4e338ebb..9defb6b87 100644 --- a/test/models/assistant/function/get_account_statement_test.rb +++ b/test/models/assistant/function/get_account_statement_test.rb @@ -1,6 +1,10 @@ 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) @@ -26,6 +30,64 @@ class Assistant::Function::GetAccountStatementTest < ActiveSupport::TestCase assert result[:statement].key?(:reconciliation_checks) end + # Nothing extracts balances from an uploaded document, so a statement archived + # over MCP has no reconciliation. The payload has to say so — an empty check + # list must never read as "the document agrees with the ledger". + test "says explicitly that an unreconciled statement was not verified" do + statement = create_statement(account: @account) + + result = @function.call("statement_id" => statement.id) + + assert result[:success] + assert_equal "unavailable", result[:statement][:reconciliation_status] + assert_empty result[:statement][:reconciliation_checks] + assert_match(/not evidence/i, result[:statement][:reconciliation_note]) + end + + test "omits the note once reconciliation is available" do + period_start = Date.new(2024, 1, 1) + period_end = Date.new(2024, 1, 31) + statement = create_statement(account: @account) + statement.update!( + period_start_on: period_start, + period_end_on: period_end, + opening_balance: 100, + closing_balance: 200, + currency: @account.currency + ) + # Checks compare against the ledger, so both sides have to exist: the + # statement's figures and a Balance row on each period boundary. + # start_balance / end_balance are generated columns, so they are driven by + # start_cash_balance rather than assigned. + @account.balances.create!(date: period_start, balance: 100, start_cash_balance: 100, currency: @account.currency) + @account.balances.create!(date: period_end, balance: 200, start_cash_balance: 200, currency: @account.currency) + + result = @function.call("statement_id" => statement.id) + + assert_equal "matched", result[:statement][:reconciliation_status] + assert_not_empty result[:statement][:reconciliation_checks] + 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/list_account_statements_test.rb b/test/models/assistant/function/list_account_statements_test.rb index 20f264f11..ac9ee3e6c 100644 --- a/test/models/assistant/function/list_account_statements_test.rb +++ b/test/models/assistant/function/list_account_statements_test.rb @@ -42,6 +42,25 @@ class Assistant::Function::ListAccountStatementsTest < ActiveSupport::TestCase assert_equal statement.id, result[:statements].first[:id] end + test "finds a statement by uppercase sha256" do + statement = create_statement(account: @account, content: "date,amount\n2024-01-01,1\n") + + result = @function.call("content_sha256" => statement.content_sha256.upcase) + + assert_equal 1, result[:returned] + assert_equal statement.id, result[:statements].first[:id] + end + + test "filters by overlapping period window" do + statement = create_statement(account: @account, content: "date,amount\n2024-01-01,1\n") + statement.update!(period_start_on: Date.new(2024, 3, 1), period_end_on: Date.new(2024, 3, 31)) + + assert_equal 1, @function.call("overlapping_from" => "2024-03-15")[:returned] + assert_equal 1, @function.call("overlapping_until" => "2024-03-15")[:returned] + assert_equal 0, @function.call("overlapping_from" => "2024-04-01")[:returned] + assert_equal 0, @function.call("overlapping_until" => "2024-02-01")[:returned] + end + test "rejects an invalid review status" do result = @function.call("review_status" => "whatever") @@ -50,7 +69,7 @@ class Assistant::Function::ListAccountStatementsTest < ActiveSupport::TestCase end test "rejects an invalid date filter" do - result = @function.call("period_start_on_or_after" => "last tuesday") + result = @function.call("overlapping_from" => "last tuesday") assert_not result[:success] assert_equal "invalid_date", result[:error] diff --git a/test/models/assistant/function/record_valuation_test.rb b/test/models/assistant/function/record_valuation_test.rb index 3ff3d8334..38d481598 100644 --- a/test/models/assistant/function/record_valuation_test.rb +++ b/test/models/assistant/function/record_valuation_test.rb @@ -33,6 +33,38 @@ class Assistant::Function::RecordValuationTest < ActiveSupport::TestCase assert result[:replaced_existing] end + test "preserves a human note when replacing a valuation" do + first = @function.call(params) + Entry.find(first[:entry_id]).update!(notes: "Appraiser said the roof needs work") + + result = @function.call(params(amount: 2000)) + + notes = Entry.find(result[:entry_id]).notes + assert_includes notes, "Appraiser said the roof needs work" + assert_includes notes, @source + end + + test "re-recording with the same citation does not stack it" do + @function.call(params) + @function.call(params(amount: 2000)) + result = @function.call(params(amount: 3000)) + + notes = Entry.find(result[:entry_id]).notes + assert_equal @source, notes + end + + test "appends a changed citation so the provenance trail survives" do + first = @function.call(params) + revised = "Revised appraisal 2024-07-15 (grade: A)" + + result = @function.call(params(source: revised)) + + notes = Entry.find(result[:entry_id]).notes + assert_includes notes, @source + assert_includes notes, revised + assert_equal first[:entry_id], result[:entry_id] + end + test "rejects a citation that does not follow the grammar" do result = @function.call(params(source: "estimated: pulled from a spreadsheet")) diff --git a/test/models/provenance/citation_test.rb b/test/models/provenance/citation_test.rb index b97f9e646..712367bbc 100644 --- a/test/models/provenance/citation_test.rb +++ b/test/models/provenance/citation_test.rb @@ -26,6 +26,26 @@ class Provenance::CitationTest < ActiveSupport::TestCase assert_not citation.estimated? end + # The suffix pre-check and the FORMAT pattern must agree on spacing, or a + # citation whose grade the pre-check accepts gets parsed as ungraded and the + # caller's reliability is silently dropped. + test "accepts a grade suffix whatever the spacing after the colon" do + [ "Doc (grade:A)", "Doc (grade: A)", "Doc (grade: A)" ].each do |raw| + citation = Provenance::Citation.parse!(raw) + + assert_equal "A", citation.grade, "expected #{raw.inspect} to parse as grade A" + assert_equal "Doc", citation.text + end + end + + test "rejects an unknown grade regardless of spacing" do + [ "Doc (grade:D)", "Doc (grade: D)" ].each do |raw| + assert_raises(Provenance::Citation::InvalidError, "expected #{raw.inspect} to be rejected") do + Provenance::Citation.parse!(raw) + end + end + end + test "rejects a blank citation" do assert_raises(Provenance::Citation::InvalidError) { Provenance::Citation.parse!(" ") } assert_raises(Provenance::Citation::InvalidError) { Provenance::Citation.parse!(nil) }