Frontmatter
| title | fix(mcp): preserve open-bag input payloads through Zod validation (#10070) |
| author | tobiu |
| state | Merged |
| createdAt | Apr 18, 2026, 10:50 PM |
| updatedAt | Apr 18, 2026, 11:03 PM |
| closedAt | Apr 18, 2026, 11:03 PM |
| mergedAt | Apr 18, 2026, 11:03 PM |
| branches | dev ← agent/10070-input-open-bag |
| url | https://github.com/neomjs/neo/pull/10071 |

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).isRootexclusion 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 readingbuildZodSchemaFromNodegets the context. Deduction: my priorfeedback_mcp_output_category_split.mdfrom 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 validatedset_instance_propertiesend-to-end via ButtonBaseNL.spec.mjs;find_instances,query_component,query_vdom,modify_state_provider,manage_neo_configrely 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]: Myfeedback_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 exceptset_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 forfind_instances/query_component/query_vdomwhich are read-path tools used heavily by agent introspection workflows.[RETROSPECTIVE]: Three durable lessons from this PR's diagnosis:
- False-positive verification trap: Earlier in this same session I reported
set_instance_propertiesworking based on{success: true}alone. A read-after-write would have caught the regression ~2 hours earlier. Codified infeedback_verify_effect_not_just_success.md. Carrying this forward as a hard verification gate for all MCP writes.- 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.
- Memory-first reflex paid off: Querying
query_raw_memoriesat 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.mdto 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.
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 whenset_instance_propertiesdidn'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 strippedRoot Cause
Before #10043,
buildZodSchemaused a hardcoded switch wheretype: objectfell through toz.any()(preserves payload as-is). #10043's recursion refactor emits strictz.object({})fortype: objectdeclarations that have no childproperties— Zod's default strict parse then drops every unknown key.Six neural-link tools declare
type: objectwith nopropertiesintentionally — the field is an open bag where the caller decides the shape:set_instance_properties.propertiesfind_instances.selectorquery_component.selectorquery_vdom.selectormodify_state_provider.datamanage_neo_config.configFix
In ai/mcp/validation/OpenApiValidator.mjs
buildZodSchemaFromNode, when walkingtype: objectthat has no declaredpropertiesAND no declaredadditionalProperties, emitz.object({}).passthrough()instead of strictz.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:
properties→ strict (unknown top-level keys = caller error)properties+ noadditionalProperties→ open-bag passthrough (this fix)additionalProperties: <schema>→ respects declaration (existing behavior){lenient}pathThis extends the schema-intent categorization the codebase has been building in #10064 / #9837 / #10067:
.passthrough()(#10067)Verification
Unit spec
Extended test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs with:
set_instance_properties.properties(the exact regression)findSilentlyStrippingOpenBagswalker: asserts no nested input object emits{type: object, no properties, additionalProperties: false}— the exact JSON-Schema shape that triggers Zod's payload stripWhitebox 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:
All three mutation tests (direct component mutation, user-driven UI → config → button, agent-driven NL → config → button) now round-trip correctly.
Avoided Traps
propertiesstay strict — unknown fields there remain legitimate caller errors (regression-guarded by the existing #9837 input-strictness test).additionalProperties: trueto 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-d58ae9a9d729bisected the regression in ~15 minutes by applying the memory-first reflex formalized in #10068/#10069:textand_textunchanged → NL routing, not reactivityz.object({})strict empty-object behavior → fix scopedThe 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-d58ae9a9d729Resolves #10070