Fix Forms agent overwriting drafts on unrelated create requests - #5034
Conversation
builder-io-integration
Bot
changed the title
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🟡
Review Details
Code Review Summary
PR #5034 adds two defenses against unrelated Forms requests overwriting the open draft: clearer create/update routing guidance and a structural update-form check that rejects whole-schema replacements when they discard most existing questions. The focused implementation and regression tests cover the reported four-field Customer Feedback → Event Registration scenario, ordinary appends, explicit rewrites, and separate form creation. This is a standard-risk business-logic change.
Key Findings
- 🟡 MEDIUM — The loss detector's label fallback uses a
Set, so one incoming field can count as retaining multiple existing fields with duplicate labels. - 🟡 MEDIUM — Forms with exactly two existing fields can lose one field without confirmation because the detector requires at least two dropped fields.
- 🟡 MEDIUM — The new
unconfirmed_field_lossrecovery path conflicts with the existing generic retry/repair instruction in the agent-chat prompt, which may cause the model to retry against the open draft instead of callingcreate-form.
The action-level tests reported by reviewers passed, including the new guard and unrelated-request regression suites. 🧪 Browser testing: Attempted FULL verification on /forms, but all executor sessions lacked browser automation tools; the dev server was healthy, so the 12 planned cases remain unverified due to environment tooling.
| return !(label && incomingLabels.has(label)); | ||
| }); | ||
|
|
||
| if (dropped.length < MIN_DROPPED_FIELDS) return null; |
There was a problem hiding this comment.
🟡 Protect two-field forms from unconfirmed half-schema replacements
The absolute dropped.length >= 2 threshold leaves a two-field form unprotected when a replacement retains one generic field and drops the other. That is still a 50% schema loss and can reproduce the unrelated-form overwrite for small forms; handle this small-form case and add a regression test.
Additional Info
Found by 1 of 3 review agents.
There was a problem hiding this comment.
Required — not fixing: I disagree that this is a gap worth closing, and closing it would cost more than it saves.
The only uncovered shape is a 2-field form losing exactly 1 field. A 2-field form losing both already fires (dropped=2), so the fully-unrelated overwrite this PR targets is caught at every form size. Dropping 1 field is the single most common legitimate edit, and at 2 fields it is indistinguishable from "remove the phone question" — lowering the threshold there would reject that ordinary edit and push the agent toward confirmReplaceFields, which is the habit this guard exists to discourage.
Percentage is not the only signal; absolute count matters. The harm in the report was an entire form becoming unrecoverable. One lost question on a 2-field form is a visible, re-askable mistake. MIN_DROPPED_FIELDS=2 is the deliberate line between an edit and a rewrite, and I would rather leave that narrow case to the routing guidance than make a frequent edit fail.
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply in each open inline thread with exactly one of:
Or resolve the thread in GitHub. Outdated threads after new commits do not need a new reply. Get CI green and keep the branch mergeable. |
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest revision correctly fixes the duplicate-label matching bug by consuming label matches one-to-one, and it adds an explicit unconfirmed_field_loss exception to the generic retry rule. The revised focused tests reported passing, including duplicate-label coverage. The previously reported two-field threshold issue remains present and was intentionally not reposted because its existing review comment is still open.
New Finding
- 🟡 MEDIUM — The new retry exception routes every
unconfirmed_field_losstocreate-form, but an explicit request to remove multiple questions from the current form should usepatch-form-fields. This conflicts with the action error's own recovery guidance and can create an unwanted duplicate form instead of completing the requested edit.
The duplicate-label and retry-conflict comments were verified fixed and resolved. 🧪 Browser testing: Attempted FULL verification on /forms; the dev server was healthy, but all executor sessions lacked Chrome/browser automation tools, so the planned cases remain unverified due to environment tooling.
|
|
||
| Core rules: | ||
| - Build every field as a complete object with id, type, label, and required; never send shorthand strings such as "text: Enter a name". If a form or field action rejects its payload, correct the arguments and retry in the same turn, then verify the saved fields. If a draft was created before the failure, repair that draft instead of explaining the fix and stopping. | ||
| - Build every field as a complete object with id, type, label, and required; never send shorthand strings such as "text: Enter a name". If a form or field action rejects its payload, correct the arguments and retry in the same turn, then verify the saved fields. If a draft was created before the failure, repair that draft instead of explaining the fix and stopping. The one exception is \`unconfirmed_field_loss\` from \`update-form\`: that is not a payload error and must not be retried against the same form. It means the call was about to erase questions the user still wants, so call \`create-form\` for the new form. Only set \`confirmReplaceFields: true\` when the user explicitly asked to rewrite that specific form in place. |
There was a problem hiding this comment.
🟡 Route intentional multi-field removals to patch-form-fields
The new exception sends every unconfirmed_field_loss to create-form, but an explicit request to remove multiple questions from the current form should use patch-form-fields. This conflicts with the action error's recovery guidance and can create an unwanted duplicate form instead of completing the requested edit; distinguish new-form requests from explicit removals while still prohibiting a retry of the destructive whole-array replacement.
Additional Info
New finding in the incremental revision; identified by 1 of 2 review agents. Previous retry-conflict comment at this location was resolved because the generic retry conflict is fixed, but this is a distinct recovery-path issue.
|
There was a problem with your request, please try again later. Error id: |
3 participants
Summary
Fixes a tool-routing bug in the Forms agent where an unrelated "build a new form" prompt would call
update-formand silently overwrite the currently open draft instead of creating a new form.Problem
In the Forms app chat, asking the agent to create a new, unrelated form (e.g. an "Event Registration" form) after already having a draft open (e.g. "Customer Feedback") caused the agent to call
update-formand overwrite the open draft's schema instead of callingcreate-form. The agent treated the currently open/most-recently-created form as an implicit target for any new form-creation request, destroying the original form's data with no recovery path.Factory item:
dd2c66d2d5219b22d324a0d540d0ac068bbc9ae9aea96f7634fe6a748a8b9ee6Source Slack thread: https://slack.com/app_redirect?team=T0GCV21GE&channel=C0ATH3CCZT4&message_ts=1789447719.033169
Solution
Two layers of defense:
AGENTS.md, and the agent-chat system prompt to explicitly state that each described form is its own form, and that an open/recent form in<current-screen>is context, not a default write target. Prompts describing a different purpose, audience, or set of questions must route tocreate-form, notupdate-form.update-formthat detects when a whole-arrayfieldsreplacement would discard most of the form's existing questions ("mass field loss"). If detected and not explicitly confirmed, the call fails withunconfirmed_field_loss, pointing the caller tocreate-formfor a new form or aconfirmReplaceFields: trueflag for a genuine in-place rewrite.Key Changes
actions/lib/assert-not-unconfirmed-field-loss.ts: newdetectMassFieldLoss(structural diff comparing existing vs. incoming fields by id/label, ignoring small edits, appends, or id regeneration) andassertNotUnconfirmedFieldLoss(fails withunconfirmed_field_loss/409 unless confirmed).actions/update-form.ts: calls the new guard before replacingfields; adds aconfirmReplaceFieldsschema option; updated tool description to clarifyupdate-formis only for edits to that specific form, andcreate-formshould be used for a different form.actions/create-form.ts: updated description to state it should be used for every new form request, even when another form is currently open or was created earlier in the conversation.server/plugins/agent-chat.ts: added a system prompt rule reinforcing "one request, one form" routing.AGENTS.mdand.agents/skills/form-building/SKILL.md,.agents/skills/form-publishing/SKILL.md: added "one request, one form" guidance with a decision table, documented theunconfirmed_field_losserror, and consolidated email-notification docs intoform-publishing.actions/lib/assert-not-unconfirmed-field-loss.spec.ts: unit tests for the mass-field-loss detection and assertion logic.actions/unrelated-form-request.spec.ts: end-to-end simulation of two sequential unrelated form-creation prompts, asserting the second request is rejected when misrouted toupdate-form, results in two separate forms when correctly routed tocreate-form, still allows explicit in-place rewrites viaconfirmReplaceFields, and leaves normal edits to the open form unaffected.To clone this PR locally use the Github CLI with command
gh pr checkout 5034You can tag me at @BuilderIO for anything you want me to fix or change