Add Adobe Marketo Engage gatekeeper - #413
Conversation
github-actions
Bot
added
delivery
Preview:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if (this.#abandonedStageExpired(staged) && !this.#submittingActions.has(actionId)) { | ||
| this.#removeStaged(actionId); | ||
| return undefined; |
There was a problem hiding this comment.
🟡 Delayed approvals lose staged writes
When #promoteStagedAction receives a delayed callback over ten minutes after an uncertain submission, it deletes the staged action. The pending approval can no longer apply its write.
Prompt for agents
In packages/gatekeeper-marketo/src/marketo.ts, submission-uncertain and submitting stages can represent actions that the ApprovalQueue durably registered before its RPC response was lost. #promoteStagedAction and #cleanupAbandonedStagedActions currently delete these rows after ABANDONED_STAGED_ACTION_TTL_MS, even though an applyAction callback may arrive much later from the still-pending approval. Preserve enough durable state for delayed apply/reject callbacks while still cleaning up stages proven never to have reached the queue. Add a restart/crash-window test where submitAction registers remotely, its response is lost, more than ten minutes pass, and applyAction still promotes and dispatches the reviewed action exactly once.
Was this helpful? React with 👍 or 👎 to provide feedback.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| }); | ||
| } | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
P1: Treat a completed reentrant apply as success here. submitAction() can invoke applyAction() and successfully persist applying:<id> = applied, then reject because its response was lost. In that case both staged and pending rows are gone, this branch still rethrows, and the caller can retry an action that already changed Marketo (including sending a campaign). Check the terminal applying outcome before propagating the submission error, as the success path below already does.
|
|
||
| #abandonedStageExpired(staged: StagedRow): boolean { | ||
| return staged.expiresAt !== undefined && staged.expiresAt <= Date.now() && ( | ||
| staged.state === "prepared" || staged.state === "submitting" || |
There was a problem hiding this comment.
P1: Do not expire an ambiguously registered approval by time alone. submission-uncertain means the queue may have durably registered the approval even though its response was lost. If the user approves it after ten minutes, #promoteStagedAction() deletes the only payload and applyAction() reports no queued action. This breaks durable approval recovery for an entirely normal delayed approval; the row needs to remain recoverable until authoritative queue evidence resolves it.
| Date.now() >= storedNonce.expiresAt || | ||
| reconnecting && currentReconnect?.value !== reconnecting.value) return false; | ||
| generation = this.#advanceCredentialGeneration(); | ||
| this.ctx.storage.kv.put<MarketoCredentials>("credentials", credentials); |
There was a problem hiding this comment.
P1: Persist a recoverable phase for initial connection completion. The nonce has already been consumed when credentials are installed, but only reconnects record awaiting-notification. A worker restart after this put and before callback.complete() leaves credentials stored, no usable nonce, and no account capability in the Workshop; the existing alarm ignores this state because credentials exist. Initial connects need the same durable notification/retry state (or an equivalent rollback mechanism).
| }; | ||
| this.ctx.storage.kv.put<PendingRow>(`pending:${actionId}`, pending); | ||
| let index = this.#pendingIndexIncludingBlocked(); | ||
| if (!index.includes(actionId)) this.ctx.storage.kv.put("pending:index", [...index, actionId]); |
There was a problem hiding this comment.
P1: Keep the pending index in action-ID order. Concurrent session calls allocate IDs before awaiting submitAction(), so approval registration for ID 2 can finish before ID 1 and produce [2, 1] here. Every overlay then applies the second mutation before the first, while #validateMutationOrder() enforces eventual dispatch in numeric ID order. Insert/sort by ID so simulated state matches the eventual provider state.
| for (let reference of this.#actionReferences(action)) { | ||
| add(this.#referenceKey(reference), identity !== undefined && this.#sameReference(reference, identity)); | ||
| } | ||
| if (isEmailDesignerAction(action) && |
There was a problem hiding this comment.
P1: Include classic lifecycle dependents in mutation ordering. DesignStudioLifecycleSnapshot.affectedDependents is captured for email-template/form approve/delete, but only Email Designer dependents become resource writes here. Thus a later classic template approval can dispatch ahead of an earlier pending mutation to a dependent email, allowing Marketo propagation and the later email write to occur in the reverse of submission order. Add the classic dependent identities to the resource set just as this does for Designer lifecycle actions.
| captured.credentials, | ||
| account, | ||
| expected, | ||
| () => account.credentialsExpired(), |
There was a problem hiding this comment.
P1: Scope expiry notification to the captured credential generation. An old generation can have its forced-refresh request in flight while reconnect installs valid replacement credentials and calls credentialsRestored(). If that old request then returns another auth error, this unconditional callback marks the newly restored account expired. Pass the expected generation into credentialsExpired() and ignore it unless that credential state is still current.
| } | ||
| let client = await this.#ctx.client(); | ||
| let query = { | ||
| workspaceId: workspace, folderId: options.folderId, folderType: options.folderType, name: options.name, |
There was a problem hiding this comment.
P2: Resolve or locally defer provisional folder/program filters. folderId accepts IDs returned by pending classic folder/program creation, but unlike templateId it is forwarded unchanged (for example ~1) to Marketo and later compared strictly against physical IDs. Listing Designer assets in a newly created location therefore fails or omits matches. Resolve through resolveAssetId, with a local-filter path while the location is still provisional.
| moreResult: boolean; | ||
| nextPageToken?: string; | ||
| }> { | ||
| let allActions = this.#ctx.pendingCampaign(); |
There was a problem hiding this comment.
P2: Overlay pending program changes when campaign filters depend on program names. The Marketo nameContains campaign query also matches the containing program name, but this listing only considers pending campaign actions. After a pending program rename from Old to New, listSmartCampaigns({ nameContains: "New" }) queries the unchanged provider and returns no campaigns, so the pending rename is not simulated. Include pending program actions in query/candidate handling.
| let state = paging(options, scope); | ||
| let { offset, maxReturn, batchSize } = state; | ||
| let client = await this.#ctx.client(); | ||
| let pending = this.#ctx.pending(); |
There was a problem hiding this comment.
P2: Include pending ProgramActions when folders include programs. This API returns both ordinary folders and Program folders, but its overlay only receives Design Studio actions. A pending program rename or deletion therefore still appears under the old name (or remains visible) through listFolders()/getFolder(..., "program"), even though findProgramsByName() simulates the same actions. Merge the pending program overlay for Program rows.
|
Submitted 9 actionable inline findings. |
1 participant
Summary
applyAction()and simulate pending actionsThis replaces #394 with a clean two-commit history based on current
main. It retains the security fixes fromb51eb7dawhile removing the review-churn history.Review Focus
Please focus the human review on:
applyAction()Testing
pnpm --filter @gadgets/marketo-gatekeeper test:run(651 tests)pnpm exec vp run -F @gadgets/marketo-gatekeeper --no-cache buildpnpm lintnode --test scripts/release/manifest-lib.test.ts