fix(core): [PER-9706] sync Automate snapshots fail with "upload is not async iterable"

[Shivanshu-07]

Problem

Fixes PER-9706 / #2311.

On @percy/cli 1.32.0–1.32.2, synchronous Percy-on-Automate snapshots (sync: true) fail. Instead of comparison JSON, SDKs receive:

{"error":"upload is not async iterable"}

Screenshot capture on BrowserStack Automate succeeds; the failure is in the CLI sync upload path after capture. Works on 1.31.8.

Root cause

percy.upload is wrapped in percy.js so the public method returns a Promise via generatePromise():

this[m] = (...args) => generatePromise(method(...args)); // m === 'upload'

The Maestro-sync change (commit 545d498, PR #2217) rewrote the sync branches of /percy/comparison, /percy/maestro-screenshot and /percy/automateScreenshot to:

const upload = percy.upload(comparisonData, { resolve, reject }, 'automate');
for await (const _ of upload) { /* drain */ } // upload is a Promise → TypeError

A Promise is not async-iterable, so for await throws upload is not async iterable, which propagates to the SDK. The raw async generator is percy.yield.upload(), not percy.upload().

Fix

generatePromise already drives the underlying generator to completion, so calling percy.upload() inside the Promise executor enqueues #snapshots and lets the sync queue resolve/reject the attached callback. Revert all three sync routes to the proven 1.31.8 pattern:

const snapshotPromise = new Promise((resolve, reject) => percy.upload(payload, { resolve, reject }, 'app'));

Tests

The original "drain canary" tests passed only because they mocked percy.upload to return an async generator — contradicting its real Promise shape, which is why the regression slipped through CI. They're replaced with regression tests that model the real return shape (a Promise that resolves the sync callback asynchronously), so async-iterating the return value would fail. All api.test.js sync specs (/percy/comparison, /percy/automateScreenshot, /percy/maestro-screenshot) pass.

🤖 Generated with Claude Code

[Shivanshu-07]

Claude Code PR Review

PR: #2314 • Head: 61cce14 • Reviewers: stack:code-reviewer (2 rounds)

Summary

Fixes a regression (@percy/cli 1.32.0–1.32.2) where synchronous Percy-on-Automate snapshots fail with {"error":"upload is not async iterable"}. The three sync relay routes were for await-ing percy.upload(), but that public method is generatePromise-wrapped and returns a Promise (the raw generator is percy.yield.upload()). The fix reverts all three routes to the proven new Promise((resolve, reject) => percy.upload(..., { resolve, reject }, ...).catch(reject)) pattern and replaces the misleading "drain canary" tests with regression tests that model the real return shape.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None added.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	N/A	No new input handling.
High	Security	No IDOR — resource ownership validated	N/A	Not applicable.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Reverts to the generatePromise-driven pattern; generatePromise drives the generator to completion and the sync queue resolves/rejects the callback.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Added .catch(reject) at all three sync sites surfaces generator errors that bypass the sync-queue callback (was a potential unhandledRejection + hung request).
High	Correctness	No race conditions or concurrency issues	Pass	First settle wins; double-settle is harmless per Promise semantics.
Medium	Testing	New code has corresponding tests	Pass	Regression tests model the real Promise shape; new /comparison test covers the .catch(reject) rejected-Promise path.
Medium	Testing	Error paths and edge cases tested	Pass	Reject path tested for /comparison and maestro; resolve path for all three.
Medium	Testing	Existing tests still pass (no regressions)	Pass	All 8 sync/upload specs pass. 2 unrelated local failures are environmental (Node-20 dual-stack localhost; stale temp symlink) in blocks untouched by this diff.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Not applicable.
Medium	Performance	Long-running tasks use background jobs	Pass	Upload remains queued/async.
Medium	Quality	Follows existing codebase patterns	Pass	Restores the pre-regression (1.31.8) pattern.
Medium	Quality	Changes are focused (single concern)	Pass	Two files, the sync upload path only.
Low	Quality	Meaningful names, no dead code	Pass	Drain IIFE removed; comments accurate.
Low	Quality	Comments explain why, not what	Pass	Comparison route documents the canonical rationale; other routes cross-reference it.
Low	Quality	No unnecessary dependencies added	Pass	None.

Findings

No Critical or High findings. The first review round raised one High (discarded upload Promise could cause unhandledRejection + hung sync request) and one Medium (tests should assert handleSyncJob completion); both were addressed in commit 61cce14e and confirmed resolved in re-review.

Remaining observations are non-blocking (Low/Info):

• File: packages/core/src/api.js (three sync sites)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Double-reject (sync-queue reject then generator re-throw) is silently swallowed — correct Promise behavior, but the invariant is uncommented.

• Suggestion: Optional one-line note; not required.

• File: packages/core/test/api.test.js (the "resolves via callback" fakes)

• Severity: Info

• Reviewer: stack:code-reviewer

• Issue: Spies resolve the callback with no argument, so handleSyncJob fetches details for undefined id — proves control flow, not id propagation (covered by integration elsewhere).

• Suggestion: None required.

---

Verdict: PASS