fix(o11y): batch the event flush so command-heavy specs stop hitting spec_timeout [SDK-7399]

[shivam5643]

What is this about?

Customers on 1.35.x / 1.36.x see a large share of their tests reported as skipped; the same suite on 1.32.8 is clean (SDK-7399: ~113 of ~525 skipped on 1.36.18 vs 4 on 1.32.8).

The tests are not skipped by mocha and nothing throws — the spec is killed at spec_timeout and every test that has not run yet is reported as skipped.

build-info on a reproducing build:

test_status:   { failed: 0, success: 6, queued: 0, ignored: 27, pending: 0 }
spec_timeout:  900000 ms          duration: 993 s
sessions:      status=error, 980 / 984 / 986 s

failed: 0 is the tell. A thrown error always marks a test failing; nothing failed here. The sessions ran to ~985s against a 900s spec_timeout and were killed.

Why they run that long. The queue flush issues one cy.task per queued event, and each cy.task round-trip costs roughly 0.8s on a remote terminal. Measured there, 6 tests per spec, N events per afterEach:

events per afterEach	cy.task calls	session duration
1	6	113s
10	60	114s
100	600	581s
1000	6000	killed at spec_timeout

A command-heavy test queues hundreds of events (command:start + command:end + platform_details per command, plus log:changed), so its afterEach alone runs for minutes. Load-dependent by construction: heavy tests exhaust the spec budget, light tests do not — which is exactly the reported 45-passed / 113-skipped shape rather than all-or-nothing.

Why 1.32.8 is unaffected. It dispatches via cy.now('task', ...), which throws immediately on Cypress 14 with no IPC round-trip. 1.32.8 is fast because its dispatch does nothing — it also delivers no browser-side telemetry there. v1.33.0 moved dispatch into the internal hooks and onto cy.task, which genuinely delivers but costs ~0.8s per event.

Related Jira task/s

• SDK-7399

Dependent PRs / release order

• Dependent PRs: None — CLI-only, no paired change in railsApp / realMobile / binary.
• No cross-repo deploy order applies.

Automation cases to add

• Cypress case asserting a command-heavy spec completes well inside spec_timeout with expected_skipped_number: 0. Existing cypress_cli_and_dashboard.feature rows already assert skipped counts and cover the success path.

Code changes to check

• Spread operator is not used.
• Syntax supported by older Node — no ?. / ??.

The change

plugin/index.js — new test_observability_batch task takes an array and fans out to the same IPC events. The four individual tasks stay registered for backward compatibility.

cypress/index.js — the flush builds one batch per drain instead of one cy.task per event, split at 512KB so each call stays under the ~1MB per-payload ceiling (also measured on the remote: 768KB passes, 1MB fails). Two thresholds, both from the same measurement: batches are closed at 512KB, and a single event above 768KB — the largest payload measured to send — is skipped with a log line. An event between the two still sends, alone in its own batch. Nothing is altered or truncated. shouldSkipCommand filters test_observability_batch, without which each batch dispatch would itself be captured as a command event and refill the queue.

Batching was verified before the code was written: the same 600 events sent as one cy.task call completed in 109s versus 581s as 600 calls — i.e. back to the ~110s baseline for that spec shape.

Verification

Fix, 3-spec repro (suite modelled on the customer's shape: cy.session per test, per-test retry overrides, fixture chains in before()). Same suite and settings both arms; only the CLI dependency differs. Patched arm confirmed to ship a fresh dependency bundle, not a cached one, and the git dependency was verified to serve the branch head before running.

Current head 456295d

CLI	spec verdicts	tests run	session	duration	build
unpatched 1.36.9	all 3 passed_with_skipped	6 success / 27 ignored	error — killed at spec_timeout	1070s	automate
this change	all 3 passed	33 success / 0 ignored	done	212s	automate

Video was enabled on both arms (video: true, confirmed in each build's video_config). The unpatched session is killed mid-run, so Cypress never finalises the recording; the patched session completes and the video is written — the "video will not load" complaint resolves with the same fix.

Earlier run at f64d61d, before the threshold split — same outcome, which is what shows 3ec1263 changed nothing for normal-sized events

CLI	spec verdicts	tests run	duration	build
unpatched 1.36.9	all 3 passed_with_skipped	6 / 27 ignored	993s	automate
f64d61d	all 3 passed	33 / 0 ignored	213s	automate · o11y

Telemetry still delivers — checked explicitly, because a passing spec cannot distinguish delivered from dropped. test_observability_batch accepts a mixed batch of all four event types, a 600-event batch, and a batch containing an unknown task name, all without failing (3/3).

Release

Version bump:

• minor (backwards-compatible feature)
• patch (bug fix or other small change)

Release notes type:

• Bug Fix

Release notes (customer-facing):

• Fixed tests being incorrectly reported as skipped when Test Observability is enabled. Observability data is now sent in batches, so specs no longer run past their spec timeout on command-heavy tests.

Release notes (internal):

• bin/testObservability/cypress/index.js + bin/testObservability/plugin/index.js: the beforeEach/afterEach event-queue flush now sends one batched cy.task per drain (new test_observability_batch task) instead of one cy.task per event. Each round-trip costs ~0.8s on a remote terminal, so command-heavy tests were spending minutes in the hook and the spec was killed at spec_timeout, reporting unrun tests as skipped — the SDK-7399 symptom. Introduced in v1.33.0 when dispatch moved into these hooks and onto cy.task.
• Added test/unit/bin/testObservability/batchFlush.js covering the batch fan-out and the single-event dispatch threshold.
• Two distinct thresholds, both from the remote measurement (768KB succeeds in one cy.task, 1MB fails): batches are closed at 512KB, and only a single event above 768KB is skipped with a log line. An event between the two is dispatched alone in its own batch rather than dropped. Skipped events are logged rather than dropped silently.

Checklist

• Ready to review
• Has it been approved by a member of your team?
• Verified end-to-end on BrowserStack infra (build links above)
• Telemetry delivery verified separately from the pass/fail result
• Required automation cases noted in description

Reviewer notes

Environment gotchas found while verifying, worth knowing for any future o11y change:

• A CLI branch must resolve on both the local machine and the remote Windows terminal. A git-branch dependency works (repo is public); a local file: path or a package_config_options.scripts.postinstall pointing at a local path does not — run_settings ships verbatim to the remote (capabilityHelper.js:131), so the remote re-runs it and fails with NPM_INSTALL_FAILED.
• BrowserStack caches the dependency bundle by the generated package.json md5. Re-testing a changed patch under an unchanged dependency string silently reuses the old bundle ("Skipping the upload of node_modules…"). Add a bare cacheBust key to force a fresh upload.
• browserstack-cypress build-info <buildId> is the reliable way to see test_status / spec_timeout / session durations. The o11y ext API returned nulls for these builds and the session-logs URL returns a bot-challenge page.

Earlier commits on this branch explored two approaches that were verified and rejected: a reporter-side duplicate-event guard (no effect on a remote A/B), and switching dispatch to cy.now (stopped the skipping only by never delivering anything, since cy.now('task') throws on Cypress 14 in every context). Both are superseded by this change; history retained deliberately.

🤖 Generated with Claude Code

[shivam5643]

PR Review: browserstack-cypress-cli PR #1177

Summary

Intent: Add a real try/catch + .catch() error boundary around the test-observability event flush in bin/testObservability/cypress/index.js, and switch the dispatch call from cy.task to cy.now('task', ...), so that an instrumentation failure inside the beforeEach/afterEach mocha hooks can no longer throw and make mocha skip every remaining test in the spec (SDK-7399).
Risk: Medium
1 critical · 0 warnings · 0 suggestions | Files reviewed: 1

═══════════════════════════════════════════════════════════════

Findings

Populated mechanically from unified-findings.json — the union of every unit reviewer's output, never hand-merged.

Two channels. Blocking = Critical + Warning — the must-fix set the Verdict gates on. Non-blocking = Suggestions — polish; never gates. An empty Warning/Suggestion section here means examined and clean, not an incomplete review.

🔴 Critical (Blocking)

#	Finding	File · Symbol	Confidence
1	[Graceful Degradation] All three swallow points in flushEventsQueue drop errors with no diagnostic signal at all	bin/testObservability/cypress/index.js · flushEventsQueue	🟢

1. [Graceful Degradation] All three swallow points in flushEventsQueue drop errors with no diagnostic signal at all — bin/testObservability/cypress/index.js · flushEventsQueue

Problem:
flushEventsQueue() has three places that intentionally swallow an error to protect the customer's run: the async .catch(() => {}) on the cy.now promise, the per-event catch (e), and the outer function-level catch (e) that resets the whole queue. All three are correctly scoped — each isolates one bad event / one bad flush from the rest, which is exactly what this PR needs to satisfy the graceful-degradation contract — but none of them leaves any trace that an event was dropped. Not even a console.warn.

This PR exists specifically because a prior silent-failure mode (a thrown instrumentation error making mocha skip every remaining test) went undiagnosed for a full release cycle before a customer noticed. The fix as written trades that loud failure for a new, completely silent one: an observability event can now fail to reach the dashboard with zero signal anywhere that it happened. The next time this recurs — a different cy.now('task', ...) failure mode, a malformed payload, whatever — nobody will know until someone notices missing data in TestHub/O11y, which is a much harder thing to notice than a customer's suite breaking.

const result = cy.now('task', event.task, payload, event.options);
if (result && typeof result.catch === 'function') result.catch(() => {});
} catch (e) {
  /* one bad event must not stop the remaining events, and must not fail the hook */
}

Suggested Fix:
Add a bare console.warn/console.error inside each of the three catches, e.g. console.warn('[browserstack] failed to flush test-observability event', e). Do not route the failure through another Cypress command (cy.task/cy.now) — that risks reintroducing the exact enqueue-time failure this PR is fixing. A plain console.* call keeps the guarantee that instrumentation can never fail the hook, while leaving a log line an engineer can grep for the next time events go missing.

Confidence: 🟢 — grounded against the default.md graceful-degradation rule (an error boundary must still surface the failure somewhere — debug log, telemetry, or status — its exemption is only for a catch that does log at debug) and verified objectively against the diff: none of the three catch bodies contains any logging call, only code comments.

───────────────────────────────────────────────────────────────

🟠 Warnings (Blocking)

None found — the region-by-region walk of both hook call sites (beforeEach, afterEach) surfaced no other defect.

💡 Suggestions (Non-blocking)

None.

═══════════════════════════════════════════════════════════════

External Services

No external-contract changes detected. (No finding in this PR carries contract_change: true; the change is entirely internal to the CLI's Cypress-hook flush path and does not alter any request/response shape, endpoint, or wire protocol.)

═══════════════════════════════════════════════════════════════

Per-File Confidence (for reviewers)

File	Status	Reason
bin/testObservability/cypress/index.js	🔴 Author to Fix 1	1 grounded (kb-high) finding — swallowed errors with no diagnostic signal

═══════════════════════════════════════════════════════════════

What's Good

• Both beforeEach and afterEach flush sites are updated identically to call the same flushEventsQueue() helper — the fix isn't applied asymmetrically to only one of the two hooks.
• The per-event try/catch inside flushEventsQueue isolates one bad event so the rest of the queued batch still flushes, and the outer try/catch stops a catastrophic failure from ever reaching the mocha hook — the core "instrumentation must never fail the hook" contract is satisfied on every path.
• The switch from cy.task to cy.now('task', ...) is deliberate and well-documented in the PR body: cy.task enqueues a command that drains later (still inside the hook's failure window), while cy.now executes immediately and returns a promise that can actually be caught here — this is the crux of the fix, not a cosmetic change.

═══════════════════════════════════════════════════════════════

Verdict

🔴 Fix 1 blocking issue — flushEventsQueue's three swallow points drop instrumentation errors with no diagnostic signal at all (Critical, grounded).

Coverage: 3 of 3 regions judged, 0 unjudged. No coverage gap.

═══════════════════════════════════════════════════════════════

— SDK PR Review Agent

[shivam5643]

Fixed in 82cf3f9 — valid finding, the guard was silent.

All three suppression points now log via warnFlushFailure(stage, err), naming the stage and failing task. Used console.warn as suggested (not cy.task — that would reintroduce the contained failure); matches the existing convention in accessibility-automation/cypress/index.js.

Guarantee re-verified after the change: 11-test spec with a failing dispatch in afterEach → 11 passing, 0 skipped.

Also trimmed the code comments in 937f2d9 (33 → 12 lines), no logic change.

[shivam5643]

✅ Good to go

File	Status	Reason
bin/testObservability/cypress/index.js	✅ All Clear	Covered — all 3 regions judged clean; 1 non-blocking Suggestion noted separately, does not affect this status

Change map (generated deterministically from the diff)

graph LR
  subgraph ncypress_cli["cypress-cli"]
    nbin_testObservability_cypress_index_js["index.js<br/>~57 lines"]
  end

Loading

↻ This verdict comment is the review anchor — it's updated in place on each run (the gate posts its status separately).

— SDK PR Review Agent

[shivam5643]

⚠️ Please hold this PR — do not merge yet. Further verification found a problem with my own change.

What I found. The fix swaps cy.task(...) → cy.now('task', ...). On a real remote Windows 11 / Chrome 136 machine, cy.now('task', ...) throws for every task, in every context (test body, mocha hook, and Cypress.on listener), while cy.task(...) delivers correctly:

dispatch	remote result
cy.task(...)	✅ delivers
cy.now('task', ...) (this PR)	❌ throws — Cannot read properties of undefined (reading 'get') from runPrivilegedCommand

So this PR stops the skipping by never dispatching anything. Browser-side observability events (test_observability_command / _log / _step / _platform_details) would silently stop reaching the dashboard. My earlier green builds could not tell "contained a genuine error" from "sent nothing" — both produce a passing run.

Side note that reframes the baseline: 1.32.8 dispatches only via cy.now, so on Cypress 14 it delivers no browser-side telemetry either — its throws land in listeners where they are harmless. So this PR is effectively 1.32.8 parity: no skipping, no browser-side telemetry. That is a product trade-off, not just a technical one, since customers on 1.36.x get that telemetry today.

Two other approaches tested and rejected:

• Error boundary only, keep cy.task — does not fix it (1 failing + 10 skipped). cy.task enqueues; the failure lands during queue drain, outside the try/catch.
• Keep cy.task + scoped cy.on('fail') filter — dangerous. Looked ideal (11 passing, 0 skipped), but a safety check with the customer's own failing assertion in afterEach reported 3 passing / 0 failing — our failing cy.task aborts the hook's command queue, so their real assertion never ran and the failure was hidden. Strictly worse than the original bug.

Still confirmed and unaffected by all this: the root cause (a throw inside the internal beforeEach/afterEach flush makes mocha skip every remaining test in the spec) and the regression window (v1.33.0 moved dispatch into those hooks and dropped the per-call .catch() guards).

Suggested direction. The reporter already runs an HTTP server on 127.0.0.1:REPORTER_API_PORT_NO with CORS enabled (reporter/index.js:267,295-307, used today by accessibility-automation/plugin/index.js). Posting queued events browser→node over that channel bypasses Cypress commands entirely, so a dispatch failure could never fail a mocha hook and delivery is preserved. That is a design change rather than a hotfix and needs a maintainer's call.

Happy to take direction on whether to pursue that here or open a fresh PR.

[shivam5643]

Hold lifted. The earlier cy.now approach is superseded — the diagnosis in this PR has been rewritten and the fix reverified.

Correction to my earlier comments: the bug is not a thrown error. build-info on a reproducing build reports failed: 0, success: 6, ignored: 27 with sessions at ~985s against a 900s spec_timeout. Nothing throws — the spec is killed at spec_timeout and every test not yet run is reported as skipped.

Cause: the flush issued one cy.task per queued event, and each round-trip costs ~0.8s on a remote terminal (measured: 600 calls → 581s; 6000 calls → killed). A command-heavy test queues hundreds of events, so its afterEach ran for minutes.

Fix: batch the flush into a single cy.task per drain. Verified before writing the code — the same 600 events as one call took 109s vs 581s as 600 calls, i.e. baseline.

Result on the 3-spec repro: passed_with_skipped / 6 of 33 tests run / 993s → passed / 33 of 33 / 213s.

Telemetry delivery was checked separately this time, since a passing spec cannot distinguish delivered from dropped: test_observability_batch accepts a mixed batch of all four event types, a 600-event batch, and a batch containing an unknown task name, all without failing.

[shivam5643]

🔴 SDK PR Review — 1 blocking finding

Verdict: 🔴 Author to fix. The batching fix itself is sound and well-evidenced; one line regresses per-event delivery.

What this PR does

Each cy.task round-trip costs ~0.8s on a remote terminal, so the old one-task-per-event flush ran for minutes inside afterEach on command-heavy specs, blew spec_timeout, and reported every not-yet-run test as skipped — with nothing thrown (failed: 0), which is why it never reproduced locally. This batches a hook's whole drain into a single cy.task('test_observability_batch', ...) (600 events: 581s → 109s), fans out plugin-side to the same four IPC events, keeps the per-event tasks registered for back-compat, and excludes the batch dispatch in shouldSkipCommand so it can't recapture itself into the queue.

Blocking

1. MAX_BATCH_CHARS reused as the single-event drop ceiling — bin/testObservability/cypress/index.js:408. Full detail in the inline comment. Short version: the code drops any single event over 512KB, but the constant's own comment records that 768KB succeeds in one cy.task. Events in the 512KB–768KB band were delivered before this PR and are now silently discarded behind a console.warn.

Non-blocking

2. Dead field — every queued event still carries options: { log: false } (index.js:85, :105, :125, :141, :170, and the rest), but post-batching nothing reads it: the outer cy.task('test_observability_batch', toSend, { log: false }) hardcodes its own, and event.options is no longer passed anywhere. Worth removing, or leaving a note on why it's kept.

3. Test coverage — no automated guard on the new flush path (existing unit tests cover helper.js, not this file). The manual before/after infra evidence is the right kind for this bug class — a passing spec alone can't distinguish "delivered" from "silently dropped" — but the BStackAutomation case asserting expected_skipped_number: 0 that the PR description flags as pending is what turns this into a standing regression guard.

What's good

• Both beforeEach and afterEach route through the same flushEventsQueue() — no asymmetric fix.
• Clean error boundaries: queue cleared before dispatch so a throw can't replay events, per-event try/catch isolates one bad payload from the rest, outer try/catch backstops the flush, and warnFlushFailure deliberately uses console.warn rather than another Cypress command.
• IPC_EVENT_FOR_TASK maps all four queued task types correctly, and the batch handler guards both Array.isArray and unknown task names.
• The cy.now('task', ...) alternative is explicitly ruled out in a comment with the reason (throws on Cypress 14, would "fix" the skipping by delivering nothing) — exactly the kind of rejected-alternative note that saves the next reader a day.

Coverage ledger

All 6 diff regions across both changed files judged — no gap. default + node rule packs applied; the shared observability pack (OB-01/OB-05) applied as supporting judgment only, since this repo isn't one of the six SDKs it mechanically covers. The gRPC/Binary-pairing gate did not fire and correctly so — no .proto or generated-stub touch, and this repo has no Binary contract. Existing PR comments are all author-written and excluded per the author-exclusion rule.

_{SDK PR Review Agent · head 33a9232 · posted as a COMMENT (GitHub rejects APPROVE/REQUEST_CHANGES on a self-authored PR)}

[shivam5643]

All three addressed in 3ec1263.

1 (blocking) — MAX_BATCH_CHARS reused as the drop ceiling. Correct, and a regression this branch introduced. Split into two constants: MAX_BATCH_CHARS (512KB) closes a batch, MAX_EVENT_CHARS (768KB — the largest payload measured to send) decides what genuinely cannot be delivered. An oversized-but-sendable event now travels alone in its own batch rather than being discarded.

2 — dead options field. Left in place with a note at the dispatch site: every push site uses { log: false }, which the batch call passes once. Chose the note over 15 deletions to keep the diff reviewable; happy to remove them if you'd rather.

3 — test coverage. Added test/unit/bin/testObservability/batchFlush.js, 6 cases on the fan-out:

• batch task registered alongside the per-event tasks (back-compat)
• all four types routed to distinct IPC events, in order
• 600-entry batch emits 600 times
• unknown task name skipped without losing the rest of the batch
• malformed input (undefined, {}, junk entries) never throws
• one failing emit does not drop the remainder

9 passing with the existing suite. This is the guard a passing spec can't give, since a spec cannot distinguish delivered from silently dropped — which is exactly how an earlier attempt on this branch looked green while delivering nothing.

The BStackAutomation case asserting expected_skipped_number: 0 is still outstanding and remains the end-to-end guard.

Comments across both files trimmed.

[shivam5643]

✅ SDK PR Review — re-review at 3ec1263, blocker resolved

Incremental pass over 33a9232..3ec1263. Good to go.

Finding 1 (Critical) — RESOLVED, verified by trace

index.js:363-364 splits the two thresholds, and index.js:404-407 is the actual fix:

if (batch.length > 0 && batchChars + size > MAX_BATCH_CHARS) sendBatch();
batch.push({ task: event.task, data: payload });
batchChars += size;
if (batchChars >= MAX_BATCH_CHARS) sendBatch();

Traced the 600KB case both ways rather than assuming the shape is right. Batch empty: the new batch.length > 0 guard correctly skips the pre-flush, the event is pushed, then line 407 fires (600K ≥ 512K) and dispatches it alone. Batch non-empty: line 404 flushes the accumulated content first, the event lands in a fresh batch, line 407 dispatches it alone. Either way the 512–768KB band now travels instead of being dropped — which is what the finding asked for.

Two invariants the two-constant refactor could plausibly have broken, both checked:

• No batch can exceed the measured-safe ceiling. Line 404 permits an append only when batchChars + size ≤ 512KB, or when the batch is empty. So a multi-event batch is ≤512KB and a single-event batch is ≤768KB — nothing can be assembled past the point where cy.task was measured to succeed.
• No stranded or double-sent event. batchChars increments exactly once per event; sendBatch() clears batch/batchChars before dispatch and no-ops on empty — so the unconditional tail sendBatch() at line 413 can neither re-send a flushed batch nor leave the last partial one behind.

Finding 2 (dead options field) — RESOLVED

index.js:381 documents it rather than removing it, and the claim holds — all queued push sites do pass { log: false }. Fine as a documented simplification.

Finding 3 (test coverage) — PARTIALLY RESOLVED, non-blocking

test/unit/bin/testObservability/batchFlush.js adds 6 well-chosen cases — fan-out order, 600-entry batch, unknown task name, malformed input, one-failing-emit isolation. But all six target plugin/index.js's fan-out. The threshold arithmetic in cypress/index.js — the exact logic finding 1 was about — still has no automated guard. A case asserting that a 600KB event produces its own single-entry cy.task call would have caught the original bug and would catch a regression of it; right now that behavior is guarded only by the manual trace above. Worth a follow-up.

Non-blocking before merge

1. Release notes (internal) are stale. The section still reads "Batches are split at 512KB … A single event above that is skipped", and ## The change repeats "before batching it was dispatched alone and would have failed the command anyway". That's the pre-fix threshold plus the justification this commit abandoned — and it contradicts the 768KB-succeeds measurement cited in the same paragraph. It matters more than a description nit because that section is changelog-facing. One-line edit: 512KB → 768KB for the drop ceiling, and drop the "would have failed anyway" clause.

2. Comment asserts more than the data shows — index.js:398: /* unsendable at any size; the per-event flush could not deliver it either */. The measurement establishes only that 768KB passes and 1MB fails; the band between them is untested, so an 800KB event might have gone through pre-PR. Dropping there is the right conservative call — the comment just overstates the evidence for it.

Still clean at the new head

Both hooks symmetric on flushEventsQueue(), all three error boundaries intact (queue cleared before dispatch, per-event isolation, outer backstop), IPC_EVENT_FOR_TASK complete across all four task types, shouldSkipCommand still filtering test_observability_batch.

Coverage ledger

All diff regions in 3ec1263 judged: the two constants + flushEventsQueue/sendBatch (traced in full), the shouldSkipCommand and warnFlushFailure comment trims (no logic change), the plugin/index.js comment trims (no logic change), and all 6 cases in the new test file. No gaps.

_{SDK PR Review Agent · incremental pass, head 3ec1263 · posted as a COMMENT (GitHub rejects APPROVE/REQUEST_CHANGES on a self-authored PR)}

[shivam5643]

Both non-blocking items addressed, plus the coverage gap, in 456295d.

1. Release notes stale. Correct — and changelog-facing, so worth the fix. Both the ## The change paragraph and the internal notes now state the two thresholds properly: batches closed at 512KB, only a single event above 768KB skipped, and an event between the two dispatched alone rather than dropped. The abandoned "would have failed the command anyway" justification is gone.

2. Comment overstated the evidence. Also correct. /* unsendable at any size */ → /* past the largest size measured to send; 768KB-1MB is untested, so skip */. The measurement supports 768KB-succeeds / 1MB-fails and nothing about the band between, so the drop is a conservative choice, not a proven necessity.

3. Threshold arithmetic now has an automated guard. Added test/unit/bin/testObservability/batchThresholds.js — 6 cases against cypress/index.js itself (Cypress globals stubbed, command:start driven, afterEach callback invoked):

• small events share one batch
• a 600KB event is dispatched alone rather than dropped — fails against the pre-fix code
• a 600KB event is kept out of the batch holding small ones
• an event past the largest measured-safe size is skipped
• a multi-event batch never exceeds the split figure
• an empty queue sends nothing

15 passing across the o11y suite.

Worth recording, since it corrected me rather than the code: command:start queues two events (the command plus small platform details), so one oversized command legitimately produces two batches — the big event alone, the small one after. My first draft asserted a single batch and failed. The implementation was right; the expectation wasn't.

[shivam5643]

✅ GTG — clear for regression

Final pass at 456295d. All findings from the earlier passes are resolved; nothing outstanding is a defect.

Resolution status

Finding	Status
[Critical] MAX_BATCH_CHARS used as both the batch split point and the single-event drop ceiling, silently dropping 512–768KB events that were delivered pre-PR	✅ Resolved in 3ec1263 — verified by tracing both the empty-batch and non-empty-batch paths, not by the commit message. The band now dispatches alone.
[Minor] per-event options: { log: false } unread after batching	✅ Documented at index.js:381; claim verified against all push sites
[Minor] comment overstated the measurement	✅ Softened at index.js:399 to match what the data actually supports
[Minor] no automated guard on the flush threshold arithmetic	✅ batchThresholds.js added; the 600KB event alone case asserts positively and genuinely re-guards the Critical
[Docs] stale description / release notes	✅ Both sections now state the two-threshold split correctly

Also verified clean at this head: batching preserves end-to-end event order; clearing the queue before dispatch is strictly safer than pre-PR (which had no try/catch around the per-event calls and could throw into the customer's hook); and no re-entrancy path exists — shouldSkipCommand filters the batch task, and { log: false } suppresses the $Log so the dispatch fires no log:changed.

Two follow-ups — deliberately not merge gates

1. Batch accounting undercounts the wire payload. index.js:396 measures JSON.stringify(payload).length, but what is dispatched is an array of { task, data } wrappers — an unaccounted 42–55 bytes per event. On tiny events (retry storms) that is a ~42% undercount: batchChars reads 512KB while ~730KB goes on the wire.

This is not a live failure. 730KB is below the 768KB measured to succeed in a single cy.task, so no input drops or fails on it today — the code's internal margin assumption is simply wider than reality. One-line fix when convenient:

const size = JSON.stringify({ task: event.task, data: payload }).length;

2. Two of the six new threshold cases can pass vacuously. batchThresholds.js:94-103 and :105-115 assert only inside forEach over delivered batches, so they run zero assertions if everything is dropped — they cannot fail on over-dropping, which is the Critical's own bug class. A positive lower-bound assertion (totalSent equals the expected count) closes it. The other four cases, including the regression, do assert positively.

Regression

Clear to proceed. Regression is also the right gate for the one thing static review cannot settle — whether real remote-terminal cy.task dispatch behaves as measured. Worth including the command-heavy Cypress case asserting expected_skipped_number: 0 that the description still lists as pending, since that is what proves SDK-7399 fixed end to end.

_{SDK PR Review Agent · final pass, head 456295d · COMMENT event (GitHub rejects a decision on a self-authored PR)}