From c6b443ff5c7a423681de017db257300c63bab286 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Fri, 9 Oct 2026 22:14:53 -0400 Subject: [PATCH 1/2] Refuse gf_update_field input it would ignore gf_update_field read field changes only from `properties`. A property sent beside it, such as `placeholder`, was dropped, the form was written back unchanged, and the call reported success. An agent had no way to tell the change never happened. The handler now refuses any top-level key it does not accept and names the property with the `properties: { ... }` shape that works. A missing, non-object, or empty `properties` is refused as well. Refusing matches how the server treats other input that would do nothing (field_values on gf_submit_form_data, top-level paging keys on gf_list_entries), rather than quietly merging the keys in. Fixes MCP-24: https://linear.app/gravitykit/issue/MCP-24/gf-update-field-reports-success-and-changes-nothing-when-a-field --- CHANGELOG.md | 6 ++ package.json | 2 +- src/field-operations/index.js | 30 +++++++++- test/update-field-input.test.js | 101 ++++++++++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 test/update-field-input.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f65f0d..a36c539 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to GravityKit MCP (formerly GravityMCP) will be documented i The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### 🐛 Fixed + +- **`gf_update_field` no longer reports success for a change it did not make.** A field property sent beside `properties` instead of inside it, such as `placeholder: "Your name"`, was dropped and the call still said it worked. The call is now refused, and the reply names the property and shows it nested under `properties`. A call with no `properties`, or an empty one, is refused too. + ## [2.6.0] - 2026-10-01 This release improves how Gravity Forms tools save the values they are sent: checkbox, multiselect, name and address values have improved structures. Requests that cannot save a value say so instead of reporting success. Feed and form updates keep the settings that weren't touched, and field edits made at the same time no longer overwrite each other. Card numbers, security codes and passwords are kept out of entries. Two library dependencies were updated based on security advisories. diff --git a/package.json b/package.json index b00641c..b5139bb 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "test": "node test/integration.test.js", "test:unit": "node test/run.js", "verify:schemas": "node scripts/verify-ability-schemas.mjs", - "test:node": "node --test test/field-manager.test.js test/field-registry.test.js test/field-dependencies.test.js test/field-positioner.test.js test/field-list-types.test.js test/helpers.test.js test/ability-catalog.test.js test/user-agent.test.js test/wp-client.test.js test/server-runtime.test.js test/entries-query.test.js test/entries-wire.test.js test/feeds-wire.test.js test/error-details.test.js test/results-wire.test.js test/client-hardening.test.js test/validation-hardening.test.js test/sanitize-hardening.test.js test/schema-hardening.test.js test/instructions.test.js test/logger-stdout.test.js test/server-lifecycle.test.js test/bench-grading.test.js test/bench-agent.test.js test/bench-cleanup.test.js test/bench-search-layout.test.js test/tool-description-claims.test.js test/entry-writes.test.js test/rejected-paging.test.js test/entry-dates.test.js test/submission-field-values.test.js test/send-notifications.test.js test/server-owned-params.test.js test/merge-guard.test.js test/entry-value-shapes.test.js test/compound-value-shapes.test.js test/field-concurrency.test.js test/sensitive-entry-values.test.js test/suite-registration.test.js", + "test:node": "node --test test/field-manager.test.js test/field-registry.test.js test/field-dependencies.test.js test/field-positioner.test.js test/field-list-types.test.js test/helpers.test.js test/ability-catalog.test.js test/user-agent.test.js test/wp-client.test.js test/server-runtime.test.js test/entries-query.test.js test/entries-wire.test.js test/feeds-wire.test.js test/error-details.test.js test/results-wire.test.js test/client-hardening.test.js test/validation-hardening.test.js test/sanitize-hardening.test.js test/schema-hardening.test.js test/instructions.test.js test/logger-stdout.test.js test/server-lifecycle.test.js test/bench-grading.test.js test/bench-agent.test.js test/bench-cleanup.test.js test/bench-search-layout.test.js test/tool-description-claims.test.js test/entry-writes.test.js test/rejected-paging.test.js test/entry-dates.test.js test/submission-field-values.test.js test/send-notifications.test.js test/server-owned-params.test.js test/merge-guard.test.js test/entry-value-shapes.test.js test/compound-value-shapes.test.js test/field-concurrency.test.js test/sensitive-entry-values.test.js test/suite-registration.test.js test/update-field-input.test.js", "test:auth": "node test/authentication.test.js", "test:forms": "node test/forms.test.js", "test:entries": "node test/entries.test.js", diff --git a/src/field-operations/index.js b/src/field-operations/index.js index 075d7e9..b4a3995 100644 --- a/src/field-operations/index.js +++ b/src/field-operations/index.js @@ -90,6 +90,33 @@ function resolveFeatureKey(feature, registry) { return FEATURE_FLAGS[folded]; } +// compact and test_mode are control flags the server strips before the handler +// runs; they are listed so a direct caller passing them is not refused. +const UPDATE_FIELD_KEYS = new Set(['form_id', 'field_id', 'properties', 'force', 'test_mode', 'compact']); + +/** + * Refuse a gf_update_field call whose field changes would be dropped. + * Field changes are read only from `properties`; anything beside it, or an + * empty `properties`, would write the form back unchanged and still report + * success. + */ +function assertUpdateFieldInput(params = {}) { + const stray = Object.keys(params).filter((key) => !UPDATE_FIELD_KEYS.has(key)); + if (stray.length > 0) { + const example = stray.map((key) => `${key}: ${JSON.stringify(params[key])}`).join(', '); + throw new Error(`gf_update_field reads field changes only from "properties", so ${stray.join(', ')} would be ignored: pass them as properties: { ${example} }`); + } + + const { properties } = params; + const isObject = properties !== null && typeof properties === 'object' && !Array.isArray(properties); + if (!isObject) { + throw new Error('gf_update_field needs "properties", an object of the field properties to change (e.g. properties: { label: "Full name" })'); + } + if (Object.keys(properties).length === 0) { + throw new Error('gf_update_field was given an empty "properties" object, so there is nothing to change: list the field properties to update (e.g. properties: { label: "Full name" })'); + } +} + /** * Field operation tool handlers for MCP integration */ @@ -117,6 +144,7 @@ export const fieldOperationHandlers = { * Update field properties */ async gf_update_field(params, { fieldManager }) { + assertUpdateFieldInput(params); const { form_id, field_id, properties, force = false } = params; // updateField gates the write on dependencies and returns the final @@ -340,7 +368,7 @@ export const fieldOperationTools = [ }, properties: { type: 'object', - description: 'Properties to update' + description: 'Field properties to change, e.g. { placeholder: "Your name" }. Every field change goes here; a field property sent at the top level is refused.' }, force: { type: 'boolean', diff --git a/test/update-field-input.test.js b/test/update-field-input.test.js new file mode 100644 index 0000000..8a307dc --- /dev/null +++ b/test/update-field-input.test.js @@ -0,0 +1,101 @@ +/** + * gf_update_field refuses input it would otherwise ignore. + * + * Measured 2026-10-09: { form_id: 73, field_id: 12, placeholder: "—" } returned + * success: true and the field kept no placeholder, because the handler reads + * field changes only from `properties`. The empty update still wrote the form + * back unchanged, so the caller had every reason to believe it had worked. + * + * The server's rule for input that would do nothing is to refuse it and name + * the shape that works (field_values on gf_submit_form_data, top-level paging + * keys on gf_list_entries). These tests pin that rule for gf_update_field, and + * that a refused call never writes the form. + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { fieldOperationHandlers, createFieldOperations } from '../src/field-operations/index.js'; +import fieldRegistry from '../src/field-definitions/field-registry.js'; +import FieldAwareValidator from '../src/config/field-validation.js'; + +const FORM_ID = 73; + +function makeRig() { + const site = { + form: { id: FORM_ID, title: 'Contact', fields: [{ id: 12, type: 'text', label: 'Name' }] }, + writes: 0, + }; + const clone = (value) => JSON.parse(JSON.stringify(value)); + const api = { + async getForm() { + return { form: clone(site.form) }; + }, + async replaceForm(id, form) { + site.writes += 1; + site.form = clone(form); + return { form: clone(form) }; + }, + }; + const ops = createFieldOperations(api, fieldRegistry, new FieldAwareValidator()); + const update = (params) => fieldOperationHandlers.gf_update_field(params, ops); + return { site, update }; +} + +test('a field property passed at the top level is refused, not ignored', async () => { + const { site, update } = makeRig(); + + await assert.rejects( + () => update({ form_id: FORM_ID, field_id: 12, placeholder: '—' }), + (error) => { + assert.match(error.message, /placeholder/, 'the ignored key is named'); + assert.match(error.message, /properties: \{ placeholder/, 'the working shape is shown'); + return true; + } + ); + assert.equal(site.writes, 0, 'a refused call writes nothing'); + assert.equal(site.form.fields[0].placeholder, undefined); +}); + +test('top-level keys are refused even when properties is also given', async () => { + const { site, update } = makeRig(); + + await assert.rejects( + () => update({ form_id: FORM_ID, field_id: 12, properties: { label: 'Full name' }, cssClass: 'wide', isRequired: true }), + /cssClass, isRequired/ + ); + assert.equal(site.writes, 0); + assert.equal(site.form.fields[0].label, 'Name', 'the half that was nested is not applied either'); +}); + +test('a call with no properties is refused', async () => { + const { site, update } = makeRig(); + + await assert.rejects(() => update({ form_id: FORM_ID, field_id: 12 }), /properties/); + assert.equal(site.writes, 0); +}); + +test('an empty properties object is refused', async () => { + const { site, update } = makeRig(); + + await assert.rejects(() => update({ form_id: FORM_ID, field_id: 12, properties: {} }), /properties/); + assert.equal(site.writes, 0); +}); + +test('properties that is not an object is refused', async () => { + const { site, update } = makeRig(); + + for (const bad of ['placeholder', ['placeholder'], 5]) { + await assert.rejects(() => update({ form_id: FORM_ID, field_id: 12, properties: bad }), /properties/); + } + assert.equal(site.writes, 0); +}); + +test('properties nested correctly still updates the field', async () => { + const { site, update } = makeRig(); + + const result = await update({ form_id: FORM_ID, field_id: 12, properties: { placeholder: '—' }, force: false }); + + assert.equal(result.success, true); + assert.equal(site.writes, 1); + assert.equal(site.form.fields[0].placeholder, '—'); +}); From f6f036b88d4737f3a79167c5248ab8826b0484db Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Fri, 9 Oct 2026 22:19:09 -0400 Subject: [PATCH 2/2] Refuse a gf_update_field properties.id it would ignore FieldManager restores the stored id after merging properties, so a properties.id different from field_id was dropped while the call reported success, the same defect as a top-level property. It is now refused. An id equal to field_id is still accepted, so a field read back with gf_get_form can be edited and sent whole; an object holding only that id counts as empty. Raised in CodeRabbit's review of PR #20. Ref MCP-24: https://linear.app/gravitykit/issue/MCP-24/gf-update-field-reports-success-and-changes-nothing-when-a-field --- src/field-operations/index.js | 8 +++++++- test/update-field-input.test.js | 28 ++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/field-operations/index.js b/src/field-operations/index.js index b4a3995..1a85c83 100644 --- a/src/field-operations/index.js +++ b/src/field-operations/index.js @@ -112,7 +112,13 @@ function assertUpdateFieldInput(params = {}) { if (!isObject) { throw new Error('gf_update_field needs "properties", an object of the field properties to change (e.g. properties: { label: "Full name" })'); } - if (Object.keys(properties).length === 0) { + // An id equal to field_id is allowed so a field read back can be sent whole. + const hasId = Object.prototype.hasOwnProperty.call(properties, 'id'); + if (hasId && String(properties.id) !== String(params.field_id)) { + throw new Error(`gf_update_field cannot change a field's id (properties.id ${JSON.stringify(properties.id)}, field_id ${JSON.stringify(params.field_id)}): remove id from properties`); + } + const changes = Object.keys(properties).filter((key) => key !== 'id'); + if (changes.length === 0) { throw new Error('gf_update_field was given an empty "properties" object, so there is nothing to change: list the field properties to update (e.g. properties: { label: "Full name" })'); } } diff --git a/test/update-field-input.test.js b/test/update-field-input.test.js index 8a307dc..91c3120 100644 --- a/test/update-field-input.test.js +++ b/test/update-field-input.test.js @@ -99,3 +99,31 @@ test('properties nested correctly still updates the field', async () => { assert.equal(site.writes, 1); assert.equal(site.form.fields[0].placeholder, '—'); }); + +test('a properties.id that differs from field_id is refused, since ids cannot change', async () => { + const { site, update } = makeRig(); + + await assert.rejects( + () => update({ form_id: FORM_ID, field_id: 12, properties: { id: 99, label: 'Full name' } }), + /id/ + ); + assert.equal(site.writes, 0); + assert.equal(site.form.fields[0].label, 'Name'); +}); + +test('a properties.id equal to field_id is accepted, so a field read back can be sent as-is', async () => { + const { site, update } = makeRig(); + + const result = await update({ form_id: FORM_ID, field_id: 12, properties: { id: '12', label: 'Full name' } }); + + assert.equal(result.success, true); + assert.equal(site.form.fields[0].label, 'Full name'); + assert.equal(site.form.fields[0].id, 12); +}); + +test('properties holding only the matching id is refused as empty', async () => { + const { site, update } = makeRig(); + + await assert.rejects(() => update({ form_id: FORM_ID, field_id: 12, properties: { id: 12 } }), /nothing to change/); + assert.equal(site.writes, 0); +});