Skip to content

ci(1874): declare the diagnose lane and stop it misreading its own loop - #2059

Merged
thymikee merged 1 commit into
mainfrom
claude/1874-diagnose-lane-declaration
Aug 28, 2026
Merged

ci(1874): declare the diagnose lane and stop it misreading its own loop#2059
thymikee merged 1 commit into
mainfrom
claude/1874-diagnose-lane-declaration

Conversation

@thymikee

@thymikee thymikee commented Aug 26, 2026

Copy link
Copy Markdown
Member

Two defects in the harness #1874 depends on, both found by dispatching it on main right after #2035 merged.

The harness was about to lie in two directions

Every non-passed iteration counted as a stall. #2035 gave the looped test an XCTSkipIf on 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 silent Executed 0 tests shape) reported identically. It now reads xcodebuild's own verdict word and counts passed / skipped / failed / no-result separately:

verdict="$(sed -n "s/.*$TEST_NAME]' \([a-z]*\).*/\1/p" "$LOG" | tail -1)"

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 than SLOW_PASS_MS.

The threshold is measured, not guessed

From the 2026-08-26 dispatch (run 32983174128), arm64 legs:

leg n min median p90 max
isolated 25 654 ms 796 ms 1131 ms 14146 ms
pair 25 628 ms 702 ms 807 ms 1106 ms

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 nonzero xcodebuild exit is now
its own run-failed verdict rather than a silent pass. The hidden-path upload fixes that started
here 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-ios input in this workflow is the shared build action's parameter, not an ownership claim — ios.yml owns that gate, and pnpm check:gate-manifest is green without a declaration. (MANUAL_ONLY_OWNERS is for a gate whose only lane is dispatch-only, so it does not apply here.)

Validation

YAML parses; pnpm check:gate-manifest green; test/ci guards green (9). The verdict extraction was exercised against fabricated logs for passed/skipped/failed/no-result. The behaviour lives in scripts/diagnose-1874-iteration.ts, exercised directly by
scripts/__tests__/diagnose-1874-iteration.test.ts — a table over every shape one iteration can
take, 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 Refs while the throttle was unidentified. It is identified now, and the issue's premise
was 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-input root-caused and fixed in #2035, the targeted XCTest
mitigated by the progress-aware budget, with 200 consecutive green loop iterations across two
dispatches.

@thymikee

Copy link
Copy Markdown
Member Author

Not ready — two P1 diagnostics defects undermine the lane’s classification.

  1. Under set -euo pipefail, phase="$(grep ... | tail -1)" exits the step whenever a skipped/no-result iteration has no type-all phase, before the verdict, summary, and artifact are recorded. Make phase extraction best-effort and preserve empty/zero duration.

  2. The aggregate still folds every non-passed/non-skipped result into fail and reports that count as stalled; a selector/build producing no-result is therefore still falsely called a stall. Track failed and no-result separately and label each honestly.

The slower-pass cadence direction is otherwise sound.

@thymikee

Copy link
Copy Markdown
Member Author

Both fixed in a861409e0f. Confirmed each before changing anything.

1. The pipefail kill is real, and it was worse than "no phase recorded".

$ bash -c 'set -euo pipefail; echo start; phase="$(grep -o nope /etc/hosts | tail -1)"; echo survived'
start
exit=1

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 || true now; empty already parsed to duration 0.

Worth recording why the two neighbours were fine: the grep -c for polls was guarded from the start, and the sed that reads the verdict exits 0 on no match (checked). So this was the only unguarded one.

2. Right — and I had made the same class of error I was fixing. no-result fell into the *) arm and got reported as "stalled", when it means the selection matched nothing or the bundle never ran: a fault in this harness, not something the product did. Four counters now, each under its own name. I also stopped labelling failed as "stalled": whether a failure was a stall is what reading the cadence decides, so the summary should not pre-judge it.

RESULT arch=… mode=…: $pass passed, $skipped skipped, $failed failed, $noresult no-result of $ITER

Exercised rather than argued. I extracted the real loop body from the workflow and ran it under the actual set -euo pipefail against fabricated logs for all five shapes:

pass.log      verdict=passed     duration=796    polls=1 dump=no
slowpass.log  verdict=passed     duration=14146  polls=1 dump=yes
skip.log      verdict=skipped    duration=0      polls=0 dump=yes
failed.log    verdict=failed     duration=3100   polls=0 dump=yes
none.log      verdict=no-result  duration=0      polls=0 dump=yes
RESULT: 2 passed, 1 skipped, 1 failed, 1 no-result   (loop exit=0)

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

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.48 MB 2.48 MB 0 B
JS gzip 834.8 kB 834.8 kB 0 B
npm tarball 958.3 kB 958.3 kB 0 B
npm unpacked 3.32 MB 3.32 MB 0 B

npm unpacked components

Component Base Current Diff
JS / dist source 2.64 MB 2.64 MB 0 B
Apple runner source/project 581.1 kB 581.1 kB 0 B
macOS helper source 54.8 kB 54.8 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 45.4 kB 45.4 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 30.4 ms 30.9 ms +0.4 ms
CLI --help 83.8 ms 86.3 ms +2.5 ms

Top changed chunks: no changes in the largest emitted chunks.

Top changed packed files

No changed packed files.

@thymikee

Copy link
Copy Markdown
Member Author

[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.

@thymikee

Copy link
Copy Markdown
Member Author

Fixed in 31b569ab82. You were right, and it is the third instance of the class — one line below the one I fixed last round.

$ bash -c 'set -euo pipefail; grep DEBUG-1874 skip.log | head -40 | tee -a out; echo after'
before dump
exit=1                     # never reaches "after"

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 isolated mode has none either (the neighbour test that would have logged one only runs in pair).

Guarded inside the subshell as you suggested, so a genuine head/tee failure still surfaces:

(grep 'DEBUG-1874' "$LOG" || true) | head -40 | tee -a stall-summary.txt

Swept the rest of the block rather than fix this one alone, since three in a row is a pattern: the find for the xctestrun exits 0 on no match, the sed that reads the verdict exits 0 on no match, system_profiler is already || true, and the phase and poll extracts were guarded earlier. This was the last unguarded pipeline in the loop.

The proof now runs the whole per-iteration body — verdict, counters, summary line, cadence dump and evidence copy — not just the classification:

iter=1 verdict=passed    rc=0  phase=type-all durationMs=796.1    polls=1
iter=2 verdict=passed    rc=0  phase=type-all durationMs=14146.4  polls=1
  [DEBUG-1874] wait start expectedLen=17 route=append
  [DEBUG-1874] poll t=1ms observedLen=0 expectedPrefixLen=0
iter=3 verdict=skipped   rc=0                                     polls=0
iter=4 verdict=failed    rc=65 phase=type-all durationMs=3100.0   polls=1
  [DEBUG-1874] wait start expectedLen=17 route=append
  [DEBUG-1874] poll t=1ms observedLen=0 expectedPrefixLen=0
iter=5 verdict=no-result rc=0                                     polls=0
RESULT: 2 passed, 1 skipped, 1 failed, 1 no-result of 5      (loop exit=0)
evidence copied: stall-evidence-{2,3,4,5}.log

Iterations 3 and 5 carry no DEBUG-1874 line at all — the matrix continues through both and still summarizes. Verified red first: the pre-fix pipeline exits 1 on the skipped log.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

[P2] Check in the no-debug loop proof and remove the stray file

The pipeline fix itself is correct: (grep ... || true) keeps an empty optional debug extract from aborting under pipefail while preserving real head/tee failures. But the required fabricated skipped/no-result full-loop regression exists only in a PR comment, so the third recurrence of this failure class still has no reproducible repository guard. This commit also accidentally adds root f.tmp containing only x. Please remove f.tmp and add the versioned shell/workflow proof that runs no-debug skipped and no-result cases through the final RESULT/evidence-copy assertions; the dispatch-only lane is not exercised by normal PR CI.

@thymikee

Copy link
Copy Markdown
Member Author

Both fixed in 98f49674d6.

f.tmp was mine and I should have caught it. It came from a bash -c command I ran in the repo root while proving the previous fix, and git add -A swept it into 31b569ab82. Removed; confirmed gone from the branch head. Removal commit rather than a force-push, since this repo squash-merges and the intermediate history collapses anyway.

The durable proof now exists: test/ci/1874-diagnose-loop.test.ts.

Your point lands harder than the file itself. Three defects have now shipped in this loop's shell, all the same shape — a grep matching nothing exits non-zero under set -euo pipefail, so the iteration that mattered is the one that kills the job before it can be summarized. Every round was caught by you reading the shell, and every proof I offered was ad-hoc and thrown away. Reading YAML cannot catch that class; only running it can.

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:

shape verdict keeps evidence
healthy pass passed no
slow pass (14.1 s) passed yes
skipped — no phase, no cadence skipped yes
failed failed yes
no-result — no phase, no cadence no-result yes

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:

mutant 1: unguard the cadence dump (this PR's P1)  -> × 1 failed
mutant 2: unguard the phase extract (previous round) -> × 1 failed
mutant 3: fold no-result back into failed            -> × 1 failed

One bash subprocess, ~2 s, no device — and it is the only thing that exercises a workflow_dispatch-only lane before someone needs it mid-investigation.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Took a holistic pass over the whole PR rather than only the reported findings, and it turned up two things worth having. cbfdb77cdc.

The || true guards were the wrong fix, and the comment-to-code ratio was the tell. Three review rounds here were all the same finding: a grep that matches nothing exits non-zero, set -euo pipefail aborts the step, and the iterations with nothing to match are precisely the ones this loop exists to name. I was treating instances — two guards and ten lines of comment explaining them. The class is that grep reports "not found" as failure. awk does not; its END block always runs.

One awk pass now yields verdict, duration and poll count together, and the dump is awk '/DEBUG-1874/'. What that removed: both guards, the three-step shell mangling that carved an integer out of durationMs=796.1, a regex built by interpolating $TEST_NAME, and ~10 lines of comment. The region is shorter and has no exit-status hazard left to guard.

The rewrite exposed a bug the new test could not see. The substring offset was 12 where durationMs= is 11 characters, so 796.1 read as 96 and 3100.0 as 100. The test still passed — it asserted verdicts and evidence-keeping, and 14146.4 mis-read as 4146 still cleared the 2000 ms threshold. So the test was under-specified in exactly the place that matters: the duration is what the slow-pass threshold judges and what a reader of the artifact sees. Both fixed — the offset, and the test now asserts the reported value.

Planted red against five defects, each caught:

1. duration offset off by one          -> caught
2. no-result folded back into failed   -> caught
3. dump reverted to bare grep          -> caught
4. slow-pass clause dropped            -> caught
5. absent verdict defaults to failed   -> caught

Also faster: 2.0 s to 0.2 s, since it is one awk per log instead of four subprocesses.

Static gates green; test/ci 10 passed.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

The red is not this PR, and the trace says something new — filed as #2080.

smoke:form-input failed on fill id="field-email" ada@example. This PR changes one workflow file, one test and vitest.config.ts; nothing it touches is reachable from the runner. Job re-run; the cadence is the interesting part:

route=synthesized-first-responder-replacement
[DEBUG-1874] synthesize posted 11 chars status=0 tookMs=1084
[DEBUG-1874] poll t=1102ms observedLen=7 expectedPrefixLen=1
[DEBUG-1874] poll t=2395ms observedLen=7 expectedPrefixLen=1
[DEBUG-1874] poll t=3717ms observedLen=7 expectedPrefixLen=1
[DEBUG-1874] poll t=5097ms observedLen=7 expectedPrefixLen=1
[DEBUG-1874] wait outcome=notObserved elapsedMs=6293

The field settled at 7 characters sharing exactly one with ada@example, and never moved again — flat expectedPrefixLen across four polls over ~4 s. That is the dropped-character corruption, not a throttle: the characters are not late, they are wrong, and the value is stable. It is also not the #2035 deadline misbehaving — progress at the first poll only, condemned 3 s later, exactly as designed.

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 fill full name settled at observedLen=12 expectedPrefixLen=12 — the placeholder collision fixed in #2035 is staying fixed.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

[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.

@thymikee
thymikee force-pushed the claude/1874-diagnose-lane-declaration branch from f6a5dd0 to 98d4f1a Compare August 27, 2026 12:07
@thymikee

Copy link
Copy Markdown
Member Author

Fixed in 98d4f1a1ff. You are right, and it is the same class a fourth time — this time hiding in the bound itself.

Reproduced, and measured, because the size matters. My first attempt at 200 matched lines survived, which nearly had me report it as not reproducible:

n=200    bytes=11890    -> survived  exit=0
n=2000   bytes=120890   -> ABORTED   exit=141
n=20000  bytes=1228890  -> ABORTED   exit=141

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 head, exits normally. While there, the bound now discloses what it dropped rather than presenting 40 lines as all there was:

… 1960 more DEBUG-1874 lines (full log in the artifact)

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 head revert only because the disclosure line went missing — the SIGPIPE never fired, so it was testing the symptom I happened to add rather than the failure you reported. At 2000 lines the revert fails with the loop actually dying:

mutant: revert the bound to head -> × Error: Command failed: bash run.sh

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: pnpm format:check was red on my first push of this and I did not notice, because I had chained the gates with ; instead of &&. Amended, and re-ran them as one chain.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

You are right, and the paragraphs were pointing at the medium rather than at any one line. 8a846256d4.

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 head killed its producer with SIGPIPE. Each round I guarded the instance and wrote a paragraph explaining why the guard was fine. The guards were not the problem — writing a parser inside YAML inside shell was.

scripts/diagnose-1874-iteration.ts now reads one iteration: verdict, type-all duration, cadence worth keeping. No pipelines, so exit status means what it normally means and there is nothing left to guard. Comments in that workflow step drop 33 lines to 3, none defending a workaround:

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)) ;;
esac

Same pattern and reasoning as scripts/xctest-run-summary.ts, which exists for this exact reason — its header even says "quoting-sensitive string building inside YAML inside shell".

Moving it exposed a bug the previous shape had introduced. My awk rewrite dropped the phase=type-all qualifier, so the duration took the last durationMs= in the log — which in a real iteration is phase=total, a different measurement from the one the 2000 ms threshold is calibrated on. The original grep -o 'phase=type-all durationMs=…' had that qualifier and I lost it in translation. Restored, and now pinned.

Mutation testing drove three gaps out of the test, which is what caught that bug: every fixture had exactly one durationMs= line and one verdict line, so first-versus-last and the missing qualifier were both invisible. Fixtures now log all three phases, cover pair mode (the neighbour test sorts first, so its phases and verdict land in the same log ahead of the measured one), a restarted test with two verdicts, and an unrecognised verdict word. Six defects planted, six caught.

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

Comment thread scripts/diagnose-1874-iteration.ts Fixed
@thymikee

Copy link
Copy Markdown
Member Author

Yes — six exports and seven CLI arguments for a script with exactly one caller. 5cc7cfa0eb.

One exported function and one type now. readIteration returns the verdict, whether to keep evidence, and the lines to write; main does the IO. Three arguments that never varied between calls — threshold, summary path, evidence dir — became constants next to the code that uses them, so the call site is:

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 wait start/wait outcome, and the fixtures do now. Seven planted, seven caught:

unrecognised verdict accepted          -> caught
duration unqualified (takes phase=total) -> caught
first phase not last                   -> caught
evidence only for failures             -> caught
cadence cap silent                     -> caught
cadence cap removed entirely           -> caught
polls counts every cadence line        -> caught

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

@thymikee

Copy link
Copy Markdown
Member Author

[P2] Preserve decimal duration at the 2000 ms evidence boundary. The runner emits one-decimal durationMs values, but this parser captures only \d+; therefore 2000.1 becomes 2000 and is classified healthy, losing the exact slow-pass evidence this lane exists to retain. Parse the complete decimal value and add boundary proof for 2000.0 versus 2000.1, including rendered summary and evidence-file behavior. The script seam and remaining classifications are otherwise sound. Also remove the implementation/test narration whose rationale is already expressed by names, types, output, and tests; keep the workflow's Catches/Evidence/Cost/Kill declaration.

@thymikee

Copy link
Copy Markdown
Member Author

P1: readIteration accepts rc only to print it; verdict and keepEvidence ignore it. In pair mode, the measured test can report passed while its neighbour or runner makes xcodebuild exit nonzero. The loop then increments pass and may discard the log, falsely reporting a green iteration and losing the evidence. Treat nonzero rc as an explicit harness/iteration failure that always preserves evidence, and add a planted-red case for measured-test-passed plus nonzero exit. Current cases all use rc=0. The final hidden-artifact upload change also lacks exact-head dispatch or a focused workflow-contract assertion.

@thymikee
thymikee force-pushed the claude/1874-diagnose-lane-declaration branch 2 times, most recently from 29370e5 to be6622f Compare August 27, 2026 14:18
@thymikee

Copy link
Copy Markdown
Member Author

Both fixed in be6622f6e9, and rebased onto bf26ab14d6.

rc was decorative — you are right. It was printed and nothing read it, so in pair mode a green measured test alongside a failing neighbour or a dead runner counted as a clean pass and discarded the log. A nonzero exit now yields its own verdict, run-failed: never counted as a pass, evidence always kept, and its own column in RESULT. It does not override failed or no-result, which already say more. Planted red with a measured-test-passed-plus-rc=65 case, and every shape in the table now carries an explicit rc.

The hidden-artifact change had no evidence behind it — and writing the contract found nine more. upload-artifact has excluded hidden files by default since v4.4 (this repo pins v4.6.2), and most diagnostics here are written under .tmp:

workflow artifact consequence
macos.yml xcresult bundle uploaded nothing
mutation-affected.yml ×3 reports, shards, envelope uploaded nothing
mutation-weekly.yml ×2 reports, shards uploaded nothing
replays-nightly.yml fuzz output uploaded nothing
xctest-nightly.yml results uploaded nothing
test-app-build-cache.yml fixture tarball if-no-files-found: errornot even silent

All fixed, and test/ci/upload-artifact-hidden-paths.test.ts now holds the class across every workflow and composite action rather than the one instance. Planted red by dropping any single flag.

That last row is worth a look independently of this PR: Test App Build Cache has been failing on main, and its upload is the one step configured to error when it finds nothing. I have not confirmed causation — the run I inspected reported no failing job — but it is the obvious first thing to check.

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 scripts/__tests__ and test/ci.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Split done, and the stale claims are corrected. Head 3b6a162edc, rebased on ea1d6b8c55.

#2091 carries the artifact policy — all ten hidden-path uploads plus upload-artifact-hidden-paths.test.ts, branched from current main and including the #1874 one. This branch no longer touches those files. Sequenced as you asked: #2091 first, then this rebases with nothing to drop.

I withdraw the Test App Build Cache claim. I said the hidden-path bug probably explained a standing main failure. You are right that it does not follow: the last eight runs are green, and the latest published zero artifacts, which is consistent with no rebuild being selected and proves nothing either way. #2091 now says so explicitly instead. I should not have offered a causal story I had not checked.

Comments cut to the #2087 rule. The reader's narration is gone; what it was explaining is in names and types now — MeasuredVerdict separates xcodebuild's three words from the two the loop derives, exitContradictsMeasured names the rc rule, and each shape's name says what the case is. Two constants remain, citing measured CI numbers that cannot be encoded. Test file: zero // comments, two short doc comments on fixture builders.

Body updated. The "confined to a dispatch-only workflow" line was written before the reader moved into scripts/, and was false. It now says where the behaviour lives and what exercises it, and records the two dispatches.

One more thing that came out of this pass, unprompted: the entry-point test spawned nine --experimental-strip-types subprocesses, one per shape. That made it slow and contention-sensitive in a lane full of subprocess tests, and it was re-covering what the direct table already asserts. One representative invocation now proves the wiring — 3.7 s to 0.07 s. While checking that, I saw fallow-fixture-policy time out at 5 s in two of four full-lane runs; unrelated to this PR, and it passes on its own.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Not ready yet; two blockers remain on the current head.

  • P1 — the retained diagnostic logs are still not uploaded. .github/workflows/1874-diagnose.yml writes evidence to hidden .tmp/stall-evidence-*.log, but its actions/upload-artifact@v4 step omits include-hidden-files: true. The nearby comment says it uses the same input as the shared diagnostics action, but .github/actions/upload-agent-device-artifacts/action.yml explicitly sets that input. Add it here (or route through the owning shared action) and prove the path contract; otherwise the evidence this PR exists to preserve is silently discarded.
  • P1 — the rc fix is not proven through the shipped entrypoint. The only spawned-script test selects a slow-pass rc=0 case. The rc=65 -> run-failed assertion calls readIteration directly, so dropping or miswiring the CLI $rc argument would restore the original false pass/evidence loss while the suite stays green. Spawn the script with rc=65 and assert run-failed, summary rc=65, and evidence creation; observe it red against the pre-fix behavior.

The parsing/verdict model and exact-head CI are otherwise sound. Remove the new private implementation/history comments under current main's comment rule; the workflow's Catches/Evidence/Cost/Kill declaration is established lane documentation and can remain.

@thymikee

Copy link
Copy Markdown
Member Author

Both blockers fixed in 50f0da7b78.

The orphaned comment was my mistake, and worse than it looked. When I split the artifact policy out to #2091, my revert removed the include-hidden-files: true line but left the prose describing it — so the file claimed a flag it did not set. Comment gone.

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 rc gap was real and the more serious of the two. The assertion went through readIteration only, so dropping or miswiring the CLI argument would have restored the false pass and the lost evidence with the suite green. The entry-point test now runs both cases through the shipped script:

rc printed verdict summary evidence
0 passed durationMs=14146 polls=1 kept (slow pass)
65 run-failed rc=65 kept

Red against both mutations — hardcoding 0 for the argument, and dropping the rule that a nonzero exit outranks the measured verdict:

CLI drops the rc argument                          -> caught
rc no longer outranks a green measured test        -> caught

Comments are down to two: the module's one-line pointer to the workflow that calls it, and the measured CI numbers behind SLOW_PASS_MS. The type doc went — MeasuredVerdict says it — as did both fixture-builder docs. The lane declaration stays, as you noted.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

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.

Integration Tests failed in the live web smoke, on a daemon-shutdown assertion:

AssertionError [ERR_ASSERTION]: expected the daemon process itself to have exited after SIGTERM
  test/integration/smoke-web-platform.test.ts:206

This PR touches four files — 1874-diagnose.yml, scripts/diagnose-1874-iteration.ts, its test, and vitest.config.ts. Nothing it changes is reachable from the daemon, the web platform, or that test.

The mechanism is an ordering assumption. The test SIGTERMs the daemon, then waits with settleManagedBrowserProcesses, which polls only until the managed browser fleet count reaches zero:

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 isProcessAlive(daemonPid) to its own timeout before asserting, the same shape settleManagedBrowserProcesses already has. Small and contained, but it belongs in its own PR against the web smoke rather than in a diagnose-loop change. Happy to write it; say the word.

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

@thymikee

Copy link
Copy Markdown
Member Author

Red again, and again not this PR — but the trace was worth pulling. Filed as #2103.

The iOS smoke failed on wait text "Agent Device Tester" with the usual "No readable snapshot capture completed" hint. The request log says something else entirely:

17:45:07.124  ios_runner_readiness_preflight  snapshot  [error]
17:45:07.124  ios_runner_session_invalidated
17:45:16.872  ios_runner_startup_cleanup_stale_bundles  durationMs=6267
17:45:19.075  ios_runner_startup_launch_xcodebuild
17:45:20.262  snapshot_capture  durationMs=14333  error="request canceled"

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 1874-diagnose.yml, scripts/diagnose-1874-iteration.ts, its test and vitest.config.ts — nothing reachable from the runner.

Running tally of unrelated infrastructure failures hit while landing these two PRs, all re-run-to-green: two fuzz-worker fork deaths (#2053), one Android capture stall, one daemon-SIGTERM ordering assertion in the web smoke (diagnosed above, fix offered), and this. I mention it only because the noise is now larger than either PR.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

The rc verdict/evidence route is now sound and exact-head CI is green, but this is not ready. The workflow still uploads hidden .tmp/stall-evidence-* without include-hidden-files:true; merge #2091 first, then rebase while preserving both that flag and this branch's renamed pattern, and rerun CI. Parse decimal durationMs so 2000.1ms satisfies the stated >2000ms threshold, with a regression. Remove the new lane-history/control-flow narration from the workflow, private script, and mutation config.

@thymikee
thymikee force-pushed the claude/1874-diagnose-lane-declaration branch from 50f0da7 to a35a690 Compare August 27, 2026 18:15
@thymikee

Copy link
Copy Markdown
Member Author

Rebased onto main now that #2091 has landed, and Closes #1874 is back — in both the body and the commit message, since squash-merge here builds the body from commit messages.

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 needs-info tracker for an unidentified input-throttle mechanism. That was right at the time. The second dispatch has since supplied the needs-info, and it says the premise was 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. That is #1105's path. With both filed symptoms resolved — smoke:form-input root-caused and fixed in #2035, the targeted XCTest mitigated with 200 consecutive green loop iterations — there is nothing left that this issue is the tracker for.

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 include-hidden-files on the diagnose workflow when I took my own version of that file. Rather than resolve eleven conflicts into a history whose value was the review trail we have already had here, it is one commit against current main. I verified nothing of main's is reverted — the only removals are the classification code this PR replaces.

Content is unchanged from 50f0da7b78 apart from that: same reader, same tests, same lane declaration. Gates green — format, lint, typecheck, and the reader plus test/ci suites.

@thymikee

Copy link
Copy Markdown
Member Author

The #2091 dependency is now integrated correctly: include-hidden-files:true and the current .tmp/stall-evidence-* pattern coexist, and the repository guard is present. Remaining blockers: parse decimal durationMs so 2000.1ms satisfies the stated >2000ms threshold, with a regression; remove the long lane-history/control-flow narration from the workflow and private script. Exact-head CI has restarted and is still incomplete. Not ready yet.

@thymikee
thymikee force-pushed the claude/1874-diagnose-lane-declaration branch from a35a690 to a5e49c8 Compare August 27, 2026 18:28
@thymikee

Copy link
Copy Markdown
Member Author

Both blockers addressed on a5e49c89bb.

Decimal durationMs. Correct, and it was a real hole: the runner logs the phase with %.1f, so every duration carries a decimal, and (\d+) captured only the integer part. At the boundary that silently discarded evidence — 2000.1 ms parsed as 2000, which is not > 2000, so the slow pass was dropped instead of kept. Now (\d+(?:\.\d+)?), and the summary prints what was parsed rather than a truncation. The regression is the 2000.1 shape in the table; observed red against the pre-fix regex (expected false to be true on keepEvidence), green after.

Narration. The workflow header is down to the four #1781 fields with the history dropped, and the script's justification comment on SLOW_PASS_MS is gone — the name carries it. Two things that fell out of re-reading it rather than trimming it:

Green locally: format:check, lint, typecheck, check:gate-manifest, check:xctest-selection, and both test files.

@thymikee

Copy link
Copy Markdown
Member Author

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.

@thymikee

Copy link
Copy Markdown
Member Author

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.
@thymikee
thymikee force-pushed the claude/1874-diagnose-lane-declaration branch from a5e49c8 to 41bc14e Compare August 28, 2026 05:55
@thymikee

Copy link
Copy Markdown
Member Author

Rebased onto 2e87347bee, addressed on 41bc14e652.

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 workflow_dispatch input (default unchanged), so #2080's fill route loops through the same harness — the corruption is only reachable on the penalized XCTest channel, which is a loaded CI machine, which is the one thing this lane provides. The criterion no longer describes an intention the YAML could not honor.

On closing #1874 separately: it stays on this PR, and that is deliberate. Its needs-info was the open question of why the input pipeline throttles; the second dispatch answered it and the premise was wrong — the pipeline does not throttle. 17 characters posted in 484 ms, commit observed on the first poll, inside a type-all of 14334 ms; the ~12.6 s is accessibility round-trips before any character is posted. Both filed symptoms are resolved (#2035). If you would still rather split the closure, say so and I will drop the keyword — but I do not want to leave an issue open whose stated question has an answer.

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 "0" before it went green against the shipped file.

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 ${{ }} splices in the run: body to env:, so an input cannot be spliced into the shell text. The iterations splice predates this PR.

Green locally: format:check, lint, typecheck, check:gate-manifest, and both test files.

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 main in run 33098266234wait timed out for text: landscape, wait_capture_stalled, readableCaptures: 0 — on a commit without this diff. I re-ran it. There is no open issue for that Android orientation stall; happy to file one with both runs if you want it tracked.

@thymikee

Copy link
Copy Markdown
Member Author

Code review is clean at 41bc14e. The parser/contract regressions are non-vacuous and completed exact-head checks are green.

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, run-failed handling, and hidden-artifact upload). Normal PR iOS smoke does not execute 1874-diagnose.yml. Dispatch the workflow from 41bc14e and attach the four-leg outcomes/artifact evidence; exact-head iOS Smoke is also still pending.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 28, 2026
@thymikee
thymikee merged commit 437465f into main Aug 28, 2026
18 checks passed
@thymikee
thymikee deleted the claude/1874-diagnose-lane-declaration branch August 28, 2026 06:33
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-28 06:35 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.

ios: text-entry lanes fail intermittently on unrelated branches (unpinned simulator keyboard state)

2 participants