Skip to content

feat(tools): harden list-tool and ratings inputs#120

Merged
alexander-wenzel-dev merged 2 commits into
mainfrom
feat/tools-list-input-hardening
Jul 6, 2026
Merged

feat(tools): harden list-tool and ratings inputs#120
alexander-wenzel-dev merged 2 commits into
mainfrom
feat/tools-list-input-hardening

Conversation

@alexander-wenzel-dev

@alexander-wenzel-dev alexander-wenzel-dev commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Changes

  • require_pagination(page, per_page) replaces require_per_page and bounds both inputs: per_page to 1..100, page to >= 1. The rejected low values are the ones Mealie mishandles: per_page=-1 is an unbounded "all rows" fetch, per_page=0 an empty page, and page<=0 gets silently coerced or returns a surprising last page. All 11 list tools call the new helper, and their wrapper docstrings now state the per_page range instead of the ceiling-only wording.
  • list_recipe_timeline_events UUID-parses recipe_id before interpolating it into the queryFilter it builds, so a value with a quote or a DSL operator cannot alter the parsed expression. The parse is a shared parse_recipe_uuid helper in _common.py, also adopted by the meal plan recipe_id parser that carried the same logic.
  • set_recipe_rating rejects a rating outside Mealie's 0..5 convention, which Mealie itself stores unchecked.
  • The resolved user id is cached per token, sparing the five rating and favorite tools a GET /api/users/self on every call after the first. The cache-hit path has a unit test.
  • Docs follow the rename: tools.md states the two-sided pagination bound and the filter-interpolation rule (and a stale _iter_tool_modules reference in "Naming and grouping" was corrected to _require_register), and the CLAUDE.md shared-helper list now names require_pagination and parse_recipe_uuid in place of the removed require_per_page.

How tested

Operator decisions taken first: bound page >= 1 (yes), rating range 0..5 only (no half-step rule). Both grounded in the live probe recorded in the task file.

uv run pytest -m "not live" -q
170 passed, 50 deselected in 1.21s
uv run pytest -m live -q
50 passed, 170 deselected in 95.77s (0:01:35)

ruff format --check, ruff check, and mypy src all clean.

Risks

None. The pagination and filter changes are input hardening with no happy-path behaviour change; the existing timeline and ratings live tests still pass. The user-id cache is process-scoped and keyed per token; this repo is one Mealie instance and one token per process, so the cache cannot serve a stale id across tokens. Rejections are raised client-side before any HTTP call and are unit-tested.

@alexander-wenzel-dev alexander-wenzel-dev force-pushed the feat/tools-list-input-hardening branch from 960cb56 to b00822b Compare July 6, 2026 15:29
@alexander-wenzel-dev

Copy link
Copy Markdown
Owner Author

/review-pr

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @alexander-wenzel-dev's task in 7m 9s —— View job


Review complete

  • Gather context (PR diff, files, CLAUDE.md rules)
  • Run /review-pr review (repo-conventions-reviewer + tool-pattern-reviewer, both dispatched since the diff touches src/mealie_mcp/tools/; live-test-reviewer skipped, no tests/live/ changes)
  • Post review summary

Findings

1. Duplicated UUID-parsing helper — src/mealie_mcp/tools/recipe_timeline.py:57-66
The new _parse_recipe_id is near-identical to the pre-existing _parse_recipe_id in src/mealie_mcp/tools/households_mealplans.py:67-74 — same try: UUID(value) except ValueError: raise ToolError(f"recipe_id must be a recipe UUID: {exc}") body and message (that one is the optional variant, str | None -> UUID | Unset). Not a hard rubric violation, but it's the kind of duplication the _common.py shared-helper convention exists to avoid, and this PR already extracted require_pagination for an analogous cross-module concern. Worth folding both into a shared helper now that a second call site exists.
Fix this →

2. No unit test for the new cache-hit path — src/mealie_mcp/tools/users_ratings.py:38-56
_user_id_by_token and its lookup in _current_user_id are pure logic (a dict read, no HTTP), so per this repo's "unit tests cover pure logic only" convention this is testable without a live call. tests/unit/test_users_ratings.py only covers the new rating-range validation from the same commit; nothing pre-seeds _user_id_by_token and asserts _current_user_id returns the cached value without hitting the client.
Fix this →

3. Minor: commit scope — b00822b
That commit bundles two unrelated concerns under one message ("validate rating range and cache the resolved user id"): the rating-range hardening (on-theme for this branch) and the user-id caching (a perf optimization, not input hardening). CLAUDE.md asks for committing in small steps; splitting these would have made each independently revertable. Not blocking.

Verified as sound (no action needed)

  • require_pagination's two-sided bound and its rollout across all 11 list tools match the updated .claude/rules/tools.md rubric; tests/unit/test_common.py::TestRequirePagination covers ceiling, floor, and both page/per_page boundaries, and each tool module keeps only a representative per-tool test rather than re-testing the shared helper.
  • The queryFilter UUID-parsing fix in recipe_timeline.py is the only place a caller value is interpolated into an internally-built queryFilter, so the new tools.md rule fully covers the diff with no stale unvalidated instance elsewhere; the injection test (recipe_id='x" or true') confirms the rejection.
  • _user_id_by_token keyed on client.token: sound for this repo's one-instance/one-token-per-process architecture (confirmed AuthenticatedClient.token is a plain str field in the generated client), and correctly scoped as module-private state, not exposed across tool boundaries.
  • Rating-range validation is placed correctly (before the client call, in the typed function not the wrapper) and matches the sibling _parse_event_type/_parse_recipe_id private-helper precedent.
  • Docstrings, writing style, and PR-body conventions (Tools/Changes, How tested, Risks sections) all check out.

@alexander-wenzel-dev alexander-wenzel-dev force-pushed the feat/tools-list-input-hardening branch from b00822b to 1b9ad8c Compare July 6, 2026 16:12
@alexander-wenzel-dev

Copy link
Copy Markdown
Owner Author

/review-pr

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @alexander-wenzel-dev's task in 3m 20s —— View job


Review in progress

  • Gather context (PR diff, files, CLAUDE.md rules)
  • Dispatch repo-conventions-reviewer and tool-pattern-reviewer (live-test-reviewer skipped, no tests/live/ changes)
  • Post review summary

@alexander-wenzel-dev

Copy link
Copy Markdown
Owner Author

/review-pr

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @alexander-wenzel-dev's task in 4m 24s —— View job


Review complete

  • Gather context (PR diff, files, CLAUDE.md rules)
  • Run /review-pr review (repo-conventions-reviewer + tool-pattern-reviewer, both dispatched again; live-test-reviewer skipped, no tests/live/ changes)
  • Post review summary

Findings

1. Stale require_per_page references in docs — CLAUDE.md:135, .claude/rules/tools.md:50
This PR renames require_per_page to require_pagination everywhere under src/mealie_mcp/tools/ (confirmed: no require_per_page symbol remains in src/), but two rule/doc spots weren't updated to match:

  • CLAUDE.md:135 still lists require_per_page among the shared _common.py helpers, and doesn't mention the new shared parse_recipe_uuid helper either.
  • .claude/rules/tools.md:50 still reads: "call require_per_page(per_page) first (the shared ceiling is 100)" — both the function name and the description are stale; it doesn't mention the new page >= 1 floor.

CLAUDE.md's own "Maintaining this file" rule requires removing/updating obsolete sections in the same commit that obsoletes them. git status shows .claude/rules/tools.md as modified in the working tree, but its committed content (and CLAUDE.md) still describe the old helper.
Fix this →

2. Minor: commit scope — 1b9ad8c (raised previously, still true)
Still bundles rating-range validation with the unrelated user-id-caching perf optimization in one commit message. Not blocking.

Resolved since the last review pass

  • UUID-parsing duplication (previously flagged): no longer an issue. recipe_timeline.py now imports and calls the shared parse_recipe_uuid from _common.py:87-92 directly; households_mealplans.py's _parse_recipe_id is a thin wrapper around the same shared function for the optional case. No duplicate parser remains.
  • Missing cache-hit unit test (previously flagged): no longer an issue. tests/unit/test_users_ratings.py:33-46 (test_returns_cached_id_without_calling_the_client) seeds _user_id_by_token, asserts the cache short-circuits before any client call, and cleans up in a finally.

Verified as sound (no action needed)

  • require_pagination is called inside each typed list_* function (never the thin @mcp.tool() wrapper) across all 11 modules; shared-helper behavior is tested once in tests/unit/test_common.py, with representative per-tool coverage elsewhere.
  • _user_id_by_token is module-private, keyed on client.token, consistent with the one-instance/one-token-per-process design; no invalidation path is needed at this scope.
  • Rating-range rejection happens client-side before any HTTP call and is unit-tested; the 0..5 bound is stated to be grounded in an out-of-band live probe (recorded in the gitignored task file per this repo's convention), consistent with prior review's acceptance of this reasoning.
  • No security-rule violations: no tokens/secrets logged, no .env touched, no real hostnames introduced.

require_pagination replaces require_per_page and bounds both inputs: per_page
to 1..100 and page to >= 1. The rejected low values are the ones Mealie
mishandles, per_page=-1 as an unbounded "all rows" fetch, per_page=0 as an
empty page, and page<=0 as silent coercion or a surprising last page. The list
tool docstrings state the per_page range in place of the ceiling-only wording.

list_recipe_timeline_events UUID-parses recipe_id before interpolating it into
the queryFilter it builds, so a value carrying a quote or a DSL operator cannot
alter the parsed expression. The parse is a shared parse_recipe_uuid helper in
_common.py, also adopted by the meal plan recipe_id parser.

tools.md states the two-sided pagination bound and the filter-interpolation
rule. The CLAUDE.md and rubric shared-helper lists name require_pagination and
parse_recipe_uuid, not the removed require_per_page.
set_recipe_rating rejects a rating outside Mealie's 0..5 convention, which
Mealie itself stores unchecked, so an out-of-range value can no longer corrupt
the user's ratings. The resolved user id is cached per token, sparing the five
rating and favorite tools a GET /api/users/self on every call after the first.
The cache-hit path has a unit test that pre-seeds the cache and asserts no
client call.
@alexander-wenzel-dev alexander-wenzel-dev force-pushed the feat/tools-list-input-hardening branch from d666e85 to 33ed762 Compare July 6, 2026 17:10
@alexander-wenzel-dev alexander-wenzel-dev enabled auto-merge (rebase) July 6, 2026 17:10
@alexander-wenzel-dev alexander-wenzel-dev merged commit 065512d into main Jul 6, 2026
4 checks passed
@alexander-wenzel-dev alexander-wenzel-dev deleted the feat/tools-list-input-hardening branch July 6, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant