Add legacy MCP tools for recent and saved food collections

[tomerh2001]

Summary

• add MCP tools for recent foods, frequent foods, and My Foods
• fetch those collections through the legacy diary-add AJAX endpoints with fresh CSRF headers
• document the newer-route logout failure mode and the supported workaround in the README

Why

Newer account routes such as /food/mine, /meal/mine, and /food/new can redirect to /account/logout in this environment even while diary reads and authenticated API token fetches still work. That made simple account-level food list requests slow and brittle to answer manually.

This change gives the MCP first-class tools for those queries and documents the non-obvious legacy path that still behaves reliably.

Notes

I attempted to create a tracking issue in the fork first to follow the normal fork flow, but GitHub Issues are disabled on this repository, so there was no issue to link.

Verification

• python -m py_compile src/mfp_mcp/server.py
• exercised mfp_get_recent_foods, mfp_get_frequent_foods, and mfp_get_my_foods locally against a live authenticated session
• confirmed the new tools returned expected counts and sample items from the account

[AdamWalt]

~1,000 lines of new logic covering HTML parsing, write operations, and entry ID matching with no tests. The PR description mentions manual verification against a live session, which is great for smoke testing, but I'd like to see at least unit tests for the pure functions before merging:

• normalize_food_collection_item — various shapes of the legacy JSON payload
• resolve_weight_id — matching by label, falling back to selected/first option
• find_replacement_entry — the progressive fallback logic (meal match, short_name match, name match, singleton)
• meal_name_to_id / meal_id_to_name — edge cases like "snack" vs "snacks"

These don't need a live MFP session and would catch regressions if MFP changes their HTML structure.

[AdamWalt]

Thanks for the contribution — the code quality here is solid. The entry_id extraction, legacy AJAX endpoint discovery, and the replacement-entry matching logic are useful additions.

A few things I'd like addressed before approving:

1. Blocker: Thread safety of the create_mfp_client monkey-patch (see comment)
2. Requested: Unit tests for the pure helper functions (see comment)

Happy to re-review once those are addressed.

[AdamWalt]

Review summary

This is the most ambitious of the open PRs and, in pure engineering terms, the highest-quality in places: it uses proper lxml/xpath parsing (not regex), confirms writes by diffing before/after diary snapshots, resolves real serving sizes, and richly normalizes the legacy AJAX payloads. The three read tools (mfp_get_recent_foods / frequent / my_foods) are clean, well-annotated, and match the stated purpose.

But it's also the riskiest to merge as written, for reasons that are mostly about scope and verification rather than code quality:

Blocking-ish concerns

1. Global cloudscraper monkeypatch (create_mfp_client) changes the HTTP transport for every request and disables Cloudflare bypass to fix a host-specific 403. This can regress users who actually need cloudscraper. Needs a more targeted fix or, at minimum, opt-in behavior + disclosure. (inline)
2. Undisclosed scope + no verification for the write/delete paths. The description and test plan cover only the three read tools, but the PR also rewrites add_food_to_diary, adds mfp_update_food_entry, and adds the destructive mfp_delete_food_entry. (inline ×2)
3. mfp_get_diary now double-fetches on every read and zips entry_ids positionally; on a count mismatch it can attach the wrong id, which then targets update/delete at the wrong row. (inline)

Cross-PR coordination (important)

• This PR and #5 both rewrite add_food_to_diary to /food/add and will hard-conflict. They disagree on the unit contract (#1 keeps serving resolution; #5 drops unit). Pick one as the basis for the write rewrite.
• This PR also touches get_mfp_client / client construction, overlapping with #3's auth changes.
• If you want the read-only food-collection tools, those are the cleanly-separable, low-risk part of this PR and could be landed on their own quickly.

Verdict

Not mergeable as-is. The read tools are close to ready and could be split out; the transport monkeypatch and the unverified write/delete rewrite need rework and verification before they should land — and the add_food_to_diary rewrite has to be reconciled with #5 either way.

---

Recap across all four open PRs

• #2 (gitignore chore): ✅ Mergeable as-is. Subset of #3 — consider merging this first and trimming #3, or closing in favor of #3.
• #3 (encryption + Chromium auto-discovery): ⚠️ Request minor changes — Windows keychain round-trip mismatch + contradictory README "recommended" wording. Solid otherwise.
• #5 (/food/add fix): ⚠️ Add path is verified and good; bundled unverified destructive remove tool + brittle regex need resolving. Conflicts with #1.
• #1 (food collections + write/delete + transport patch): ❌ Not mergeable as-is — read tools are nearly ready and splittable; monkeypatch + unverified writes need rework. Conflicts with #1/#5/#3.

Suggested order: land #2, then #3 (after its fixes), then decide #1-vs-#5 for the diary-write rewrite (I'd lean toward #5's leaner verified add + #1's read-only collection tools split out), saving update/delete for a follow-up with real verification.

---

Generated by Claude Code

[AdamWalt]

This temporarily replaces myfitnesspal_client.cloudscraper.create_scraper at the module level and restores it in a finally block. Since this is an async MCP server, concurrent tool calls could race here — one call restores the original while another is mid-construction.

Could you either:

• Guard this with a threading.Lock, or
• Avoid the global mutation entirely (e.g., construct the session directly and pass it into the client, or subclass the client)

This is the main blocker for me before approving.

[AdamWalt]

This global monkeypatch is the riskiest thing in the PR. create_mfp_client reaches into myfitnesspal.client.cloudscraper and swaps create_scraper for a plain requests.Session, then restores it in finally. Two concerns:

1. It disables Cloudflare bypass for everyone. cloudscraper exists specifically to get past Cloudflare challenges. Replacing it fixes the 403 on /user/auth_token you saw on this host, but for users who actually hit a Cloudflare interstitial, the plain session will now fail where it previously worked. That's trading a fix on one environment for a regression on another.
2. It mutates a module global. Even with the finally restore, it's not concurrency-safe — if two clients are created concurrently (or the lambda leaks), behavior is undefined.

If the 403 is really about the scraper, a more targeted fix (e.g. passing a custom session only on the specific failing request, or making the plain-session fallback opt-in / try-cloudscraper-then-fallback) would avoid globally defeating cloudscraper. At minimum this deserves a prominent comment and a mention in the PR description, since it changes the transport for every request, not just the new tools.

---

Generated by Claude Code

[AdamWalt]

Scope vs. description + verification gap. The PR description says it adds "recent foods, frequent foods, and My Foods" tools, and the verification section only exercises those three read tools. But the diff also: rewrites add_food_to_diary end-to-end to the /food/add endpoint, adds a write tool (mfp_update_food_entry), adds a destructive tool (mfp_delete_food_entry), and globally swaps the HTTP transport. None of the write/delete/add-rewrite paths appear in the test plan.

The delete here (/food/remove/{id} with _method=delete) does at least confirm by re-reading the diary and asserting the entry is gone — good — but a destructive operation with no stated verification is exactly what I'd want proven before merge. Recommend either splitting the write/delete work into its own PR or adding explicit before/after verification for update and delete to the test plan.

---

Generated by Claude Code

[AdamWalt]

mfp_get_diary now does an extra full page fetch + parse (extract_diary_entry_ids) on every read to attach entry_ids. That's a real cost added to the hottest read path, and it's only needed when the caller intends to edit/delete. The count-mismatch warning also implies the id↔entry alignment is positional and can silently drift (the diary parse and the library's meal.entries ordering must agree). Two thoughts: (a) consider making the entry-id enrichment opt-in via a flag on GetDiaryInput so plain reads stay single-fetch; (b) when the counts mismatch you currently still zip them positionally — that can attach the wrong entry_id to an entry, which then drives update/delete against the wrong row. Safer to attach None for the whole meal on mismatch rather than risk a misaligned id.

---

Generated by Claude Code

[AdamWalt]

Nice touch: update_food_entry snapshots the diary before/after and uses find_replacement_entry to re-locate the row when MFP rotates the entry_id. That heuristic (meal match → short_name → name substring → single-new-entry fallback) is reasonable, but it's doing fuzzy matching to confirm a write succeeded, and the fallbacks can mis-identify when a meal has multiple similar items edited in quick succession. Given how much inference is happening here, this is another reason update/delete deserve dedicated verification rather than riding in on a "food collections" PR.

---

Generated by Claude Code