Skip to content

fix(test): make the iOS simulator smoke lane's red/green signal meaningful (#2491) - #3336

Merged
thymikee merged 3 commits into
mainfrom
fix/ios-smoke-lane-reliability-2491
Oct 9, 2026
Merged

thymikee merged 3 commits into
mainfrom
fix/ios-smoke-lane-reliability-2491

Conversation

@thymikee

@thymikee thymikee commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Summary

Repairs two ruling defects that made the iOS simulator smoke lane's signal unreliable (attribution in #2491).

  • Addresses the assertion side of ci(ios): a regular depth-1 snapshot carries XCTest tree quality metadata when the AX bridge probe circuit is open #3328 — the snapshotQuality assertion in assertSimulatorSnapshotTreeDepthFrontier demanded an absence the product never promised. The product contract is a disclosure: the AX-bridge path publishes no verdict (the bridge has no quality model); the XCTest runner always stamps its strategy. A healthy tree verdict on the circuit-disabled fallback is legitimate acquisition, not a quality regression. Replaced with assertSimulatorSnapshotAcquisition, keyed on the typed verdict and reason code (rejects sparse; rejects recovered unless pre-selected deferred/requested-backend), never on the warning text. Both recorded CI payloads (runs 37459193672, 37145696803) replay through the new assertion and pass.
  • Retry policy decision for the infra-flaked wait class (observed 5x on main: wait_runner_restart_exhausted, wait_capture_stalled, wait_readiness_exhausted): a shared harness seam (reattemptInfrastructureMiss) re-issues one failed live step, enabled by the iOS lane via a typed predicate on error.retriable === true AND a transport/observation wait reason. Hard failures stay red: wait_target_absent, wait_deadline_exceeded, wrong asserted values, runner crashes, non-wait failures. Parallel-run replays already had --retries 2; the E2E step had no layer. Field confirmation of the conjunction's narrowness on this PR's own lane: run 37843633972 hit wait_deadline_exceeded (24/25 readable polls) on the webview cold-start step and the harness declined to re-issue it, exactly the readable-miss case the set excludes; recorded in iOS smoke lane: two untracked failure signatures from the #2491 attribution pass (MAIN_THREAD_TIMEOUT on alert dismiss; replay timeout_cleanup_pending) #3337.

Test/harness only — no production lines changed.

Validation

Head ffd575ae7:

  • pnpm check:quick — clean (lint + typecheck).
  • pnpm test:integration:node — 147 tests, 135 pass, 0 fail, 12 skipped.
  • node --test on the three touched test files — 28 pass, 0 fail.
  • pnpm check:affected --run — all runnable checks passed.
  • Regression proofs: old runtime fails both new re-issue tests; old frontier assertion rejects the new acquisition test.

Unresolved risks: no live lane rerun performed (test-only change; CI re-runs the lane on this PR). Retried misses currently share one runner/session, so a runner-dead miss may re-fail — bounded at one re-issue; per-scenario fresh-runner teardown is the lane rewrite follow-up. Untracked new-signature failures observed during attribution (MAIN_THREAD_TIMEOUT on alert dismiss in 37660079219; gesture-replay timeout_cleanup_pending in 36115336398) need issue triage outside this scope.

View guided diff Turn on auto-fix

… snapshot assertion (#3328)

The lane asserted a regular snapshot must not carry snapshotQuality at all,
but the product's contract is a disclosure, not an absence: the AX bridge
publishes no verdict, and the XCTest runner the route falls back to always
stamps its serving strategy. When the bridge probe circuit is open for the
app generation (a legitimate cold/loaded-host state), the runner-served
healthy 'tree' verdict made the lane red twice on main (runs 37145696803,
37459193672). The assertion now keys on the typed verdict: an absent
(bridge) or healthy verdict is accepted, a degraded 'recovered' or
'sparse' verdict still fails, never on the warning string.
…tion was prevented (#2491)

The replay steps in the iOS smoke job run with --retries 2 while the
fixture-backed E2E step runs node --test with no retry layer, so one cold
runner restart inside one 10s wait fails the whole job. Seven of the last
ten main failures are this one class: a wait whose own error names
wait_capture_stalled, wait_runner_restart_exhausted, or
wait_readiness_exhausted and carries retriable: true — the product itself
says 'retry' on those verdicts.

The harness gains an opt-in per-step classifier; the iOS lane feeds it a
predicate keyed on that typed conjunction only (wire retriable AND the
wait taxonomy reason), never on error text. A re-attributed miss re-issues
the step once; a readable miss (target absent, deadline exhausted after a
readable capture, a wrong asserted value, a runner crash) stays a hard
failure. Steps that already own a miss policy (allowFailure/expectFailure)
are exempt so existing retry loops keep their semantics.
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.13 MB 5.13 MB 0 B
Package (unpacked) 5.13 MB 5.13 MB 0 B
Package (download) 1.54 MB 1.54 MB +1 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 19.9 ms 19.1 ms -0.9 ms
CLI --help 56.6 ms 52.7 ms -3.9 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 7 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread test/integration/ios-simulator-e2e/live-snapshot-depth-frontier.ts
…ulary

The acquisition assertion only rejected 'sparse' and unexempt 'recovered', so a
verdict with any other state (renamed enum, diverged runner) fell through both
branches and certified depth facts off a tree the lane never classified. The
state set is now pinned to the kernel-declared SNAPSHOT_QUALITY_STATES, with a
negative test for states outside it.
@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Coordinator pass actions (head now 98321a2)

Cubic P2 (unrecognized snapshotQuality state read as green) — fixed in 98321a2. The present-verdict branch now asserts membership in the kernel-declared SNAPSHOT_QUALITY_STATES before handling any state, so an unknown/renamed/missing state fails with the response that proved it instead of exhausting nothing. Negative test added (unknown / degraded / absent state all throw); the three legitimate states behave exactly as before. Reply in the review thread: #3336 (comment)

pnpm check:affected --run on 98321a2: all runnable checks passed (after repo-wide pnpm format + pnpm check:quick clean; focused file 10/10 pass; regression proof: reverting only the assertion makes the new test fail).

The red Smoke Tests run is not this diff — recorded as instructed, not chased.

Is #3328 addressed here or owed? Addressed on the assertion side; one completion item owed. #3328's required behavior is conditional: if the bridge-disabled fallback is legitimate for a regular snapshot, the assertion must assert the disclosed fallback through typed reasons instead of demanding absence — that is the decision this PR implements (assertSimulatorSnapshotAcquisition, keyed on verdict state + reason code, never the warning string), including both recorded circuit-disabled payloads. What is still owed per #3328's Completion: (a) the demo was a replay of the two real CI circuit-disabled payloads, not a fresh live simulator run, and (b) the sub-question whether "probe circuit open + XCTest tree capture exceeded its 8s time slice" is itself a product defect (owned under #2972) is not investigated here. Maintainers: keep #3328 open for those two items rather than letting the merge auto-close it.

@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Re-run outcome (requested in the coordinator pass)

Re-ran the lane at head 98321a279: 37843633972.

Checks on 98321a279: the only failing check is this Smoke Tests lane, failing on the #3343 signature, not on anything this diff asserts.

@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Coordinator pass 2 — disposition (head unchanged, 98321a279)

Gate field-confirmation recorded in the PR body. Run 37843633972 is the first field witness that the re-issue conjunction refuses what it must refuse: wait text "Jump to form" 20000 failed with reason: wait_deadline_exceeded, 24/25 readable polls, and the harness declined to re-issue (zero (re-issue after observation-prevented miss) records in step-history.json) — a readable miss is the product answering. No widening of OBSERVATION_PREVENTED_WAIT_REASONS happened or will happen in this PR; the owning fix for a real latency class is the 20000 ms budget or the page-load path, and I've written that decision rule into the issue itself.

Signature disposition — consolidated per your preference, then corrected against the record.

Re-run: re-ran failed jobs of 37843633972 @ 98321a279 (the fixed head — no old-head ambiguity this time). Still queued behind macOS runner congestion as of this comment; T3 is watching the PR and the outcome will be appended here on wake. Nothing in this PR's assertions changed since the last green pnpm check:affected --run on 98321a279 (all runnable checks passed); no code pushed since, so no new gate run owed.

@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Re-run outcome (as promised): 37843633972 @ 98321a279 re-ran and all checks now pass — the Smoke Tests job went fully green, including smoke:webview-remote-content (the #3337 third-signature step) on the identical head. Same diff, same simulator, one host later: the webview cold-start miss did not reproduce, matching the #3337 decision rule's cold-host-flake shape (no main reproduction, page content present in the prior post-failure snapshot). The preflight class likewise stayed non-reproducing. PR head unchanged at 98321a279 — awaiting-human.

@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

The code looks right at 98321a2, but I am calling this evidence-pending: I did not download the run artifacts, so I could not confirm that the recorded payloads replay through the new assertion, or whether the green re-run absorbed any re-issue. All 19 checks pass at 98321a2, and the Smoke Tests job, which runs the changed harness and the depth-frontier scenario, passed on re-run 37843633972. I did not check whether a failed test suite step (replay-suite with --retries) carries a nested wait reason plus retriable, which would stack a second retry layer. I did not run the tests locally. No code defect blocks this, and there are no conflicts. Please confirm the points above from the run artifacts, and consider the first two notes below before or soon after merge.

Not blocking, take or leave: the re-issue predicate in live-harness.ts runs on every iOS step, including replay and batch steps, whose failures carry the nested wait's retriable: true and details.reason to the top level, so a whole script could be re-run after earlier mutating steps (today's replays start with open --relaunch or launchApp clearState, so I found no wrong outcome); the rule could be to re-issue only a step whose own command is a declared read, for example resolveCommandRecordingEffect(step) === 'observes-app'. Also, when a re-issue turns a step green, the miss is recorded only in step-history.json (runtime.ts), so one stderr line or annotation with the typed reason would let you count absorbed misses. The waitFailure() fixtures in the retry policy test hand-write the wire shape, so building them with normalizeError or from a recorded CLI payload would keep them honest. PRE_SELECTED_REASON_CODES in live-snapshot-depth-frontier.ts restates a rule that also lives in quality-warnings.ts:58 and snapshot-quality-latch.ts:55, so one exported predicate could serve all three. The new assertion at line 348 accepts a capture served by the runner fallback, because the bridge's fallback reason is only a diagnostic event today, so that check can move to #3328/#2972 once the route is typed on the wire.

Is there a smaller design than this? I looked and found none: the re-issue layer is opt-in, sits on the shared harness, allows one extra attempt, and keys on typed reason plus retriable, while product-side retries inside wait would change the meaning of the reasons #2491 attributes. For the assertion to check the fallback disclosure as typed data, the snapshot route's circuit-disabled or bridge-fallback reason must first appear as a typed field in the snapshot response (#2972).

@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Artifact confirmations and the unchecked gap (from run artifacts at 98321a279)

(a) The recorded payloads replay through the new assertion — shown, not claimed. /tmp/replay-3328.mjs extracts the exact JSON.stringify(result) object the OLD assertion printed in each failing CI payload and feeds it to the new one at head 98321a279:

$ node --experimental-strip-types /tmp/replay-3328.mjs
37145696803 payload accepted: {"backend":"tree","state":"healthy"}
37459193672 payload accepted: {"backend":"tree","state":"healthy"}

The shape the assertion reads is what the artifacts contain: data.snapshotQuality = {state:'healthy', backend:'tree', timing:{…}} plus the circuit-disabled warning — the same shape as the unit-test fixtures, which the recorded payloads (not invented shapes) seeded. Live corroboration from the green re-run: smoke:regular-visible-depth-frontier passed 7/7 steps under the new assertion, and the earlier failing run's own failed-step-102-snapshot.json shows the identical disclosure (healthy/tree + circuit-disabled warning) on a live CI simulator.

(b) The green re-run absorbed NOTHING — it is not retry-absorbed green. Attempt-2 step-history.json (100 steps): zero records carry the (re-issue after observation-prevented miss) suffix the harness stamps on every re-issued issue, and there are zero nonzero-status steps outside expectFailure visibility probes. The webview step that failed at 21126 ms in attempt 1 passed in 1688 ms in attempt 2 — page rendered promptly, no second bite anywhere. I agree the deeper point though: an absorbed miss today IS only visible inside step-history.json, and that observability gap is exactly #2491's subject — filed, see note 2 below.

The unchecked gap, answered from the wire path: a failed test suite step cannot stack a second retry. (1) A suite that runs and has failing scripts returns ok:true with {failed:N} counts (test-command.ts returns {ok:true, data} whenever the scheduler completes; per-script failures live inside data.tests[], nested, never hoisted) — result.status === 0, and the predicate's first line returns false before reading anything; the harness's own assert.equal(suite.json?.data?.failed, 0) is what rules. (2) The scheduler-level failure path is errorResponse(outcome.error.code, outcome.error.message) — code and message only, no details, no retriable — and request-finalization.ts preserves that emptiness on rebuild. The predicate needs top-level retriable === true AND a wait-taxonomy reason, so neither path fires. Where hoisting IS real: a single-script replay step returns the failed step's own wire response verbatim via readLastResponse() (native-command.ts), so a nested wait's retriable/reason does reach the top level there — benign today (replays are relaunch-first), and that's the step that note 1's scoping should close.

Non-blocking notes — one owner each, no comment-thread TODOs

Re: "is there a smaller design" — agreed, and the run 37843633972 webview miss remains the counterexample that keeps it this small: the conjunction refused it, the step stayed red, and the signature went to #3337 instead of into a green column.

@thymikee

thymikee commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

Thanks, this answers what I left open at 98321a2. The two recorded payloads pass the new assertion, the green re-run shows no re-issued step in its step history, and a failed test suite step cannot stack a second retry because it carries no top-level retriable or wait reason. The remaining notes now live in #3344 and #2972, so nothing blocks this from my side. There are no conflicts and all checks pass.

@thymikee
thymikee merged commit 51d5906 into main Oct 9, 2026
19 of 21 checks passed
@thymikee
thymikee deleted the fix/ios-smoke-lane-reliability-2491 branch October 9, 2026 06:56
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-09 06:57 UTC

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant