LearnNewsExamplesServices
Frontmatter
titlefix(mcp): preserve open-bag input payloads through Zod validation (#10070)
authortobiu
stateMerged
createdAtApr 18, 2026, 10:50 PM
updatedAtApr 18, 2026, 11:03 PM
closedAtApr 18, 2026, 11:03 PM
mergedAtApr 18, 2026, 11:03 PM
branchesdevagent/10070-input-open-bag
urlhttps://github.com/neomjs/neo/pull/10071
Merged
tobiu
tobiu commented on Apr 18, 2026, 10:50 PM

Summary

Six Neural Link write tools have been silently no-op'ing since #10043. The MCP call returned {success: true} while the actual payload was stripped during Zod validation and never reached the worker. User tobiu surfaced it when set_instance_properties didn't change a button's DOM text despite a "success" response.

Concrete reproduction (pre-fix):

buildZodSchema(doc, setInstancePropertiesOp).parse({id: 'neo-button-1', properties: {text: 'X'}})
// → {id: 'neo-button-1', properties: {}}    payload silently stripped

Root Cause

Before #10043, buildZodSchema used a hardcoded switch where type: object fell through to z.any() (preserves payload as-is). #10043's recursion refactor emits strict z.object({}) for type: object declarations that have no child properties — Zod's default strict parse then drops every unknown key.

Six neural-link tools declare type: object with no properties intentionally — the field is an open bag where the caller decides the shape:

Tool Broken field Effect (pre-fix)
set_instance_properties .properties Silent no-op on config writes
find_instances .selector Selector empty — matches all or none
query_component .selector Same
query_vdom .selector Same
modify_state_provider .data Silent no-op on state updates
manage_neo_config .config Silent no-op on global config changes

Fix

In ai/mcp/validation/OpenApiValidator.mjs buildZodSchemaFromNode, when walking type: object that has no declared properties AND no declared additionalProperties, emit z.object({}).passthrough() instead of strict z.object({}). Intent signal: the absence of a declared shape means "caller decides the keys". Strict parsing only makes sense when the author explicitly listed expected fields.

Branches:

  • Input + has declared propertiesstrict (unknown top-level keys = caller error)
  • Input + no declared properties + no additionalPropertiesopen-bag passthrough (this fix)
  • Input + explicit additionalProperties: <schema> → respects declaration (existing behavior)
  • Output side → unchanged, still lenient via #10067's earlier {lenient} path

This extends the schema-intent categorization the codebase has been building in #10064 / #9837 / #10067:

Side Has declared properties? Correct strategy
Input (this PR) No Open — the omission signals "caller decides the shape"
Input Yes Strict — unknown keys are caller errors
Output (closed contract) Yes, stable Strict OK
Output (runtime introspection) Varies / none Open via .passthrough() (#10067)

Verification

Unit spec

Extended test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs with:

  • Direct payload-preservation assertion for set_instance_properties.properties (the exact regression)
  • Per-server findSilentlyStrippingOpenBags walker: asserts no nested input object emits {type: object, no properties, additionalProperties: false} — the exact JSON-Schema shape that triggers Zod's payload strip
$ npx playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/ai/mcp/validation/
  23 passed (934ms)

Whitebox E2E

The existing test/playwright/e2e/ButtonBaseNL.spec.mjs was passing 1/4 before this PR (only the non-mutating "Layout vs DOM Physics" test survived). After the fix:

$ npx playwright test test/playwright/e2e/ButtonBaseNL.spec.mjs -c test/playwright/playwright.config.e2e.mjs
  4 passed (8.0s)

All three mutation tests (direct component mutation, user-driven UI → config → button, agent-driven NL → config → button) now round-trip correctly.

Avoided Traps

  • Not reverting #10043. Its recursion refactor was correct for declared object shapes; the bug was only in the undeclared-properties edge case.
  • Not blanket-loosening all input validation. Inputs with declared properties stay strict — unknown fields there remain legitimate caller errors (regression-guarded by the existing #9837 input-strictness test).
  • Not updating the six handlers to re-extract raw request bodies. The validator is the right layer; per-handler fixes would scatter the same workaround six times.
  • Not bundling an explicit YAML audit (add additionalProperties: true to the 6 affected schemas). Worth doing eventually for clarity, but the validator change handles it robustly today. Flagged as optional Step 3 in #10070's body.

Direct Motivation

Session 7258d884-6c5d-40d5-b180-d58ae9a9d729 bisected the regression in ~15 minutes by applying the memory-first reflex formalized in #10068/#10069:

  1. User reported set_instance_properties no-op
  2. Memory query showed prior successful uses in 2026-02-25 + 2026-03-24 → real regression confirmed
  3. ButtonBaseNL.spec.mjs dropped 1/4 → framework-wide
  4. Read+write bisection: both text and _text unchanged → NL routing, not reactivity
  5. Traced to Zod z.object({}) strict empty-object behavior → fix scoped

The bisection also surfaced a durable lesson saved as feedback memory: {success: true} from an MCP write is not proof of effect. Always read-back-after-write. The hollow success response in this bug looks identical to real success.

Origin Session ID

7258d884-6c5d-40d5-b180-d58ae9a9d729

Resolves #10070

tobiu
tobiu commented on Apr 18, 2026, 10:53 PM

Input from Claude Opus 4.7 (Claude Code):

❋ # PR Review Summary

Status: Comment (Self-Review)

Self-Review Opening: Self-review of #10070. The tightest, highest-impact PR of this session's arc — restores actual user-facing functionality (six silently-broken Neural Link write tools) rather than tolerating drift. The fix itself is one conditional branch. What makes this PR worth scrutinizing is the diagnosis path: I fell for the exact false-positive pattern earlier in this session, then had to live-bisect my way out. Honest accounting below.

Tech Debt Radar: deliberately not invoked. Single-branch fix to buildZodSchemaFromNode, no architectural shift. Scope confined to a specific schema-validation edge case.


📊 Evaluation Metrics

  • [ARCH_ALIGNMENT]: 90 — Clean extension of the schema-intent categorization the codebase has been evolving through #10064 / #9837 / #10067. The "undeclared properties = open-bag" rule lands as a third category in a coherent four-cell table (input × output × declared-shape × undeclared). isRoot exclusion in the spec walker is the right nuance — top-level input strictness is intentional, only nested open-bags were the trap.
  • [CONTENT_COMPLETENESS]: 92 — Full root-cause chain with concrete repro, before/after test numbers, category table expansion. Inline code comment references #10070 so the next agent reading buildZodSchemaFromNode gets the context. Deduction: my prior feedback_mcp_output_category_split.md from earlier today is now incomplete — it framed two output categories but didn't consider inputs. Should be updated post-merge to cover the full four-cell table.
  • [EXECUTION_QUALITY]: 93 — Two-layer verification: unit spec asserts the validator-level invariant (23/23), whitebox e2e asserts the end-to-end user-facing behavior (4/4 up from 1/4). Clean diffs. Honest deduction: e2e coverage for the other 5 affected tools doesn't exist. I validated set_instance_properties end-to-end via ButtonBaseNL.spec.mjs; find_instances, query_component, query_vdom, modify_state_provider, manage_neo_config rely on the unit-level guard alone. Parametrized e2e probe per tool would close this gap.
  • [PRODUCTIVITY]: 100 — Full workflow with tobiu's corrections from prior PRs baked in: assigned first, dedup swept, read-back verification applied (the diagnostic step that cracked it).
  • [IMPACT]: 95 — Six production-critical NL write tools restored. Higher impact than my earlier PRs in this arc because it restores core functionality rather than tolerating symptoms.
  • [COMPLEXITY]: 40 — Small code delta, moderate cognitive work (distinguishing four validation branches + understanding Zod's strict-object semantics + proving the fix doesn't leak into input strictness).
  • [EFFORT_PROFILE]: Quick Win — high impact for ~20 validator lines + ~50 test lines.

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #10070
  • Related Graph Nodes: Sibling to #10064 (array-missing-items), #9837 (output-schema-drift, my prior PR #10067). All three are post-#10043 discoveries — same validator refactor exposed strictness inversions in three distinct code paths.
  • Origin Session ID: 7258d884-6c5d-40d5-b180-d58ae9a9d729

🧠 Graph Ingestion Notes

  • [KB_GAP]: My feedback_mcp_output_category_split.md (saved earlier today after #9837) is now stale — it named two output categories only and missed the input symmetry this PR exposed. The full taxonomy has four cells. Worth an update so future agents see the complete picture when reasoning about schema strictness.
  • [TOOLING_GAP]: Five of the six affected tools (all except set_instance_properties) have no end-to-end coverage proving they work post-fix. Unit-level payload-preservation guard catches the class of bug, but per-tool e2e smoke tests would be stronger. Worth a targeted follow-up — especially for find_instances/query_component/query_vdom which are read-path tools used heavily by agent introspection workflows.
  • [RETROSPECTIVE]: Three durable lessons from this PR's diagnosis:
    1. False-positive verification trap: Earlier in this same session I reported set_instance_properties working based on {success: true} alone. A read-after-write would have caught the regression ~2 hours earlier. Codified in feedback_verify_effect_not_just_success.md. Carrying this forward as a hard verification gate for all MCP writes.
    2. Ambitious refactors seed multi-session bugs: #10043 was a single refactor that produced #9837 (output side) and now #10070 (input side) as post-merge discoveries. Future large recursive refactors in validation code need explicit payload round-trip tests at merge time, not just emitted-schema-shape tests. Worth considering as a commit-gate rule for this specific file.
    3. Memory-first reflex paid off: Querying query_raw_memories at minute one returned prior-successful uses from 2026-02-25 + 2026-03-24 in under 30 seconds, confirming real regression vs misconfiguration. Treated the symptom as diagnosable-not-noise from the start. Exactly the pattern #10068/#10069 formalized — this diagnosis is the first evidence the reflex works.

📋 Required Actions

  • Human QA: merge + refresh devindex, visually confirm set_instance_properties({text: ...}) on a button produces the expected DOM update.
  • (Optional follow-up) Update feedback_mcp_output_category_split.md to reflect the four-cell input × output × declared/undeclared taxonomy from this PR's body.
  • (Optional follow-up) Add parametrized e2e smoke coverage for the remaining 5 affected tools — [TOOLING_GAP] #2 above.
  • (Optional follow-up, tracked in #10070 body) Step 3: make the six YAML declarations explicit with additionalProperties: true.

No block-level code changes. The fix is tight and verified at both layers.

Handoff: awaiting human merge per pull-request-workflow.md §5 (Human-in-the-Loop). I will not self-merge.