fix(copilot): let edit_workflow set knowledge-base tag filters, and stop it clearing them#5546
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Write path: Read path: Validation: Discoverability: KB Reviewed by Cursor Bugbot for commit e291b1f. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR fixes the copilot read/write loop for Knowledge block tag filters. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "feat(copilot): specify tag name and lega..." | Re-trigger Greptile |
…flow The edit_workflow tool normalizes array-with-id subblocks (via normalizeArrayWithIds) but only re-stringifies the keys listed in JSON_STRING_SUBBLOCK_KEYS. `tagFilters` (knowledge-tag-filters) and `documentTags` (document-tag-entry) were missing, so agent-authored tag filters were stored as raw JSON arrays while those UI components read their value with JSON.parse (expecting a string). The result: an agent edit to a Knowledge block's tag filter persisted correctly but rendered as an empty filter in the editor (JSON.parse on an array throws -> []). - Add `tagFilters` and `documentTags` to JSON_STRING_SUBBLOCK_KEYS so edit_workflow stores them in the same shape the UI writes. - Make both components' parsers tolerate an already-parsed array on read, self-healing values already persisted in the broken (array) shape. Search execution was unaffected (parseTagFilters accepts arrays), so the value was never lost — only the editor render and round-trip were broken. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Surface each knowledge base's defined tags (displayName -> tagSlot) inline in its meta.json via serializeKBMeta, loaded in one batched query (loadKbTagDefinitions), so the agent can bind a knowledge-tag filter to a real tag slot instead of guessing a tag name it cannot otherwise see. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The nested-node merge path normalized array-with-id subblocks but never re-serialized the JSON_STRING_SUBBLOCK_KEYS, so editing a block nested in a loop/parallel container still persisted tagFilters/documentTags (and conditions/routes) as raw arrays -- the exact shape the subblock components cannot JSON.parse. Route all four write paths through a single normalizeSubblockValue helper so the normalize and re-stringify steps cannot drift apart again, and extract the duplicated string-or-array read logic into parseJsonArrayValue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Derive KbTagDefinitionSummary from the canonical TagDefinition instead of restating its fields, make parseJsonArrayValue generic so callers drop their `as T[]` casts, and unexport the three builders helpers that no longer have consumers outside the module now that normalizeSubblockValue fronts them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… workflow view
sanitizeForCopilot dropped `tagFilters` and `documentTags` from the workflow state the
agent reads (workflows/{name}/state.json), while edit_workflow is allowed to write both.
The field was therefore write-only: on a follow-up edit the agent read back an absent
field, concluded no filter was set, and cleared the user's tag filter.
The redaction was introduced for workflow *export* (#1628) and is already enforced there
by sanitizeWorkflowForSharing's key list. The duplicate in the copilot-only
sanitizeSubBlocks was redundant for export and destructive for the agent. Removes it and
pins the contract with a regression test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…filter `knowledge-tag-filters` and `document-tag-entry` had no arm in the `edit_workflow` input validator, so they fell through to the pass-through default. Any non-array value the agent supplied -- a double-encoded JSON string, an object, an unparseable string -- reached `normalizeSubblockValue`, where `normalizeArrayWithIds` coerces unparseable input to `[]`. The write path then persisted `"[]"` over the tag filter the user had configured. `condition-input` and `router-input` already guard against exactly this and return an actionable error to the model. Extend that arm to cover the two KB subblock types. It keys on subblock type, so the unrelated `tagFilters` short-input on the Algolia block is unaffected. `null`/`undefined` and empty arrays still clear the field, so intentional clears keep working. Also wrap `loadKbTagDefinitions` in try/catch. Tag definitions are an optional meta.json enrichment, but the query ran inside the top-level `Promise.all`, so a transient failure would reject the entire workspace VFS materialize and leave the agent unable to read any file. Now it degrades to a meta.json without tag definitions, matching the sibling materializers. Adds regression tests for both, plus the first tests for `parseJsonArrayValue`, the helper that keeps pre-fix raw-array rows readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…flow builders `normalizeArrayWithIds` and `normalizeConditionRouterIds` each hand-rolled the same "accept a raw array or the JSON string these subblocks persist" parse. Extract `parseJsonArray`, which returns null when the value is neither, so each caller keeps its own distinct fallback: `[]` for the former, the untouched original value for the latter. Behavior-preserving. An empty array is truthy, so `[]` and `"[]"` still parse through rather than hitting either fallback. `validation.ts` has a third copy, but `builders.ts` already imports from it, so sharing the helper across the two would introduce an import cycle. Left as is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`tagDefinitions` exposed `displayName`, but a `tagFilters` entry must carry the key `tagName`. An entry written with `displayName` passes validation and persists, then filters nothing -- a silent failure. Rename the field at the serializer boundary; the DB column is untouched. Also emit the operators legal for each tag's `fieldType`, reusing `getOperatorsForFieldType`. `between` is valid for number and date but not for text or boolean, and the agent has no way to infer that. An unrecognized fieldType yields an empty list rather than throwing. Still unspecified, and deliberately out of scope: a filter entry's value key is `tagValue` (but `value` on documentTags), and `between` needs `valueTo`. Those describe the subblock entry shape, not the knowledge base, so meta.json is the wrong place for them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d444bfa to
7e69f8d
Compare
|
@cursor review |
…izing "[]" `validateValueForSubBlockType` accepts null as an explicit clear, but `normalizeSubblockValue` then ran it through `normalizeArrayWithIds`, which coerces any non-array to `[]`, and persisted the string "[]". No data is lost either way -- "[]" and an absent field both mean "no filters". But it left the field present when the caller asked for it to be unset, so `sanitizeForCopilot` showed the agent an empty filter rather than an absent one, contradicting the absent-means-unset invariant the sanitizer documents. It also made Algolia's `if (params.tagFilters)` see a set value, since "[]" is truthy. An explicitly empty array still serializes to "[]" -- clearing with a value is distinct from clearing by omission. Reported by Cursor Bugbot on #5546. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e291b1f. Configure here.
Summary
The copilot could not edit a Knowledge block's tag filter at all. Asking Sim to set or change a tag filter appeared to succeed, but the value never showed up in the block.
edit_workflowpersistedtagFiltersanddocumentTagsas raw arrays, while their UI components store and read a JSON string. The write landed in the database in a shape nothing downstream could render, so the filter silently stayed empty. The same was true on the nested-node edit path.Fixing that surfaced a second, worse bug underneath it. Once the agent could actually write a tag filter, it still could not read one back:
sanitizeForCopilotstripped both keys out of the workflow state the agent sees. The field was write-only. So on a follow-up edit — "change it back" — the agent saw an absent field, concluded no filter was set, and cleared the user's filter. From the trace:It was telling the truth. That redaction came from #1628, which added it in two places for workflow export. Export is now independently protected by
sanitizeWorkflowForSharing, so the copy in the copilot read path had become redundant — and destructive.This PR closes four gaps along the same read/write loop:
tagFilters/documentTagsto a JSON string on write vianormalizeSubblockValue, on both the flat and nested-node edit paths, so an agent-authored filter actually renders and executes.parseJsonArrayValuekeeps rows written by older builds readable, so no migration is needed.sanitizeForCopilot, so the agent can read back what it writes. Export/sharing redaction is untouched.nullstill omits the key, so absent now unambiguously means unset.knowledge-tag-filtersanddocument-tag-entryhad no arm in theedit_workflowvalidator and fell through to a pass-through default. Any non-array value (a double-encoded string, an object, unparseable text) was coerced to[]bynormalizeArrayWithIdsand persisted as"[]"over the user's filter.condition-input/router-inputalready guarded against exactly this; the KB types now do too.meta.jsonexposeddisplayName, but a filter entry must carrytagName; an entry written withdisplayNamevalidates, persists, and filters nothing. Renamed at the serializer boundary (DB column unchanged) and added the operators legal perfieldType, sincebetweenis valid for number/date but not text/boolean and the agent cannot infer that.Together: the first bullet makes editing a tag filter work, the next three make it keep working across turns.
Reported by @justin. No linked issue.
Type of Change
tagFilters/documentTagsnow persist as JSON strings (readers accept both shapes; no migration)Testing
Unit tests — 176 passing across 15 files, covering every touched path:
validation.test.ts— 74/74, including 8 new cases pinning that a double-encoded string, an object, a number, and unparseable text are all rejected rather than coerced to"[]", whilenulland[]still clear the fieldjson-sanitizer.test.ts(new) —tagFilters/documentTagssurvivesanitizeForCopilot; anullvalue still omits the keyserializers.test.ts—tagNamerename, and thebetweenasymmetry pinned per field type; an unrecognizedfieldTypeyields[]rather than throwingsub-block/utils.test.ts(new) —parseJsonArrayValueacross JSON string, raw array,null, malformed JSON, and JSON parsing to a non-arraybuilders.test.ts,operations.test.ts,mutations.test.ts— round-trip through both edit pathsStatic checks:
tsc --noEmitexit 0;biome checkclean on all changed files. Both re-run after rebasing onto currentdev.Verified manually in the running app. Both halves of the bug: asking Sim to change a Knowledge block's tag filter to a different tag definition and value now actually applies it to the block, and asking it to change back restores the original rather than clearing it. This is the check that matters most here — an earlier pass on this bug looked correct statically and still had a second data-loss path underneath it, which the unit tests alone did not surface.
Reviewers should focus on:
json-sanitizer.ts— confirmsanitizeSubBlocksreally is copilot-only (one caller,sanitizeForCopilot) and that removing the strip cannot widen what workflow export, sharing, or templates expose. Export redaction lives incredential-extractor.tsand is untouched.validation.ts— the new arm keys on subblock type, so the unrelatedtagFiltersshort-inputon the Algolia block is deliberately not affected. Worth confirming that intent.workspace-vfs.ts—loadKbTagDefinitionsis now wrapped in try/catch. It runs inside the top-levelPromise.all, so an unguarded failure would have rejected the entire workspace VFS materialize over an optionalmeta.jsonenrichment, leaving the agent unable to read any file.serializers.test.ts— resolved an add/add conflict during rebase;devhad independently added a file at this path. Both suites were kept.Known, pre-existing, and deliberately out of scope:
ARRAY_WITH_ID_SUBBLOCK_TYPESandJSON_STRING_SUBBLOCK_KEYSkey on the subblock id, not its type, and ids collide across blocks. PostHog and RDS both expose aconditionssubblock that is not acondition-input, so a copilot edit still flattens them to"[]". That predates this branch (both keys were already inARRAY_WITH_ID_SUBBLOCK_TYPESat the merge-base) and fixing it changes behavior for those blocks, so it belongs in its own PR.validateValueForSubBlockTypealready switches on type and is the working template.Checklist
Screenshots/Videos
No UI changes. The failure is observable in mothership traces and in whether a Knowledge block's tag filter survives a round trip through the copilot.
🤖 Generated with Claude Code