fix(workflows,connectors): close pre-merge audit findings - #6783
fix(workflows,connectors): close pre-merge audit findings#6783waleedlatif1 wants to merge 7 commits into
Conversation
Recover subblock values orphaned by the id renames in this release, and stop truncated knowledge-base listings from reporting themselves complete. - Add operation-scoped subblock id migrations so a saved workflow's stored value survives a rename. Cloudflare create/update DNS record, ServiceNow read record, and Okta deactivate/delete previously lost their stored value: the create path substituted a seeded default (an A record where the user chose CNAME, and unproxied where they chose proxied), and the update path silently no-opped while reporting success. A migration is used rather than a legacy-id fallback so no subblock id carries two value spaces at runtime. - Webflow, Zendesk: a listing that stops for a reason the connector cannot rule out now reports as capped instead of exhausted. A malformed envelope, an unfollowable continuation link, or an absent collection list previously read as a complete listing and let deletion reconciliation hard-delete every document past the truncation point. - Sentry: pin the listing window in the request rather than inheriting the server default, so the range cannot silently narrow into hard deletes. - Fork sync: a parent re-pick no longer writes a blank over a hidden optional dependent's stored target value, and a required field stays on screen once it is filled. Add hook-level coverage for the submitted payload. - Fork file copy: a file whose name is already taken in a reused target folder is de-duplicated instead of dropped. - Delete an orphaned Shopify OAuth route that built a credential from unsigned cookies. It had no writer, no caller, and no inbound link. - Tailwind: drop two content globs that scanned 5.4k files to emit one unused rule, keeping the ones that fix brand tile icon color. - Correct the API route-count baseline, add an Evernote docs redirect, align library copy with the language rules, and fix a stale turbo filter.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Workflow subblock migrations are reworked from flat rename maps into operation- and value-scoped rules. Legacy Cloudflare DNS create/update fields, ServiceNow read projections vs JSON bodies, and Okta deactivation email toggles are recovered on load without stealing values from other operations (e.g. proxied flags, Connector listing now treats ambiguous pagination as truncated ( Other changes: remove dead Shopify OAuth store route and Reviewed by Cursor Bugbot for commit d4a768b. Configure here. |
Greptile SummaryThe PR closes a collection of workflow migration, connector reconciliation, fork synchronization, OAuth cleanup, and audit findings. One ServiceNow migration edge case still crosses the write-body and read-projection value spaces.
Confidence Score: 4/5The PR is not yet safe to merge because the ServiceNow migration can permanently move and delete a persisted write-body draft when it is not valid JSON at migration time. The new JSON.parse-based discriminator treats incomplete, malformed, and reference-templated write bodies as read projections; those values are moved to readFields, persisted, and sent as sysparm_fields while the original fields entry is deleted. Files Needing Attention: apps/sim/lib/workflows/migrations/subblock-migrations.ts
|
| Filename | Overview |
|---|---|
| apps/sim/lib/workflows/migrations/subblock-migrations.ts | Adds operation- and value-scoped migrations, but malformed or templated ServiceNow write bodies are still misclassified as read projections and deleted. |
| apps/sim/lib/workflows/migrations/subblock-migrations.test.ts | Extends migration coverage for valid ServiceNow JSON bodies and scalar forms, but does not cover persisted invalid or incomplete JSON. |
| apps/sim/connectors/webflow/webflow.ts | Makes uncertain listing termination report a capped result so reconciliation does not infer exhaustion. |
| apps/sim/connectors/zendesk/zendesk.ts | Makes malformed or unfollowable pagination outcomes deletion-safe by reporting a capped listing. |
| apps/sim/connectors/sentry/sentry.ts | Pins the issue-listing time window instead of inheriting a potentially narrower server default. |
| apps/sim/lib/credentials/deletion.ts | Updates fork file-copy behavior so reused-folder filename collisions are de-duplicated rather than dropped. |
| apps/sim/app/api/auth/oauth2/shopify/store/route.ts | Removes an orphaned Shopify OAuth credential-storage route. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Saved ServiceNow fields value] --> B{Read Records selected}
B --> C{JSON.parse succeeds}
C -->|Yes| D[Keep write body under fields]
C -->|No| E[Classify as read projection]
E --> F[Move to readFields and delete fields]
F --> G[Persist migrated workflow]
G --> H[Send as sysparm_fields]
Reviews (5): Last reviewed commit: "test(connectors,credentials): tie two as..." | Re-trigger Greptile
#6776 landed on staging as a competing fix for the same fork-sync defects this branch addressed. Take its work wholesale and keep only the part of ours it does not cover. Kept from upstream (#6776): the `sameDependencyScope` cascade guard, `getDisplayedDependentFields` with the "Edit configuration" chip, per-scope provider indexing, the `blockChainState(block, field, ...)` scope filter, and the `dependent-reconfigs` / `workspace-fork` contract changes that emit `dependencyScope` and per-scope context keys for nested tool params. Kept from this branch: `DEPENDENT_CLEARED_BY_PARENT` and `submittedDependentValue`. The scope guard decides WHICH descendants a re-pick invalidates; the sentinel decides HOW an invalidated one is represented, and those are different layers. Without the sentinel the cascade still writes `''` into the reconfig map and `buildDependentValues` still submits it, so a hidden optional dependent's stored target value is destroyed by a parent re-pick the user never applied to it. The scope guard does not close this: two top-level block subblocks both have `dependencyScope === undefined`, so it is a no-op there. #6776 also widens the exposure by emitting context keys for nested tool params that previously could never be cascaded onto. Also kept: the `previousValue` no-op guard in `applyDependentRepick` (a separate bug - re-selecting the value a field already shows must not invalidate its descendants) and the post-sync reset in `use-fork-sync`. Dropped from this branch: the sticky-visibility predicate in `isDependentConfigurationActionable` and its three tests. It and upstream's edit chip are two mechanisms for one visibility problem, and it is unnecessary - a marked REQUIRED field reads as `''` through `effectiveDependentValue`, so the existing `required && value === ''` arm keeps it on screen and keeps it gating Sync. Only marked OPTIONAL fields drop out of the default view, and those are omitted from the payload, so hiding them costs nothing; the edit chip brings them back. Reconciled the cascade assertions in `dependent-value.test.ts` to the sentinel, including upstream's nested-tool-instance case.
A legitimacy review found several changes closed no live defect, and two introduced problems of their own. - Zendesk: narrow the cursor fix to a signal change. Treating a missing meta envelope as truncation had also made the walk follow links.next and keep paginating, and the ticket cursor has no page-depth valve, so a source advertising a next page with no meta could loop without terminating. The page-fetch set now matches the previous behavior; only the flag is new. - Zendesk: drop the search next_page branch. The existing count check already caps every case where a missing key could lose documents. - Webflow: drop the empty-collections flag. The sync engine already blocks the first sync on an empty listing and reconciles only when a second sync agrees, which handles a transient fault better and still removes documents when a source is genuinely emptied. The flag short-circuited that and suppressed reconciliation permanently. Restore the previous loud failure on a non-array envelope, and drop the unreachable collection-id filter. - Webflow: soften a docstring that claimed pagination.total is always present. It is documented optional, so its absence proves nothing either way and treating it as unprovable truncation is the fail-safe reading. - Sentry: drop the pinned statsPeriod. Sentry's issue search floors every query at 90 days in the executor regardless of the request, and the endpoint this release moved away from hit the same floor, so there was no window to close. Keep the tests and the docstring recording that. - Fork copy: drop the renamed counter, which no caller reads. - Repair check-block-registry, which stopped exempting migrated subblock ids when the migration map became an array — `in` was testing array indices. - Drop mdx from a Tailwind content glob that emits nothing, and loosen an exact compiled-SQL assertion to the invariant it was pinning.
Review findings from the first round. - A legacy ServiceNow block can hold a Create/Update Record JSON body under `fields` while its stored operation is Read Records: the id served both value spaces before the rename, and a subblock value is not cleared when the operation changes. The scoped migration moved that body onto `readFields`, where it would reach the wire as sysparm_fields. Migration entries can now carry a `whenValue` predicate for the case where the stored operation alone cannot separate two value spaces, and the ServiceNow entry uses it to move only a plausible comma-separated projection. - Type the fork copy test harness instead of using `any`, without weakening it: every predicate shape it does not model still throws rather than matching. - Correct the dependent-omission comments. Omitting a parent-invalidated field preserves the target's stored value on Save and across an undo, where the parent nets out unchanged; on a Sync the written state is source-derived, so what it prevents there is an explicit blank reaching the fields the remap's clearing pass does not cover, nested tool params in particular. Okta's migration scope is left as-is: `okta_remove_user_from_app` and the sendEmail split shipped in the same release, so no saved block can hold legacy state for it, and widening the scope would promote an activation-era value onto the deactivation switch. Tests document the boundary.
|
@cursor review |
|
@cursor review |
…y parsing
The guard tested for a `{` or `[` prefix, so a stored scalar body — `true`,
`"short_description"`, `42` — read as a field list and was promoted onto
`readFields`, where it would go out as sysparm_fields.
A Create/Update Record body is JSON and a projection is a bare comma-separated
field list, which is never valid JSON, so parsing is the whole test rather than
a guess at its opening character. Ambiguity still resolves to "not a
projection", leaving the value where the Create/Update control owns it.
|
@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 a61cad8. Configure here.
…y prove - Webflow: a non-array collections envelope reaching `for...of` throws, which is the intended loud failure. Assert the spec-mandated TypeError plus a single request and no write-back, rather than matching V8's wording. - Credentials: the second guard test cannot observe "not deleted" — the proxy driver replays canned rows — so name it for what it does verify, that the reference check carries no workspace predicate and an empty RETURNING logs nothing. Making the driver decide the outcome would fake the database. - Drop `vi.importActual`; a plain `drizzle-orm/pg-proxy` import works now that `drizzle-orm` is un-mocked.
|
@cursor review |
| } catch { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Malformed bodies become projections
When a saved Create/Update Record body contains incomplete, malformed, or reference-templated JSON and the block is switched to Read Records, this predicate classifies the body as a projection. The migration then deletes the original fields entry, persists the value under readFields, and sends it to ServiceNow as an invalid sysparm_fields value, permanently losing the stored write-body draft.
Knowledge Base Used: Blocks Module
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 d4a768b. Configure here.
Summary
Type of Change
Testing
Full
apps/simsuite: 2000 files / 27,084 tests passing.bun run check:audits29/29,bunx turbo run type-check24/24,bun run lintclean, block-registry check clean.Every fix ships a test that fails without it — each was verified by reverting the fix in place, confirming the suite went red for the right reason, and restoring. Three pre-existing invariant suites under
apps/sim/tools/pass unmodified.Checklist