fix: YNAB API 1.86 parity (Milestone 0) + eval dry-run intent capture - #18
fix: YNAB API 1.86 parity (Milestone 0) + eval dry-run intent capture#18klauern wants to merge 11 commits into
Conversation
…ark report Implements g57.8 (simulated mutation-intent capture) and completes the reporting half of g57.5/g57.6: - server: MCP_YNAB_EVAL_DRY_RUN_INTENTS_PATH opt-in intercepts every mutating handler (except the Code Mode dispatcher) and persists the validated payload to intended_writes.json instead of dispatching. No env var => production behavior unchanged. - dual runner: per-surface intent artifacts seeded before each run, grading.json written for every task/config after runs finish. - grading: deterministic assertions (completion, write-safety, expected read ops, task-specific text expectations, intended-write payload shape). - benchmark: viewer-compatible benchmark.json + token-delta-first benchmark.md at the iteration root. - evals.json: adds intent_expectation + text expectations to all tasks. - README documents the dry-run recorder and benchmark workflow.
Raise the dependency floor to ynab>=4.3.0,<5 and refresh uv.lock. Pins OpenAPI 1.86 contracts with focused tests: - until_date exists on all five transaction-list methods - goal_frequency is modeled on NewCategory (monthly/weekly/yearly) - update_transactions decodes an HTTP 200 SaveTransactionsResponse (4.1.0 mapped bulk-write success to 209, which the live API no longer returns) - get_transactions_by_category is owned by TransactionsApi, and the permissive CategoriesApi mock route is removed
…cp-ynab-parity-account-types) The account formatter iterated only 8 of 13 AccountType values, silently dropping cash, lineOfCredit, personalLoan, medicalDebt and otherDebt accounts from display and totals. - add the five missing types with display names and correct asset/liability classification (cash is an asset; the rest are liabilities) - keep unknown future account types visible in their own group instead of dropping them; unknown types are never miscounted into totals - use _format_dollar_amount for summary signs (fixes $-500.00 output) - tests cover all 13 enum values, asset/liability totals, and unknown-type visibility
…p (mcp-ynab-parity-category-read) get_transactions_by_category constructed CategoriesApi and called get_transactions_by_category on it, but that method exists only on TransactionsApi — a guaranteed AttributeError against a real SDK. Permissive MagicMock tests invented the missing method and hid the bug. - call TransactionsApi.get_transactions_by_category instead - update unit tests to mock the TransactionsApi route - add a real-SDK ownership assertion: TransactionsApi has the method, CategoriesApi does not
…r default (mcp-ynab-parity-unbounded) API 1.85 defaults an omitted since_date to one year ago. merge_payees, attention queries with days_back=None, "defaults to all" reconciliation reads, and alternate-ID scans all relied on that implicit truncation and could silently miss transactions older than one year. - add date_bounds.ALL_HISTORY_SINCE_DATE (1970-01-01) as the explicit all-history bound - merge_payees: pass the bound and report the scanned range in its output - get_transactions_needing_attention: days_back=None now fetches all history and reports the range - get_account_reconciliation_profile / subset-matches: since_date=None now means an explicit all-history bound - alternate-ID resolution: transfer/matched ids scan all history instead of YNAB's default; unparseable import_id dates fall back to the bound - regression tests cover every site, including pre-one-year fixtures
…parity-scheduled-dates) The official API requires a scheduled transaction start date strictly in the future and no more than five years out. create_scheduled_transaction defaulted to local today and passed any value straight to the SDK. - default start date is tomorrow in UTC (strictly future) - today and past dates fail locally with a clear ValueError - dates more than five calendar years out fail locally (Feb 29 handled) - the exact five-year limit is accepted - nothing invalid ever reaches the API (no HTTP 400 round-trip) - tests cover UTC reference frame, today/past rejection, the five-year boundary, and over-limit rejection
…6 progress Closed: mcp-ynab-parity-sdk, -account-types, -category-read, -unbounded, -scheduled-dates, the Milestone-0 epic, and g57.8 (dry-run intent capture, forced past the orthogonal g57.7 read-snapshot dependency).
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds deterministic grading and benchmarking for Code Mode and direct-tool evaluations, records dry-run mutation intents, expands evaluation expectations, and corrects YNAB account, transaction-date, scheduled-transaction, category-lookup, and SDK handling. ChangesYNAB API and transaction correctness
Dry-run mutation capture
Evaluation grading and benchmarking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EvalRunner
participant MCPServer
participant DryRunInterceptor
participant Grader
participant BenchmarkAggregator
EvalRunner->>MCPServer: Launch Code Mode or direct-tool evaluation
MCPServer->>DryRunInterceptor: Intercept validated mutation call
DryRunInterceptor-->>EvalRunner: Persist intended_writes.json
EvalRunner->>Grader: Grade run artifacts
Grader-->>EvalRunner: Write grading.json
EvalRunner->>BenchmarkAggregator: Aggregate configuration results
BenchmarkAggregator-->>EvalRunner: Write benchmark.json and benchmark.md
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_tools.py (1)
2193-2200: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winControl the UTC date in scheduled transaction tests.
A UTC midnight can change the expected date during a test. February 29 also makes direct five-year construction invalid for a non-leap target year.
tests/test_tools.py#L2193-L2200: inject or freeze the UTC date before calling the tool.tests/test_tools.py#L2224-L2245: derive the boundary with the same leap-day handling as production code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tools.py` around lines 2193 - 2200, Control the UTC date in tests/test_tools.py#L2193-L2200 by injecting or freezing the UTC clock before invoking create_scheduled_transaction, then assert against that controlled date plus one day. In tests/test_tools.py#L2224-L2245, derive the five-year boundary using the same leap-day handling as the production date logic instead of directly constructing an invalid non-leap-year February 29 date.
🧹 Nitpick comments (1)
src/mcp_ynab/tools/transactions.py (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Python 3.12 union syntax.
Replace
Optional[int]withint | None.Proposed fix
-def _explicit_since_date(days_back: Optional[int]) -> date: +def _explicit_since_date(days_back: int | None) -> date:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp_ynab/tools/transactions.py` at line 40, Update the _explicit_since_date function signature to use Python 3.12 union syntax, replacing Optional[int] with int | None while preserving the existing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evals/aggregate_benchmark.py`:
- Line 20: Reformat the variance calculation in the aggregate benchmark logic so
no line exceeds 100 characters. Split the conditional expression across lines or
assign the denominator separately while preserving the existing sample-variance
behavior and 0.0 result for fewer than two values.
In `@evals/evals.json`:
- Around line 17-21: Update the regex expectations in evals/evals.json at lines
17-21, 72-76, 195-198, and 230-232: require currency-formatted amounts for
dining spending and cash balance, and use bounded patterns matching either
$50/50 dollars or $200/200 dollars for the respective checks, preventing
unrelated numbers or larger amounts from satisfying them.
In `@evals/run_dual_eval.py`:
- Around line 201-209: Update the Agent SDK execution path used by _run_config
and drive_prompt so server_env_overrides, including
MCP_YNAB_EVAL_DRY_RUN_INTENTS_PATH, is forwarded through _drive_via_agent_sdk;
alternatively, explicitly reject Agent SDK execution in this runner. Ensure
Agent SDK mutation runs cannot proceed without either the configured dry-run
intent artifact or a clear rejection.
In `@src/mcp_ynab/dry_run.py`:
- Around line 64-79: Update tests/test_dry_run.py (lines 22-85) to exercise the
intercepted tools through the real FastMCP call_tool() dispatch, covering both
valid arguments and invalid arguments to verify validation still runs. Adjust
src/mcp_ynab/dry_run.py (lines 64-79) only as needed so the replacement
performed by the interceptor remains compatible with FastMCP dispatch and
preserves argument validation.
In `@src/mcp_ynab/tools/code_mode.py`:
- Around line 39-46: Update the Code Mode mutation documentation near the
statements describing code_mode_mutations_enabled to note that dry-run mode is
an explicit exception: it permits ynab.write.* handlers even when that
preference is disabled. Keep the existing preference requirement documented for
normal operation.
In `@src/mcp_ynab/tools/transactions.py`:
- Around line 1203-1206: Update the docstring for the scheduled transaction tool
near _validate_scheduled_transaction_date to state that start_date defaults to
tomorrow in UTC, matching the fallback assigned to txn_date. Leave the
validation and date calculation unchanged.
- Around line 497-500: Update the days-back rendering condition in the
transaction markdown construction to check whether days_back is not None, so a
value of 0 renders as a zero-day lookback while None retains the all-history
message.
---
Outside diff comments:
In `@tests/test_tools.py`:
- Around line 2193-2200: Control the UTC date in tests/test_tools.py#L2193-L2200
by injecting or freezing the UTC clock before invoking
create_scheduled_transaction, then assert against that controlled date plus one
day. In tests/test_tools.py#L2224-L2245, derive the five-year boundary using the
same leap-day handling as the production date logic instead of directly
constructing an invalid non-leap-year February 29 date.
---
Nitpick comments:
In `@src/mcp_ynab/tools/transactions.py`:
- Line 40: Update the _explicit_since_date function signature to use Python 3.12
union syntax, replacing Optional[int] with int | None while preserving the
existing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c50a82f4-29ab-41cb-ab99-f415c87bde3f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.beads/interactions.jsonl.beads/issues.jsonlevals/README.mdevals/aggregate_benchmark.pyevals/evals.jsonevals/grading.pyevals/run_dual_eval.pypyproject.tomlsrc/mcp_ynab/date_bounds.pysrc/mcp_ynab/dry_run.pysrc/mcp_ynab/formatters.pysrc/mcp_ynab/server.pysrc/mcp_ynab/tools/budgeting.pysrc/mcp_ynab/tools/code_mode.pysrc/mcp_ynab/tools/transactions.pytests/integration/_llm_eval_harness.pytests/test_dry_run.pytests/test_dual_eval_runner.pytests/test_server.pytests/test_tools.pytests/test_ynab_sdk_contracts.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79ecb57d63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if acct_type in asset_types: | ||
| output["summary"]["total_assets"] += group_total | ||
| elif acct_type in liability_types: | ||
| output["summary"]["total_liabilities"] += abs(group_total) |
There was a problem hiding this comment.
Keep positive liability balances positive in net worth
When a liability-type account has a positive balance, such as an overpaid credit card or line of credit, applying abs() treats that credit as debt and then subtracts it from net worth. For example, a lone lineOfCredit balance of +$100 is reported as $100 of liabilities and -$100 net worth instead of contributing +$100. Compute net worth from signed account balances and count only outstanding negative liability balances as liabilities.
Useful? React with 👍 / 👎.
What changed
Closes all five Milestone-0 parity issues plus the eval dry-run capture work, leaving a clean baseline:
YNAB API 1.86 parity (P0)
mcp-ynab-parity-sdk):ynab>=4.3.0,<5; contract tests pinuntil_dateon all transaction-list methods,goal_frequencymodeling, and HTTP 200SaveTransactionsResponsedecoding (4.1.0 still expected 209).mcp-ynab-parity-account-types): cash / lineOfCredit / personalLoan / medicalDebt / otherDebt were silently dropped from display and totals. Unknown future types now stay visible instead of vanishing; summary signs fixed ($-500→-$500).mcp-ynab-parity-category-read):get_transactions_by_categorycalled a method that only exists onTransactionsApiviaCategoriesApi— guaranteedAttributeErroron a real SDK. Permissive MagicMock tests were hiding it.mcp-ynab-parity-unbounded): merge_payees,days_back=Noneattention reads, reconciliation/subset "defaults to all" reads, and alternate-ID scans now pass an explicit all-history bound (1970-01-01) instead of YNAB's implicit one-yearsince_datetruncation; user-visible ranges are reported.mcp-ynab-parity-scheduled-dates): start date must be strictly future and ≤ 5 years out (UTC); today/past/over-limit fail locally before any API call; default is tomorrow UTC.Eval harness (g57.8 + g57.5/g57.6 partial)
MCP_YNAB_EVAL_DRY_RUN_INTENTS_PATHintercepts every mutating handler and persists validated payloads tointended_writes.json(no env var → production unchanged).grading.jsonper task/config, viewer-compatiblebenchmark.json+ token-delta-firstbenchmark.md, richerevals.jsonassertions, README docs.Verification
Beads
Milestone-0 epic and its five children closed; g57.8 closed. g57.5/g57.6/g57.7 remain tracked with progress notes.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation