Skip to content

fix(workflows,connectors): close pre-merge audit findings - #6783

Merged
waleedlatif1 merged 8 commits into
stagingfrom
fix/pre-merge-6765-audit
Aug 17, 2026
Merged

fix(workflows,connectors): close pre-merge audit findings#6783
waleedlatif1 merged 8 commits into
stagingfrom
fix/pre-merge-6765-audit

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Add operation-scoped subblock id migrations so a saved workflow's stored value survives an id rename. Cloudflare create/update DNS record, ServiceNow read record and Okta deactivate/delete lost theirs in this release — the create path substituted a seeded default (an A record where the user chose CNAME, unproxied where they chose proxied) and the update path silently no-opped while returning success. Used a migration rather than a legacy-id fallback so no subblock id carries two value spaces at runtime.
  • Webflow and 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 complete and let deletion reconciliation hard-delete every document past the truncation point.
  • Sentry: pin the listing window in the request instead of 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 filled. Added 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.
  • Deleted an orphaned Shopify OAuth route that built a credential from unsigned cookies — no writer, no caller, no inbound link.
  • Tailwind: dropped two content globs that scanned 5.4k files to emit one unused rule, keeping the ones that fix brand tile icon color.
  • Corrected the API route-count baseline, added an Evernote docs redirect, aligned library copy with the language rules, fixed a stale turbo filter.

Type of Change

  • Bug fix

Testing

Full apps/sim suite: 2000 files / 27,084 tests passing. bun run check:audits 29/29, bunx turbo run type-check 24/24, bun run lint clean, 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

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

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.
@vercel

vercel Bot commented Aug 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 17, 2026 10:57pm

Request Review

@cursor

cursor Bot commented Aug 17, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes affect workflow execution parameters after id renames and KB sync deletion reconciliation—both can cause silent wrong API calls or permanent document loss if mis-scoped.

Overview
Closes several audit findings around saved workflow state, knowledge-base sync listing, and dead OAuth surface.

Workflow subblock migrations are reworked into operation-scoped renames with optional value guards. Legacy Cloudflare DNS create/update values (type, proxied, tags, etc.), ServiceNow read projections on fields, and Okta deactivation toggles on the old shared sendEmail id are recovered on load without stealing values still used by other operations or post-rename block state.

Webflow, Zendesk, and Sentry connectors treat truncated or ambiguous listings as listingCapped instead of complete, so deletion reconciliation does not hard-delete documents past a partial walk. Webflow tightens pagination.total handling; Zendesk distinguishes exhausted vs truncated cursor/search continuation; Sentry documents the 90-day search floor and pins listingCapped to maxIssues only.

Removes the orphaned Shopify OAuth store route and shopifyStoreCookieSchema (unsigned cookie path with no callers). Adds Evernote docs redirects, trims Tailwind content globs, updates API route-count baseline, and expands tests for migrations and deleteOrphanedOAuthAccount reference guarding.

Reviewed by Cursor Bugbot for commit d857ef4. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR addresses pre-merge audit findings across workflow subblock migration, connector listing completeness, credential cleanup, documentation routing, and repository checks.

  • Adds operation-scoped migration coverage for renamed Cloudflare, Okta, and ServiceNow subblocks.
  • Hardens Webflow and Zendesk listing-completeness handling and updates Sentry issue listing behavior.
  • Removes the orphaned Shopify credential-storage route and updates related contracts and audit baselines.
  • Adds focused regression coverage and adjusts documentation, Tailwind scanning, and developer commands.

Confidence Score: 3/5

The PR does not yet appear safe to merge because untouched fork-dependent values can still be cleared and ambiguous ServiceNow write drafts can still be migrated into the read projection.

The reply on the fork-sync thread accepted that blank dependent submissions can overwrite target-draft values, but current HEAD still submits dependentValueFor directly without the omission fix. The ServiceNow reply also states that malformed write drafts must remain under fields, yet the current allowlist still moves a bare field-name or comma-separated partial draft into readFields.

Files Needing Attention: apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts; apps/sim/lib/workflows/migrations/subblock-migrations.ts

Important Files Changed

Filename Overview
apps/sim/lib/workflows/migrations/subblock-migrations.ts Adds operation-scoped subblock migrations, but the ServiceNow value-space discriminator still accepts an ambiguous malformed write-body shape as a projection.
apps/sim/connectors/webflow/webflow.ts Makes uncertain pagination envelopes non-authoritative so deletion reconciliation does not treat truncated listings as complete.
apps/sim/connectors/zendesk/zendesk.ts Preserves deletion safety when pagination continuation cannot be followed reliably.
apps/sim/connectors/sentry/sentry.ts Moves issue listing to the organization-scoped endpoint while retaining explicit pagination-cap signaling.
apps/sim/lib/credentials/deletion.ts Updates credential-deletion behavior and adds regression coverage for the affected cleanup path.

Reviews (6): Last reviewed commit: "fix(migrations): identify a ServiceNow p..." | Re-trigger Greptile

Comment thread apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts Outdated
Comment thread apps/sim/lib/workflows/migrations/subblock-migrations.ts Outdated
Comment thread apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts Outdated
Comment thread apps/sim/lib/workflows/migrations/subblock-migrations.ts
#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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.ts Outdated
The dependent-omission fix and the fork file-copy de-duplication are reviewed
separately in #6787. They are the only changes here that overlap #6776, and
they carry their own design tradeoff, so they should not ride along with the
unrelated audit fixes in this PR.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/workflows/migrations/subblock-migrations.ts
…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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/workflows/migrations/subblock-migrations.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

Recognising a write body was the wrong way round. A saved body is not always
well-formed: it can be a half-typed draft or carry an unquoted block reference,
so neither "opens with a brace" nor "fails to parse as JSON" identifies one —
and a body misread as a projection is moved to readFields with its original key
dropped, losing the draft.

Match the projection instead: a comma-separated list of ServiceNow field names,
which are word characters plus the dot of a dotted walk. A brace, quote, colon,
angle bracket or interior space fails that shape. Parsing then removes the bare
scalars that satisfy it by accident.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 d857ef4. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile — two things on the 3/5, one of which is stale context on your side.

1. The fork-sync omission fix is not missing; it moved. use-fork-sync.ts is at staging state on this branch on purpose — the fork-sync and fork file-copy changes were split out to #6787 in 455ae1357f, after the thread reply you're citing. git diff 746a4496 HEAD -- apps/sim/ee/workspace-forking/ on this branch is empty. The reply described the fix as it exists in #6787, not as something claimed to be here.

2. On the remaining ServiceNow ambiguity — you're right that it exists, and it's irreducible. A stored value that is textually a bare field name or a comma-separated list is indistinguishable from a write draft that happens to look like one; no predicate can separate them, because there is nothing to separate. What's left is choosing which way to fail.

Migrating it is the better failure. If the value really is a projection, it is recovered — which is the whole point, since a Read Records block silently returning every column of every row was the original defect. If it is a body draft that happens to have no brace, quote, colon or space anywhere in it, it lands in readFields on a block whose operation is already Read Records, where a body would have been meaningless anyway.

The shapes that are actually recoverable are excluded: a brace, quote, colon, angle bracket or interior space fails the allowlist, so half-typed bodies, unquoted <block.output> references, and trailing-comma drafts all stay on fields. Those are the drafts worth protecting, and they're covered by tests that go red if the allowlist is loosened back to a parse check.

Not changing this further — tightening past this point would start rejecting legitimate projections, which reopens the defect the migration exists to close.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1
waleedlatif1 merged commit c8f559a into staging Aug 17, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/pre-merge-6765-audit branch August 17, 2026 23:03
@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Both points taken.

1. Understood — the fork-sync coverage is in #6787, not here. That was stale context on my end; the comment no longer applies to this PR.

2. The tradeoff framing is sound. The allowlist targets the only shapes that are unambiguously projections at rest (bare identifiers, comma-separated lists of bare identifiers), and the exclusion set ({, ", :, <, interior space) covers the drafts that are actually worth protecting. Migrating an identifier that was a body draft landing in readFields on a Read Records block is a no-op in practice — a body would have been meaningless there anyway. And the tests that go red on allowlist relaxation are the right guard. No further objection.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 d857ef4. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant