feat: modernize MCP server and align docs/tests - #3

projectbrief.md (1)

1-17: Unfilled template — populate before merging.

All sections remain placeholder text. If this is intended as living documentation for the project, filling it in at commit time prevents the repo accumulating stale scaffolding that nobody ever revisits.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@projectbrief.md` around lines 1 - 17, The Project Brief is a placeholder
template and must be populated before merging: replace the header "# Project
Brief: [Project Name]" with the real project name and fill the "## Overview",
"## Core Requirements", "## Goals", and "## Project Scope" sections with
concrete, actionable content (specific requirements, measurable goals, and clear
in/out of scope boundaries) so this becomes living documentation rather than
scaffolding; ensure each bullet under "## Core Requirements" and "## Goals" is
specific and measurable and that the "## Project Scope" clearly lists included
and excluded items.

progress.md (1)

1-20: Unfilled template — same note as projectbrief.md.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@progress.md` around lines 1 - 20, This file contains an unfilled template;
replace placeholder headings and bracketed items under "Progress: [Project
Name]", "## What Works", "## What's Left to Build", "## Known Issues and
Limitations", and "## Evolution of Project Decisions" with real project-specific
content: set the actual project name in the top heading, list implemented
features under "What Works" (concrete feature names), enumerate remaining tasks
under "What's Left to Build", detail reproducible known issues under "Known
Issues and Limitations", and write a short narrative under "Evolution of Project
Decisions" explaining major decisions and why they changed; remove the bracketed
placeholders (e.g., "[Feature 1]") and ensure each section contains actionable,
specific items.

.beads/.gitignore (1)

2-6: Minor redundancy: *.db?* already covers the explicit patterns below it.

*.db-wal, *.db-shm, and *.db-journal are all matched by *.db?*, so lines 5–6 and the journal entry are redundant. Leaving them is harmless and arguably self-documenting, but worth noting if you prefer minimal gitignore files.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.beads/.gitignore around lines 2 - 6, The .gitignore contains redundant
patterns: the glob "*.db?*" already matches the explicit entries "*.db-journal",
"*.db-wal" and "*.db-shm"; to clean this up either remove the explicit patterns
("*.db-journal", "*.db-wal", "*.db-shm") or delete the broader "*.db?*" and keep
the explicit ones if you prefer self-documenting entries—update the file
accordingly so only one of these matching strategies remains.

techContext.md (1)

1-22: Unfilled template — same note as projectbrief.md.

All sections are placeholder text. Consider populating before merge.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@techContext.md` around lines 1 - 22, The techContext.md is a placeholder
template — replace the stub entries under the headings "Technologies Used",
"Development Setup", "Technical Constraints", "Dependencies", and "Tool Usage
Patterns" with concrete project details: list the actual technologies and
versions used, provide step‑by‑step development environment setup and any
required environment variables, enumerate real technical constraints and
rationale, list production and dev dependencies with versions and where they are
declared, and describe how project tools (build, test, CI/CD, linters) are used
in practice so the file is fully populated before merge.

productContext.md (1)

1-13: Unfilled template — same note as projectbrief.md.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@productContext.md` around lines 1 - 13, The Product Context file currently
contains an unfilled template under the headings "## Problem Statement", "##
User Experience Goals", and "## Success Metrics"; replace the placeholder lines
with concrete content: write a concise Problem Statement describing the user
problem this project solves, populate "## User Experience Goals" with 3–5
measurable UX goals (replace [UX Goal 1], etc.), and list 2–4 specific Success
Metrics under "## Success Metrics" (replace [Metric 1], etc.); ensure the
wording aligns with the corresponding entries in projectbrief.md and use the
exact headings "## Problem Statement", "## User Experience Goals", and "##
Success Metrics" so the document remains discoverable and consistent.

systemPatterns.md (1)

1-20: Replace placeholder content or explicitly mark this as a template.

Line 1 through Line 20 still contain unresolved placeholders. If this is meant to be a live memory-bank file, it currently provides no usable architecture context.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@systemPatterns.md` around lines 1 - 20, The document contains unresolved
placeholders under headings like "System Architecture", "Key Technical
Decisions", "Design Patterns in Use", "Component Relationships", and "Critical
Implementation Paths"; either populate each section with concrete,
project-specific information (a short architecture overview, 3–5 concrete
technical decisions with rationale, listed design patterns in use, how major
components interact, and the critical implementation paths) or explicitly mark
the file as a template by replacing placeholder lines with a clear template
header and TODO markers (e.g., "TEMPLATE: fill in project-specific
architecture") so reviewers know it is not production content; ensure the
"System Architecture" and "Critical Implementation Paths" sections at minimum
contain actionable, non-placeholder descriptions.

activeContext.md (1)

1-23: Populate this file with project-specific context (or rename as a template).

Line 1 through Line 23 are placeholders, so this currently can’t serve as actionable active context for contributors/agents.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@activeContext.md` around lines 1 - 23, activeContext.md currently contains
placeholder headings and cannot serve as actionable context; replace
placeholders under the headings "Current Work Focus", "Recent Changes", "Next
Steps", "Active Decisions and Considerations", "Important Patterns and
Preferences", and "Learnings and Project Insights" with concrete,
project-specific details (e.g., current ticket/feature IDs, short status, recent
PRs/commits, imminent tasks, decision rationale and owners, coding/architecture
patterns, and key learnings) or, if you intend it to remain a template, rename
the file to indicate "template" and add a usage note at the top explaining how
and when to populate it and who is responsible for updates so contributors can
rely on it.

pyproject.toml (1)

9-9: Add an upper bound for pydantic to future-proof against major-version changes.

While Pydantic 3.0 has not yet been released, adding a constraint like <3 is a best practice that prevents unexpected breaking changes when the next major version becomes available. This is consistent with the principle of explicit version bounds for external dependencies.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pyproject.toml` at line 9, Update the pydantic dependency declaration in
pyproject.toml to add an upper bound to prevent accidental upgrades to the next
major version; locate the existing "pydantic>=2.10.0" entry and change it to
include a <3 upper bound (e.g., "pydantic>=2.10.0,<3") so the project stays
pinned to Pydantic 2.x until you've tested compatibility with 3.x.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

memory_bank_instructions.md (1)

81-87: ⚠️ Potential issue | 🟡 Minor

Wrap the Act Mode Mermaid block in a fenced code block.

Line 82 starts a Mermaid flowchart without a ```mermaid fence, so it may render as plain text instead of a diagram.

✏️ Proposed fix

 ### Act Mode
+```mermaid
 flowchart TD
     Start[Start] --> Context[Check Memory Bank]
     Context --> Update[Update Documentation]
     Update --> Execute[Execute Task]
     Execute --> Document[Document Changes]
+```

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@memory_bank_instructions.md` around lines 81 - 87, The "Act Mode" Mermaid
flowchart block (the flowchart TD containing nodes Start, Context, Update,
Execute, Document) is not wrapped in a fenced code block so it may render as
plain text; wrap the entire Mermaid block with a starting ```mermaid fence
before "flowchart TD" and a closing ``` fence after the last line to ensure it
renders as a diagram.

src/mcp_ynab/server.py (1)

703-711: Use explicit mutating annotations for idempotent write tools.

At Line 703 and Line 710, these tools mutate local state but only set idempotentHint=True. Consider explicitly setting readOnlyHint=False (and destructiveHint=False) for clearer MCP client behavior.

♻️ Proposed refactor

 READ_ONLY_TOOL = types.ToolAnnotations(readOnlyHint=True, idempotentHint=True)
 MUTATING_TOOL = types.ToolAnnotations(readOnlyHint=False, destructiveHint=True)
+IDEMPOTENT_MUTATING_TOOL = types.ToolAnnotations(
+    readOnlyHint=False, idempotentHint=True, destructiveHint=False
+)
...
-@mcp.tool(annotations=types.ToolAnnotations(idempotentHint=True))
+@mcp.tool(annotations=IDEMPOTENT_MUTATING_TOOL)
 async def set_preferred_budget_id(budget_id: str) -> str:
...
-@mcp.tool(annotations=types.ToolAnnotations(idempotentHint=True))
+@mcp.tool(annotations=IDEMPOTENT_MUTATING_TOOL)
 async def cache_categories(budget_id: str) -> str:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_ynab/server.py` around lines 703 - 711, The MCP tool decorators on
set_preferred_budget_id and cache_categories currently only set
idempotentHint=True but these functions mutate local state; update their
annotations to explicitly mark readOnlyHint=False and destructiveHint=False in
the types.ToolAnnotations passed to `@mcp.tool` (keep idempotentHint=True) so the
MCP client understands these are idempotent writes rather than reads; modify the
decorators on the functions named set_preferred_budget_id and cache_categories
accordingly.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/mcp_ynab/server.py (2)

613-622: 404-specific exception handling now correctly scoped.

The previous broad except Exception has been replaced with a targeted except ApiException as exc that inspects exc.status and only swallows 404s, re-raising everything else. This aligns with the official YNAB SDK pattern of from ynab.rest import ApiException and correct status-code-based branching.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_ynab/server.py` around lines 613 - 622, Replace the broad exception
catching with a targeted ApiException handler around the call to
transactions_api.get_transaction_by_id so that you only swallow 404 responses:
catch ApiException as exc, set target_transaction = None when exc.status == 404,
and re-raise the exception for any other exc.status; ensure this change is
applied in the try/except block that assigns target_transaction from
single_response.data.transaction so other errors are not silently ignored.

---

213-228: UTF-8 encoding and JSONDecodeError fallback correctly applied.

Both _load_json_file and _save_json_file now explicitly specify encoding="utf-8", and _load_json_file catches json.JSONDecodeError with a warning log and a safe {} fallback. The test_load_json_file_handles_invalid_json test in the test file validates this path end-to-end.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_ynab/server.py` around lines 213 - 228, _save_json_file currently
writes directly and can fail silently on missing directories or
write/serialization errors; update _save_json_file to ensure the target
directory exists (Path(filename).parent.mkdir(parents=True, exist_ok=True)) and
wrap the open/json.dump in a try/except that catches OSError and TypeError (or
json.JSONEncodeError if available) and logs an error via logger.error with the
filename and exception details, re-raising or returning as appropriate for your
error semantics; reference the function name _save_json_file to locate the
change and mirror the logging style used in _load_json_file.

src/mcp_ynab/server.py (2)

233-237: MUTATING_TOOL (destructiveHint=True) may be too strong for categorize_transaction.

destructiveHint and idempotentHint are only meaningful when readOnlyHint is false, and destructiveHint flags operations that can irreversibly destroy or overwrite data. Re-categorizing a transaction is a reversible update: running it twice with the same category_id produces no additional state change. IDEMPOTENT_MUTATING_TOOL (readOnlyHint=False, idempotentHint=True, destructiveHint=False) would be a more accurate annotation and would prevent MCP clients from showing unnecessary destructive-action confirmation prompts.

♻️ Proposed fix

-@mcp.tool(annotations=MUTATING_TOOL)
+@mcp.tool(annotations=IDEMPOTENT_MUTATING_TOOL)
 async def categorize_transaction(

Also applies to: 581-581

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_ynab/server.py` around lines 233 - 237, The MUTATING_TOOL annotation
is too strong for reversible actions like categorize_transaction; update usages
so that categorize_transaction (and the other occurrence currently using
MUTATING_TOOL) use IDEMPOTENT_MUTATING_TOOL instead (i.e., replace references to
MUTATING_TOOL with IDEMPOTENT_MUTATING_TOOL for the categorize_transaction
handler/function and the second location noted) so the tool is marked
readOnlyHint=false, idempotentHint=true, destructiveHint=false.

---

44-54: Module-level YNABResources() creates ~/.config/mcp-ynab on every import.

_resolve_config_dir calls config_dir.mkdir(parents=True, exist_ok=True) unconditionally. Because ynab_resources = YNABResources() is at module level (Line 294), importing mcp_ynab.server in any context — including the test suite running in CI — creates the directory under $HOME/.config. Tests are properly isolated via tmp_path, but the side effect still pollutes the developer/CI home directory.

Consider deferring directory creation to first write (e.g., inside set_preferred_budget_id / cache_categories) rather than on init.

Also applies to: 294-294

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp_ynab/server.py` around lines 44 - 54, Module import currently creates
~/.config/mcp-ynab because _resolve_config_dir() always calls
config_dir.mkdir(...) and YNABResources() is instantiated at module level as
ynab_resources; change to defer directory creation: remove the unconditional
mkdir from _resolve_config_dir (or stop calling _resolve_config_dir during
module import) and instead ensure the directory is created lazily inside
write-path methods such as YNABResources.set_preferred_budget_id and
YNABResources.cache_categories (call mkdir(parents=True, exist_ok=True) just
before writing files); also consider removing the module-level instantiation
ynab_resources or replace it with a lazy getter to avoid side effects on import.

tests/test_server.py (3)

253-260: Incomplete field preservation assertions — var_date, payee_id, payee_name, and subtransactions not verified.

The test name and PR description both emphasize that categorize_transaction preserves all existing transaction fields, but only six of the ten fields carried through ExistingTransaction are actually asserted. The payee and date fields (var_date, payee_id, payee_name) and subtransactions are set on DummyTransaction and passed through ExistingTransaction in server.py (lines 633–642) but never checked here.

♻️ Proposed additional assertions

 assert captured["flag_color"] == "blue"
+assert captured["var_date"] == "2026-02-01"
+assert captured["payee_id"] == "payee-1"
+assert captured["payee_name"] == "Coffee Shop"
+assert captured["subtransactions"] == [{"amount": -500, "category_id": "cat-old"}]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_server.py` around lines 253 - 260, The test currently asserts only
six fields after calling server.categorize_transaction but omits var_date,
payee_id, payee_name, and subtransactions which are set on DummyTransaction and
passed through ExistingTransaction; update the test to also assert that
captured["var_date"] matches the DummyTransaction var_date, captured["payee_id"]
matches payee_id, captured["payee_name"] matches payee_name, and
captured["subtransactions"] matches the DummyTransaction.subtransactions so all
fields preserved by categorize_transaction/ExistingTransaction are verified.

---

216-216: Optional: same RUF012 mutable class attribute as above.

♻️ Proposed fix

-    subtransactions = [{"amount": -500, "category_id": "cat-old"}]
+    subtransactions: ClassVar[list] = [{"amount": -500, "category_id": "cat-old"}]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_server.py` at line 216, The test defines a mutable
class/module-level list named subtransactions which can be mutated across tests
(RUF012); change it to be created fresh where used (e.g., move the [{"amount":
-500, "category_id": "cat-old"}] literal into the test function or into a helper
function like make_subtransactions()) so each test gets a new list instance
instead of sharing the module-level subtransactions variable.

---

126-126: Optional: annotate mutable class attribute to silence RUF012.

subtransactions = [] on a class body is flagged by Ruff (RUF012). While there's no mutation risk here because DummyTransaction is only ever instantiated once per test, the fix is a one-liner:

♻️ Proposed fix

+from typing import ClassVar
 ...
 class DummyTransaction:
     id = "tx-1"
     ...
-    subtransactions = []
+    subtransactions: ClassVar[list] = []

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_server.py` at line 126, Annotate the mutable class attribute to
silence RUF012: change the class-level declaration subtransactions = [] inside
DummyTransaction to be explicitly typed as a ClassVar, e.g. subtransactions:
ClassVar[list] = [], and add the necessary import for ClassVar from typing if
missing; this keeps the attribute as a shared class container but satisfies the
linter.

Configuration used: defaults

Review profile: CHILL

Plan: Pro