[TAN-7901] Test suite for the mcp engine#14250
Open
adessy wants to merge 21 commits into
Open
Conversation
mcp engine
Collaborator
|
6b11d5d to
3b5cf28
Compare
`update_project`, `update_phase`, `update_resource` and `replace_form_fields` called neither `authorize_project!` nor Pundit `authorize`, so the documented draft-only constraint was not enforced for updates: a super admin could mutate published projects through the MCP.
…les for MCP specs
…form fields) New specs for `update_phase`, `get_form_fields` and `replace_form_fields`; deepened `create_phase` (feature-flag-driven schema, participation-method variants) and `update_phase_permission` (demographic_questions semantics, rollback, verified expiry). Also fixes a real round-trip bug the new specs caught: `Serializers::CustomField` emitted a `constraints` key that is not a `CustomField` attribute, so echoing `get_form_fields` output back into `replace_form_fields` crashed `UpdateAllService`. The MCP serializer now drops the key (constraints are reported at the top level of the response).
…tening and Serializers::Base Direct unit coverage for Pagination (envelope, clamping, serialization paths), MultilocMerge, ResponseHelpers, the multiloc schema helper, JSONAPI flattening (id/ids suffixes, inlining) and the serializer base class (wraps, inline, from-scratch, collection semantics). base_tool_spec gains suite-wide definition invariants and `.unauthorized_message` coverage.
The request spec is the only place the HTTP gating is exercised: Doorkeeper `mcp:access` scope, `McpPolicy` (super admin + feature flag), the RFC 9728 WWW-Authenticate challenge, and end-to-end `tools/list` / `tools/call` through a real bearer token.
… Runner contract Download failures surface as `carrierwave_download_error` model validation errors (not the `CarrierWave::DownloadError` rescue, which turns out to be defensive-only for the remote_url path). Adds `stub_failing_remote_download` next to the existing download stubs.
Tool runners trust their input, so the controller now enables `validate_tool_call_arguments` explicitly instead of relying on the gem default. A request-spec example pins end to end that non-conforming input is rejected.
…able messages
The tool used to relay `UpdateAllService` failures as a raw JSON dump (`Validation failed: {"form":[{"error":"stale_data"}]}`), which gives an MCP client no hint of what to do. Form-level error keys now map to prose that says how to recover. Unmapped keys fall back to the raw key, and the raw error hash rides along in `structuredContent[:errors]`.
`remote_header_bg_url` was typed as a non-nullable string, so an MCP client could set a header background but never remove one. The field is now nullable and `update_project` translates an explicit null into `remove_header_bg!`.
…nts` and `list_causes`
With `visible_to: 'groups'`, the group list was invisible and unreachable: the project payload carried no group information, the create/update schemas had no way to set it, and `list_groups`' `project_id` filter matched smart-group *rules* referencing the project — not the groups that can see it. The project schemas now accept `group_ids` (replace-wholesale, like `area_ids`), the MCP project serializer surfaces `group_ids`, and the misleading `list_groups` filter is removed: the natural flow is reading `group_ids` from the project payload and resolving names via the unfiltered `list_groups`.
`paginated_response` had two serialization branches: MCP serializers, and an `as_json(only: ...)` fast path used by three flat-payload tools that also leaked string keys into `structuredContent`.
The mounts of all affected uploaders keep CarrierWave's default download/integrity validations, so download failures never raise out of `save!` — they are captured on assignment as `:carrierwave_download_error` validation errors and handled by the `RecordInvalid` rescue (as the download-failure specs assert). The dedicated `CarrierWave::DownloadError`/`IntegrityError` rescues were dead code on every path.
…clearable through the MCP
Three fields were settable but not clearable ("Pass null to …" now works for all of them):
- `end_at` is nullable in the phase schema; clearing it is the only way to make the last phase open-ended (the model already rejects it elsewhere).
- `access_denied_explanation_multiloc` accepts null and, while here, aligns with the other multiloc fields by merging per locale. An explicit null clears a multiloc to `{}`
- An explicit null on `remote_<uploader>_url` removes the file: CarrierWave ignores nil assignment, so a generic `Runner#clear_uploaders!` translates it to `remove_<uploader>!`, driven by the model's uploader registry.
JSON Schema accepts undeclared keys by default and the MCP gem passes arguments to tools verbatim (verified at runtime), so the two Runners that splat params into models were mass-assignable: `create_phase` (`Phase.new`) and `replace_form_fields`, whose field/option/matrix-statement params reach `assign_attributes` through `UpdateAllService` — an undeclared `resource_id` would re-parent a field to another form. These schemas now set `additionalProperties: false`. To keep the `get_form_fields` → `replace_form_fields` round-trip working with closed schemas, the two tools now share a single form-field schema (the builder's input/output modes are gone) and directional keys carry the standard JSON Schema annotations: `readOnly: true` on the server-owned keys (`created_at`, `updated_at`, `resource_id`), `writeOnly: true` on `temp_id`. The annotations are validation-neutral. `BaseTool.for` enforces the readOnly semantics by stripping annotated keys from the params at dispatch (`BaseTool::ReadonlyStrip`, unit-tested as a plain function). The strip walker deliberately supports only the schema subset the engine uses; a registry tripwire fails if a schema ever declares readOnly anywhere else.
9d71797 to
373f5f3
Compare
Contributor
Author
|
@amanda-anderson @kogre for whoever has the time... or the courage. |
amanda-anderson
approved these changes
Jul 14, 2026
amanda-anderson
left a comment
Contributor
There was a problem hiding this comment.
LGTM! Didn't look as deeply into the individual specs themselves, but the ones I did look at made sense to me :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The MCP engine was built velocity-first with minimal test coverage. This PR adds a systematic test suite, plus the production fixes the suite surfaced.
Production fixes (found while writing the suite)
update_project,update_phase,update_resourceandreplace_form_fieldscalled neitherauthorize_project!nor Punditauthorize, so the draft-only constraint was not enforced for updates: a super admin could mutate published projects through the MCP.get_form_fields→replace_form_fieldsround-trip —Serializers::CustomFieldemitted aconstraintskey that is not aCustomFieldattribute; echoing fetched fields back crashedUpdateAllService. The serializer now drops the key (constraints are reported at the top level).list_eventsandlist_causesfiltered without looking up the parent, so a nonexistentproject_id/phase_idreturned[](an LLM reads that as "no events" rather than "wrong id"). They now return the standard not-found error like their siblings.remote_header_bg_urlwas a non-nullable string; it now acceptsnull, whichupdate_projecttranslates intoremove_header_bg!(CarrierWave ignores plain nil assignment), mirroring the web API'sremove_image_if_requested!.visible_to: 'groups', the project payload carried no group information, the schemas had no way to set the group list, andlist_groups'project_idfilter matched smart-group rules referencing the project rather than the groups that can see it. The project schemas now acceptgroup_ids(replace-wholesale, likearea_ids), the serializer surfacesgroup_ids, and the misleading filter is removed (read ids from the payload, resolve names vialist_groups).replace_form_fieldsvalidation errors — form-level error keys (stale_data,locked_deletion, …) were relayed as a raw JSON dump; they now map to actionable prose (re-fetch and retry, echo locked fields back, structural page rules), with the raw errors still instructuredContent.end_at(the only way to make the last phase open-ended),access_denied_explanation_multilocand uploader files (remote_<x>_url) now acceptnullto clear. Uploader clearing is a generic Runner helper driven by CarrierWave's mount registry (nil assignment is otherwise silently ignored); the permission multiloc also now merges per locale like every other multiloc field instead of replacing wholesale.save!(CarrierWave converts them into:carrierwave_download_errorvalidation errors on assignment), so the dedicated rescues in the attach tools were dead code on every path.create_phase(Phase.new(**params)) andreplace_form_fields(assign_attributesviaUpdateAllService— includingresource_id, which re-parents a field to another form) were mass-assignable. Their schemas are now closed withadditionalProperties: false. To keep the fetch → edit → replace round-trip working, the form-field schema is now shared between both tools with standardreadOnly/writeOnlyannotations on the directional keys;BaseTool.forstripsreadOnlykeys from the params at dispatch (generic schema walker, unit-tested as a function), and a registry tripwire fails with the offending JSON-Pointer path if areadOnlyever lands where the walker can't reach it (onlyproperties/itemspositions are supported — full coverage would take a JSON Schema evaluator).BaseTool::Authorizationconcern is gone (itspundit_userwas shadowed byPundit::Authorization's identical default; the Runner now includes Pundit directly), and the draft-guard refusal no longer ends in a doubled period (reasons own their punctuation; the template only terminates its own fallback).Refactors that rode along
paginated_response'sas_json(only: …)fast path is gone; the three flat-payload tools got from-scratch serializers (nowraps), andSerializers::Base#to_henforces symbols-as-top-level-keys so payload key types can't diverge.MCP::Configuration.new(validate_tool_call_arguments: true)instead of relying on the gem default; Runners trust schema-validated input, and a request-spec example pins the rejection end to end.Coverage (lines,
engines/commercial/mcp_server/app/**)The two uncovered lines are defensive branches (an unreachable
RecordInvalidrescue inreplace_form_fields, the non-redirectable branch of the OAuth authorize endpoint).Notes / follow-ups (not addressed here)
additionalProperties: falsestructural instead of per-tool discipline.policy_scope) instead of querying unscoped. TodayMcpPolicyrestricts the endpoint to super admins so nothing leaks, but scoping would make the invariant local.Changelog
Technical