From 74b3fbac8b214f8cb5747f50e04aa8b8f81dbff5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 17:15:24 +0000 Subject: [PATCH] Correct the vault tools' reconciliation claims and citation parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings from @diegomarino, all verified against the code before changing anything. The reconciliation claim was the serious one. get_account_statement told agents the checks were "the trustworthy part" and returned "the balances read off it" — but nothing reads balances off a document. MetadataDetector never touches them and create_from_prepared_upload! never sets them; they are user-editable fields in the Statement Vault UI. So a statement archived over MCP always came back with an empty check list, which an agent could easily read as "the document agrees with the ledger" when it means "nobody has entered the figures". The description now says so, and the payload carries a reconciliation_note spelling it out for anything reading only the JSON. Also noted that these checks are ledger agreement, not parse integrity: nothing here verifies a document's parts sum to its printed total. Provenance::Citation had two patterns disagreeing about spacing. GRADE_SUFFIX allowed "(grade:A)" but FORMAT required exactly one space, so that citation passed the pre-check and then parsed as ungraded with the grade swallowed into the text — silently discarding the reliability the caller supplied, which is the one thing this parser exists to prevent. list_account_statements downcases content_sha256 before querying. The column is constrained to lowercase hex, so uppercase input could never match, and an agent would read the empty result as "not archived" and upload a duplicate. Its period filters are renamed overlapping_from / overlapping_until, since they match on overlap and the old names claimed otherwise to anyone reading the schema without the descriptions. has_more now explains that there is no cursor and the way forward is a bigger limit or narrower filters. record_valuation no longer overwrites the entry's notes. Re-recording a date would destroy a note a person had written there. Nothing is removed now: an identical citation is a no-op, a changed one is appended, and the trail of what was cited when survives. Detecting "did this tool write that line?" is not possible — almost any prose parses as a valid ungraded citation — so the code does not guess. Minor: accept urlsafe base64 on upload, and explain in the code why record_valuation checks the account ACL rather than the vault manager role, so nobody "tightens" it into the wrong permission later. Tests cover each: the grade-spacing cases both ways, uppercase SHA lookup, overlap window boundaries, note preservation and no-stacking, the unavailable reconciliation note appearing and disappearing, and — per the review — that the download URL's signed id actually expires, rather than trusting the description's claim. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JFDp9HhXDeswadu4cxFojn --- .../function/get_account_statement.rb | 44 ++++++++++--- .../function/list_account_statements.rb | 36 +++++++---- .../assistant/function/record_valuation.rb | 30 ++++++++- .../function/upload_account_statement.rb | 14 +++-- app/models/provenance/citation.rb | 8 ++- docs/hosting/mcp.md | 2 +- .../function/get_account_statement_test.rb | 62 +++++++++++++++++++ .../function/list_account_statements_test.rb | 21 ++++++- .../function/record_valuation_test.rb | 32 ++++++++++ test/models/provenance/citation_test.rb | 20 ++++++ 10 files changed, 237 insertions(+), 32 deletions(-) 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) }