fix(mcp): distinguish missing vs empty in validate_required (#371)#401
Conversation
…ylor#371) distillery_search(query="") previously returned "Missing required fields: query" even though the field was present — an agent parsing the message would retry with the same empty payload and loop. The helper now separates absence (field not in arguments or explicitly None) from emptiness (a present string that is empty or whitespace-only), and returns a distinct "Field 'x' must be a non-empty string" message for the latter. Missing takes precedence when both categories fail in one call. Whitespace-only strings (" ") are now rejected at the validator rather than silently succeeding and producing confusing downstream behaviour (empty search results, NOT_FOUND lookups, etc.). Also migrated _handle_aggregate off its ad-hoc "if not group_by" check so empty-string group_by gets the same treatment, and added unit coverage for validate_required, validate_type, validate_enum, and validate_positive_int in a new tests/test_mcp_common.py. While auditing norrietaylor#372 I confirmed the reported repros already pass on main (entry_type and action are validated before the DB lookup); added regression tests to lock that in so the ordering does not silently regress later.
📝 WalkthroughWalkthroughvalidate_required now separates failures into Missing (absent or None) and Empty (present but empty/whitespace string) and returns distinct messages; the aggregate handler uses this validator for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/distillery/mcp/tools/_common.py (1)
128-142: LGTM — clean missing vs empty split.Logic correctly partitions fields:
arguments.get(f)collapses absent and explicitNoneintomissing(matching the docstring), and theisinstance(value, str) and not value.strip()guard means non-string falsy values (0,False,[],{}) correctly skip both branches. Missing-first precedence is sensible for agent recovery.Minor nit (optional): the singular message uses
repr()-quoted field name (Field 'query' must be a non-empty string) while the plural form joins names unquoted (Fields must be non-empty strings: a, b). Consider quoting in the plural form too for consistency, e.g.', '.join(repr(f) for f in empty). Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/distillery/mcp/tools/_common.py` around lines 128 - 142, The plural error message is inconsistent with the singular one because the singular uses repr() quoting while the plural joins raw names; update the branch that builds the plural message (the code that returns f"Fields must be non-empty strings: {', '.join(empty)}") to join repr(f) for each f in empty so field names are quoted consistently with the singular message — locate the logic around the local variables missing, empty, fields and the final return paths and replace the join with a repr-based join.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/distillery/mcp/tools/_common.py`:
- Around line 128-142: The plural error message is inconsistent with the
singular one because the singular uses repr() quoting while the plural joins raw
names; update the branch that builds the plural message (the code that returns
f"Fields must be non-empty strings: {', '.join(empty)}") to join repr(f) for
each f in empty so field names are quoted consistently with the singular message
— locate the logic around the local variables missing, empty, fields and the
final return paths and replace the join with a repr-based join.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dad7a02b-dafa-46eb-bcfe-e358417c20d7
📒 Files selected for processing (6)
src/distillery/mcp/tools/_common.pysrc/distillery/mcp/tools/search.pytests/test_mcp_classify.pytests/test_mcp_common.pytests/test_mcp_coverage_gaps.pytests/test_mcp_server.py
CodeRabbit caught that the plural branch of validate_required joined raw
field names ("Fields must be non-empty strings: a, b") while the singular
branch used repr quoting ("Field 'a' must be a non-empty string"). Align
the two by repr-quoting each name in the plural join.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_mcp_common.py (1)
24-159: Solid coverage of the new contract.Tests pin down the missing/empty split, ordering precedence, whitespace handling, and the non-string falsy cases called out in issue
#371. Thevalidate_positive_intbool-rejection test is a nice guard against Python'sbool <: intquirk.One minor gap worth considering: there's no assertion that
validate_positive_intreturns theINVALID_PARAMScode (only that the result is a tuple). Since the tuple is consumed byerror_response(code, message), a quickresult[0] == "INVALID_PARAMS"check would lock in the public contract and catch accidental code-string drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_common.py` around lines 24 - 159, Add assertions in the TestValidatePositiveInt tests that currently assert a tuple result to also verify the error code is the expected public contract string; specifically, in tests test_missing_without_default_returns_error, test_zero_rejected, test_negative_rejected, test_non_int_rejected, and test_bool_rejected where validate_positive_int(...) returns a tuple, assert result[0] == "INVALID_PARAMS" in addition to the existing isinstance(result, tuple) checks so the tests lock in the error code returned by validate_positive_int.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_mcp_common.py`:
- Around line 24-159: Add assertions in the TestValidatePositiveInt tests that
currently assert a tuple result to also verify the error code is the expected
public contract string; specifically, in tests
test_missing_without_default_returns_error, test_zero_rejected,
test_negative_rejected, test_non_int_rejected, and test_bool_rejected where
validate_positive_int(...) returns a tuple, assert result[0] == "INVALID_PARAMS"
in addition to the existing isinstance(result, tuple) checks so the tests lock
in the error code returned by validate_positive_int.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ea2ded8-8ae0-4998-8862-296fa85cccd7
📒 Files selected for processing (2)
src/distillery/mcp/tools/_common.pytests/test_mcp_common.py
2 participants
Closes #371. Also adds regression tests for #372.
The motivating case is
distillery_search(query=""): the server returned"Missing required fields: query"even though the field was present in the payload, and an agent that parses the message to correct its call would retry with the same emptyqueryand loop. The fix splitsvalidate_requiredinto two categories — missing (absent from the dict or explicitlyNone) and empty (a present string that is empty or whitespace-only) — and produces a distinct"Field 'x' must be a non-empty string"message for the empty case. When both categories fail in one call, missing is reported first because it is the more fundamental error.Whitespace-only strings like
" "previously slipped through validation and produced confusing downstream behaviour (empty search results, NOT_FOUND on phantom ids). They are now rejected at the validator with the same "non-empty string" message.I also migrated
_handle_aggregateoff its ad-hocif not group_bycheck so it benefits from the same treatment, and added unit coverage forvalidate_required,validate_type,validate_enum, andvalidate_positive_intin a newtests/test_mcp_common.py— the public contract of these helpers was previously only tested via the tool handlers that call them.While looking at #372 I confirmed the reported repros already pass on main:
distillery_classifyvalidatesentry_typebefore the DB lookup (fixed in commit 7a199e8), anddistillery_resolve_reviewvalidatesactionbefore the lookup. I added regression tests to both handlers so the ordering cannot silently regress.#372itself can be closed once this lands.Non-string falsy values (
0,False,[],{}) remain valid — that never regressed, but it is now explicitly asserted.Summary by CodeRabbit
Bug Fixes
Tests