Conversation
Full MCP server for YNAB with 100% API coverage: - 55 tools: accounts, budgets, categories, transactions, payees, months - 22 analytics tools: spending trends, budget health, goal tracking, etc. - 3 system tools: rate limit status, audit log, health check - Rate limiting (180/hr budget), caching, and input validation - Read-only mode with PII-redacted audit logging - 123 unit tests passing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughAdds a TypeScript YNAB MCP server: env loader, stdio bootstrap, RateLimiter, TTL Cache, AuditLog, YnabClient wrapper, ~50 tool handlers (analytics, budgets, accounts, transactions, system), utilities, fixtures, tests, and repo/tooling configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as MCP Server (stdio)
participant Dispatcher as Dispatcher
participant Handler as Tool Handler
participant YnabClient as YnabClient
participant RateLimiter as RateLimiter
participant Cache as Cache
participant API as YNAB API
participant AuditLog as AuditLog
Client->>Server: send request (toolName, args)
Server->>Dispatcher: forward request
Dispatcher->>Handler: invoke handler
Handler->>Handler: validate args (Zod)
Handler->>YnabClient: request data / perform action
YnabClient->>RateLimiter: acquire()
RateLimiter->>RateLimiter: refill / wait
YnabClient->>Cache: get(key)
alt cache hit
Cache-->>YnabClient: cached data (rgba(46, 204, 113, 0.5))
else cache miss
YnabClient->>API: HTTP request (rgba(52, 152, 219, 0.5))
API-->>YnabClient: response
YnabClient->>Cache: set(key)
end
alt mutating operation
YnabClient->>AuditLog: log(entry) (rgba(231, 76, 60, 0.5))
end
YnabClient-->>Handler: result
Handler-->>Client: formatted JSON response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Security notes (concise, flagging potential vulnerabilities)
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 72
🤖 Fix all issues with AI agents
In `@README` copy.md:
- Around line 84-156: The Markdown headings in the "Available Tools" section
(e.g., "### User Tools (1)", "### Budget Tools (3)", "### Account Tools (3)",
etc.) are missing required blank lines; update the README copy to ensure there
is at least one blank line before each subsection heading and at least one blank
line after each heading line (so each "### ..." heading is separated above and
below by an empty line) to satisfy markdownlint MD022 across the listed
headings.
In `@src/config/environment.ts`:
- Around line 20-38: parseBoolean currently treats any non-empty unrecognized
string as false which can silently disable read-only; change parseBoolean to
validate explicit true/false only: accept 'true'|'1'|'yes' => true and
'false'|'0'|'no' => false; if value is undefined or empty return defaultValue;
if the value is non-empty and not one of these canonical values throw an Error.
To make errors actionable, extend parseBoolean signature to accept an optional
varName (e.g., parseBoolean(value, defaultValue, varName)) and update the call
in loadConfig for YNAB_READ_ONLY to pass 'YNAB_READ_ONLY' so the thrown error
mentions the offending environment variable.
In `@src/index.ts`:
- Around line 56-58: The startup error handler currently logs the entire error
object from main().catch, which may expose sensitive config details; change the
catch to extract and log only the error message (e.g., derive msg = error
instanceof Error ? error.message : String(error)) and pass that message to
console.error (preserving the existing prefix "Failed to start YNAB MCP
Server:") instead of the full error object so secrets like tokens are not
leaked.
In `@src/server.ts`:
- Around line 54-79: The error responses currently leak sensitive data; update
formatErrorResponse (and the YnabApiError handling path inside it) to sanitize
both error.details and error.message: for YnabApiError only include a
whitelisted subset of safe fields from error.details (e.g., code, status,
userMessage) and drop any nested objects or raw payloads, and for generic errors
strip stack traces and file paths from error.message by removing newlines and
path-like patterns before returning a short safe message; ensure
formatErrorResponse returns a generic user-facing message plus any small,
explicit safe fields (not the raw error object) so callers like
server.setRequestHandler(CallToolRequestSchema) never send unsanitized details
or full messages to clients.
In `@src/services/audit-log.ts`:
- Around line 49-68: The log method on the AuditLog service currently writes raw
error text into AuditLogEntry and stderr; update log(entry: Omit<AuditLogEntry,
'timestamp'>) to defensively sanitize any error strings before storing or
printing: locate fields like entry.error, entry.message or entry.details in the
AuditLogEntry shape, replace/strip sensitive patterns (API keys, bearer tokens,
file system paths) and truncate to a safe max length (e.g., 200 chars), then
construct fullEntry and push it to this.entries and use the sanitized values in
the console.error call; ensure the sanitizer is a small helper used in log so
all persisted and printed audit data is redacted/truncated.
In `@src/services/cache.ts`:
- Around line 43-45: The set<T> method currently computes expiresAt using ttlMs
which can be NaN/Infinity; validate the ttl before use in set<T> (and fall back
to this.defaultTtlMs) by checking Number.isFinite(ttlMs) and using a safeTtl =
Math.max(0, Number.isFinite(ttlMs) ? ttlMs : this.defaultTtlMs) so expiresAt is
always a finite timestamp, then call this.store.set(key, { value, expiresAt });
this touches the set<T> method and ensures get() can correctly expire entries
and avoid never‑expiring data.
In `@src/services/rate-limiter.ts`:
- Around line 26-37: The acquire() method has a race between
refill/check/decrement allowing concurrent callers to bypass limits; wrap the
refill(), token check, wait and token decrement in a single async critical
section (mutex) so only one caller runs that sequence at a time. Concretely, add
a lock used by RateLimiter.acquire() that guards calls to refill(), the tokens <
1 check, the await sleep(...) branch, and the final tokens -= 1; you can
implement this with a simple promise-queue or an async mutex library and ensure
refill() is only called inside the locked section so token updates are atomic.
In `@src/services/ynab-client.ts`:
- Around line 152-186: The audit log currently records raw error.message in the
catch block of createAccount (and the other mutation methods); replace that with
a sanitized error representation that only includes safe fields such as
error.name (or 'UnknownError') and, if available, a numeric HTTP status (e.g.,
error.status || error.statusCode), and avoid logging error.message or full API
responses; update the auditLog.log call in createAccount (and mirror the same
change in the other mutation methods) to set error: { name: error.name ||
'UnknownError', status: error.status || error.statusCode || null } instead of
error.message so only sanitized fields are recorded.
In `@src/tools/accounts/create-account.ts`:
- Around line 102-113: The response currently embeds the raw user-provided
account.name in both the message string and the account payload; replace uses of
account.name with the sanitized value from sanitizeName (import and call
sanitizeName(account.name)) before building the returned object so both the
message (`message: \`Account "${...}" created successfully\``) and the payload
field (`name:`) use the sanitizedName; update the import if missing and ensure
you only sanitize strings (leave other fields like id, type, balance unchanged).
In `@src/tools/accounts/get-account.ts`:
- Around line 61-88: The debt-related map fields debt_interest_rates,
debt_minimum_payments, and debt_escrow_amounts are output raw; update the
serialization in get-account.ts to sanitize these maps consistently (similar to
sanitizeName/sanitizeMemo): for each of debt_interest_rates,
debt_minimum_payments, and debt_escrow_amounts, map over their entries and apply
the appropriate sanitizer/formatter (e.g., sanitize strings with
sanitizeMemo/sanitizeName and numeric currency values with formatCurrency)
before returning the JSON so the output uses sanitized values rather than raw
user-provided objects.
In `@src/tools/accounts/list-accounts.ts`:
- Around line 83-89: The account.name value is user-provided and must be
sanitized before emitting JSON; update the byType[type].push call that sets
name: account.name to instead pass the sanitized/escaped value (e.g., use an
existing sanitize/escape helper or add a small escapeHtml function) so that the
object pushed contains name: sanitize(account.name). Locate the block where
byType[type].push is called and replace the raw account.name with the sanitized
result, and ensure the sanitizer handles HTML/JS special characters and
null/undefined safely.
- Around line 93-123: The liability total is computed using Math.abs per account
which treats overpayments as debt; instead sum the signed balances for liability
accounts (use liabilityTypes and accounts) into a variable (e.g.,
signedLiabilities = accounts.filter(a =>
liabilityTypes.includes(String(a.type))).reduce((s,a) => s + a.balance, 0)),
then compute totalLiabilities as the positive debt amount: totalLiabilities =
Math.max(0, -signedLiabilities); keep totalAssets as-is and use
formatCurrency(totalLiabilities) and formatCurrency(totalAssets -
totalLiabilities) for the summary so overpayments correctly reduce liabilities
and net_worth.
In `@src/tools/analytics/age-of-money.ts`:
- Around line 98-105: The estimatedBuffer variable currently just echoes
ageOfMoney and adds little value; remove the estimatedBuffer variable and any
references to it from the age-of-money calculation and the returned object to
avoid confusing consumers (look for the symbol estimatedBuffer and where the
function computes/returns ageOfMoney in src/tools/analytics/age-of-money.ts and
delete the declaration, assignment, and any places that include it in the result
payload).
In `@src/tools/analytics/budget-health.ts`:
- Around line 225-232: Extract an explicit remaining budget variable and use it
in the month_progress block to make the intent clear: compute remainingBudget =
totalBudgeted + totalActivity (noting totalActivity is typically negative), then
keep the daysRemaining > 0 guard and pass Math.round(remainingBudget /
daysRemaining) into formatCurrency for daily_budget_remaining; update references
in the month_progress structure (daysRemaining, percentMonthComplete,
formatCurrency) to use remainingBudget so the calculation is clearer and less
error-prone.
- Around line 173-181: The sorting currently parses formatted currency strings
from categoryHealth (using spent) which is fragile; instead, preserve a raw
numeric value (e.g., spent_raw) when building categoryHealth (same approach as
list-categories.ts) and use that numeric field in the sort comparator (reference
statusPriority and categoryHealth.sort). After sorting, decide whether to remove
or keep spent_raw in the final output; ensure all code that reads spent for
calculations or sorting uses spent_raw rather than reparsing the formatted spent
string.
In `@src/tools/analytics/budget-suggestions.ts`:
- Around line 79-84: The month strings are generated from Date and then using
toISOString(), which can shift the day in non-UTC locales; update the loop that
builds months (variables: months, monthCount, and date) to format the local date
for the first day of each month (YYYY-MM-DD) instead of using
toISOString().slice(0,10); use a local formatter (e.g., Intl.DateTimeFormat or a
shared date util) or build the YYYY-MM-DD from date.getFullYear(),
date.getMonth()+1, and date.getDate() so the resulting month strings reflect
local calendar dates for YNAB.
In `@src/tools/analytics/budget-vs-actuals.ts`:
- Around line 131-137: The current logic treats category.activity with Math.abs
which incorrectly counts positive inflows/refunds as spending; change to an
outflow-only definition and use it consistently: compute activity as
category.activity < 0 ? Math.abs(category.activity) : 0 (or equivalent) and then
add that same activity variable to totalActivity (replace the conditional at
totalActivity += ...), while leaving budgeted and available as-is so totals and
per-category values use the same net/outflow definition (refer to variables
category.activity, activity, totalActivity, totalBudgeted, budgeted).
- Around line 224-226: The previous-month comparison currently calls
getPreviousMonth() without context, causing it to use the current date instead
of the requested month; change the logic so that when validated.include_previous
is true you compute prevMonth relative to the requested month (e.g., use
validated.month or the incoming month parameter) or update getPreviousMonth to
accept a base date and call getPreviousMonth(validated.month) before calling
client.getBudgetMonth(budgetId, prevMonth) so the previous-month query aligns
with the user-specified month.
In `@src/tools/analytics/cash-flow-forecast.ts`:
- Around line 171-194: The code currently limits recurring occurrences to 10 in
the for-loop, which undercounts frequent schedules; replace the fixed for (let i
= 0; i < 10; i++) loop with a loop that advances currentDate by intervalDays
until currentDate > endDate (e.g., while (true) { advance currentDate; if
(currentDate > endDate) break; ... }) so all occurrences within the forecast
range are generated; keep the existing guards (skip if currentDate < today) and
reuse getIntervalDays, formatLocalDate, sanitizeName, formatCurrency, and
frequencyLabels when pushing to scheduledItems.
- Around line 96-160: The comparison is dropping same-day scheduled items
because today includes current time while parseLocalDate/nextDate are at
midnight; normalize the date boundaries by setting today and endDate to
start-of-day (e.g., setHours(0,0,0,0)) before computing endDate and before
comparisons in the scheduled transaction loop so nextDate (from parseLocalDate)
can be correctly compared to today and endDate in the code handling the
scheduled loop and variables today, endDate, parseLocalDate, and nextDate.
In `@src/tools/analytics/credit-card-status.ts`:
- Around line 73-76: The filter currently compares account types via
String(a.type) === 'creditCard', which is brittle; update the check to compare
a.type against the YNAB SDK enum (e.g., use ynab.AccountTypeEnum.CreditCard or
the SDK's AccountType constant) so the code reads like
accountsResponse.data.accounts.filter(a => a.type ===
ynab.AccountTypeEnum.CreditCard && !a.deleted && !a.closed); import or reference
the correct enum symbol from the SDK and replace the String(...) comparison in
the creditCards variable to use the enum for type-safe matching.
In `@src/tools/analytics/detect-recurring.ts`:
- Around line 71-83: The RecurringTransaction interface currently exposes the
internal payee_id by default; update handling so payee_id is omitted unless
explicitly requested (e.g., add an optional include_ids boolean flag on the
function that returns RecurringTransaction objects) and ensure the code paths
that construct results (references: RecurringTransaction type and the
function(s) that produce the recurring results around the other occurrence
noted) only include payee_id when include_ids is true; modify the public return
shape to exclude payee_id by default and add tests or callers to pass
include_ids when they require IDs.
- Around line 104-106: The current filter for `transactions` only excludes
deleted items and positive inflows, causing transfers (which are outflows) to be
misclassified as recurring; update the predicate used on
`transactionsResponse.data.transactions` (the lambda that sets `transactions`)
to also exclude transfer records by checking the transaction transfer/type flags
your API uses (for example add conditions like `!t.is_transfer && t.type !==
'transfer' && !t.transferId` or the equivalent fields your backend returns) so
transfers are filtered out before recurring-detection logic runs.
In `@src/tools/analytics/goal-progress.ts`:
- Around line 135-140: The daysUntilTarget calculation uses new
Date().toISOString().split('T')[0] which may mismatch YNAB/UTC handling; replace
that ad-hoc today generation with a consistent date utility (e.g., add/use
getToday() in dates.ts) and call that here when computing daysUntilTarget for
targetDate; ensure the getToday() returns the same date-string normalization
used by daysBetween so daysBetween(today, targetDate) is consistent.
- Around line 179-192: The filter enum used by this tool is missing the
'complete' status so users cannot filter for completed goals; update the input
schema enum (the filter type defined near inputSchema) to include 'complete'
alongside 'on_track', 'behind', and 'underfunded', and ensure any validation
referencing the filter variable (used where filteredGoals is computed and where
statusPriority is defined) accepts 'complete' as a valid value so filtering and
sorting (statusPriority) will correctly handle completed goals.
In `@src/tools/analytics/monthly-comparison.ts`:
- Around line 192-213: The percent calculation forces spendingChangePercent to 0
when previousSpending === 0, which misclassifies real increases as "similar";
update the logic around spendingChange, spendingChangePercent, and
status/message in monthly-comparison.ts so that if previousSpending === 0 you
treat the cases specially: if currentSpending === 0 keep status 'similar', if
currentSpending > 0 set status 'worse' and craft message like "Spending
increased from $0 to $X" (or use an infinite/100%+ indicator), and if
currentSpending < 0 (if possible) handle accordingly; ensure
spendingChangePercent is only used when previousSpending !== 0 and adjust the
if/else that sets status/message (currently using spendingChangePercent < -10 /
> 10) to first check the previousSpending === 0 branch and then the
percent-based branch.
In `@src/tools/analytics/net-worth.ts`:
- Around line 145-157: The net-worth mismatch comes from counting all debt
account balances as liabilities; update the liability calculation so
debtAccounts is still selected via debtTypes (debtAccounts) but totalLiabilities
only sums absolute values of negative balances (i.e., debtAccounts.filter(a =>
a.balance < 0).map(a => Math.abs(a.balance))). Keep assetAccounts as the current
filter (assetTypes or tracking with positive balance) and totalAssets as the sum
of positive balances (assetAccounts.filter(a => a.balance > 0).map(a =>
a.balance)) so assets and liabilities align with netWorth; reference variables:
accounts, assetTypes, assetAccounts, totalAssets, debtTypes, debtAccounts,
totalLiabilities.
In `@src/tools/analytics/overspending-alerts.ts`:
- Line 10: The code currently converts the incoming dollar `threshold` by
multiplying by 1000 which risks floating-point precision errors; replace those
manual conversions with the `toMilliunits` helper from `milliunits.ts` so the
`threshold` (dollars) is converted to milliunits using Decimal.js rounding.
Update the usage in the overspending calculation (where `threshold` is
converted) and the other occurrence noted around line 74 to call
`toMilliunits(threshold)` (or `toMilliunits(threshold, /* if signature needs
currency param */)`) and remove any `* 1000` arithmetic, keeping
`formatCurrency` usage unchanged for display. Ensure you import `toMilliunits`
at the top and use the converted milliunit value in the comparisons and alert
logic (e.g., in functions handling overspend checks and any variables named like
`thresholdMilli` or `thresholdValue`).
In `@src/tools/analytics/quick-summary.ts`:
- Around line 70-80: budgetAccounts currently includes credit cards because its
filter only excludes debtTypes; update the budgetAccounts filter to also exclude
creditTypes so credit-card accounts are only counted in debtAccounts (i.e.,
change the predicate for budgetAccounts to a.on_budget &&
!debtTypes.has(String(a.type)) && !creditTypes.has(String(a.type))). Keep
debtAccounts as-is and verify any metric named total_in_budget_accounts is
computed from budgetAccounts only.
In `@src/tools/analytics/savings-opportunities.ts`:
- Around line 103-107: The month-subtraction can overflow when the current day
is 29–31; before calling sinceDate.setMonth(...) set the day to 1 to pin the
date (e.g., call setDate(1) on the sinceDate) so subtracting months can’t roll
into the wrong month—update the logic around the sinceDate creation/adjustment
(the sinceDate variable and its setMonth call) to pin the day to 1 prior to
subtracting months and then continue computing sinceDateStr as before.
- Around line 187-280: The code is inserting unsanitized user strings
(lookup.name and payeeName) into opportunity objects (fields category,
description, suggestion) which can leak control/unsafe characters; fix by
normalizing/sanitizing these before use (create or use a helper like
sanitizeName(s: string): string that trims, removes/control-characters (e.g.,
strip non-printables and newlines), limits length, and escapes/normalizes
Unicode), then replace direct uses of lookup.name and payeeName in the
opportunities pushed in the discretionary, high_variance, recurring_expense, and
large_single blocks (and any other push where lookup.name/payeeName appear) to
use sanitizeName(...) instead.
In `@src/tools/analytics/spending-analysis.ts`:
- Around line 61-68: The CategorySpending interface is exposing an internal
category_id; remove it or make it optional/opt-in to avoid leaking internal IDs.
Update the CategorySpending definition to omit category_id (or mark it
optional), and then update all code paths that construct or serialize
CategorySpending objects (search for usages of CategorySpending and the code
that maps category fields) so they no longer include category_id in outputs;
also adjust any exported types, API responses, and tests/assertions accordingly
(the same change applies to the similar structure referenced at lines 166-168).
- Around line 151-223: The code returns user-provided category and group names
directly (used in categorySpending.push and breakdown.push), so sanitize
lookup.name and lookup.group before including them in responses; update the
places that set category_name, group_name and breakdown.name to use a sanitizer
(e.g., sanitizeString or sanitizeUserInput — create one if none exists) that
strips/escapes HTML and control characters and apply it to default values
('Uncategorized', 'Other') as well to ensure all returned names are safe.
In `@src/tools/analytics/spending-pace.ts`:
- Around line 66-89: currentMonth is computed in local time via
getCurrentMonth() but subsequent day/month calculations use UTC
(now.getUTCFullYear(), getUTCMonth(), getUTCDate()), causing mismatched month
boundaries; make the timezone basis consistent by switching the UTC calls in the
days calculation to local-time equivalents (use now.getFullYear(),
now.getMonth(), now.getDate() and construct daysInMonth with new Date(year,
month + 1, 0).getDate()) so the month used by currentMonth and the day/percent
calculations align.
In `@src/tools/analytics/spending-trends.ts`:
- Around line 91-97: The month strings generated for the months array can be
wrong in non-UTC timezones because toISOString() converts to UTC and may roll
the date to the previous day; replace the toISOString() call in the loop that
builds months (using variables months, now, monthCount) with a local formatter
or simple construction using date.getFullYear(), (date.getMonth()+1)
zero-padded, and the literal day "01" so each entry is exactly "YYYY-MM-01" in
local time (or factor this into a shared date helper used across the module).
In `@src/tools/analytics/unused-categories.ts`:
- Around line 102-109: The loop over transactions
(transactionsResponse.data.transactions) only records parent txn.category_id,
missing categories inside splits; update the loop to also iterate
txn.subtransactions (if present) and for each subtxn with a category_id (and not
deleted) add that category_id to activeCategoryIds and update lastActivityMap
using the subtransaction date if available or falling back to txn.date; use the
same existence/recency check you use for parent transactions so you reference
activeCategoryIds and lastActivityMap consistently when processing
txn.subtransactions.
In `@src/tools/budgets/get-budget-settings.ts`:
- Around line 53-66: The response currently echoes the incoming budgetId
directly into the JSON.stringify output (the budget_id field) which can expose
malformed or unsafe values; before returning, normalize/sanitize budgetId (e.g.,
validate format, trim whitespace, enforce allowed characters or UUID format, or
reject/throw on invalid input) and use the sanitized value when constructing the
object passed to JSON.stringify; update the code path that builds the response
(the variable/parameter budgetId and the return block in get-budget-settings
where budget_id is set) to perform this validation/transformation and handle
invalid values consistently.
In `@src/tools/budgets/get-budget.ts`:
- Around line 56-121: The response returns user-controlled names unsanitized;
update the JSON construction to run all user string fields through the
sanitizeName utility (import sanitizeName from src/utils/sanitize.ts) before
returning: sanitize budget.name and any other top-level budget string fields,
sanitize each account.name when building accountsByType, sanitize group.name in
categoryGroups, and sanitize each category.name when mapping budget.categories;
ensure you call sanitizeName where formatCurrency is used only for numeric
fields so strings remain safe.
In `@src/tools/budgets/list-budgets.ts`:
- Around line 53-67: Sanitize user-provided names and format balances with the
budget's currency: replace direct uses of budget.name and acc.name in the
budgetInfo construction with a sanitized helper (e.g., sanitizeString or
escapeHtml) before returning, and change formatCurrency(acc.balance) to
formatCurrency(acc.balance, budget.currency_format) so non-USD budgets show
correct currency; update the mapping logic inside the budgets.map (budgetInfo
and the inner accounts map) to use the sanitizer and pass budget.currency_format
into formatCurrency.
In `@src/tools/categories/get-category.ts`:
- Around line 49-99: Sanitize user-controlled fields before returning them from
handleGetCategory: apply the existing sanitize utility to category.name and
category.note (and any other string fields from the YNAB API that may be
rendered later) when building the returned object so that name and note are safe
for downstream rendering contexts; keep validation via inputSchema and all other
logic unchanged.
In `@src/tools/categories/list-categories.ts`:
- Around line 77-94: The group and category display names are emitted raw;
update the mapping in list-categories.ts so group.name and cat.name are passed
through the project's sanitization helper (e.g., sanitizeName or sanitizeString)
before returning them. Specifically, in the object returned for each group
(where you currently reference group.name) and each category (where you
reference cat.name inside the categories.map), replace those raw values with the
sanitized result and import the sanitizer function at the top of the module if
it isn’t already available.
- Around line 111-120: The overspent/underfunded filters are parsing formatted
currency strings (using parseFloat with replace(/[^-\d.]/g, '')) which is
fragile; update the pipeline so comparisons use raw numeric fields instead of
formatted strings: ensure each category in allCategories has numeric properties
(e.g., balanceValue and goalUnderFundedValue or similar) populated before
formatting, then change the filters to use those numeric fields (referencing
overspent, underfunded, allCategories and the current parseFloat/replace usage)
so you remove the regex parsing and rely on the precomputed numbers for
comparisons.
- Around line 96-109: The summary totals use `groups` while `allCategories` is
built from `categoryGroups`, causing inconsistent totals when `includeHidden` is
true; update the totals (`totalBudgeted`, `totalActivity`, `totalBalance`) to
derive from the same source as `allCategories` and apply a single, consistent
hidden filter based on the `includeHidden` flag (e.g., use `allCategories` as
the base and if `includeHidden` is false filter by `!c.hidden`, otherwise
include all) so totals and `allCategories` reflect the same set of categories.
In `@src/tools/categories/update-category.ts`:
- Around line 85-102: The response currently returns category.name unsanitized;
update the return payload in the function that builds the response (the block
using JSON.stringify) to use sanitizeName(category.name) wherever the name is
used (the message template and the category.name field) and ensure sanitizeName
is imported/available in this module (src/tools/categories/update-category.ts)
so the returned name is consistent with other tools like get-account.ts.
- Around line 29-64: updateCategoryTool's description is missing the READ_ONLY
warning present in other write tools; update the description string in the
updateCategoryTool object (symbol: updateCategoryTool, name:
'ynab_update_category') to include a clear note that this is a write operation
and requires YNAB_READ_ONLY=false (consistent with create-transaction and
delete-transaction tools), placing the warning near the top or end of the
existing description so users know the env var requirement before invoking the
tool.
In `@src/tools/months/get-month.ts`:
- Around line 18-21: The Zod input schema for the "month" field (the month:
z.string().regex(...).describe(...) entry) rejects the literal "current" even
though the description mentions it; update the validator to accept either
"current" or a YYYY-MM-DD date by replacing the regex with something like
/^(current|\d{4}-\d{2}-\d{2})$/ (or, if "current" is not supported by the
downstream API, remove the "current" mention from the describe text instead);
modify the month schema in src/tools/months/get-month.ts accordingly so
validation and description are consistent.
In `@src/tools/months/list-months.ts`:
- Around line 57-60: The month total formatting currently hard-codes USD via
formatCurrency; update the code that builds the month object (where income,
budgeted, activity, to_be_budgeted are set) to use the budget's actual currency
rather than the default "$": fetch the budget currency metadata (e.g.,
budget.currency_format or a helper like getBudgetCurrencySymbol(budgetId)), pass
that symbol or a currency-format option into formatCurrency (or change
formatCurrency to accept a currency parameter), and cache the currency
per-budget to avoid repeated lookups; ensure the new call site uses
formatCurrency(month.income, currency) (or the equivalent option) so non‑USD
budgets display the correct symbol or return raw milliunits with currency
metadata if you prefer.
- Around line 54-56: The mapped output in formattedMonths is returning raw user
content in month.note; update the months.map callback to null-safely sanitize
the note before returning (e.g., use the project's string-sanitizer utility like
sanitize or escapeHtml), e.g. set note to month.note ? sanitize(month.note) :
null so user input is escaped and types remain string|null; import the sanitizer
where needed and apply it in the formattedMonths mapping.
In `@src/tools/payees/get-payee.ts`:
- Around line 57-64: The payee.name is user-provided and must be sanitized
before being returned to prevent XSS/injection; in the get-payee response (the
object built where payee: { id: payee.id, name: payee.name, ... }) call the
project sanitize helper (e.g., sanitize or sanitizeString) on payee.name and
return that sanitized value instead of raw payee.name; add the appropriate
import for the sanitize helper at top of the file and replace the reference in
the JSON payload so all tool responses emit sanitized payee names.
- Around line 11-18: Update the payee_id field to validate UUIDs: in the
inputSchema object replace the current payee_id z.string() with
z.string().uuid().describe('The payee UUID to retrieve'); also ensure any tool
metadata / exported tool inputSchema that mirrors this schema uses the same
z.string().uuid() for payee_id so the JSON Schema includes format:"uuid" (Zod
v3.23.8 supports z.string().uuid()).
In `@src/tools/payees/list-payee-locations-by-payee.ts`:
- Around line 58-66: Sanitize validated.payee_id before returning it: create a
safePayeeId (e.g., const safePayeeId = String(validated.payee_id).trim(); then
validate/normalize it against your expected format—use an existing validator or
a regex/UUID parse and throw if invalid) and replace validated.payee_id with
safePayeeId in the object passed to JSON.stringify; this ensures the returned
payee_id is a normalized, validated string rather than raw user input.
In `@src/tools/payees/list-payee-locations.ts`:
- Around line 56-62: The payee_id coming from locations must be sanitized before
being returned; update the mapping that builds payee_locations (where you
currently reference loc.payee_id) to normalize and validate the ID (e.g., coerce
to string, trim whitespace, enforce allowed pattern or length, and reject/omit
or replace unsafe values) or call a shared sanitizer (e.g., sanitizePayeeId) and
use its result as the payee_id in the returned object; ensure this logic is
applied in the map over locations so payee_locations contains only normalized,
safe payee_id values.
In `@src/tools/payees/list-payees.ts`:
- Around line 7-10: This file is missing an import of the sanitization utility
used to cleanse user-provided payee names; add the appropriate sanitizer import
(for example the project's sanitizeString/sanitizeUserInput function) near the
other imports and then apply that sanitizer wherever payee names are read or
returned (e.g., in the list-payees handler that uses YnabClient results) so all
payee name values are sanitized before being included in responses or schema
validation with z.
- Around line 58-62: The mapped payee names are not sanitized before returning;
update the activePayees mapping (the formattedPayees creation in list-payees.ts)
to call sanitizeName(payee.name) when building the name field, and ensure
sanitizeName is imported (same helper used by get-account.ts) so all
user-provided payee.name values are normalized/sanitized before being included
in the response.
In `@src/tools/scheduled-transactions/get-scheduled.ts`:
- Around line 61-88: The JSON currently returns raw user strings from the
scheduled transaction (fields like memo, account_name, payee_name, category_name
and subtransaction memo) so sanitize them before serialization: import and use
the shared sanitizer (e.g., sanitizeUserInput or similar) and replace usages in
the object building inside the function that returns JSON (the
scheduled_transaction object construction where txn and txn.subtransactions are
mapped) to call the sanitizer for memo, account_name, payee_name, category_name
and each sub.memo; ensure you sanitize the values prior to passing them into
JSON.stringify.
In `@src/tools/scheduled-transactions/list-scheduled.ts`:
- Around line 59-79: The mapped response exposes unsanitized user strings and
assumes txn.subtransactions is always an array; update the formattedTransactions
mapping to sanitize all user-controlled fields (txn.memo, txn.payee_name,
txn.category_name, txn.account_name and each sub.memo) before returning them
(use the project's HTML/JSON sanitizer or escape utility), and guard access to
subtransactions using a nullish fallback (e.g., (txn.subtransactions ?? []) )
rather than calling .length directly so you don't access .length on
null/undefined; keep formatCurrency usage for amounts and apply sanitization
only to string fields.
In `@src/tools/system/health-check.ts`:
- Around line 63-67: The catch block that calculates apiCheckDuration (using
apiCheckStart) currently logs the raw error object via console.error which risks
leaking sensitive headers/credentials; replace that raw logging with a sanitized
log—either call an existing redaction helper (e.g., sanitizeError or
redactSensitive) or log only non-sensitive fields such as err?.message and
apiCheckDuration using the project's logger (not console), and ensure no
headers, stack traces, or request metadata are included before pushing to
checks.
- Around line 7-33: The health-check tool accepts arbitrary payloads and logs
raw errors; add strict input validation at the start of handleHealthCheck by
validating _args with a zod schema identical to rate-limit-status (use
z.object({}).strict()) and reject/throw when validation fails, and replace any
logging of the raw error object in the catch path with a sanitized message
(e.g., include only error.message or a safe summary) so healthCheckTool and
handleHealthCheck no longer accept unexpected properties or leak sensitive
details.
In `@src/tools/transactions/create-transaction.ts`:
- Around line 152-170: The response serializes unsanitized user fields
(txn.payee_name and txn.memo); update the JSON assembly in create-transaction.ts
to sanitize those fields before returning by importing and using the existing
sanitizeName and sanitizeMemo helpers (same ones used in get-account.ts) for
both the human-readable message interpolation and the transaction object fields
(payee_name and memo); ensure other displayed fields remain unchanged and that
you apply sanitization where txn.payee_name and txn.memo are referenced (also
update the message template to use the sanitized payee name).
- Around line 128-144: Replace the untyped Record<string, unknown> for
transactionData with a proper interface/type that matches the createTransaction
API shape (e.g., TransactionPayload or the client’s TransactionCreate type) and
use that type for the transactionData variable; ensure each field (account_id,
date, amount, payee_id, payee_name, category_id, memo, cleared, approved,
flag_color) matches the interface types (apply toMilliunits(validated.amount) to
the typed amount field), and prefer using the client’s exported types for the
createTransaction(budgetId, { transaction: ... }) call to get compile-time
safety instead of Record<string, unknown>.
In `@src/tools/transactions/create-transactions.ts`:
- Around line 103-120: The mapping that builds ynabTransactions uses
Record<string, unknown>, losing type safety; change it to construct and return a
properly typed array (e.g., SaveTransaction or a local interface matching YNAB's
SaveTransaction) instead of Record<string, unknown>. Import or declare the
SaveTransaction type and update the mapped variable ynabTransactions to be
SaveTransaction[]; ensure required fields (account_id, date, amount from
toMilliunits) are typed and optional fields (payee_id, payee_name, category_id,
memo, cleared, approved, flag_color, import_id) are declared optional in that
interface so the compiler catches any field-name or type mismatches in the
mapping logic.
- Around line 95-124: handleCreateTransactions is sending { transactions:
ynabTransactions } to client.createTransaction, but YnabClient.createTransaction
expects data.transaction (singular), causing undefined audit fields and a bad
payload; fix by introducing a bulk API in YnabClient (e.g.,
createTransactions(budgetId, { transactions: [...] })) that reads
data.transactions and performs the bulk request/audit logging, then update
handleCreateTransactions to call client.createTransactions(budgetId, {
transactions: ynabTransactions }); ensure the new YnabClient method mirrors the
single-create audit/log structure but iterates or handles the array payload
appropriately.
In `@src/tools/transactions/delete-transaction.ts`:
- Around line 57-75: The code assumes response.data.transaction is non-null when
calling client.deleteTransaction; add a null-check for response.data.transaction
(txn) and return a consistent sanitized error JSON if it's null, mirroring the
create-transaction.ts handler behavior; when txn exists, sanitize fields (amount
via formatCurrency, payee_name/category_name/account_name fallback to null or
'Unknown' as appropriate) before building the success message and
deleted_transaction object to avoid runtime crashes and ensure consistency with
other tools.
In `@src/tools/transactions/get-transaction.ts`:
- Around line 59-96: The returned transaction object is emitting user-controlled
string fields unsanitized; wrap all string fields with the existing sanitizeName
utility (sanitizeName) before serializing: apply sanitizeName to txn.memo,
txn.payee_name, txn.category_name, txn.account_name, txn.import_payee_name,
txn.import_payee_name_original and to each subtransaction's memo, payee_name and
category_name inside the txn.subtransactions.map, leaving numeric IDs and
formatted amounts untouched; ensure you import or reference sanitizeName where
get-transaction constructs the returned object so every user-provided string is
sanitized prior to JSON.stringify.
In `@src/tools/transactions/list-account-transactions.ts`:
- Around line 103-123: The response currently returns raw user-provided strings;
sanitize account_id and text fields before serializing by applying a
sanitization/escaping helper to validated.account_id and to memo, payee_name,
and category_name inside the transactions.map (the formattedTransactions
construction). Add or reuse a sanitizeText(input: string) function and call it
when mapping transactions (for txn.memo, txn.payee_name, txn.category_name) and
when building the top-level account_id so the JSON contains sanitized values
instead of raw user input.
In `@src/tools/transactions/list-category-transactions.ts`:
- Around line 101-112: The payee names used in aggregation and output should be
sanitized: import and use the existing sanitizeName function when deriving the
payee key and when producing the topPayees display. In the loop that builds
byPayee (using outflows and byPayee), replace payee = txn.payee_name ??
'Unknown' with a sanitized value (e.g., sanitizeName(txn.payee_name ??
'Unknown')) so grouping uses normalized names; likewise, when mapping topPayees
from Object.entries(byPayee) ensure the mapped name is the
sanitized/display-safe version (use sanitizeName on the entry key if necessary).
In `@src/tools/transactions/list-payee-transactions.ts`:
- Around line 101-124: The code returns raw user strings in transactions and
category keys; create or import a sanitizer (e.g., sanitizeString or escapeHtml)
and apply it wherever user-provided text flows out: use
sanitizeString(txn.memo), sanitizeString(txn.category_name) and
sanitizeString(txn.account_name) in the formattedTransactions mapping, and
sanitize category before using it as the key when building byCategory (e.g.,
const category = sanitizeString(txn.category_name ?? 'Uncategorized')). Ensure
the sanitizer is used consistently for topCategories labels as well (sanitize
names before sorting/mapping) so no uncontrolled control characters or unsafe
HTML are returned.
- Around line 97-115: The spending summary currently uses absolute values from
all `transactions` (variables `totalSpent`, `byCategory`, `topCategories`,
`avgAmount`), which includes inflows/refunds; filter `transactions` to only
outflows (txn.amount < 0) before computing summaries: compute `totalSpent` from
sumMilliunits on the filtered list, build `byCategory` by iterating over the
filtered outflows and aggregating Math.abs(txn.amount), produce `topCategories`
from that filtered `byCategory` and format with `formatCurrency`, and compute
`avgAmount` as totalSpent divided by the number of outflows (or zero if none)
rather than using the full `transactions` array.
In `@src/tools/transactions/list-transactions.ts`:
- Around line 121-165: Sanitize all user-provided string fields before returning
the JSON: in the formattedTransactions map, run sanitizeName on txn.memo,
txn.payee_name, txn.category_name, txn.account_name and for subtransactions use
sanitizeName on sub.memo, sub.payee_name, sub.category_name; also sanitize the
echoed filter input validated.since_date (i.e., pass validated.since_date
through sanitizeName before including it in filters_applied.since_date.input).
Locate the mapping that builds formattedTransactions and the filters_applied
block and replace raw string uses with sanitized values via the existing
sanitizeName helper.
In `@src/tools/transactions/update-transaction.ts`:
- Around line 124-136: The updateData object uses a loose Record<string,
unknown>; replace it with a typed interface (e.g., UpdateTransactionPayload or
similar) that lists each optional property (account_id, date, amount, payee_id,
payee_name, category_id, memo, cleared, approved, flag_color) with appropriate
types (amount as number of milliunits if you prefer, or keep amount as the input
type and convert via toMilliunits when assigning), then change the variable
declaration in update-transaction.ts (updateData) to use that interface and
update any assignments from validated to match the typed fields; this will give
compile-time safety for the block that builds updateData and any downstream
usage inside the updateTransaction function or related code paths.
In `@src/utils/dates.ts`:
- Around line 128-135: The year-subtraction branch using pastYearsMatch in
src/utils/dates.ts can produce Feb 29 -> Mar 1 on non-leap years; to fix,
normalize the day before changing year in the code that creates pastDate: create
pastDate from today, setDate(1) (or otherwise clamp day to the start of month),
call pastDate.setFullYear(today.getFullYear() - years), then adjust the day to
the lesser of the original day and the last day of the resulting month (e.g.,
compute last day by creating a new Date for the next month day 0) before
returning formatDate(pastDate) so Feb 29 maps to Feb 28 in non-leap years.
In `@src/utils/errors.ts`:
- Around line 147-160: The YnabApiError branch in formatErrorResponse currently
returns error.message and error.details directly; change it to return sanitized
user-facing fields (e.g., message: errorInfo.message or a generic "An upstream
error occurred", omit or replace details with a non-sensitive placeholder like
"See server logs"), and move the full raw error payload (error.message,
error.details, stack, code) into a server log using the existing logger before
returning the sanitized JSON; apply the same sanitization pattern to the other
error-handling branch referenced around the second occurrence (the block that
mirrors lines 193-199) so no raw tokens/headers/paths are echoed to consumers
while full details are preserved in server logs.
- Add Zod input validation to health-check.ts - Add sanitizeErrorMessage() to redact sensitive data from errors: - API tokens and Bearer headers - File system paths - Stack trace details - Control characters - Update formatErrorResponse() to use sanitized messages - Update audit log error entries to use sanitized messages - Add 15 tests for sanitizeErrorMessage() Addresses CodeRabbit pre-merge check failures: - Input Validation: health-check.ts now validates inputs - Error Message Safety: errors are now sanitized before responses Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/errors.ts`:
- Around line 149-205: Replace raw console.error calls inside the YnabApiError,
isYnabSdkError, and generic error branches to log only sanitized summaries: for
YnabApiError (when checking instanceof YnabApiError) call console.error with a
short, non-sensitive tuple like ['YnabApiError', error.code,
sanitizeErrorMessage(error.message)] instead of error.details; for
isYnabSdkError use ['YnabSdkError', statusCode,
sanitizeErrorMessage(error.error?.detail ?? error.error?.message)] and do not
include headers or full objects; and for the generic UnknownError branch log
only ['UnknownError', sanitizeErrorMessage(error)] rather than the full error
object. Keep using sanitizeErrorMessage and existing code paths (YnabApiError,
isYnabSdkError, isZodError) and ensure no tokens, headers, stack traces or raw
detail objects are emitted to console.
Update all console.error calls to use sanitizeErrorMessage(): - YnabApiError: log only code and sanitized message (not raw details) - YnabSdkError: log only status code and sanitized detail - UnknownError: log only sanitized error message (not full object) Addresses CodeRabbit feedback on incremental review. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/utils/errors.ts`:
- Around line 72-105: Update the YNAB_ERROR_CODES mapping to avoid leaking
implementation details: change the '401' entry in YNAB_ERROR_CODES so its
suggestion no longer mentions "access token" and instead uses a generic string
such as "Authentication failed. Verify your credentials and try again."; for
'403' enhance handling by either adding distinct subtype entries (e.g., keys
representing subscription_lapsed, trial_expired, unauthorized_scope,
data_limit_reached) to YNAB_ERROR_CODES or implement parsing logic where
YNAB_ERROR_CODES is consumed to map YNAB's 403 subtypes to specific, actionable
suggestions (ensure any new keys or the parsing logic references the existing
YNAB_ERROR_CODES constant and the '403' key).
- Around line 130-197: The Zod error handling in isZodError currently maps
issue.message directly; update formatErrorResponse to sanitize Zod messages by
applying sanitizeErrorMessage() when building issues (in the isZodError branch),
i.e., change the issues mapping to use message:
sanitizeErrorMessage(issue.message) and also use the sanitized message where the
top-level "message" or any returned text is derived from the issues; keep the
rest of the structure the same so Zod messages are normalized like Ynab errors.
- Change 401 error suggestion to generic "Authentication failed" message instead of mentioning "access token" (avoids leaking implementation details) - Sanitize Zod validation error messages using sanitizeErrorMessage() - Update test to match new generic error message Addresses CodeRabbit feedback on incremental review. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/utils/errors.ts`:
- Around line 186-190: The mapping over error.issues inside the isZodError
branch can crash if an issue lacks a proper path array or message string; update
the map in the isZodError branch (where error.issues is processed and
sanitizeErrorMessage is used) to defensively check each issue: ensure issue.path
is an array before calling join('.') (fallback to '' or String(issue.path) when
invalid) and ensure issue.message is a string before passing to
sanitizeErrorMessage (fallback to a default like 'Invalid issue' or
String(issue.message)). Keep using isZodError and sanitizeErrorMessage but add
these lightweight guards so malformed issue shapes won’t throw during
formatting.
- Around line 111-146: The branches returning JSON for ReadOnlyModeError,
RateLimitError, ValidationError, and NotFoundError include error.message
verbatim; replace those usages with a sanitized, non-sensitive string (e.g.,
call a new or existing helper sanitizeErrorMessage(error) or return a fixed safe
message like "An error occurred" or context-specific safe text) so no user input
or upstream details are leaked. Update the JSON returned in the
ReadOnlyModeError, RateLimitError, ValidationError, and NotFoundError branches
to use the sanitized message (keep safe numeric/field properties like
retry_after_ms and field if needed) and ensure the helper is applied
consistently in those JSON.stringify calls.
- Use generic messages for ReadOnlyModeError and RateLimitError - Sanitize ValidationError and NotFoundError messages - Add defensive checks for Zod error handling (path array, message string) - Update tests for new generic error messages Addresses CodeRabbit feedback on error sanitization. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
environment.ts: - Harden parseBoolean to reject unrecognized values - Prevents typos like YNAB_READ_ONLY=ture from enabling writes - Throws clear error with variable name hint index.ts: - Sanitize error logging on startup failure - Only log error message, not full error object - Prevents leaking sensitive config details errors.ts: - Use generic messages for ReadOnlyModeError and RateLimitError - Sanitize ValidationError and NotFoundError messages - Add defensive checks for malformed Zod error shapes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add sanitizeName/sanitizeMemo to all tools returning user data: - accounts: create-account.ts, list-accounts.ts - budgets: get-budget.ts, list-budgets.ts - categories: get-category.ts, update-category.ts - payees: get-payee.ts, list-payees.ts - scheduled-transactions: get-scheduled.ts, list-scheduled.ts - months: list-months.ts Prevents potential injection in downstream clients consuming MCP output. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
monthly-comparison.ts: - Handle previousSpending=0 case properly - Now returns 100% increase when previous=0 and current>0 savings-opportunities.ts: - Fix month subtraction overflow on 29th-31st - Pin date to 1st before subtracting months Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/config/environment.ts`:
- Around line 43-56: The loadConfig function currently uses parseInt for
cacheTtlMs and rateLimitPerHour which allows partial parses (e.g., "10ms" ->
10); change these to strict numeric validation by reading
process.env['CACHE_TTL_MS'] and process.env['RATE_LIMIT_PER_HOUR'] as strings
and validating them with a strict integer check (e.g., /^\d+$/) before
converting to Number, or better yet defer parsing to the Zod schema that
validates numeric range/types; update the rawConfig entries for cacheTtlMs and
rateLimitPerHour (the symbols: loadConfig, rawConfig, CACHE_TTL_MS,
RATE_LIMIT_PER_HOUR, parseInt) to reject non-numeric values and ensure Zod
receives proper numbers for range checks.
♻️ Duplicate comments (1)
src/utils/errors.ts (1)
149-183: Do not return upstream error text to clients; use mapped messages only.
Even sanitized upstream messages can still leak user-specific data (resource names/IDs). PrefererrorInfo.messagefor client responses and keep details in logs only. This is especially important for SDK errors wheredetailoften contains resource-specific info.🔒 Proposed hardening
return JSON.stringify({ error: true, type: 'ynab_api_error', code: error.code, - message: sanitizeErrorMessage(error.message) || errorInfo.message, + message: errorInfo.message, suggestion: errorInfo.suggestion, }, null, 2); @@ return JSON.stringify({ error: true, type: 'ynab_api_error', code: statusCode, - message: sanitizeErrorMessage(error.error?.detail) || errorInfo.message, + message: errorInfo.message, suggestion: errorInfo.suggestion, }, null, 2);As per coding guidelines, error messages must not leak sensitive information.
cache.ts: - Guard against non-finite TTLs (NaN, Infinity, negative, zero) - Throws clear error message on invalid TTL - Added 4 tests for TTL validation get-month.ts: - Fix "current" validation - now accepts both YYYY-MM-DD and "current" - Convert "current" to actual month date in handler - Add sanitization for category names and month notes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace placeholder URLs with actual repository path: - git clone URL now points to auzroz/ynab-mcp - Claude Desktop config path updated to match Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses CodeRabbit review feedback: 1. environment.ts: Add parseInteger() with strict validation that rejects partial parses like "10ms" -> 10. Only accepts digit-only strings to prevent configuration footguns. 2. errors.ts: Use only mapped error messages in client responses, not upstream error text (even when sanitized) to prevent leaking user-specific data like resource names/IDs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 86-156: Multiple markdown headings in README.md (e.g., "### User
Tools (1)", "### Budget Tools (3)", "### Account Tools (3)", "### Category Tools
(4)", "### Payee Tools (5)", "### Month Tools (2)", "### Transaction Tools
(10)", "### Scheduled Transaction Tools (2)", "### Analytics Tools (22)") are
not followed by a blank line causing MD022 linter failures; fix by inserting a
single blank line immediately after each of those heading lines so the list
content beneath each heading starts on a new paragraph, then run the markdown
linter to confirm MD022 is resolved.
In `@src/services/cache.ts`:
- Around line 17-19: The constructor currently assigns defaultTtlMs without
validation; update the constructor to validate that defaultTtlMs is a finite
number >= 0 (reject NaN, Infinity, negatives) and throw an appropriate
RangeError or TypeError if invalid, mirroring the TTL checks used in set();
either call the existing TTL validation helper (if present) or add a small
validateDefaultTtl function and assign the validated value to this.defaultTtlMs.
In `@src/tools/accounts/create-account.ts`:
- Around line 14-29: The hard-coded accountTypes array can drift from
ynab.AccountType; import the SDK enum (e.g. AccountType from 'ynab') and replace
or derive accountTypes from its members so the compiler enforces alignment (for
example, build the array using AccountType.Checking, AccountType.Savings, etc.,
or create a mapped/const assertion that converts the enum to a readonly string
tuple), ensuring accountTypes is typed as readonly and matches ynab.AccountType
at compile time; update any code referencing accountTypes to use the new symbol
name if changed.
In `@src/tools/analytics/monthly-comparison.ts`:
- Around line 47-56: The per-category percent calculation should treat cases
where last_month is 0 as "new" instead of reporting 100%: modify the
CategoryChange handling so when rawChange is positive and last_month === 0 you
set change (string) to "new", set change_percent to null (or undefined) and set
direction to a new value 'new' (update the CategoryChange type union for
direction to include 'new'); ensure logic that computes change_percent only
divides when last_month > 0 and that any callers/serializers handle
change_percent being null and direction === 'new' accordingly.
In `@src/tools/analytics/savings-opportunities.ts`:
- Around line 232-253: The loop over data.byPayee drops the payeeId and then
resolves the payee name by searching transactions only by amount, which can
attach the wrong name; change the iteration to capture the payeeId (e.g., for
(const [payeeId, payeeAmounts] of data.byPayee)) and when finding the
transaction use that payeeId in the predicate (match t.payee_id === payeeId and
the amount inclusion) so payeeName is resolved for the specific payee being
analyzed instead of any payee with the same amount.
In `@src/tools/categories/update-category.ts`:
- Around line 24-26: The budgeted field in the category update schema currently
uses z.number() which permits Infinity/-Infinity; update the validator for the
budgeted schema entry (the symbol "budgeted" in
src/tools/categories/update-category.ts) to use z.number().finite() so
non-finite values are rejected before they reach toMilliunits and produce
invalid API payloads, and ensure any tests or callers that pass Infinity are
adjusted accordingly.
In `@src/tools/scheduled-transactions/get-scheduled.ts`:
- Around line 60-89: The code assumes txn.subtransactions is always an array and
calls .length/map, which will throw when the YNAB API returns null for non-split
items; update every usage to use the null-coalescing pattern (e.g., replace
direct accesses of txn.subtransactions with txn.subtransactions ?? []) in
get-scheduled.ts (variable txn in the JSON return block), list-scheduled.ts,
get-transaction.ts, and list-transactions.ts so you safely check length and map
over (txn.subtransactions ?? []).map(...), preserving existing formatting
helpers like formatCurrency and sanitizeMemo/sanitizeName for each
subtransaction entry.
In `@tests/unit/services/cache.test.ts`:
- Around line 1-157: Add unit tests that validate the Cache constructor rejects
invalid default TTLs and accepts a valid positive TTL: create a new
"constructor" describe block that calls new Cache(...) with NaN, Infinity,
negative, and zero values and asserts each throws 'Invalid default TTL', and
include one test that constructs Cache(5000) and verifies set/get works;
reference the Cache constructor and reuse existing test helpers (vitest
expect/it/describe) in the same tests/unit/services/cache.test.ts file.
♻️ Duplicate comments (9)
src/tools/payees/get-payee.ts (1)
13-43: Add UUID validation forpayee_idin both schemas.Right now any string passes. Enforce UUID format in the Zod schema and mirror it in the Tool inputSchema. As per coding guidelines, validate all inputs with Zod before use.
✅ Proposed fix
- payee_id: z.string().describe('The payee UUID to retrieve'), + payee_id: z.string().uuid().describe('The payee UUID to retrieve'),payee_id: { type: 'string', + format: 'uuid', description: 'The payee UUID to retrieve', },Zod v3.23.8: does z.string().uuid() exist and map to JSON Schema format "uuid"?src/tools/months/list-months.ts (1)
58-61: Hard-coded$currency symbol may misrepresent non-USD budgets.
formatCurrency()defaults to$, which will display incorrect currency symbols for international users. Consider fetching the budget's currency settings and passing the appropriate symbol.src/tools/budgets/list-budgets.ts (1)
62-69: Account balance formatting ignores budget's currency.The
formatCurrency(acc.balance)call on line 66 uses the default currency symbol (likely$), but the budget's actual currency is available inbudget.currency_format. This causes incorrect currency display for non-USD budgets (e.g., a EUR budget would show "$100.00" instead of "€100.00").💱 Proposed fix to use budget currency
if (budget.accounts != null && budget.accounts.length > 0) { + const currencySymbol = budget.currency_format?.currency_symbol ?? '$'; budgetInfo['accounts'] = budget.accounts.map((acc) => ({ name: sanitizeName(acc.name), type: acc.type, - balance: formatCurrency(acc.balance), + balance: formatCurrency(acc.balance, currencySymbol), closed: acc.closed, })); }Verify that
formatCurrencyaccepts a second parameter for currency symbol:#!/bin/bash # Check formatCurrency function signature and parameters ast-grep --pattern $'function formatCurrency($$$) { $$$ }' # Also check if it's an arrow function or const rg -n "export.*formatCurrency" --type ts -A 10src/tools/budgets/get-budget.ts (1)
108-128: LGTM! Secure response construction.The response correctly:
- Sanitizes
budget.name(line 111)- Exposes
budget_idandserver_knowledgeappropriately (needed for delta sync and user reference)- Avoids leaking any sensitive information like tokens or internal paths
The previous review concern about unsanitized user-controlled names has been fully addressed.
src/tools/accounts/list-accounts.ts (1)
112-124: Liability totals treat overpayments as debt.
UsingMath.absper liability account makes positive balances (credits) look like debt, understating net worth. Sum signed liabilities first and only convert negative totals to debt.🧮 Proposed fix
- const totalLiabilities = accounts - .filter((a) => liabilityTypes.includes(String(a.type))) - .reduce((sum, a) => sum + Math.abs(a.balance), 0); + const liabilityBalance = accounts + .filter((a) => liabilityTypes.includes(String(a.type))) + .reduce((sum, a) => sum + a.balance, 0); + const totalLiabilities = Math.abs(Math.min(0, liabilityBalance)); ... - net_worth: formatCurrency(totalAssets - totalLiabilities), + net_worth: formatCurrency(totalAssets + liabilityBalance),src/tools/analytics/savings-opportunities.ts (1)
191-281: Sanitize category/payee strings before emitting responses.User-provided names are returned in
category,description, andsuggestionwithout sanitization, which can leak control characters or unsafe content. As per coding guidelines, sanitize these strings before output.🔒 Suggested fix
-import { formatCurrency, sumMilliunits } from '../../utils/milliunits.js'; +import { formatCurrency, sumMilliunits } from '../../utils/milliunits.js'; +import { sanitizeName } from '../../utils/sanitize.js'; for (const [categoryId, data] of byCategory) { const lookup = categoryLookup.get(categoryId) ?? { name: 'Uncategorized', group: 'Other', isDiscretionary: false, }; + const safeCategory = sanitizeName(lookup.name); // 1. High discretionary spending if (lookup.isDiscretionary && monthlyAvg > 5000) { const potentialSavings = monthlyAvg * 0.2; // Suggest 20% reduction opportunities.push({ type: 'discretionary', - category: lookup.name, - description: `Discretionary spending on ${lookup.name}`, + category: safeCategory, + description: `Discretionary spending on ${safeCategory}`, potential_monthly_savings: formatCurrency(potentialSavings), current_monthly_spend: formatCurrency(monthlyAvg), - suggestion: `Consider reducing ${lookup.name} spending by 20%`, + suggestion: `Consider reducing ${safeCategory} spending by 20%`, confidence: 'medium', }); } ... - const payeeName = txn?.payee_name ?? 'Unknown'; + const payeeName = sanitizeName(txn?.payee_name ?? 'Unknown'); opportunities.push({ type: 'recurring_expense', - category: lookup.name, - description: `Recurring charge: ${payeeName}`, + category: safeCategory, + description: `Recurring charge: ${payeeName}`, potential_monthly_savings: formatCurrency(avg), current_monthly_spend: formatCurrency(avg), suggestion: `Review if you still need ${payeeName}. Cancel if unused.`, confidence: 'high', }); ... if (monthlyImpact > 20000 && lookup.isDiscretionary) { opportunities.push({ type: 'large_single', - category: lookup.name, - description: `Large purchases in ${lookup.name}`, + category: safeCategory, + description: `Large purchases in ${safeCategory}`, potential_monthly_savings: formatCurrency(monthlyImpact * 0.3), current_monthly_spend: formatCurrency(monthlyImpact), - suggestion: `Review large ${lookup.name} purchases. Consider waiting periods before big buys.`, + suggestion: `Review large ${safeCategory} purchases. Consider waiting periods before big buys.`, confidence: 'low', }); }As per coding guidelines, sanitize all user-provided strings before returning tool responses.
src/tools/analytics/monthly-comparison.ts (1)
192-216: Still misleading when previous month spending is $0.
spendingChangePercentis forced to 100%, so the status/message read as “up 100%,” which is misleading from a zero baseline. Consider a dedicated branch with an absolute‑value message.🛠️ Suggested fix
- if (spendingChangePercent < -10) { + if (previousSpending === 0) { + if (currentSpending === 0) { + status = 'similar'; + message = 'No spending in either month'; + } else { + status = 'worse'; + message = `Spending increased from $0 last month to ${formatCurrency(currentSpending)}`; + } + } else if (spendingChangePercent < -10) { status = 'better'; message = `Spending is down ${Math.abs(Math.round(spendingChangePercent))}% from last month`;src/tools/categories/update-category.ts (1)
32-42: Add READ_ONLY warning in the tool description (consistency with other write tools).This was flagged previously and still appears missing. Please add the
YNAB_READ_ONLY=falsewarning to the description.src/tools/scheduled-transactions/list-scheduled.ts (1)
60-79: Guard against nullablesubtransactionsbefore calling.length.Same issue as previously flagged: if
txn.subtransactionsis null/undefined, Line 72 throws. Also consider always returning an array ([]) for a stable JSON shape.✅ Suggested fix
- const formattedTransactions = activeTransactions.map((txn) => ({ - id: txn.id, - date_first: txn.date_first, - date_next: txn.date_next, - frequency: txn.frequency, - amount: formatCurrency(txn.amount), - memo: sanitizeMemo(txn.memo), - payee_name: sanitizeName(txn.payee_name), - category_name: sanitizeName(txn.category_name), - account_name: sanitizeName(txn.account_name), - flag_color: txn.flag_color, - subtransactions: - txn.subtransactions.length > 0 - ? txn.subtransactions.map((sub) => ({ - amount: formatCurrency(sub.amount), - memo: sanitizeMemo(sub.memo), - payee_id: sub.payee_id, - category_id: sub.category_id, - })) - : undefined, - })); + const formattedTransactions = activeTransactions.map((txn) => { + const subs = txn.subtransactions ?? []; + return { + id: txn.id, + date_first: txn.date_first, + date_next: txn.date_next, + frequency: txn.frequency, + amount: formatCurrency(txn.amount), + memo: sanitizeMemo(txn.memo), + payee_name: sanitizeName(txn.payee_name), + category_name: sanitizeName(txn.category_name), + account_name: sanitizeName(txn.account_name), + flag_color: txn.flag_color, + subtransactions: + subs.length > 0 + ? subs.map((sub) => ({ + amount: formatCurrency(sub.amount), + memo: sanitizeMemo(sub.memo), + payee_id: sub.payee_id, + category_id: sub.category_id, + })) + : [], + }; + });Please verify the YNAB typings to confirm whether
subtransactionscan be nullable:#!/bin/bash # Locate ScheduledTransaction typings and subtransactions definition rg -n "ScheduledTransaction" -C 3 src rg -n "subtransactions" -C 3 src fd -a -e d.ts -g "*ynab*" . | head -20
1. cache.ts: Validate defaultTtlMs in constructor (reject NaN, Infinity, <= 0) 2. update-category.ts: Add .finite() to budgeted field, add READ_ONLY warning 3. get-scheduled.ts, list-scheduled.ts: Guard against nullable subtransactions 4. get-payee.ts: Add .uuid() validation for payee_id 5. savings-opportunities.ts: Use payeeId in lookup, sanitize category/payee names 6. monthly-comparison.ts: Handle previousSpending=0 with absolute value message, treat new categories (previous=0) with 'new' direction instead of 100% Added 5 constructor validation tests for cache (147 total tests passing). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/errors.ts`:
- Around line 110-211: The RateLimitError branch in formatErrorResponse
hardcodes "180 requests per hour"; update it to use a single source of truth by
importing or referencing the configured limiter constant (e.g.,
RATE_LIMIT_PER_HOUR or the limiter config) and build the suggestion string
dynamically (e.g., `suggestion: \`Wait before making more requests. The server
has a budget of ${RATE_LIMIT_PER_HOUR} requests per hour.\``) inside the
formatErrorResponse function so the user-facing text always matches the actual
limiter configuration; update any related tests or exports to expose the
constant if needed.
♻️ Duplicate comments (1)
README.md (1)
86-156: Add blank lines after tool headings to satisfy MD022.markdownlint reports MD022 for these headings; insert a single blank line after each tool category heading so the lists render consistently.
📝 Example fix pattern
### User Tools (1) + - `ynab_get_user` - Get authenticated user information
1. errors.ts: Remove hardcoded rate limit value, suggest using ynab_rate_limit_status tool instead 2. README.md: Add blank lines after tool headings to satisfy MD022 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/tools/analytics/savings-opportunities.ts`:
- Around line 296-312: The code currently parses formatted currency strings
(potential_monthly_savings) back to numbers for sorting and totals; instead add
and populate a raw numeric field (e.g., potential_monthly_savings_raw) on each
opportunity when creating/merging them and use that raw field for sorting
(deduped.sort using potential_monthly_savings_raw), for totalPotentialSavings
(reduce over potential_monthly_savings_raw), and for highConfidenceSavings
(reduce over potential_monthly_savings_raw on highConfidenceOpportunities); keep
the existing formatted potential_monthly_savings for output but perform all
arithmetic with potential_monthly_savings_raw and use
sumMilliunits(transactions.map(...))/months unchanged for current spend.
In `@src/tools/categories/update-category.ts`:
- Around line 19-27: Update the input schema in update-category.ts to enforce
UUID for category_id and require the month to be the first day of the month:
replace the current month regex (which allows any day) with a regex that only
matches YYYY-MM-01 (e.g., /^\d{4}-\d{2}-01$/) and change category_id from
z.string() to z.string().uuid(); keep the existing budgeted validation unchanged
and ensure you update any descriptive text for month/category_id to reflect the
stricter validation.
In `@tests/unit/services/cache.test.ts`:
- Around line 114-130: Add a unit test to cover the edge case where
deleteByPrefix finds no matching keys: create a new test using the Cache class,
seed it with a non-matching key (e.g., cache.set('other:key', 'value')), call
cache.deleteByPrefix('budget:'), and assert the returned count is 0 and that the
existing key still returns its value via cache.get; place this alongside the
existing describe('deleteByPrefix') block to ensure deleteByPrefix correctly
returns 0 when there are no matches.
♻️ Duplicate comments (2)
src/tools/payees/get-payee.ts (1)
38-41: JSON Schema missingformat: 'uuid'forpayee_id- inconsistent with Zod validation.The Zod schema correctly validates
payee_idas UUID, but the JSON Schema metadata exposed to MCP clients doesn't includeformat: 'uuid'. This creates a documentation inconsistency where clients won't know the expected format.Proposed fix
payee_id: { type: 'string', + format: 'uuid', description: 'The payee UUID to retrieve', },src/tools/analytics/monthly-comparison.ts (1)
203-260: Overall change_percent is misleading when last month is $0.
Percent change from zero is undefined; returning 100 can understate large jumps and conflicts with the “new” semantics used elsewhere. Consider returning null (or “new”) in thecomparison.spending.change_percentfield whenpreviousSpendingis 0.🛠️ Suggested fix
- change_percent: Math.round(spendingChangePercent), + change_percent: + previousSpending === 0 ? null : Math.round(spendingChangePercent),
Adds docstrings to 55 handler functions to meet the 80% documentation coverage threshold required by the pre-merge check. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 32
🤖 Fix all issues with AI agents
In `@src/tools/accounts/create-account.ts`:
- Around line 26-38: The balance validator in inputSchema currently uses
z.number() which permits NaN/Infinity; update the balance field to use
z.number().finite() so only finite numbers are accepted (i.e., change the
balance schema chain in inputSchema to include .finite()), and apply the same
.finite() addition to other amount/balance validators (the equivalent validators
used in create-transaction, update-transaction, update-category modules) to
prevent NaN/Infinity from propagating into toMilliunits/YNAB API.
In `@src/tools/accounts/get-account.ts`:
- Around line 34-44: The tool inputSchema in get-account.ts currently marks
budget_id and account_id as type: 'string' but Zod expects UUIDs; update both
properties to include format: 'uuid' (e.g., budget_id: { type: 'string', format:
'uuid', ... } and account_id: { type: 'string', format: 'uuid', ... }) so
client-side JSON schema validation enforces UUIDs; if budget_id legitimately
accepts the sentinel "last-used", adjust the schema or handling (e.g., accept
that value separately or document it) to avoid conflicting with the UUID
constraint.
- Around line 66-89: Remove the internal YNAB transfer_payee_id from the account
response by deleting the transfer_payee_id property in the account object
constructed in get-account.ts (the code that builds the account: {...} return).
Update any related serialization or types if present (e.g., response shape or
Account DTO) so the field is not emitted; note that list_accounts already omits
this field so mirror that behavior to avoid exposing internal IDs.
In `@src/tools/analytics/budget-suggestions.ts`:
- Around line 95-101: The current fetch uses Promise.all with
client.getBudgetMonth calls (see getCategories, getBudgetMonth, months) which
will reject if any month is missing; replace this with Promise.allSettled (or
wrap each getBudgetMonth in a try/catch) and then filter for successful results
(status === "fulfilled") to build categoriesResponse, currentMonthResponse, and
historicalMonthResponses from only the fulfilled values so the analytics
functions can run on available history without failing for non-existent months.
In `@src/tools/analytics/budget-vs-actuals.ts`:
- Around line 20-24: The month schema's regex (z.string().regex for the month
field) is too permissive and allows invalid months like 00 or 13; tighten the
regex to only allow months 01–12 (e.g. /^\d{4}-(0[1-9]|1[0-2])-01$/) and ensure
the previous-month logic used when include_previous is true parses the validated
year/month into a real Date (or equivalent date library) and computes the
previous month via date arithmetic (e.g., new Date(year, monthIndex - 1, 1) then
subtract one month) instead of string manipulation so invalid months are
rejected and previous-month calculation is correct.
In `@src/tools/analytics/goal-progress.ts`:
- Around line 215-220: The statusCounts computation iterates goalsInfo four
times; change it to a single-pass accumulator (e.g., using
Array.prototype.reduce or a single for loop) to tally complete, on_track,
behind, and underfunded in one traversal; update the existing statusCounts
variable so it initializes counts for those four keys and increments the
appropriate key for each g.status as you iterate through goalsInfo (ensure
unknown statuses are ignored or handled as needed).
In `@src/tools/analytics/income-expense.ts`:
- Around line 140-214: totalIncome is treated as 0 and yields a misleading
overallSavingsRate and "warning" status when incomes are zero; change the logic
that computes overallSavingsRate, status, and the returned totals.savings_rate
so that when totalIncome === 0 you return savings_rate as "N/A" and set status
to "concern" if totalExpenses > 0 (and an appropriate non-concern status if both
income and expenses are zero), e.g. update the calculations that use
overallSavingsRate and the status/statusMessage assignment to branch on
totalIncome === 0 before the existing thresholds and adjust the final JSON
output (fields: overallSavingsRate, status, statusMessage, totals.savings_rate)
accordingly.
In `@src/tools/analytics/monthly-comparison.ts`:
- Around line 90-121: Replace the manual per-category income calculation with
the month-level income field: stop deriving currentIncome and previousIncome by
summing filtered category activity in the loops that iterate
currentData.categories and previousData.categories; instead set currentIncome =
currentData.income and previousIncome = previousData.income (leave the
budgeted/spending sums and the groupLookup/category filters intact). Update
references to currentIncome and previousIncome used later in this module so they
use the assigned month-level values.
In `@src/tools/analytics/reconciliation-helper.ts`:
- Around line 148-155: The construction of the UnclearedTransaction uses
sanitizeName(txn.payee_name) but txn.payee_name can be null; update the payee
field to guard against null by passing a safe fallback (e.g.
sanitizeName(txn.payee_name ?? '') or sanitizeName(txn.payee_name || '')) so
sanitizeName never receives null and the object creation in
reconciliation-helper.ts (UnclearedTransaction) won't throw; locate the object
literal where payee is set and replace the direct call with the null-safe
variant.
- Around line 110-112: When validated.account_id is present, after applying
accounts = accounts.filter((a) => a.id === validated.account_id) detect if
accounts.length === 0 and throw a clear, explicit error (e.g., "Account not
found" with the provided account_id) instead of letting the function continue
and potentially return "up_to_date"; update the handler around
validated.account_id and accounts to perform this check and raise the error so
callers receive an explicit failure when a non-existent account_id is specified.
In `@src/tools/analytics/savings-opportunities.ts`:
- Around line 44-57: The tool's inputSchema in savings-opportunities.ts does not
expose the Zod constraints for the months field, so update the
inputSchema.properties.months entry to include "minimum": 2 and "maximum": 12
(and update the description to mention the 2–12 range and default of 3) so
clients inspecting the tool see the same validation as the Zod schema (the
months Zod validator that calls min(2) and max(12)); keep required unchanged.
In `@src/tools/analytics/spending-by-payee.ts`:
- Around line 54-75: Update the JSON inputSchema to mirror the Zod validation
constraints: add "integer": true and "minimum": 1 and "maximum": 12 for months,
add "integer": true and "minimum": 1 and "maximum": 50 for limit, and add
"integer": true and "minimum": 1 for min_transactions; keep budget_id as a
string but you may add a description or pattern if Zod enforces one; ensure
these changes align with the Zod validation used later (the z.object(...) schema
validated at line ~98) so the MCP schema accurately documents the same bounds
clients must follow.
In `@src/tools/analytics/spending-pace.ts`:
- Around line 144-151: Define a Status union for allowed statuses and type the
map as Record<Status, number> (or declare statusPriority with a const assertion)
so missing/extra keys are compile-time errors; update the statusPriority
declaration to use that type and ensure the items in categoryPaces have status:
Status (or cast a.status/b.status to Status) before calling
categoryPaces.sort((a, b) => (statusPriority[a.status] ?? 99) -
(statusPriority[b.status] ?? 99)) so indexing is type-safe and key drift is
prevented.
In `@src/tools/analytics/transaction-search.ts`:
- Around line 238-247: The category mapping in the limitedTransactions.map that
builds results uses the expression category: t.category_id ?
sanitizeName(categoryLookup.get(t.category_id) ?? '') || null : null which
returns the string "Unknown" when a category_id exists but lookup is missing;
change the logic so that when t.category_id is present but
categoryLookup.get(...) is undefined you return null instead of
sanitizeName('Unknown'). Update the mapping around limitedTransactions.map /
results to first check t.category_id, then retrieve const name =
categoryLookup.get(t.category_id) and return name ? sanitizeName(name) : null
(keeping sanitizeName and accountLookup usage unchanged).
- Around line 15-53: Update the inputSchema by adding a maximum length
constraint to the string fields used for free-text filtering: add a .max(<n>) to
the z.string().optional() definitions for query, payee, and category in
inputSchema (e.g., .max(256) or whatever project limit you choose), and update
each field's .describe() text to mention the max length; ensure the validation
error messages remain meaningful and tests (if any) are adjusted accordingly.
In `@src/tools/budgets/get-budget.ts`:
- Around line 60-119: The code currently calls formatCurrency without using the
budget's currency_format; update formatting to use the budget-provided currency
settings by passing budget.currency_format into your formatter (or add a wrapper
like formatCurrencyForBudget(budget.currency_format, amount) or extend
formatCurrency to accept an optional currencyFormat argument). Apply this change
where balances are rendered: the accounts mapping (inside the accountsByType
push), category amounts in categoryGroups (budgeted, activity, balance), and the
summary fields total_assets, total_liabilities, and net_worth so all monetary
outputs use budget.currency_format rather than defaulting to USD.
In `@src/tools/budgets/list-budgets.ts`:
- Around line 58-74: The budget.currency_format value is user-controlled and
must be sanitized before being returned or passed into formatting; update the
mapping that builds budgetInfo (the block that sets budgetInfo['accounts']) to
run budget.currency_format through a sanitizer function (reuse or create e.g.,
sanitizeCurrencyFormat) and use that sanitized value both when assigning
currency_format on budgetInfo and when calling
formatCurrencyWithFormat(acc.balance, ...); ensure the sanitized format (not the
raw budget.currency_format) is included in the returned budgetInfo and passed to
formatCurrencyWithFormat, leaving sanitizeName, formatCurrencyWithFormat, and
formatCurrency usages otherwise intact.
In `@src/tools/categories/get-month-category.ts`:
- Around line 46-49: Update the month field description in the tool schema in
get-month-category.ts to reflect the actual required format (YYYY-MM-01) and
note that the day must be "01" (i.e., use the first of the month); locate the
month property in the schema object (the month: { type: 'string', description:
... } entry) and replace the current "YYYY-MM-DD" text with "YYYY-MM-01" and an
explicit note like "use the first of the month" so callers are not misled.
In `@src/tools/categories/update-category.ts`:
- Around line 53-56: The schema description for the month property is
inconsistent with the Zod validation: update the month property's description
(the month field in the tool schema) to match the Zod rule that enforces the
first day of the month (e.g., "YYYY-MM-01 format (first day of month)") so
consumers see the correct format; verify that this description change aligns
with the Zod validation referenced near the month validation (the Zod schema
that enforces YYYY-MM-01) and adjust only the description text if validation is
correct.
In `@src/tools/months/get-month.ts`:
- Around line 76-115: The response currently exposes internal category_group_id
by building categoriesByGroup keyed on category.category_group_id; change this
to group by the category group name (e.g., use category.category_group_name or
lookup the group name) and emit categories_by_group as an array of objects {
group_name: sanitizeName(...), categories: [...] } rather than a map keyed by
ID; update the loop that populates categoriesByGroup and the final returned
payload construction (references: categoriesByGroup, month.categories,
sanitizeName, sanitizeMemo, formatCurrency) to sanitize group names and keep IDs
out of the serialized response.
In `@src/tools/months/list-months.ts`:
- Around line 66-88: Remove the extra getBudgetSettingsById call and stop using
per-budget currency formatting: delete the
settingsResponse/getBudgetSettingsById usage and instead return numeric fields
directly (income, budgeted, activity, to_be_budgeted) without calling
formatAmount (or if you must include a symbol, use the default '$' as in
list-budgets.ts), so adjust the mapping that builds formattedMonths accordingly;
also ensure sanitizeMemo is null-safe by guarding or updating its use
(sanitizeMemo(month.note)) before mapping; referenced symbols:
getBudgetSettingsById, getBudgetMonths, formatAmount, sanitizeMemo,
formattedMonths.
In `@src/tools/payees/get-payee-location.ts`:
- Around line 7-10: The returned payee location fields loc.id and loc.payee_id
are emitted raw; validate and sanitize them before returning. In the function
that builds the return object (references: loc, loc.id, loc.payee_id and the
get-payee-location logic around where the response is constructed — also the
similar block at the later return lines 53-63), apply a zod-based
sanitizer/validator (e.g., z.string().trim().transform or z.string().regex(...)
then .parse) or a small utility sanitizeId function to enforce allowed
characters/length and strip/escape unsafe characters, and replace the raw loc.id
and loc.payee_id with the parsed/sanitized values in the returned object. Ensure
both places that return loc.* are updated.
In `@src/tools/payees/get-payee.ts`:
- Around line 7-10: The returned payee fields are emitted raw; sanitize any
user-originated ID strings before returning by passing payee.id and
payee.transfer_account_id (and any other emitted ID-like fields around the
get-payee logic, e.g., lines handling transfer_account_id at the other block)
through the existing sanitizeName utility used in this module so the response
contains sanitized strings rather than raw user data; update the return/response
construction in the get-payee handling to replace payee.id and
transfer_account_id with sanitizeName(payee.id) and
sanitizeName(payee.transfer_account_id) (and mirror for the second occurrence
noted) so all emitted IDs are sanitized.
In `@src/tools/payees/list-payees.ts`:
- Around line 7-10: The payee response returns raw identifiers; use the existing
sanitizeName utility to sanitize payee.id and payee.transfer_account_id before
including them in the returned object (and similarly sanitize any
transfer_account_id handled in the block around lines 62-66). Update the
mapping/return logic in list-payees (where payees are transformed) to replace
raw id and transfer_account_id with sanitizeName(id) and
sanitizeName(transfer_account_id) so all outward-facing identifiers are cleaned.
In `@src/tools/system/audit-log.ts`:
- Around line 81-84: The function handleAuditLog currently accepts args and
calls inputSchema.parse but omits a `client` parameter intentionally; add a
brief inline comment above the handleAuditLog signature explaining that `client`
is intentionally omitted because this tool is local-only (no external client
dependency) and to document the intentional signature variance for future
maintainers, referencing handleAuditLog and inputSchema so it's clear this is
deliberate for local-only system tools.
- Around line 11-33: The input schema defined as inputSchema should be made
strict to reject unexpected properties; update the z.object(...) used to create
inputSchema by calling .strict() on it (e.g., inputSchema =
z.object({...}).strict()) so the parser validates and rejects extra fields,
keeping behavior consistent with other system tools.
- Around line 119-127: Audit read serialization currently returns entries with
an unredacted details field; add a defensive redaction step to sanitize details
at read time. Update the serialization in the function that builds the JSON (the
block that returns JSON.stringify of { entries, count, summary }) to map over
entries and replace entry.details with a sanitized version (reuse or create a
helper similar to sanitizeErrorMessage, e.g., sanitizeDetails) before
stringifying; ensure you only alter the serialized output (not stored entries)
and keep the existing summary and sanitized error messages intact.
In `@src/tools/transactions/import-transactions.ts`:
- Around line 61-70: The response currently includes internal transaction IDs
(transaction_ids: importedIds) unconditionally; modify the import flow in
import-transactions.ts (the function that builds this JSON response) to accept
an include_ids boolean option (e.g., include_ids or options.include_ids) and
only add the transaction_ids field to the returned object when include_ids is
true, otherwise omit that property; keep imported_count always present and use
the existing importedIds variable to populate IDs when the flag is set.
In `@src/tools/transactions/list-category-transactions.ts`:
- Around line 36-70: The JSON inputSchema on listCategoryTransactionsTool is too
loose compared to the Zod schema used elsewhere; tighten it so emitted inputs
are valid by (1) adding constraints for limit (integer, minimum 1, maximum 500,
default 100), (2) making budget_id accept "last-used" as default or allow empty
string, (3) constraining since_date to match YYYY-MM-DD or a simple
natural-language pattern or provide a descriptive enum/format, and (4) ensure
required includes category_id only; update the inputSchema properties
(budget_id, category_id, since_date, limit) to include these JSON Schema
keywords (type, minimum, maximum, default, pattern/format) so the tool's JSON
schema aligns with the Zod validation logic used by the handler.
In `@src/tools/transactions/list-payee-transactions.ts`:
- Around line 93-100: The code mutates response.data.transactions by calling
sort and slice on the transactions array; instead create a shallow copy first
(e.g., using [...response.data.transactions] or Array.from) and perform the sort
and slice on that copy so response.data remains unchanged—update the block that
sets and processes the transactions variable (the reference to
response.data.transactions, the transactions.sort(...) call, and the
transactions = transactions.slice(...) line) to operate on the copied array.
In `@src/tools/user/get-user.ts`:
- Around line 17-24: The tool description currently promises richer account data
by listing "Show my account info" but the tool only returns the user ID; update
the description field (the description string in the get-user tool) to remove or
narrow that example so it only suggests ID-focused prompts (e.g., keep "Who am I
logged in as?" and "What's my YNAB user ID?" and remove or replace "Show my
account info"). Ensure the description text clearly states it returns the user's
ID so callers won't expect full account details.
- Around line 11-28: Update the empty Zod input schema to be strict: change the
z.object({}) referenced by inputSchema to z.object({}).strict() so unexpected
keys are rejected; ensure the getUserTool definition (getUserTool and its
inputSchema usage) uses this strict Zod schema or is consistent with it so the
tool enforces strict validation like the other no-argument tools.
♻️ Duplicate comments (8)
src/tools/analytics/net-worth.ts (1)
161-165: Exclude positive-balance debt accounts from liabilities.Debt accounts with positive balances (e.g., overpaid credit cards) are currently counted as liabilities, which overstates liabilities and breaks the summary consistency (assets + liabilities vs net worth). Filter to negative balances only.
🔧 Proposed fix
- const totalLiabilities = sumMilliunits( - debtAccounts.map((a) => Math.abs(a.balance)) - ); + const totalLiabilities = sumMilliunits( + debtAccounts.filter((a) => a.balance < 0).map((a) => Math.abs(a.balance)) + );src/tools/categories/update-category.ts (1)
23-23: Add.uuid()validation tocategory_idfor consistency with sibling tools.The
get-category.tsandget-month-category.tstools in this same module validatecategory_idwithz.string().uuid(). This tool should follow the same pattern for consistency and to provide clearer client-side validation errors rather than relying on YNAB API rejection.Proposed fix
- category_id: z.string().describe('The category UUID to update'), + category_id: z.string().uuid().describe('The category UUID to update'),src/tools/scheduled-transactions/list-scheduled.ts (1)
63-86: Good security posture - sanitization properly applied.The previous review concern is fully addressed:
- ✓
txn.subtransactions ?? []guards against null/undefined access- ✓ All user-controlled strings (
memo,payee_name,category_name,account_name, subtransactionmemo) are sanitized viasanitizeMemo/sanitizeNameInternal IDs (
id,payee_id,category_id) are appropriately exposed for transaction identification and cross-referencing purposes.src/tools/analytics/unused-categories.ts (1)
118-126: Deleted subtransactions still not filtered.The subtransaction loop correctly processes split transactions (addressing the prior review), but still doesn't skip deleted subtransactions. This was previously flagged and acknowledged as deferred, but remains a correctness gap where a deleted subtransaction will incorrectly mark its category as active.
src/tools/transactions/list-transactions.ts (1)
163-166: Consider sanitizing echoedsince_dateinput for defense-in-depth.The
filters_applied.since_date.inputechoes user-provided input. While Zod validates it as a string, sanitizing it before including in the response provides an extra layer of protection against any unexpected content propagating downstream.♻️ Optional improvement
+import { sanitizeString } from '../../utils/sanitize.js'; ... filters_applied: { since_date: validated.since_date - ? { input: validated.since_date, parsed: options.sinceDate } + ? { input: sanitizeString(validated.since_date, 50), parsed: options.sinceDate } : null,src/tools/analytics/quick-summary.ts (3)
73-87: Previous issues resolved — account categorization is now correct.The fixes from earlier reviews are properly applied:
budgetAccountsnow excludes bothdebtTypesandcreditTypes(line 78), preventing double-countingdebtAccountsincludes bothcreditTypesanddebtTypes(line 82), ensuring all debt types are captured
89-119: LGTM! Spending and alerts calculations are secure and correctly implemented.
- All user-visible names pass through
sanitizeName()for proper sanitization- All currency values pass through
formatCurrency()- Nullish coalescing
(c.goal_under_funded ?? 0)correctly handles undefined values- The redundant credit type check has been removed from
lowBalanceAccountsas per the earlier fix
157-165: Previous field naming issue resolved —debt_accountsnow correctly describes the content.The field was appropriately renamed from
credit_cardstodebt_accountssince it includes all debt types (mortgages, auto loans, student loans, etc.), not just credit cards.
Critical: - list-months.ts: Remove extra API call for currency format to preserve rate limit budget Major: - income-expense.ts: Handle zero-income periods properly (return N/A instead of misleading 0%) - reconciliation-helper.ts: Throw error if account_id not found, guard against null payee_name - list-budgets.ts: Sanitize currency_format fields before returning - get-payee.ts, list-payees.ts: Sanitize payee IDs and transfer_account_id - get-payee-location.ts: Sanitize location IDs Minor: - get-month-category.ts, update-category.ts: Fix month format description (YYYY-MM-01) - get-user.ts: Narrow example prompt to avoid overpromising - budget-suggestions.ts: Handle insufficient history gracefully with allSettled pattern Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
JSON Schema alignment: - get-account.ts: Add format: 'uuid' to account_id - list-category-transactions.ts: Add format: 'uuid' to category_id - savings-opportunities.ts: Add minimum/maximum constraints to months - spending-by-payee.ts: Add minimum/maximum constraints to months/limit Type safety & performance: - goal-progress.ts: Single-pass status counting (O(n) vs O(4n)) - spending-pace.ts: Tighten statusPriority to CategoryPace['status'] - list-payee-transactions.ts: Copy array before sorting to avoid mutation Hardening: - audit-log.ts: Add .strict() to input schema, document omitted client param - transaction-search.ts: Add max length constraints on query/payee/category - get-budget.ts: Use budget's currency_format for monetary fields Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- create-account.ts: Add .finite() to balance validation to reject NaN/Infinity - get-month.ts: Use sanitized group names as keys instead of internal IDs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI agents
In `@src/tools/analytics/budget-suggestions.ts`:
- Around line 41-54: The inputSchema currently exposes months as a generic
number; update inputSchema.properties.months to reflect the Zod constraints by
using type: 'integer' and adding minimum: 1 and maximum: 12 (matching the Zod
schema that enforces months as an integer between 1 and 12), and keep the
description in sync; ensure no change to required unless you intend to make
months mandatory.
In `@src/tools/analytics/goal-progress.ts`:
- Around line 15-24: Update the inputSchema's budget_id to validate UUIDs:
replace z.string().optional() with z.string().uuid().optional() (keeping
.describe) so Zod enforces the UUID format while still allowing omission for the
"last-used" fallback; additionally update the generated JSON Schema for
budget_id to include "format": "uuid" (or "pattern" matching UUID) so the
OpenAPI/JSON Schema rejects non-UUID strings, and run/update any tests that
assert schema shape or example values referencing budget_id.
In `@src/tools/analytics/income-expense.ts`:
- Around line 40-52: The JSON Schema in inputSchema is missing the integer and
bounds constraints for months that exist in the Zod validation; update the
months property in inputSchema to include type: 'integer' (or 'number' with
format/int constraint), minimum: 1 and maximum: 12 to match the Zod
.int().min(1).max(12) rules (ensure you update the months entry alongside
budget_id in the inputSchema definition so documentation/clients see the same
constraints).
In `@src/tools/analytics/reconciliation-helper.ts`:
- Around line 145-149: The calculation for daysPending uses txnDate/new
Date(txn.date) and can yield negative values for future-dated transactions;
clamp the result to a minimum of 0 (or set an explicit "future" flag) before
assigning days_pending. Update the code that computes daysPending (using txnDate
and daysPending) to max(0, computedDays) so future-dated uncleared entries do
not produce negative pending days and adjust any downstream usage of daysPending
accordingly.
- Around line 175-223: The summary field accounts_with_uncleared should be
derived from actual uncleared counts rather than accountResults.length (which
can include a requested account with zero uncleared). Change the computed value
used in the returned JSON to count entries with uncleared_count > 0 (e.g., use
accountResults.filter(a => a.uncleared_count > 0).length or compute from
accountData.values()) so accounts_with_uncleared accurately reflects only
accounts that have uncleared transactions; update the reference in the return
object where summary.accounts_with_uncleared is set.
- Around line 14-51: The input validation currently accepts any string and extra
keys, causing malformed IDs and extra properties to slip through; update the Zod
schema (inputSchema) to use .strict() and validate budget_id and account_id as
UUIDs (e.g., z.string().uuid().optional()) so invalid IDs are rejected, and then
update the reconciliationHelperTool.inputSchema JSON schema to reflect the UUID
format and disallow additionalProperties so both schemas remain in sync.
In `@src/tools/analytics/savings-opportunities.ts`:
- Around line 244-266: Skip aggregated "unknown" payee buckets before treating
them as recurring subscriptions: in the loop over data.byPayee, bail out early
(continue) when payeeId === 'unknown' (or any sentinel used for missing payee
IDs) so we don't generate false recurring-expense hits; update the logic around
data.byPayee and the subsequent transactions.find/payeeName resolution
(references: data.byPayee loop, payeeId, transactions.find, sanitizeName) to
only perform the average/similarity checks and lookup for real payee IDs.
In `@src/tools/analytics/spending-by-payee.ts`:
- Around line 183-186: The code currently calls data.dates.sort() which mutates
the original array; change this to sort a copy instead so the internal
data.dates is not mutated—create a shallow copy of data.dates (e.g., via slice()
or spread) and call sort() on that, then assign to sortedDates (the variable
shown) and keep firstDate/lastDate logic the same; this mirrors the non-mutating
pattern used elsewhere (see list-payee-transactions handling).
In `@src/tools/analytics/spending-pace.ts`:
- Around line 179-185: The statusCounts object is built by filtering
categoryPaces four times; refactor to a single-pass accumulator over
categoryPaces to improve efficiency: iterate once (e.g., with forEach or reduce)
and increment counters for 'overspent', 'behind', 'on_track', and 'ahead' into
the statusCounts object. Update the code that currently defines statusCounts to
initialize counters and populate them in that single pass, keeping the same keys
and using the existing categoryPaces variable name.
In `@src/tools/analytics/transaction-search.ts`:
- Around line 27-34: The min_amount and max_amount zod schemas in
transaction-search.ts currently allow Infinity/NaN; update the schema
definitions for min_amount and max_amount to use z.number().finite().optional()
(i.e., add .finite() to the existing chains) so the values are validated as
finite numbers before later multiplication and Math.abs comparisons (used where
amounts are multiplied by 1000 and compared).
- Around line 70-116: The JSON Schema in inputSchema is missing the string
length and numeric bounds present in the Zod validation; update the properties
for query, payee, and category to include maxLength (query: 500, payee: 200,
category: 200) and add min and max for limit (min: 1, max: 100) so the exposed
schema matches the Zod rules used at runtime; keep the existing types,
descriptions and the type/enum for type unchanged but ensure these constraints
are added under the corresponding properties in inputSchema.
In `@src/tools/budgets/get-budget.ts`:
- Around line 60-64: The fmt helper currently passes user-controlled
budget.currency_format directly into formatCurrencyWithFormat; sanitize the
currency_format first (reuse the same sanitizer used in list-budgets.ts, e.g.,
sanitizeCurrencyFormat or equivalent) and pass the sanitized value to
formatCurrencyWithFormat instead of the raw budget.currency_format so
separators/symbols are validated before formatting.
In `@src/tools/categories/get-month-category.ts`:
- Around line 15-18: Tighten the budget_id Zod schema in get-month-category.ts:
replace the permissive z.string().optional() for the budget_id field with a
schema that only accepts a UUID or the explicit token "last-used" (e.g.,
z.union([z.string().uuid(), z.literal('last-used')]).optional()), so callers
either pass a valid UUID or the supported token; update the describe text if
needed to reflect the stricter validation.
In `@src/tools/months/get-month.ts`:
- Around line 87-126: The current logic uses sanitized group names as object
keys via categoriesByGroup which risks name collisions and unsafe keys like
"__proto__"; change to group-by-ID internally (e.g., use a Record<string, {
group_name: string; categories: Array<...> }> or a Map keyed by
category_group_id) while still populating each entry's group_name with
sanitizeName(groupNameLookup.get(category.category_group_id) ?? 'Other') and
pushing formatted category entries (sanitizeName(category.name),
formatCurrency(...)) into the categories array; finally, convert that internal
map into an array for categories_by_group in the returned JSON where each
element is { group_name, categories } so IDs remain internal and you no longer
use user-derived strings as object keys (update the categoriesByGroup
declaration, the for-loop that builds it, and the return to output an array).
In `@src/tools/system/audit-log.ts`:
- Around line 64-67: The JSON Schema for the query parameter "limit" in
src/tools/system/audit-log.ts does not include the same constraints as the Zod
schema (which enforces integer, minimum 1, maximum 100); update the JSON Schema
entry for limit to add "type: 'integer'" and include "minimum: 1" and "maximum:
100" (preserving the existing description/default) so docs match the Zod
validation used by the code that defines the limit parameter.
In `@src/tools/transactions/list-payee-transactions.ts`:
- Around line 47-69: The JSON inputSchema's payee_id property lacks the UUID
format hint even though the runtime Zod schema enforces .uuid(); update the
inputSchema object to add the JSON Schema "format": "uuid" for the payee_id
property so clients and tools can validate it earlier (modify the inputSchema ->
properties -> payee_id entry to include format:"uuid" while leaving other fields
like since_date and limit unchanged).
♻️ Duplicate comments (8)
src/tools/user/get-user.ts (1)
11-28: Add strict validation for empty input schema (repeat).Same concern as earlier: Zod’s default object behavior strips unknown keys, and the JSON schema omits
additionalProperties: false. Consider.strict()plusadditionalProperties: falseto explicitly reject unexpected inputs.src/tools/system/audit-log.ts (1)
120-136: Consider sanitizing thedetailsfield at read time for defense-in-depth.Entries are returned directly from
auditLog.getFiltered(). While error messages are sanitized at write time, thedetailsfield relies on caller compliance with the security contract. For defense-in-depth, consider sanitizing thedetailsfield before serialization to prevent future callers from inadvertently leaking sensitive data.src/tools/analytics/spending-by-payee.ts (1)
73-76: Add minimum constraint to min_transactions in JSON schema.The Zod schema enforces
.min(1)formin_transactions, but the JSON inputSchema doesn't expose this constraint. This inconsistency was noted in a previous review for other fields that have since been corrected.♻️ Suggested fix for consistency
min_transactions: { type: 'number', - description: 'Minimum transactions to include payee (default 1)', + description: 'Minimum transactions to include payee (default 1)', + minimum: 1, },src/tools/transactions/list-category-transactions.ts (1)
47-70: Align tool JSON schema with Zod constraints forlimit.
The tool schema still allows any number, while Zod requires an integer in the 1–500 range with a default. This mismatch can trigger avoidable validation failures.♻️ Suggested alignment
limit: { - type: 'number', + type: 'integer', + minimum: 1, + maximum: 500, + default: 100, description: 'Maximum number of transactions to return (default 100, max 500)', },src/tools/payees/get-payee.ts (1)
38-41: JSON schema missingformat: 'uuid'for payee_id.The Zod schema correctly validates UUIDs (line 18), but the JSON
inputSchemametadata doesn't reflect this. For consistency and better API documentation, add the format specifier:♻️ Proposed fix
payee_id: { type: 'string', + format: 'uuid', description: 'The payee UUID to retrieve', },src/tools/categories/update-category.ts (1)
19-27: Enforce UUID validation forcategory_id.
category_idis described as a UUID but accepts any string (Line 23), allowing invalid values to reach the API. Tighten the schema with.uuid().🔧 Proposed fix
- category_id: z.string().describe('The category UUID to update'), + category_id: z.string().uuid().describe('The category UUID to update'),Zod v3.23.8 z.string().uuid() validation behaviorAs per coding guidelines, validate inputs with Zod.
src/tools/budgets/list-budgets.ts (1)
79-90: Use sanitized currency format when formatting account balances.Line 81 assigns
budget.currency_format(the raw, unsanitized version) tocurrencyFormat, but you've already createdsanitizedCurrencyFormatabove specifically for this purpose. This inconsistency means user-controlled strings (separators, currency symbols) pass throughformatCurrencyWithFormatunsanitized.🔒 Proposed fix
if (budget.accounts != null && budget.accounts.length > 0) { // Use budget's currency format if available, otherwise fall back to default formatting - const currencyFormat = budget.currency_format; + const currencyFormat = sanitizedCurrencyFormat; budgetInfo['accounts'] = budget.accounts.map((acc) => ({ name: sanitizeName(acc.name), type: acc.type, balance: currencyFormat ? formatCurrencyWithFormat(acc.balance, currencyFormat) : formatCurrency(acc.balance), closed: acc.closed, })); }src/tools/accounts/get-account.ts (1)
67-80: Removetransfer_payee_idfrom the response payload.
This internal YNAB identifier isn’t required by the tool contract and unnecessarily exposes internal IDs. As per coding guidelines, avoid exposing internal IDs unless needed.🔧 Suggested fix
cleared_balance: formatCurrency(account.cleared_balance), uncleared_balance: formatCurrency(account.uncleared_balance), - transfer_payee_id: account.transfer_payee_id, direct_import_linked: account.direct_import_linked,
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- get-budget.ts: Sanitize currency_format before use - reconciliation-helper.ts: Clamp daysPending to 0, fix uncleared count - savings-opportunities.ts: Skip unknown payees in recurring expenses - transaction-search.ts: Add .finite() to amount validations - get-month.ts: Return categories_by_group as array to avoid dynamic keys - budget-suggestions.ts: Add integer/min/max to JSON schema - budget-vs-actuals.ts: Add pattern to month JSON schema - get-month-category.ts: Add pattern and uuid format to JSON schema - monthly-comparison.ts: Add income change_percent, fix dead code Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/tools/analytics/budget-suggestions.ts`:
- Around line 101-118: The historical month results lose their corresponding
month indexes causing analysis_period to misalign when some month fetches fail;
when creating the promises for client.getBudgetMonth(map), include the original
month with each settled result (e.g., resolve to { status: 'fulfilled', value:
response, month: m } or { status: 'rejected', reason: error, month: m }), then
change the type guard on historicalMonthResults to narrow to the fulfilled shape
and map to an array of { month, value } (historicalMonthResponses). Use that
resulting months array (the months extracted from fulfilled entries) wherever
analysis_period or months.slice(...) is computed so the returned months align
exactly with the successful data; apply the same change to the other occurrence
referenced around the later block (lines ~265-269) to keep consistency.
In `@src/tools/analytics/monthly-comparison.ts`:
- Around line 35-44: The inputSchema's budget_id property lacks a UUID format
constraint; update the schema for inputSchema -> properties -> budget_id to
include "format": "uuid" (and optionally add a pattern if stricter validation is
desired) so the validator enforces UUIDs for budget_id used by the
monthly-comparison tool. Ensure you modify the inputSchema object where
budget_id is defined to add the format key without changing other property
names.
In `@src/tools/budgets/get-budget.ts`:
- Around line 55-58: The delta sync never persists updated server knowledge:
after calling client.getBudgetById(budgetId, lastKnowledge) and reading
response.data (budget and server_knowledge), call
client.updateServerKnowledge(budgetId, response.data.server_knowledge) to store
the new knowledge before returning; locate the usage around getServerKnowledge,
getBudgetById, response.data.budget and ensure updateServerKnowledge is invoked
with the returned server_knowledge.
♻️ Duplicate comments (8)
src/tools/analytics/budget-vs-actuals.ts (1)
20-24: Tighten month validation to reject invalid months (00/13).Line 22 and Line 53 allow invalid months, which can trigger bad API calls and wrong previous-month calculations. Use a stricter regex that only permits 01–12 in both the Zod schema and the tool JSON schema.
🔧 Proposed fix
- month: z - .string() - .regex(/^\d{4}-\d{2}-01$/, 'Month must be first-of-month format (YYYY-MM-01)') + month: z + .string() + .regex(/^\d{4}-(0[1-9]|1[0-2])-01$/, 'Month must be first-of-month format (YYYY-MM-01)')- month: { - type: 'string', - pattern: '^\\d{4}-\\d{2}-01$', + month: { + type: 'string', + pattern: '^\\d{4}-(0[1-9]|1[0-2])-01$', description: 'Month to analyze in YYYY-MM-01 format (first of month). Defaults to current month', },Also applies to: 51-54
src/tools/categories/get-month-category.ts (1)
15-18: Tightenbudget_idvalidation to UUID or explicit token.The
budget_idfield still accepts any string, which weakens input validation. Per the coding guidelines requiring Zod validation before use, this should be restricted to valid UUIDs or the explicit"last-used"token to catch invalid inputs early rather than letting them propagate to the API.🔧 Proposed fix
- budget_id: z - .string() - .optional() - .describe('Budget UUID. Defaults to YNAB_BUDGET_ID env var or "last-used"'), + budget_id: z + .union([z.string().uuid(), z.literal('last-used')]) + .optional() + .describe('Budget UUID. Defaults to YNAB_BUDGET_ID env var or "last-used"'),Also update the JSON Schema to match:
budget_id: { type: 'string', + pattern: '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$|^last-used$', description: 'Budget UUID. Defaults to YNAB_BUDGET_ID env var or "last-used"', },src/tools/months/get-month.ts (1)
87-111: Using sanitized names as object keys still carries prototype pollution risk.While the code now groups by name instead of ID (good!), using user-derived strings as object keys in a plain
Record<string, ...>can still cause issues:
- Prototype pollution: A group named
"__proto__"or"constructor"could cause unexpected behavior- Name collisions: Two groups with identical sanitized names silently merge
The past review suggested using a
Mapkeyed by group ID internally, then converting to an array—that approach avoids both issues.🔧 Recommended fix using Map for safe grouping
- // Group categories by category group name (not ID) - const categoriesByGroup: Record< - string, - Array<{ - name: string; - budgeted: string; - activity: string; - balance: string; - }> - > = {}; + // Group categories by ID internally (avoids key collisions and prototype pollution) + const categoriesByGroup = new Map< + string, + { + group_name: string; + categories: Array<{ + name: string; + budgeted: string; + activity: string; + balance: string; + }>; + } + >(); for (const category of month.categories) { if (category.hidden) continue; - const groupName = sanitizeName(groupNameLookup.get(category.category_group_id) ?? 'Other'); - if (categoriesByGroup[groupName] === undefined) { - categoriesByGroup[groupName] = []; - } - categoriesByGroup[groupName].push({ + const groupId = category.category_group_id; + let group = categoriesByGroup.get(groupId); + if (group === undefined) { + group = { + group_name: sanitizeName(groupNameLookup.get(groupId) ?? 'Other'), + categories: [], + }; + categoriesByGroup.set(groupId, group); + } + group.categories.push({ name: sanitizeName(category.name), budgeted: formatCurrency(category.budgeted), activity: formatCurrency(category.activity), balance: formatCurrency(category.balance), }); }And update the return statement:
- categories_by_group: Object.entries(categoriesByGroup).map(([groupName, categories]) => ({ - group_name: groupName, - categories: categories, - })), + categories_by_group: Array.from(categoriesByGroup.values()),src/tools/analytics/reconciliation-helper.ts (1)
14-52: Consider tightening input validation with UUID format and strict mode.The previous review suggested adding
.uuid()validation tobudget_idandaccount_idfields, plus.strict()to reject unexpected properties. The JSON schema could also benefit fromadditionalProperties: falseandformat: 'uuid'. This prevents malformed IDs from reaching the YNAB API.As per coding guidelines, all user inputs should be rigorously validated before use.
src/tools/analytics/transaction-search.ts (2)
72-119: JSON Schema still missing constraints that Zod enforces.This was flagged in a previous review. The JSON Schema (used for API documentation/MCP tooling) still lacks the
maxLength,minimum, andmaximumconstraints that the Zod schema enforces at runtime.
241-250: Category fallback behavior still returns 'Unknown' for orphaned IDs.As noted in the previous review: when
t.category_idexists but isn't found in the lookup,sanitizeName('')returns'Unknown'(persanitize.tsbehavior), then'Unknown' || nullevaluates to'Unknown'—notnull.This differentiates "no category assigned" (
null) from "category deleted/unknown" ('Unknown'), which may be intentional, but consumers might expectnullfor both cases.src/tools/analytics/monthly-comparison.ts (1)
90-122: Use month-levelincomefield instead of calculating from filtered categories.This concern was raised in a previous review and remains unresolved. The code calculates income by summing positive
cat.activitywhile filtering out "Internal Master Category". However, YNAB'sMonthDetailResponseprovides amonth.incomefield directly.The current approach may undercount income because "Inflow: Ready to Assign" (which captures most income) is part of Internal Master Category and gets filtered out at line 97/114.
🔧 Suggested fix
- // Calculate overall metrics for current month - let currentIncome = 0; let currentSpending = 0; let currentBudgeted = 0; + const currentIncome = currentData.income; for (const cat of currentData.categories) { const groupName = groupLookup.get(cat.id) ?? 'Other'; if (groupName === 'Internal Master Category' || cat.hidden) continue; currentBudgeted += cat.budgeted; if (cat.activity < 0) { currentSpending += Math.abs(cat.activity); - } else if (cat.activity > 0) { - currentIncome += cat.activity; } } - // Calculate overall metrics for previous month - let previousIncome = 0; let previousSpending = 0; let previousBudgeted = 0; + const previousIncome = previousData.income; for (const cat of previousData.categories) { const groupName = groupLookup.get(cat.id) ?? 'Other'; if (groupName === 'Internal Master Category' || cat.hidden) continue; previousBudgeted += cat.budgeted; if (cat.activity < 0) { previousSpending += Math.abs(cat.activity); - } else if (cat.activity > 0) { - previousIncome += cat.activity; } }src/tools/budgets/get-budget.ts (1)
62-81: Currency format strings use type checks but not content sanitization.The
currency_symbol,decimal_separator,group_separator, etc. are user-controlled strings. While type checking prevents crashes from wrong types, it doesn't sanitize malicious content that could exist in correctly-typed strings. Per coding guidelines, user-provided strings should be sanitized before inclusion in responses.Consider using
sanitizeString(already imported indirectly viasanitizeName) for string fields:🔒 Proposed fix
+import { sanitizeName, sanitizeString } from '../../utils/sanitize.js'; -import { sanitizeName } from '../../utils/sanitize.js';const sanitizedCurrencyFormat = budget.currency_format ? { iso_code: typeof budget.currency_format.iso_code === 'string' - ? budget.currency_format.iso_code : 'USD', + ? (sanitizeString(budget.currency_format.iso_code) ?? 'USD') : 'USD', example_format: typeof budget.currency_format.example_format === 'string' - ? budget.currency_format.example_format : '$1,234.56', + ? (sanitizeString(budget.currency_format.example_format) ?? '$1,234.56') : '$1,234.56', decimal_digits: typeof budget.currency_format.decimal_digits === 'number' ? budget.currency_format.decimal_digits : 2, decimal_separator: typeof budget.currency_format.decimal_separator === 'string' - ? budget.currency_format.decimal_separator : '.', + ? (sanitizeString(budget.currency_format.decimal_separator) ?? '.') : '.', symbol_first: typeof budget.currency_format.symbol_first === 'boolean' ? budget.currency_format.symbol_first : true, group_separator: typeof budget.currency_format.group_separator === 'string' - ? budget.currency_format.group_separator : ',', + ? (sanitizeString(budget.currency_format.group_separator) ?? ',') : ',', currency_symbol: typeof budget.currency_format.currency_symbol === 'string' - ? budget.currency_format.currency_symbol : '$', + ? (sanitizeString(budget.currency_format.currency_symbol) ?? '$') : '$', display_symbol: typeof budget.currency_format.display_symbol === 'boolean' ? budget.currency_format.display_symbol : true, } : null;
- JSON Schema alignment: add integer types, min/max, format constraints - Security: remove transfer_payee_id, gate transaction_ids behind flag - Code quality: avoid mutations, single-pass counting, strict schemas - Logic: fix month regex, use month.income, persist server_knowledge - Data: refactor get-month to array structure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@src/tools/analytics/budget-suggestions.ts`:
- Around line 122-141: The early-return for no historical data (when
historicalMonthResponses.length === 0) should keep the analysis_period shape
consistent by including an empty months array; update the returned object from
the block that references analysis_period to add months: [] alongside
months_requested and months_available (leave months_requested and
months_available as-is), so clients always receive analysis_period.months even
when no history is present.
In `@src/tools/analytics/budget-vs-actuals.ts`:
- Around line 51-55: The JSON schema for the month parameter (the month object
in src/tools/analytics/budget-vs-actuals.ts) currently allows invalid months
like 00 and 13; update its pattern to exactly match the Zod month validation by
restricting the month portion to 01–12 (i.e., four-digit year, a hyphen, then a
two-digit month between 01 and 12, then -01), so schema and the Zod validator
are consistent.
In `@src/tools/analytics/reconciliation-helper.ts`:
- Around line 155-163: The category field construction in the
UnclearedTransaction object mixes semantics: calling sanitizeName('') yields
"Unknown" when category_id exists but lookup misses, while absence of
category_id yields null. Change the logic in the UnclearedTransaction creation
(the category assignment using categoryLookup, category_id, and sanitizeName) so
that if category_id is present but not found in categoryLookup the category is
set to null (consistent with no category_id), otherwise set to
sanitizeName(foundName); update any affected tests to reflect the normalized
null behavior.
In `@src/tools/analytics/spending-pace.ts`:
- Around line 104-113: The code currently treats only negative cat.activity as
spending and zeroes out positive refunds; change to use net activity so refunds
reduce spent: set spent to the net outflow (e.g., spent = -cat.activity so
negative activity becomes positive spent and positive activity reduces/negates
spent), keep remaining = budgeted - spent, and leave the skip check (if budgeted
=== 0 && spent === 0) and the totalBudgeted/totalSpent accumulation but ensure
they now use the new spent value; update references in this block that mention
cat.activity, spent, remaining, totalBudgeted, and totalSpent in
spending-pace.ts.
In `@src/tools/system/audit-log.ts`:
- Around line 47-76: The JSON Schema object named inputSchema currently permits
unknown fields but the code uses zod .strict() semantics; update inputSchema by
adding additionalProperties: false at the root of the schema object so unknown
properties are rejected (matching the strict Zod validation) and ensure this
change applies to the same inputSchema that defines operation, resource_type,
success, limit, and summary_only.
In `@src/tools/transactions/import-transactions.ts`:
- Around line 60-84: The handler accesses response.data.transaction_ids directly
in handleImportTransactions which can throw if the API changes; change the code
to defensively fall back to an empty array (e.g., derive importedIds from
response?.data?.transaction_ids ?? []) before using .length or spreading into
transaction_ids (keep validated.include_ids behavior), and ensure imported_count
and the success message use that safe importedIds variable; this protects
client.importTransactions consumers from a missing/undefined transaction_ids
field.
In `@src/tools/transactions/list-category-transactions.ts`:
- Around line 96-102: The code currently assigns transactions by reference
(transactions = response.data.transactions) and then mutates it with sort/slice;
avoid mutating the API response by creating a shallow copy first (e.g., copy
response.data.transactions into a new array) and perform the sort and slice on
that copy so response.data.transactions remains unchanged; update the
transactions variable handling in list-category-transactions (the transactions
and response.data.transactions references around the sort/slice/limit logic)
accordingly.
In `@src/tools/transactions/list-payee-transactions.ts`:
- Around line 64-67: The JSON inputSchema's "limit" field doesn't reflect the
Zod validation (.int().positive().max(500)); update the "limit" entry in the
input schema used by listPayeeTransactions (or the inputSchema object) to use
type: "integer" and add minimum: 1 and maximum: 500 (keep the description), so
client-side/schema validation matches the Zod rules.
♻️ Duplicate comments (6)
src/tools/budgets/get-budget.ts (1)
65-91: Sanitizecurrency_formatstring fields before formatting output.Type checks alone don’t neutralize unsafe content in
currency_symbol, separators, orexample_format. These strings are user-controlled via YNAB and end up in responses throughformatCurrencyWithFormat. Please sanitize them first.🔧 Proposed fix
-import { sanitizeName } from '../../utils/sanitize.js'; +import { sanitizeName, sanitizeString } from '../../utils/sanitize.js'; @@ - const sanitizedCurrencyFormat = budget.currency_format - ? { - iso_code: typeof budget.currency_format.iso_code === 'string' - ? budget.currency_format.iso_code : 'USD', - example_format: typeof budget.currency_format.example_format === 'string' - ? budget.currency_format.example_format : '$1,234.56', - decimal_digits: typeof budget.currency_format.decimal_digits === 'number' - ? budget.currency_format.decimal_digits : 2, - decimal_separator: typeof budget.currency_format.decimal_separator === 'string' - ? budget.currency_format.decimal_separator : '.', - symbol_first: typeof budget.currency_format.symbol_first === 'boolean' - ? budget.currency_format.symbol_first : true, - group_separator: typeof budget.currency_format.group_separator === 'string' - ? budget.currency_format.group_separator : ',', - currency_symbol: typeof budget.currency_format.currency_symbol === 'string' - ? budget.currency_format.currency_symbol : '$', - display_symbol: typeof budget.currency_format.display_symbol === 'boolean' - ? budget.currency_format.display_symbol : true, - } - : null; + const sanitizedCurrencyFormat = budget.currency_format + ? { + iso_code: sanitizeString(budget.currency_format.iso_code) ?? 'USD', + example_format: sanitizeString(budget.currency_format.example_format) ?? '$1,234.56', + decimal_digits: typeof budget.currency_format.decimal_digits === 'number' + ? budget.currency_format.decimal_digits : 2, + decimal_separator: sanitizeString(budget.currency_format.decimal_separator) ?? '.', + symbol_first: typeof budget.currency_format.symbol_first === 'boolean' + ? budget.currency_format.symbol_first : true, + group_separator: sanitizeString(budget.currency_format.group_separator) ?? ',', + currency_symbol: sanitizeString(budget.currency_format.currency_symbol) ?? '$', + display_symbol: typeof budget.currency_format.display_symbol === 'boolean' + ? budget.currency_format.display_symbol : true, + } + : null;As per coding guidelines, sanitize user-provided strings before returning.
src/tools/categories/get-month-category.ts (1)
15-18: Consider tighteningbudget_idvalidation to accept only UUID or explicit tokens.The current schema accepts any string for
budget_id, which means invalid values will only fail at the API layer rather than during input validation. This was flagged in a prior review cycle.The description mentions "UUID or 'last-used'" but the schema doesn't enforce this constraint.
🔧 Proposed fix
budget_id: z - .string() + .union([z.string().uuid(), z.literal('last-used')]) .optional() .describe('Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"'),And update the JSON schema accordingly:
budget_id: { type: 'string', + oneOf: [ + { format: 'uuid' }, + { const: 'last-used' } + ], description: 'Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"', },src/tools/months/get-month.ts (1)
105-113: Internalgroup_idis still exposed in the response.The past review comments asked to keep internal IDs private in the output. While the switch to
Mapaddressed the prototype pollution / key collision concerns,group_idis still included in the final JSON response (line 108). Per coding guidelines: "Don't expose internal IDs or paths unnecessarily."Since
group_nameis already present and sufficient for display and user reference, consider removinggroup_idfrom the output:Proposed fix
let groupEntry = groupMap.get(groupId); if (groupEntry === undefined) { groupEntry = { - group_id: groupId, group_name: groupName, categories: [], }; groupMap.set(groupId, groupEntry); }And update the type on line 96:
- const groupMap = new Map<string, { group_id: string; group_name: string; categories: CategoryInfo[] }>(); + const groupMap = new Map<string, { group_name: string; categories: CategoryInfo[] }>();src/tools/analytics/monthly-comparison.ts (1)
14-44: Tightenbudget_idvalidation (UUID or "last-used") in both schemas.
Right now any string is accepted, which can defer invalid IDs to runtime API errors. Consider restricting to UUID or the literal"last-used"in both Zod and the Tool JSON schema.🔧 Proposed fix
-const inputSchema = z.object({ - budget_id: z - .string() - .optional() - .describe('Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"'), -}); +const inputSchema = z.object({ + budget_id: z + .union([z.string().uuid(), z.literal('last-used')]) + .optional() + .describe('Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"'), +});inputSchema: { type: 'object', properties: { budget_id: { - type: 'string', + anyOf: [ + { type: 'string', const: 'last-used' }, + { type: 'string', format: 'uuid' }, + ], description: 'Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"', }, }, required: [], },As per coding guidelines, inputs should be validated as tightly as possible.
In Zod 3.23.8, what is the recommended way to validate a field that accepts either a UUID string or the literal "last-used"? Does `z.string().uuid()` exist and how should it be combined with `z.literal()`?src/tools/system/audit-log.ts (1)
118-135: Redact audit entries before returning them.Raw
entriesare returned as-is; if any caller accidentally logs PII (memo, payee name, tokens), this tool will expose it. Add a defensive redaction/omit step at read time. At minimum, dropdetailsfrom the response unless explicitly needed.🔒 Suggested mitigation
- const entries = auditLog.getFiltered(filterOptions); + const entries = auditLog + .getFiltered(filterOptions) + .map(({ details, ...rest }) => rest);As per coding guidelines, sanitize user-provided strings before including them in responses and avoid exposing internal identifiers unnecessarily.
src/tools/analytics/goal-progress.ts (1)
15-24: Tightenbudget_idvalidation while preserving thelast-usedsentinel.Right now any string is accepted, which can pass malformed IDs to the API. Recommend validating UUIDs while still allowing the
"last-used"sentinel. This also keeps the JSON schema consistent with the Zod schema.🔧 Proposed fix
const inputSchema = z.object({ - budget_id: z - .string() - .optional() - .describe('Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"'), + budget_id: z + .union([z.string().uuid(), z.literal('last-used')]) + .optional() + .describe('Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"'), filter: z .enum(['all', 'on_track', 'behind', 'underfunded', 'complete']) .optional() .describe('Filter goals by status (default: all)'), }); @@ properties: { budget_id: { - type: 'string', - description: 'Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"', + anyOf: [ + { type: 'string', format: 'uuid' }, + { type: 'string', enum: ['last-used'] }, + ], + description: 'Budget UUID or "last-used". Defaults to YNAB_BUDGET_ID env var or "last-used"', },Also applies to: 41-49
auzroz
commented
Jan 26, 2026
- budget-suggestions.ts: Add months: [] for consistent response shape - budget-vs-actuals.ts: Align JSON schema month pattern with Zod regex - reconciliation-helper.ts: Fix category null semantics consistency - audit-log.ts: Add additionalProperties: false to JSON schema - import-transactions.ts: Add defensive null checking for transaction_ids - list-category-transactions.ts: Avoid in-place mutation of response - list-payee-transactions.ts: Update limit constraints and avoid mutation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/tools/analytics/reconciliation-helper.ts`:
- Around line 89-93: The code uses a hardcoded 90-day lookback (created via
sinceDate, sinceDateStr) which can hide older uncleared transactions; update the
reconciliation logic (where sinceDate/sinceDateStr are used) to either accept a
configurable lookback parameter or include the lookback value in the output
payload (e.g., add a field like lookback_days or since_date to the
reconciliation response) so callers can see the window used for "uncleared"
checks; ensure any callers or tests that construct sinceDate are updated to
pass/configure this value or to assert the new response field.
In `@src/tools/system/audit-log.ts`:
- Around line 91-103: The summary_only branch returns auditLog.getSummary() with
camelCase keys while the full response uses snake_case; update the
validated.summary_only handling (where auditLog.getSummary() is used) to
map/normalize the summary object to the same snake_case schema as the full
response (e.g., total_operations, success_count, failure_count) and update any
message access to use those snake_case fields so both summary-only and full
responses share identical key names.
♻️ Duplicate comments (1)
src/tools/transactions/list-payee-transactions.ts (1)
106-110: Sanitize category before grouping to avoid duplicate labels.
If two raw category names sanitize to the same value, you can end up with duplicate category entries intop_categoriesand split totals. Consider sanitizing before aggregating.🔧 Suggested fix
- for (const txn of outflows) { - const category = txn.category_name ?? 'Uncategorized'; + for (const txn of outflows) { + const category = sanitizeName(txn.category_name ?? 'Uncategorized'); byCategory[category] = (byCategory[category] ?? 0) + Math.abs(txn.amount); }
- spending-pace.ts: Use net activity so refunds reduce spent (matches YNAB) - ynab-client.ts: Document budget_id validation design decision - audit-log.ts: Document details field sanitization design decision Design decisions documented: - budget_id accepts "last-used" sentinel, so UUID validation intentionally omitted - audit-log details field relies on caller contract (documented, all callers compliant) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- reconciliation-helper.ts: Disclose 90-day lookback in response summary - audit-log.ts: Normalize summary_only response to snake_case for consistency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/services/audit-log.ts`:
- Around line 139-141: The current check in the function that applies pagination
uses "if (options.limit)" which treats 0 as false and prevents callers from
requesting an explicit zero-length result; update the conditional around
applying the slice on the "filtered" array to check for undefined instead (e.g.,
"options.limit !== undefined" or "typeof options.limit !== 'undefined'") so that
a provided limit of 0 returns an empty list; keep the slice call on
"filtered.slice(0, options.limit)" and only change the conditional, referencing
the "filtered" variable and "options.limit" in this function in
src/services/audit-log.ts.
In `@src/services/ynab-client.ts`:
- Around line 160-168: The createAccount method invalidates only the accounts
cache but not the budgets-with-accounts cache, so stale data can remain for
callers of getBudgets(true); update createAccount (and similarly any
account-mutating methods) to also delete the `budgets:true` cache key in
addition to `accounts:${budgetId}` by calling this.cache.delete('budgets:true')
after acquiring the rate limiter and before returning, referencing the
createAccount function and cache.delete invocation to locate where to add the
extra cache eviction.
In `@src/tools/analytics/spending-pace.ts`:
- Around line 197-205: The percent_spent field in the overall object currently
returns '0%' when totalBudgeted is 0 which can mislead; update the calculation
in src/tools/analytics/spending-pace.ts (the overall object where percent_spent
is computed) to return a neutral indicator like 'N/A' (or '—') when
totalBudgeted === 0 instead of '0%', otherwise compute `${Math.round((totalSpent
/ totalBudgeted) * 100)}%`; keep references to totalBudgeted and totalSpent and
ensure formatting remains a string.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/tools/system/audit-log.ts`:
- Around line 47-77: The JSON schema for inputSchema is missing the documented
runtime default for the limit property; update the limit property in inputSchema
to include the runtime default (e.g., add "default: 20" to the limit schema) or,
at minimum, update its description to state "default 20" so it matches the
runtime behavior where validated.limit ?? 20 is used; change the limit entry
under inputSchema -> properties -> limit accordingly.
- Around line 81-123: handleAuditLog currently ignores provided filters when
validated.summary_only is true; update it to honor filters by constructing the
same filterOptions (using validated.operation, validated.resource_type,
validated.success) and pass those options into auditLog.getSummary(...) instead
of calling getSummary() with no args, then map the returned summary fields to
snake_case as before; ensure limit is not required for summary (omit or ignore
limit) and keep the existing message formatting.
- audit-log.ts (service): Allow explicit limit: 0 to return empty list - audit-log.ts (tool): Fix summary_only to respect filters, add default to schema - ynab-client.ts: Fix cache invalidation keys for budgets cache - spending-pace.ts: Handle percent_spent when totalBudgeted is zero Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/services/ynab-client.ts`:
- Around line 168-170: In the createAccount method in
src/services/ynab-client.ts, remove the unnecessary cache invalidation call
this.cache.delete('budgets:false') since getBudgets(false) does not include
account data and account creation cannot affect that cached response; keep the
existing invalidations for `accounts:${budgetId}` and `budgets:true` intact.
getBudgets(false) doesn't include account data, so there's no need to invalidate that cache entry when an account is created. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
npm test)npm run typecheck)npm run lint)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Infrastructure
Configuration
Tests & Docs
✏️ Tip: You can customize this high-level summary in your review settings.