ci(1874): declare the diagnose lane and stop it misreading its own loop - #2059
Conversation
|
Not ready — two P1 diagnostics defects undermine the lane’s classification.
The slower-pass cadence direction is otherwise sound. |
|
Both fixed in 1. The So a skipped or no-result iteration killed the step before the verdict, the summary line and the artifact copy — silently truncating the loop at that iteration and losing precisely the one worth keeping. Phase extraction is Worth recording why the two neighbours were fine: the 2. Right — and I had made the same class of error I was fixing. Exercised rather than argued. I extracted the real loop body from the workflow and ran it under the actual The healthy pass is the only shape that produces no dump, and the loop now completes across the iterations that previously terminated it. 🤖 Addressed by Claude Code |
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. Top changed packed filesNo changed packed files. |
|
[P1] Do not let an empty debug extract abort the diagnostic matrix The workflow runs with set -euo pipefail, but every non-passed/slow iteration executes grep DEBUG-1874 "$LOG" | head -40 | tee -a stall-summary.txt. A genuine no-result/bad-selector case may contain no DEBUG-1874 line, so grep exits 1 and aborts the step before later iterations and the final artifact summary. That again turns the harness fault this PR is trying to distinguish into a truncated job. Guard the optional extract (for example (grep ... || true) | head ...) and add a fabricated no-result log without debug lines to the shell proof so the matrix is shown to continue and summarize it. |
|
Fixed in So the two verdicts this PR added specifically to name a harness fault were both still able to truncate the job before it summarized anything: a no-result log has no cadence line by definition, and a skipped iteration in Guarded inside the subshell as you suggested, so a genuine (grep 'DEBUG-1874' "$LOG" || true) | head -40 | tee -a stall-summary.txtSwept the rest of the block rather than fix this one alone, since three in a row is a pattern: the The proof now runs the whole per-iteration body — verdict, counters, summary line, cadence dump and evidence copy — not just the classification: Iterations 3 and 5 carry no 🤖 Addressed by Claude Code |
|
[P2] Check in the no-debug loop proof and remove the stray file The pipeline fix itself is correct: |
|
Both fixed in
The durable proof now exists: Your point lands harder than the file itself. Three defects have now shipped in this loop's shell, all the same shape — a The test slices the per-iteration region out of the workflow between declared markers — editing inside them is safe, removing them fails loudly — and runs it under the same shell options against every shape one iteration can take:
It asserts each verdict, which iterations keep evidence, and the four-counter summary line. A non-zero exit is the regression, so the loop aborting partway fails the test by construction. Planted red against all three defects, not just the current one: One bash subprocess, ~2 s, no device — and it is the only thing that exercises a 🤖 Addressed by Claude Code |
|
Took a holistic pass over the whole PR rather than only the reported findings, and it turned up two things worth having. The One awk pass now yields verdict, duration and poll count together, and the dump is The rewrite exposed a bug the new test could not see. The substring offset was 12 where Planted red against five defects, each caught: Also faster: 2.0 s to 0.2 s, since it is one awk per log instead of four subprocesses. Static gates green; 🤖 Addressed by Claude Code |
|
The red is not this PR, and the trace says something new — filed as #2080.
The field settled at 7 characters sharing exactly one with Worth noting for #1874 specifically: this is the first red lane since that fix where the trace was readable straight out of the uploaded artifacts with nothing added, which was the claim the instrumentation was kept for. Also visible in the same trace: the preceding 🤖 Addressed by Claude Code |
|
[P1] Make the 40-line cadence bound SIGPIPE-safe The previous no-match abort is fixed and the checked-in full-loop proof is valuable, but the replacement at .github/workflows/1874-diagnose.yml:154 still runs awk | head -40 | tee under pipefail. When a diagnostic log contains more than 40 DEBUG-1874 lines, head closes the pipe and awk exits on SIGPIPE; the pipeline returns 141 and aborts the loop before its final RESULT, recreating the same truncated-diagnostic failure class. I reproduced that exact shape locally under pipefail. Bound the match count inside awk (so it exits normally without head) and add a >40-debug-line full-loop case to the checked-in proof. |
f6a5dd0 to
98d4f1a
Compare
|
Fixed in Reproduced, and measured, because the size matters. My first attempt at 200 matched lines survived, which nearly had me report it as not reproducible: It fires once the matched output outgrows the pipe buffer, not at 40 lines. So at the volume one wait produces today it would not have fired — this is hardening rather than a live bug, and I would rather say that than overstate it. What makes it worth fixing anyway is the shape: it triggers on the iteration with the most cadence to report, which is the one worth keeping. Bounded inside awk as you suggested — no The proof gains a sixth shape, sized to reproduce the abort rather than only the missing disclosure. That distinction turned out to matter: at 45 lines the test caught a Five earlier defects still caught: duration offset, no-result folded into failed, dump reverted to bare grep, slow-pass clause dropped, absent verdict defaulting to failed. One process note: 🤖 Addressed by Claude Code |
|
You are right, and the paragraphs were pointing at the medium rather than at any one line. Four rounds here were all one class: a shell pipeline's exit status means "did this match", so every read of a log that legitimately matches nothing had to be talked out of killing the job, and a bound written with
verdict="$(node --experimental-strip-types scripts/diagnose-1874-iteration.ts \
"$LOG" "$TEST_NAME" "$i" "$rc" "$SLOW_PASS_MS" stall-summary.txt .tmp)"
case "$verdict" in
passed) pass=$((pass+1)) ;;
skipped) skipped=$((skipped+1)) ;;
no-result) noresult=$((noresult+1)) ;;
*) failed=$((failed+1)) ;;
esacSame pattern and reasoning as Moving it exposed a bug the previous shape had introduced. My awk rewrite dropped the Mutation testing drove three gaps out of the test, which is what caught that bug: every fixture had exactly one The test also stopped slicing bash out of the workflow and now calls the module directly — typed, and it exercises the shipped entry point end to end rather than a reconstruction of it. 🤖 Addressed by Claude Code |
|
Yes — six exports and seven CLI arguments for a script with exactly one caller. One exported function and one type now. verdict="$(node --experimental-strip-types scripts/diagnose-1874-iteration.ts \
"$LOG" "$TEST_NAME" "$i" "$rc")"102 lines to 79, and the workflow step now carries no comments at all — nothing left to explain. The test shrank with it, since the separate assertions were for functions that no longer exist: one table over the shapes an iteration can take, plus one end-to-end through the shipped entry point. Asserting the rendered lines rather than a duration field is what keeps it honest — that is what the threshold judges and what a reader of the artifact sees, so an extraction off by a digit has to show. Mutation testing found one more gap while shrinking it. Every fixture's cadence was poll lines only, so counting all cadence lines as polls read identically. Real waits bracket their polls with Net for the PR: the lane declaration the file never had, four honest verdicts instead of pass-or-stall, cadence kept for absorbed episodes, and the reading moved somewhere it can be tested. The remaining diff is mostly that header and the test. 🤖 Addressed by Claude Code |
|
[P2] Preserve decimal duration at the 2000 ms evidence boundary. The runner emits one-decimal |
|
P1: |
29370e5 to
be6622f
Compare
|
Both fixed in
The hidden-artifact change had no evidence behind it — and writing the contract found nine more.
All fixed, and That last row is worth a look independently of this PR: Scope note: fixing nine other workflows is more than this PR set out to do. It came out of writing the assertion you asked for, and a guard that fails on nine existing gaps cannot land without them. Happy to split it into its own PR if you would rather review it separately. Gates green on the rebased head: format, lint, typecheck, gate-manifest, and 216 tests across 🤖 Addressed by Claude Code |
154605e to
3b6a162
Compare
|
Split done, and the stale claims are corrected. Head #2091 carries the artifact policy — all ten hidden-path uploads plus I withdraw the Comments cut to the #2087 rule. The reader's narration is gone; what it was explaining is in names and types now — Body updated. The "confined to a dispatch-only workflow" line was written before the reader moved into One more thing that came out of this pass, unprompted: the entry-point test spawned nine 🤖 Addressed by Claude Code |
|
Not ready yet; two blockers remain on the current head.
The parsing/verdict model and exact-head CI are otherwise sound. Remove the new private implementation/history comments under current |
|
Both blockers fixed in The orphaned comment was my mistake, and worse than it looked. When I split the artifact policy out to #2091, my revert removed the To be explicit about where the flag went: #2091 carries this workflow's upload along with the other nine, because it is the same one-line rule in ten places and you asked for them together. Once #2091 lands, this branch rebases and the evidence path works with nothing further to do here. If you would rather this PR not depend on that ordering, say so and I will duplicate the single line here instead. The
Red against both mutations — hardcoding Comments are down to two: the module's one-line pointer to the workflow that calls it, and the measured CI numbers behind 🤖 Addressed by Claude Code |
|
The red is not this PR, and it is a real flake with a small fix — worth its own change rather than a re-run.
This PR touches four files — The mechanism is an ordering assumption. The test SIGTERMs the daemon, then waits with const after = await settleManagedBrowserProcesses(status);
assert.equal(after.count, 0, …);
assert.equal(isProcessAlive(daemonPid), false, '…exited after SIGTERM');The daemon reaps the fleet and then exits, so there is a window where the fleet is already zero and the daemon is still on its way out. The fleet's settle is being used as the daemon's wait, and the two are not ordered — nothing gives the daemon a deadline of its own. The fix is to settle each on its own deadline — poll I will re-run the job once the workflow finishes (it cannot be re-run while still in progress) so this PR has a clean head, but the flake will keep surfacing on unrelated PRs until that assertion gets its own wait. 🤖 Addressed by Claude Code |
|
Red again, and again not this PR — but the trace was worth pulling. Filed as #2103. The iOS smoke failed on A preflight snapshot failed, the runner session was invalidated, and the runner restarted inside the wait — ~12 s of it, against a 10 s budget. So the wait could not have succeeded whatever was on screen, and the error sent the reader to the app instead of to the restart. Two separable problems, both in #2103: the caller's budget is charged for a restart it did not ask for, and the error names the surface rather than the invalidation the daemon log records. Fixing the second alone would make this class self-diagnosing. Job re-run. This PR still touches only Running tally of unrelated infrastructure failures hit while landing these two PRs, all re-run-to-green: two 🤖 Addressed by Claude Code |
|
The rc verdict/evidence route is now sound and exact-head CI is green, but this is not ready. The workflow still uploads hidden |
50f0da7 to
a35a690
Compare
|
Rebased onto To be explicit about reversing your earlier P1 rather than quietly undoing it: you asked for a non-closing reference on the grounds that #1874 remained the open If you would rather it close as mitigated with the premise corrected by hand instead of by merge, say so and I will drop the keyword again and close it with the evidence comment. The branch was rebuilt rather than rebased. Its history had eleven commits, several of which added and then removed the files #2091 now owns; rebasing them onto a main that already has those files produced a conflict per commit and, in one case, silently reverted main's Content is unchanged from |
|
The #2091 dependency is now integrated correctly: |
a35a690 to
a5e49c8
Compare
|
Both blockers addressed on Decimal Narration. The workflow header is down to the four #1781 fields with the history dropped, and the script's justification comment on
Green locally: |
|
Decimal duration handling and the 2000.1ms regression are fixed, and the hidden-artifact pairing remains correct. Still not ready: the workflow's deletion criterion points to unrelated #2080 even though this lane owns/closes #1874; align lifecycle/linkage with the actual investigation. Remove remaining workflow/script issue-history/control-flow narration. Android smoke is red pending failure classification and the rest of exact-head CI is incomplete. |
|
The verdict parser, rc=65 script path, hidden-artifact handling, and exact-head CI are clean, but the lane lifecycle is contradictory. This workflow runs #1874's test and the PR closes #1874, while its kill criterion says to delete it when unrelated #2080 closes; #1874 remains open/needs-info and #2080 tracks synthesized fill corruption. Align the lane owner/kill criterion with the actual targeted test and decide #1874 closure separately. Also add a small workflow-contract assertion that the shipped YAML forwards the captured $rc to diagnose-1874-iteration.ts; the direct script test would stay green if YAML regressed to 0. Then trim the workflow/script narration to the concise mandatory Catches/Evidence/Cost/Kill declaration under current comment policy. |
The loop that #1874 is investigated with could not tell the truth about itself. It classified every non-`passed` iteration as a stall, which after #2035 gave the looped test an XCTSkipIf meant an environment flip would report a 100% stall rate; it captured cadence only for failures, though an absorbed episode now passes; and it read its logs with shell pipelines whose exit status means "did this match", so an iteration that legitimately matched nothing killed the job before it could be summarized. scripts/diagnose-1874-iteration.ts reads one iteration: xcodebuild's own verdict, the `type-all` duration, and the cadence worth keeping. A nonzero exit outranks a green measured test — in `pair` mode the neighbour or the runner can fail while the measured test passes — and a run that produced no verdict is named as ours rather than counted as a stall. The workflow gains the #1781 lane declaration it never had. Its kill criterion names #2080, which the loop can now serve rather than merely claim to: the looped test is a dispatch input, so the fill route that #2080 traces loops the same way. One test pins the contract the script cannot check about itself — that the workflow hands it the status xcodebuild returned rather than a literal. Closes #1874. Both filed symptoms are resolved. `smoke:form-input` was root-caused and fixed in #2035: the fixture's placeholder was identical to the value every suite filled, so `fill` could never be verified on the penalized route — deterministic, not a flake, and only visible under load because that route is gated on a penalized XCTest channel. The targeted XCTest is mitigated by the progress-aware commit budget, with 200 consecutive green loop iterations across two dispatches. The issue's remaining question — why the input pipeline throttles — is answered by the second dispatch, and the premise was wrong: it does not. Posting 17 characters took 484 ms and the commit was observed on the first poll, inside an iteration whose `type-all` measured 14334 ms. The ~12.6 s went to accessibility round-trips before any character was posted, which is #1105's path, not the input pipeline's.
a5e49c8 to
41bc14e
Compare
|
Rebased onto The lifecycle contradiction was real, and my first answer to it was worse than you knew. I tried to re-point the kill criterion at #1105 — the issue the second dispatch's evidence actually lands on — and found it is CLOSED. So there was no third issue to hide behind, which left the honest choice: either the lane dies with #1874, or it is genuinely the reproduction path for something still open. It is now the latter by construction rather than by assertion. The looped test is a On closing #1874 separately: it stays on this PR, and that is deliberate. Its The $rc contract. Good catch: the script test would stay green against a YAML that passed a literal. Added, and observed red against a planted Narration. The header is now the four #1781 fields and nothing else — the preamble is gone, and so is the script's file comment, which the new contract test makes redundant. One thing not asked for, in lines this change already touched: both dispatch inputs moved from Green locally: Android smoke, classified. Not this PR: this diff is a dispatch-only workflow, a private script and its test, none of which that lane reads. The same signature failed on |
|
Code review is clean at This is not merge-ready yet: the cited workflow dispatches ran at earlier heads, while this head changes the actual iOS diagnostic workflow wiring (dispatchable test input, |
|
Two defects in the harness #1874 depends on, both found by dispatching it on
mainright after #2035 merged.The harness was about to lie in two directions
Every non-
passediteration counted as a stall. #2035 gave the looped test anXCTSkipIfon an ambient software keyboard, so an environment flip would now be reported as a 100% stall rate — the artifact would say "25/25 STALL" with zero cadence lines. A selection matching nothing (the silentExecuted 0 testsshape) reported identically. It now reads xcodebuild's own verdict word and countspassed/skipped/failed/no-resultseparately:Checked against real log shapes for all four cases.
Cadence was captured only for failing iterations. Since #2035 the commit wait absorbs an episode that keeps making progress — so an episode now passes, and the loop was recording only the episodes bad enough to still be red. It now also dumps
[DEBUG-1874]for any iteration slower thanSLOW_PASS_MS.The threshold is measured, not guessed
From the 2026-08-26 dispatch (run 32983174128), arm64 legs:
Both legs: 25/25 passed, 0 stalled. But the isolated leg had two outliers — 4.0 s and 14.1 s — and under the old rule neither produced a single cadence line, because they passed.
Keyed on elapsed time rather than poll count because those outliers settled on the first poll (
polls=1): the time went somewhere before the wait, not inside it. A poll-count threshold would have missed exactly the shape this dispatch found. 2000 ms sits above every healthy iteration measured (n=50) and below every episode on record, including the 2487 ms/5-poll one from 2026-08-21.Since first review
The reader moved out of YAML into a typed script, after four rounds of the same defect — a shell
pipeline treating "nothing matched" as failure under
pipefail. A nonzeroxcodebuildexit is nowits own
run-failedverdict rather than a silent pass. The hidden-path upload fixes that startedhere moved to #2091, which should land first.
Lane declaration
The file never had the #1781 declaration. Added: Catches, Evidence, Cost, and a Kill criterion that names the commit which should delete it (the one closing #1874).
No gate-manifest entry, deliberately: the
gate: swift-runner-iosinput in this workflow is the shared build action's parameter, not an ownership claim —ios.ymlowns that gate, andpnpm check:gate-manifestis green without a declaration. (MANUAL_ONLY_OWNERSis for a gate whose only lane is dispatch-only, so it does not apply here.)Validation
YAML parses;
pnpm check:gate-manifestgreen;test/ciguards green (9). The verdict extraction was exercised against fabricated logs for passed/skipped/failed/no-result. The behaviour lives inscripts/diagnose-1874-iteration.ts, exercised directly byscripts/__tests__/diagnose-1874-iteration.test.ts— a table over every shape one iteration cantake, plus one invocation of the shipped entry point. The workflow around it is
workflow_dispatch-only, so CI cannot exercise that half; two dispatches have(32983174128,
33072911858).
Closes #1874 — restored after the second dispatch answered the question that kept it open.
It was
Refswhile the throttle was unidentified. It is identified now, and the issue's premisewas wrong: the input pipeline is not throttled. In the 14.3 s outlier, posting 17 characters took
484 ms and the commit was observed on the first poll; the ~12.6 s went to accessibility
round-trips before any character was posted — #1105's path, not the input pipeline's. Both filed
symptoms are resolved:
smoke:form-inputroot-caused and fixed in #2035, the targeted XCTestmitigated by the progress-aware budget, with 200 consecutive green loop iterations across two
dispatches.