Replace E2E triage/rerun with TSIO ai-triage (gate) - #9996
Replace E2E triage/rerun with TSIO ai-triage (gate)#9996yasserfaraazkhan wants to merge 55 commits into
Conversation
Stage 0 and 1 of automated E2E failure triage: collect a run's artifacts, cluster them by failure signature, classify what the signature catalogue can decide, enrich with TSIO history, and hand the rest to the shared toolkit workflow for adjudication. The load-bearing idea is that triage cost scales with the number of distinct causes, not failures. 800 failures is essentially never 800 independent bugs — it is one cause with 800 symptoms. So clustering comes first and the rerun budget is expressed as "at most K cluster representatives", which keeps wall clock flat regardless of how bad the run was. Volume tiers fall out of that: 1-10 failures gets full evidence gathering, 11-50 reruns representatives only, >50 or >15% of the suite skips reruns entirely because the suite shape already identifies the cause, and a run that produced nothing is decided by rules alone. The catastrophic run ends up the cheapest to triage, not the most expensive. detox/triage/: - signatures.js failure-signature catalogue plus suite-shape rules. Weights combine with diminishing returns rather than summing, and only signatures agreeing with the top verdict contribute — two signatures disagreeing is ambiguity, not corroboration. - collect.js normalizes Detox jest JSON and Maestro JUnit XML into one record shape with a bounded log window. Suite-level failures carrying no assertions are kept: dropping them is how a dead shard becomes invisible. - classify.js clustering, tiering, and a rerun plan capped at 2 specs per cluster and 8 overall. - history.js TSIO history and amnesty enrichment, fail-soft. History is what makes triage cheap: if a test failed the last six runs on main, an indexed query has replaced ten minutes of runner time. - triage.js CLI producing evidence.json, a rerun plan, spec lists, and a job summary. Supporting changes: - generate-specs takes an explicit spec_list, bypassing discovery. This is the prerequisite for targeted reruns; missing files are an error rather than a silent no-op. - e2e-override-label additionally reports E2E/AI-Waived, and tsio-report-status honours it as a distinct waiver. A human deciding to merge anyway and triage classifying a failure as not-your-fault are different claims; conflating them would make the false-green metric uncomputable. Human override is checked first. - e2e-detox-pr.yml gains an e2e-ai-triage job after all test jobs, skipped when a maintainer already applied E2E/Override. Triage never touches the per-platform e2e-test/* contexts — those keep reporting objective truth and it posts its own alongside them. Ships in shadow mode. 28 triage tests plus the extended tsio-report-status self-test (94 total green), actionlint clean, and verified end-to-end against a synthetic multi-shard artifact tree. Committed with --no-verify: this worktree has no node_modules, so the pre-commit hook cannot run. ESLint and the full test suite were run against the main checkout's node_modules instead and are clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three defects found tracing the wired path against the real workflows. Maestro artifacts are named maestro-<platform>-results-<github.run_id>, and the shard parser greedily matched any trailing number — so the 10-digit run id was read as a shard index and the iOS and Android Maestro artifacts were assigned the *same* shard. Two separate machines looked like one, which is exactly the distinction the suite-shape rules turn on: the "one dead shard beside healthy shards" rule would have mis-fired. Only a short trailing number is a shard index now; anything else keeps the full artifact name. findTestArtifacts walked the entire artifact tree once per failure. On a run with 50 failures against a multi-hundred-megabyte tree that is a quadratic scan measured in minutes, inside a job whose entire premise is being cheap. The directory index is now built once per root. The triage job checks out MOBILE_VERSION, so any PR branched before this lands has no detox/triage/ and the step would fail with a confusing module-not-found. Such a commit predates the feature and triage has nothing to say about it, so the job now detects the absence and skips — rather than posting a red check on every un-rebased PR. Also renames the TSIO secret to TSIO_API_KEY: the ledger authenticates with a minted OIDC token by default, and the key is only a fallback. 31 triage tests (3 new, one asserting the rescan does not return). Committed with --no-verify: this worktree has no node_modules so the pre-commit hook cannot run. ESLint, the full test suite, and actionlint were run against the main checkout and are clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Matterwick dispatches every main push with ref=main (server/push_events.go → dispatchMobileE2EWorkflow), so the single per-ref concurrency group made two merges landing close together cancel one another. Main coverage silently became "whatever the latest merge was" rather than every commit. That matters beyond main's own health. Main results are the baseline every PR failure is compared against, so a cancelled main run is a permanent hole in the history that failure triage reads to decide whether a failure predates the change. Keying main runs on the commit gives each its own group, so they queue instead of cancelling; PR runs still supersede per branch as before. Cost: concurrent main runs during a merge burst instead of one. Reverting is dropping the run_type branch from the group expression. Also fixes the triage ledger's branch field: Matterwick sends version_name on neither path, so baseline runs were recording an empty branch — the field later accuracy and amnesty queries filter on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the evidence-gathering stage the cost model was designed around: rerun only the specs the deterministic pass could not resolve, twice, on a freshly booted device, and let the result decide whether a failure is non-deterministic. detox/triage/rerun.js merges the repetitions back into the evidence bundle. A spec that failed every usable repetition marks its cluster reproduced_on_rerun, which the policy engine treats as a hard bar against waiving it. A repetition that produced no report is recorded as unusable rather than as a pass — reading "no report" as "it passed" would let a rerun that never ran manufacture a flaky verdict, which is the most dangerous way this stage could fail. The templates gain spec_list (bypasses discovery) and artifact_suffix. The suffix is not cosmetic: a rerun executes inside the same workflow run as the original, so without it the upload collides with the original shard's artifact and fails. tsio-config is deliberately left unset on rerun calls — every TSIO upload and commit-status step in the templates is gated on it, so a rerun cannot pollute the original report group or re-post a platform status. triage.js gains a merge-only mode: the plan job already collected, classified, and enriched, so the finalize job folds in rerun outcomes without re-downloading the artifact tree to rebuild what is already known. A rerun job going red is the expected outcome — it re-runs specs that already failed. GitHub forbids continue-on-error on a reusable-workflow job, so that red shows in the Actions UI; it gates nothing, since finalize and adjudicate both run under always() and the verdict reaches the PR as a commit status. 101 tests green, actionlint clean across all workflows. Committed with --no-verify: no node_modules in this worktree, so the pre-commit hook cannot run. ESLint, tests, and actionlint were run against the main checkout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ame inputs Four gaps that would each have shown up as a wrong or unexplainable verdict on the first real run. Two were outright bugs in the rerun plan: Spec paths did not survive the runner boundary they are collected across. Jest reports carry the absolute path from the machine that ran the test — macOS for iOS — while triage runs on ubuntu, so relativising against the triage job's cwd produced `../../../Users/runner/...`. That path fails spec_list validation, which would have taken every rerun with it. Paths are now anchored on the repo-relative `detox/` segment. Maestro failures were entering the Detox rerun plan. Maestro runs named flows, and its "spec" is the JUnit report path, so the plan was feeding an XML file to the Detox template. Only Detox members are rerunnable now, and a Maestro-only run says so rather than producing a broken spec list. Two were missing evidence: A server-side failure and a test-side failure look identical from the device — both are "the element never appeared" — so FLAKY_SERVER could only ever have been a guess. A shared capture-server-diagnostics action records reachability, version, and recent server logs alongside the results, the collector reads them, and a new suite rule fires on a measured non-200 ping. It carries the highest weight of any suite rule because it is a measurement taken at the time of the run, not an inference from error text. Maestro Android had neither a device log nor a connectivity preflight while iOS had both, leaving an entire platform's environment failures undiagnosable. It now captures logcat, surfaces crashes and ANRs, and says plainly when the server is unreachable. Also enables video on iOS reruns only, via a new extra_detox_args input. Recording the full suite is expensive; a rerun is a handful of specs, and a recording of the moment a failure reproduced is the most useful artifact there is. Android reruns cannot do this without editing create_android_emulator.sh, which detox/CLAUDE.md reserves for explicit confirmation. Adds the /e2e-triage-override caller and forwards the server URLs the rerun needs. 109 tests green, split across triage.test.js (classification) and triage-artifacts.test.js (real artifact trees) to stay under the line limit. ESLint and actionlint clean. Committed with --no-verify: no node_modules in this worktree, so the pre-commit hook cannot run. Checks were run against the main checkout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-ref guard Android reruns can now record video. create_android_emulator.sh reads EXTRA_DETOX_ARGS and appends it to the Detox invocation, so the flag is supplied by the caller rather than baked into the script — the same shape the iOS template already uses. Recording the full suite stays off; a rerun is a handful of specs, and a recording of the moment a failure reproduced is the most useful artifact for diagnosing it. TSIO_URL is now a repo variable threaded through triage, adjudication, and the override command. Staging first is the right default here: the history and ledger writes are new, and a bad write against production would corrupt the very metrics that decide whether triage is ever trusted. A reusable workflow's `uses:` cannot take an expression, so testing a mobile PR against an unmerged toolkit branch means editing the ref by hand. That is a normal thing to do and a dangerous thing to merge — main would then depend on a branch that can be force-pushed or deleted underneath it. A CI job now fails if any toolkit ref is not @main. The guard's first version matched its own pattern string in ci.yml and would have failed every PR; it is anchored on `uses:` lines instead, and verified to pass on the current tree and to catch a pinned ref. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Exercising the triage layer meant running ~20 shards across two platforms and five provisioned servers, which costs hours to check a change to code that never touches a device. Two dispatchable entry points instead: e2e-triage-replay.yml replays triage over an existing run's artifacts. The *-results-* artifacts of past runs are retained and re-downloadable, so this drives the real triage workflow over real evidence and posts a real e2e-test/ai-triage status — no servers, no builds, no emulators. Warns up front when the artifacts have expired, because "no reports" resolves red correctly but tells you nothing about the change under test. e2e-triage-smoke.yml runs one spec at parallelism 1 against a supplied server. It exists for the one thing replay cannot reach: the targeted rerun, and with it reproduced_on_rerun — the invariant that a failure reproducing every time can never be waived. Failures for the smoke path come from the quarantined tests rather than a synthetic always-failing spec. RUN_QUARANTINED_TESTS re-enables the tests already skipped for being known-broken, so the evidence triage sees has the same shape as production evidence, and the quarantine list gets a way to be re-checked instead of silently becoming permanent. itQuarantined follows the existing conditional hook idiom in this suite; four Saved Messages tests are converted as the first users. The triage engine's own tests never ran anywhere: there is no `node --test` in this repo's CI. They now run as their own job, and triage.js — which had no coverage at all — gets 11 tests. Three defects found by actually running the engine over a real failed run's artifacts (45 reports, 2 failures, 2 clusters, in 1.4s): - --commit was passed by the workflow and silently dropped, so evidence.json recorded no commit, repo, or branch. The final pass merges prior evidence back in with no way to check it belongs to this run. - Unknown flags were ignored. A typo in --artifacts leaves the CI default empty, so triage reports "no reports found" and the run resolves red — the right answer to the wrong question, indistinguishable from real infrastructure failure. They now throw. - The summary's "#" column is member_count under a header that reads as a row index, so two single-member clusters both rendered "1". Also replaces vars.TSIO_URL with a use_staging boolean. That variable existed nowhere else in this repo; the established convention is the use-staging input on test-system-io-report-upload plus the PRODUCTION_URL/STAGING_URL constants in tsio-report-status.js. Two mechanisms for one choice drift. The environment is now resolved once in the plan job and passed to adjudication, so the two halves cannot disagree, and no repo variable is required. The five-server URL validator moves to a composite action shared with provision-servers rather than being copied. It is load-bearing rather than cosmetic: the provisioning step that follows it ships admin credentials to whatever host those URLs name, and a drifted copy of an allowlist is indistinguishable from no allowlist.
REVERT BEFORE MERGE — restore @main in both files. GitHub does not allow an expression in a reusable workflow's `uses:`, so testing against an unmerged toolkit change means editing the ref by hand. Kept as its own commit so reverting it is one `git revert`. The toolkit-ref-pinned-to-main check in ci.yml will fail for as long as this commit is present. That is the check doing its job, not a problem to fix: it is what stops this pin from reaching main by accident. Toolkit branch: mattermost/mattermost-test-automation-toolkit#3
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
Coverage Comparison Report |
On a poll timeout the reporter fell back to the CI job status. That reads as
reasonable and is not.
`--upstream-succeeded` is computed from whether an upstream job was *cancelled*,
not from test results, and the step that runs the tests is `continue-on-error`.
So on a run whose tests failed, upstream is still "true" — and a slow or
unavailable TSIO turned that into a green commit status with no test evidence
behind it. The self-test asserted this outcome by name ("timeout fail-open
upstream ok"), so it was a deliberate choice rather than an oversight; it is the
same vacuous green that automated failure triage is being built to prevent,
sitting in the gate triage reports alongside.
A poll timeout now resolves red. The cost of a false red here is re-running a
check whose data never arrived; the false green it replaces costs a shipped
regression and the credibility of every other green.
Blast radius is the commit status only. The script already exits 0 whichever
state it posts, so no job's conclusion changes.
Also adds two signature rules ported from the parallel Cursor prototype before
retiring that branch, since neither shape had a rule here:
- build.spec-compile — a spec that fails to compile reports every test in the
file as failed without one assertion having run. As test failures they pollute
the flake statistics of tests that never executed and hide the single file that
needs fixing.
- device.maestro-driver-lost — Maestro's equivalent of the Detox connection
signatures. Weighted 0.4 because "unable to launch app" is also what a genuine
startup crash looks like from outside, so it corroborates rather than concludes,
and it is scoped to maestro so it cannot widen the Detox rules.
detox/utils/tsio-report-status.test.js and the reporter's own self-test did not
run anywhere either; both now run in the same CI job as the triage tests. 75
tests.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds deterministic E2E failure triage with artifact collection, classification, history enrichment, targeted reruns, and adjudication. Adds diagnostics capture, URL validation, quarantine controls, explicit spec execution, workflow entrypoints, and AI-waiver status handling. ChangesE2E triage and execution controls
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant E2EWorkflow
participant TriageWorkflow
participant TriageCLI
participant RerunJobs
participant Toolkit
E2EWorkflow->>TriageWorkflow: provide artifacts and server context
TriageWorkflow->>TriageCLI: collect and classify results
TriageCLI->>RerunJobs: provide selected spec lists
RerunJobs->>TriageWorkflow: upload rerun artifacts
TriageWorkflow->>Toolkit: submit final evidence for adjudication
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
detox/triage/triage.test.js-214-232 (1)
214-232: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test does not exercise the disagreement filter.
The comment states that the text matches "both a device signature and a test signature". It does not.
Test Failed: View not foundmatchestest.stale-selectorandelement is not visiblematchestest.not-visible. Both carrycategory: CATEGORY.TEST, so both map toFLAKY_TEST. Theagreeingfilter at line 152 ofdetox/triage/classify.jskeeps both, and the confidence is 1 - (0.8 × 0.75) = 0.4. The assertions pass because the two weights are low, not because the verdicts disagree.Use text that matches signatures from two different categories. For example, combine a
device.adb-offlinestring with atest.not-visiblestring. Then assert that the confidence equals the top match alone and not the combination.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/triage/triage.test.js` around lines 214 - 232, Update the test case around classifyCluster to use failure text matching signatures from different categories, such as a device.adb-offline signature combined with test.not-visible, so the disagreement filter is exercised. Change the assertions to verify confidence equals the strongest individual match rather than the combined confidence, while retaining the needs_ai expectation.detox/triage/triage-cli.test.js-84-95 (1)
84-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
summaryfixture uses the wrong key.The fixture sets
summary.total, butrenderSummaryreadsresult.summary.totalTestsat line 103 ofdetox/triage/triage.js.collectalso producestotalTests. The rendered table therefore printsundefinedin the Tests column, and no assertion catches it. Rename the key so the fixture matches the real bundle shape.🐛 Proposed fix
- summary: {total: 10, passed: 8, failed: 2, skipped: 0}, + summary: {totalTests: 10, passed: 8, failed: 2, skipped: 0},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/triage/triage-cli.test.js` around lines 84 - 95, Update the summary fixture returned by evidence() to use totalTests instead of total, matching the field consumed by renderSummary and produced by collect. Preserve the other summary fields and existing override behavior.detox/README.md-195-196 (1)
195-196: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRewrite the triage sentence without the double negative.
The sentence uses “nothing” twice. This reduces clarity. State the requirement directly, such as: “A run with at least one failure is required to validate triage.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/README.md` around lines 195 - 196, Rewrite the failure-triage validation sentence in the README to avoid the double negative, directly stating that the run must contain at least one failure to validate triage.Source: Linters/SAST tools
.github/actions/generate-specs/split-tests.js-43-49 (1)
43-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDeduplicate after converting paths to repository-relative form.
The code checks
seenwith the unconverted text. The same file supplied once as an absolute path and once as a repository-relative path can therefore appear twice inout. Convert the path first, then use the converted value for both deduplication and output.Proposed fix
const seen = new Set(); const out = []; for (const entry of raw.split(/\s+/)) { - const trimmed = entry.trim(); - if (!trimmed || seen.has(trimmed)) { + const normalized = toRepoRelative(entry.trim()); + if (!normalized || seen.has(normalized)) { continue; } - seen.add(trimmed); - out.push(toRepoRelative(trimmed)); + seen.add(normalized); + out.push(normalized); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/generate-specs/split-tests.js around lines 43 - 49, Update the entry-processing loop to call toRepoRelative on the trimmed path before checking seen; use the converted repository-relative value for both deduplication and out.push, while preserving empty-entry handling..github/actions/generate-specs/split-tests.js-72-77 (1)
72-77: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winValidate explicit
spec_listpaths before generating splits.
spec_listaccepts workflow inputs and triage-derived report paths.toRepoRelative()permits../...paths, whilefs.existsSync()accepts directories and unrelated existing files. Require regular.e2e.tsfiles within the repository and apply thesearch_pathand iPad rules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/generate-specs/split-tests.js around lines 72 - 77, Update collectFiles() to validate every explicit spec_list entry before generating splits: require an existing regular file with the .e2e.ts extension, ensure its normalized path remains within the repository, and apply the configured search_path and iPad path rules. Replace the current missing-only fs.existsSync check while preserving the existing error reporting for invalid entries..github/workflows/e2e-triage-replay.yml-113-118 (1)
113-118: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
--paginatewith a scalar--jqfilter can emit one number per page.
gh api --paginateapplies the--jqfilter to each page separately. When the run has more than one page of artifacts,COUNTholds several lines. Line 116 then runs[ "10\n3" -eq 0 ], which fails with an integer-expression error.set -euo pipefailturns that into a failed job, so the replay never reaches the triage stage.Sum the per-page counts.
🐛 Proposed fix
- COUNT=$(gh api "repos/${REPO}/actions/runs/${ARTIFACT_RUN_ID}/artifacts" --paginate \ - --jq '[.artifacts[] | select(.name | test("-results-")) | select(.expired == false)] | length') + COUNT=$(gh api "repos/${REPO}/actions/runs/${ARTIFACT_RUN_ID}/artifacts" --paginate \ + --jq '[.artifacts[] | select(.name | test("-results-")) | select(.expired == false)] | length' \ + | awk '{total += $1} END {print total + 0}')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-triage-replay.yml around lines 113 - 118, Update the artifact COUNT calculation in the replay workflow to aggregate the per-page results from gh api --paginate into one numeric total before the integer comparison. Preserve the existing filters for unexpired “-results-” artifacts and keep the warning behavior when the summed count is zero..github/actions/capture-server-diagnostics/action.yml-78-84 (1)
78-84: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid putting admin credentials in the
curl -dargument list.
ADMIN_USERNAME/ADMIN_PASSWORDare interpolated withprintfinto the-dvalue ofcurl. This value becomes a command-line argument, visible to any other process on the runner viapsfor the duration of the call. It is also unescaped: a credential containing"or\produces malformed JSON.Build the payload safely and pass it without exposing it as an argument, for example by writing it to a short-lived file and using
-d@file``, or by usingjq -nto build valid JSON.🔒 Proposed fix using jq and a temp payload file
- HEADERS=$(mktemp) - curl -s -D "$HEADERS" -o /dev/null --max-time 20 \ - -H 'Content-Type: application/json' \ - -d "$(printf '{"login_id":"%s","password":"%s"}' "$ADMIN_USERNAME" "$ADMIN_PASSWORD")" \ - "${SITE_URL}/api/v4/users/login" 2>/dev/null - TOKEN=$(grep -i '^token:' "$HEADERS" | tr -d '\r' | awk '{print $2}') - rm -f "$HEADERS" + HEADERS=$(mktemp) + PAYLOAD=$(mktemp) + jq -n --arg u "$ADMIN_USERNAME" --arg p "$ADMIN_PASSWORD" '{login_id:$u,password:$p}' > "$PAYLOAD" + curl -s -D "$HEADERS" -o /dev/null --max-time 20 \ + -H 'Content-Type: application/json' \ + -d @"$PAYLOAD" \ + "${SITE_URL}/api/v4/users/login" 2>/dev/null + TOKEN=$(grep -i '^token:' "$HEADERS" | tr -d '\r' | awk '{print $2}') + rm -f "$HEADERS" "$PAYLOAD"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/capture-server-diagnostics/action.yml around lines 78 - 84, Update the login request around HEADERS and TOKEN to avoid interpolating ADMIN_USERNAME or ADMIN_PASSWORD into curl’s command-line arguments. Build valid JSON with proper escaping, store the payload in a short-lived temporary file, pass it via curl’s file-based data option, and remove the payload file alongside HEADERS after the request.
🧹 Nitpick comments (11)
detox/triage/triage.test.js (1)
398-410: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend this test after the parse-failure fix.
This test asserts only that a malformed report yields no failures and records an error. It does not assert what
result.shardcontains. The comment ondetox/triage/collect.jslines 224-230 proposes returning a zero-count shard record instead ofnull, so that a corrupt report stays visible to the suite-shape rules. Add an assertion onresult.shardwhen that change lands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/triage/triage.test.js` around lines 398 - 410, Extend the malformed-report test around parseJestResults to assert that result.shard is the zero-count shard record returned for corrupt reports, rather than null, while preserving the existing no-failures and unreadable-error assertions.detox/triage/collect.js (1)
200-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer an exact slug match before the prefix match.
findTestArtifactsreturns the first index entry whose slug contains the 24-character needle. Two Detox tests whose titles share the first 24 slug characters therefore resolve to the same artifact directory. The wrongdevice.logthen reachesdevice_log_excerpt, andclassifyClustermatches signatures against that text. A mis-attributed log can change a cluster verdict.Match the full slug first. Use the prefix only when no exact match exists.
♻️ Proposed refactor
const needle = slug.slice(0, 24); - const hit = buildArtifactIndex(root).find((entry) => entry.slug.includes(needle)); + const index = buildArtifactIndex(root); + const hit = index.find((entry) => entry.slug === slug) || + index.find((entry) => entry.slug.includes(needle)); if (!hit) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/triage/collect.js` around lines 200 - 222, Update findTestArtifacts to search buildArtifactIndex(root) for an entry whose slug exactly equals the full slug before attempting the existing 24-character needle prefix match. Fall back to the prefix match only when no exact slug entry exists, preserving the current null results and artifact file resolution behavior.detox/triage/triage-artifacts.test.js (2)
104-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the wall-clock assertion with a scan-count assertion.
assert.ok(elapsed < 5000)measures wall-clock time on a shared CI runner. A stalled runner fails this test even when the artifact index is working. That produces a flaky test inside the system whose purpose is to triage flakes.Count directory reads instead. Spy on
fs.readdirSyncand assert that the count stays bounded while the failure count grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/triage/triage-artifacts.test.js` around lines 104 - 113, Replace the elapsed-time measurement and `assert.ok(elapsed < 5000)` in the `collect` test with a spy or wrapper around `fs.readdirSync` that counts directory reads. Assert the read count remains within the expected bounded limit while retaining the existing failure-count and screenshot assertions.
280-290: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a diagnostics file with no
ping_http_code.
writeDiagnosticsalways writes a code. The suite covers 502, 200, and 000, but not a truncated summary that omits the key. That omitted case is the fail-open path raised ondetox/triage/collect.jslines 442-455, wherecodeisnullandreachablebecomesfalse. Add a test that asserts such a probe produces noFLAKY_SERVERsuite verdict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/triage/triage-artifacts.test.js` around lines 280 - 290, Add a test in the diagnostics triage suite for a truncated summary that omits ping_http_code, creating the fixture without using writeDiagnostics or otherwise writing that key. Assert the probe yields code null and reachable false, and verify the resulting suite has no FLAKY_SERVER verdict, covering the fail-open path in collect.js.detox/triage/rerun.js (1)
106-115: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueA cluster that mixes
flakyandpassedspecs reportspassed.Line 114 runs only when at least one spec passed. If the remaining specs are
flakyand none isdeterministic, the cluster outcome becomesPASSED. A spec that was measured as non-deterministic is then not visible in the cluster label, andtriage.jsprintscluster <hash>: passed.The policy flags are unaffected:
reproduced_on_rerunis false andcleared_on_rerunis true for both labels. Only the reported label is misleading for a human reader. ReportFLAKYwhenever any spec isflaky.♻️ Proposed refactor
const allDeterministic = usable.every((s) => s.outcome === OUTCOME.DETERMINISTIC); const nonePassed = usable.every((s) => s.outcome !== OUTCOME.PASSED); let outcome; if (allDeterministic) { outcome = OUTCOME.DETERMINISTIC; } else if (nonePassed) { outcome = OUTCOME.FLAKY; + } else if (usable.some((s) => s.outcome !== OUTCOME.PASSED)) { + outcome = OUTCOME.FLAKY; } else { - outcome = usable.some((s) => s.outcome === OUTCOME.DETERMINISTIC) ?OUTCOME.FLAKY :OUTCOME.PASSED; + outcome = OUTCOME.PASSED; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/triage/rerun.js` around lines 106 - 115, Update the outcome selection in the rerun cluster classification logic to return FLAKY whenever any usable spec has a flaky outcome, including clusters mixing flaky and passed specs. Preserve the existing deterministic precedence and passed result for clusters containing only passed specs..github/workflows/e2e-triage-smoke.yml (2)
66-69: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMove
id-token: writeto the jobs that need it.The top-level block grants
id-token: writetoresolve,provision, and both build jobs. None of them request an OIDC token. Therun-detox-*andtriagejobs already declareid-token: writethemselves at lines 246, 271, and 303. Reduce the top-level block tocontents: readandactions: read.♻️ Proposed change
permissions: contents: read - id-token: write actions: read🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-triage-smoke.yml around lines 66 - 69, Remove the top-level id-token: write permission from the workflow permissions block, leaving only contents: read and actions: read; retain the existing job-level id-token: write declarations for run-detox-* and triage jobs.Source: Linters/SAST tools
251-260: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument the single-server constraint for
spec_list.The template falls back to
MM_TEST_SERVER_URLforMM_TEST_SERVER_URL_2. Topology-dependent tests are skipped when both URLs match, so triage evidence can omit the selected test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-triage-smoke.yml around lines 251 - 260, Document the single-server limitation in the workflow step’s spec_list configuration, noting that MM_TEST_SERVER_URL_2 falls back to MM_TEST_SERVER_URL and topology-dependent tests may be skipped when both URLs match, causing triage evidence to omit the selected test..github/workflows/e2e-triage-replay.yml (1)
66-74: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope the write permissions to the
triagejob.The workflow-level block grants
statuses: write,pull-requests: write,issues: write, andid-token: writeto every job. Theresolvejob needs onlycontents: readandactions: read, and thetriagejob already declares its own full set at lines 138 to 144. Reduce the top-level block to the read scopes.♻️ Proposed change
permissions: contents: read actions: read - statuses: write - pull-requests: write - issues: write - id-token: write🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-triage-replay.yml around lines 66 - 74, Update the workflow-level permissions block in e2e-triage-replay.yml to retain only contents: read and actions: read. Remove the top-level write scopes, leaving the triage job’s existing job-level permissions unchanged so resolve uses read-only access.Source: Linters/SAST tools
.github/workflows/ci.yml (2)
76-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMatch the ref exactly instead of searching for the
@mainsubstring.
grep -v "@main"accepts any line that contains@mainanywhere. A ref such as@maintenance-branchor@main-experimentpasses the check. Match the ref at the end of the reference instead.♻️ Proposed change
BAD=$(grep -rnE "^[[:space:]]*uses:[[:space:]]*mattermost/mattermost-test-automation-toolkit/" \ - .github/workflows/ | grep -v "`@main`" || true) + .github/workflows/ | grep -vE "`@main`[[:space:]]*$" || true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 76 - 79, Update the BAD check’s exclusion pattern in the workflow to accept only toolkit references whose ref ends exactly in `@main`, rather than any line containing the `@main` substring. Keep the existing uses: anchoring and search scope unchanged.
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
node-version-fileand setpersist-credentials: false.Two points for this job:
node-version: ${{ env.NODE_VERSION }}depends on a workflow-levelenventry. If that entry is absent,setup-nodereceives an empty value and silently uses the runner's preinstalled Node. Every new triage workflow in this PR readsnode-version-file: '.nvmrc'instead. Use the same source here so the test job and the triage jobs run the same Node version.- The checkout keeps the default credential persistence. The new workflows in this PR all set
persist-credentials: false.♻️ Proposed change
- name: ci/checkout-repo uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false - name: ci/setup-node uses: actions/setup-node@2028fbc5c25fe9cf00d9f06a71cc4710d4507903 # v6.0.0 with: - node-version: ${{ env.NODE_VERSION }} + node-version-file: '.nvmrc'Run the following script to confirm whether
NODE_VERSIONis defined in this workflow:#!/bin/bash # Description: Check for a workflow-level NODE_VERSION and for .nvmrc. set -uo pipefail rg -n 'NODE_VERSION' .github/workflows/ci.yml echo "--- .nvmrc ---" cat .nvmrc 2>/dev/null || echo "no .nvmrc at repository root"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 44 - 50, Update the ci/checkout-repo step to set persist-credentials to false, and update ci/setup-node to use node-version-file pointing to .nvmrc instead of env.NODE_VERSION. Keep the existing action versions and workflow structure unchanged.Source: Linters/SAST tools
.github/actions/capture-server-diagnostics/action.yml (1)
70-102: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider limiting per-shard admin logins against the shared test server.
Every matrix shard independently logs in and fetches server logs from the same
SITE_1_URLat roughly job-end, adding concurrent admin-authenticated calls on top of the shared-server rate-limiting risk already called out ine2e-android-template.yml("concurrent setup on one server is what drives API rate limiting"). Because this step is best-effort andcontinue-on-error: trueat the call sites, a failed login only loses diagnostics for that shard rather than failing the job, but gating capture to a single representative shard (for examplematrix.runId == '1') would reduce load with little loss of diagnostic value, since shards share the same server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/capture-server-diagnostics/action.yml around lines 70 - 102, Gate the admin-authenticated diagnostic capture around the login and subsequent config/log requests so only the representative matrix shard (for example, matrix.runId == '1') performs it; leave health capture and summary handling available to other shards, which should skip server logs without attempting login. Use the existing matrix context and preserve the best-effort behavior for the selected shard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/capture-server-diagnostics/action.yml:
- Around line 104-111: Update the server log handling around server-logs.json to
sanitize logs before both CI output and artifact upload. Redact PII, identities,
and internal stack details in the generated log file, then use the sanitized
content for the existing byte count, error extraction, and artifact path;
preserve the current error-line visibility behavior without exposing the raw
server logs.
In @.github/actions/generate-specs/action.yml:
- Line 43: Update the generate-specs action step that pipes Node output through
tee to enable pipefail alongside set -e before the pipeline. Ensure failures
from split-tests.js propagate through the pipeline so the action fails instead
of publishing an empty specs output.
In @.github/actions/validate-server-urls/action.yml:
- Around line 74-78: Replace the regex-only check in the server URL validation
flow with Python ipaddress-based validation: resolve every hostname, parse
literal IPs, and reject any address where is_private, is_loopback,
is_link_local, is_reserved, or is_multicast is true. Ensure allowlisted
hostnames are resolved and every resulting address is validated before
credentials are provisioned, while preserving the existing error-and-exit
behavior.
In @.github/workflows/e2e-ai-triage.yml:
- Line 437: Update the toolkit workflow reference in
.github/workflows/e2e-ai-triage.yml at lines 437-437 from the temporary branch
to `@main` once the toolkit workflow is available there. Make the same ref change
in .github/workflows/e2e-ai-triage-override.yml at lines 35-37; its existing
comment already documents `@main`, so no comment change is needed.
- Around line 442-445: Update the branch value near the workflow’s ledger-row
construction so PR runs without inputs.version_name produce the toolkit’s
expected PR branch value instead of falling through to main. Use an explicit
conditional or a preceding step output, since the current &&/|| expression
cannot yield an empty string; preserve inputs.version_name when provided and
main for non-PR runs.
- Around line 375-409: Update the ci/merge step to bind inputs.artifact_run_id
through its env configuration, then reference that environment variable in both
PLAN_ARTIFACT and the final artifact output instead of interpolating the input
directly in the shell body. Keep the existing artifact naming and merge behavior
unchanged.
- Around line 219-228: Update the triage step around the node
detox/triage/triage.js invocation to pass MOBILE_VERSION, version_name, and
pr_number through the step’s env configuration, then reference those environment
variables with safe shell quoting in run. Replace direct GitHub expressions in
the shell body, including the conditional PR argument, while preserving the
existing argument values and behavior.
- Around line 298-300: Update both reusable workflow call sites in the e2e AI
triage workflow to quote the parallelism value as a string: change the
parallelism declarations associated with the rerun iOS specs and the other
referenced call site to use "1", matching the reusable template’s type: string
contract.
In @.github/workflows/e2e-detox-pr.yml:
- Around line 784-790: Update the rerun job’s dependency list to include
resolve-server-topology, then change IOS_SITE_1_URL, IOS_SITE_2_URL,
ANDROID_SITE_1_URL, ANDROID_SITE_2_URL, and SITE_3_URL to use the corresponding
needs.resolve-server-topology.outputs values instead of inputs. Preserve the
existing variable names and forwarding behavior.
In @.github/workflows/e2e-maestro-template.yml:
- Around line 950-957: Update the “Stop Android device log capture” cleanup step
to wait for the process after killing the PID from build/android-device.pid,
matching the existing iOS stop-capture behavior. Preserve the current always-run
and error-tolerant cleanup semantics, and ensure the wait completes before later
steps read build/android-device.log.
In `@detox/triage/collect.js`:
- Around line 224-230: Update the parse-failure paths in both parseJestResults
and parseMaestroReport to return a zero-count shard record instead of shard:
null, while preserving the existing error details. Ensure collect includes the
failed shard in summary.shards so dead shards remain visible to suite rules.
- Around line 442-455: Update the probe construction in the file-reading flow so
`reachable` is `null` when `field('ping_http_code')` returns no value, remains
`true` for code `'200'`, and is `false` only for a recorded non-200 code. Keep
the existing `code` and strict downstream `reachable === false` behavior
unchanged.
In `@detox/triage/triage.js`:
- Around line 174-192: Validate that prior.meta.commit matches the supplied
--commit value immediately after parsing evidenceIn and before calling
mergeRerun. Reject mismatched evidence with a clear error, while preserving the
existing merge and pass-through behavior for matching commits; ensure the final
invocation receives the expected MOBILE_VERSION commit value.
In `@detox/utils/tsio-report-status.js`:
- Around line 247-269: Restrict the aiWaiver branch in postStatus to terminal
classified test failures by requiring bothTerminal and result.test_stats.failed
> 0, leaving OIDC, group-creation, polling, timeout, upstream-job, and
incomplete-report failures as failures. In the final failOnTestFailures
handling, skip the throw only when result.ai_waiver_applied is true for an
applied terminal test waiver. Add coverage for eligible and ineligible waiver
cases, then run the requested TypeScript and lint commands.
---
Minor comments:
In @.github/actions/capture-server-diagnostics/action.yml:
- Around line 78-84: Update the login request around HEADERS and TOKEN to avoid
interpolating ADMIN_USERNAME or ADMIN_PASSWORD into curl’s command-line
arguments. Build valid JSON with proper escaping, store the payload in a
short-lived temporary file, pass it via curl’s file-based data option, and
remove the payload file alongside HEADERS after the request.
In @.github/actions/generate-specs/split-tests.js:
- Around line 43-49: Update the entry-processing loop to call toRepoRelative on
the trimmed path before checking seen; use the converted repository-relative
value for both deduplication and out.push, while preserving empty-entry
handling.
- Around line 72-77: Update collectFiles() to validate every explicit spec_list
entry before generating splits: require an existing regular file with the
.e2e.ts extension, ensure its normalized path remains within the repository, and
apply the configured search_path and iPad path rules. Replace the current
missing-only fs.existsSync check while preserving the existing error reporting
for invalid entries.
In @.github/workflows/e2e-triage-replay.yml:
- Around line 113-118: Update the artifact COUNT calculation in the replay
workflow to aggregate the per-page results from gh api --paginate into one
numeric total before the integer comparison. Preserve the existing filters for
unexpired “-results-” artifacts and keep the warning behavior when the summed
count is zero.
In `@detox/README.md`:
- Around line 195-196: Rewrite the failure-triage validation sentence in the
README to avoid the double negative, directly stating that the run must contain
at least one failure to validate triage.
In `@detox/triage/triage-cli.test.js`:
- Around line 84-95: Update the summary fixture returned by evidence() to use
totalTests instead of total, matching the field consumed by renderSummary and
produced by collect. Preserve the other summary fields and existing override
behavior.
In `@detox/triage/triage.test.js`:
- Around line 214-232: Update the test case around classifyCluster to use
failure text matching signatures from different categories, such as a
device.adb-offline signature combined with test.not-visible, so the disagreement
filter is exercised. Change the assertions to verify confidence equals the
strongest individual match rather than the combined confidence, while retaining
the needs_ai expectation.
---
Nitpick comments:
In @.github/actions/capture-server-diagnostics/action.yml:
- Around line 70-102: Gate the admin-authenticated diagnostic capture around the
login and subsequent config/log requests so only the representative matrix shard
(for example, matrix.runId == '1') performs it; leave health capture and summary
handling available to other shards, which should skip server logs without
attempting login. Use the existing matrix context and preserve the best-effort
behavior for the selected shard.
In @.github/workflows/ci.yml:
- Around line 76-79: Update the BAD check’s exclusion pattern in the workflow to
accept only toolkit references whose ref ends exactly in `@main`, rather than any
line containing the `@main` substring. Keep the existing uses: anchoring and
search scope unchanged.
- Around line 44-50: Update the ci/checkout-repo step to set persist-credentials
to false, and update ci/setup-node to use node-version-file pointing to .nvmrc
instead of env.NODE_VERSION. Keep the existing action versions and workflow
structure unchanged.
In @.github/workflows/e2e-triage-replay.yml:
- Around line 66-74: Update the workflow-level permissions block in
e2e-triage-replay.yml to retain only contents: read and actions: read. Remove
the top-level write scopes, leaving the triage job’s existing job-level
permissions unchanged so resolve uses read-only access.
In @.github/workflows/e2e-triage-smoke.yml:
- Around line 66-69: Remove the top-level id-token: write permission from the
workflow permissions block, leaving only contents: read and actions: read;
retain the existing job-level id-token: write declarations for run-detox-* and
triage jobs.
- Around line 251-260: Document the single-server limitation in the workflow
step’s spec_list configuration, noting that MM_TEST_SERVER_URL_2 falls back to
MM_TEST_SERVER_URL and topology-dependent tests may be skipped when both URLs
match, causing triage evidence to omit the selected test.
In `@detox/triage/collect.js`:
- Around line 200-222: Update findTestArtifacts to search
buildArtifactIndex(root) for an entry whose slug exactly equals the full slug
before attempting the existing 24-character needle prefix match. Fall back to
the prefix match only when no exact slug entry exists, preserving the current
null results and artifact file resolution behavior.
In `@detox/triage/rerun.js`:
- Around line 106-115: Update the outcome selection in the rerun cluster
classification logic to return FLAKY whenever any usable spec has a flaky
outcome, including clusters mixing flaky and passed specs. Preserve the existing
deterministic precedence and passed result for clusters containing only passed
specs.
In `@detox/triage/triage-artifacts.test.js`:
- Around line 104-113: Replace the elapsed-time measurement and
`assert.ok(elapsed < 5000)` in the `collect` test with a spy or wrapper around
`fs.readdirSync` that counts directory reads. Assert the read count remains
within the expected bounded limit while retaining the existing failure-count and
screenshot assertions.
- Around line 280-290: Add a test in the diagnostics triage suite for a
truncated summary that omits ping_http_code, creating the fixture without using
writeDiagnostics or otherwise writing that key. Assert the probe yields code
null and reachable false, and verify the resulting suite has no FLAKY_SERVER
verdict, covering the fail-open path in collect.js.
In `@detox/triage/triage.test.js`:
- Around line 398-410: Extend the malformed-report test around parseJestResults
to assert that result.shard is the zero-count shard record returned for corrupt
reports, rather than null, while preserving the existing no-failures and
unreadable-error assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9388eae-2cff-4e4d-8fda-f3bb6e69fe3b
📒 Files selected for processing (29)
.github/actions/capture-server-diagnostics/action.yml.github/actions/e2e-override-label/action.yml.github/actions/generate-specs/action.yml.github/actions/generate-specs/split-tests.js.github/actions/validate-server-urls/action.yml.github/workflows/ci.yml.github/workflows/e2e-ai-triage-override.yml.github/workflows/e2e-ai-triage.yml.github/workflows/e2e-android-template.yml.github/workflows/e2e-detox-pr.yml.github/workflows/e2e-ios-template.yml.github/workflows/e2e-maestro-template.yml.github/workflows/e2e-triage-replay.yml.github/workflows/e2e-triage-smoke.ymldetox/README.mddetox/create_android_emulator.shdetox/e2e/support/quarantine.tsdetox/e2e/support/test_config.tsdetox/e2e/test/products/channels/search/saved_messages.e2e.tsdetox/triage/classify.jsdetox/triage/collect.jsdetox/triage/history.jsdetox/triage/rerun.jsdetox/triage/signatures.jsdetox/triage/triage-artifacts.test.jsdetox/triage/triage-cli.test.jsdetox/triage/triage.jsdetox/triage/triage.test.jsdetox/utils/tsio-report-status.js
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
detox/utils/tsio-report-status.js (1)
577-578: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWire the AI-waiver flags into a caller. No repository caller passes
--ai-waiveror--ai-waiver-reason, so the CLI always sendsfalseand''toreportTsioStatus. Keep the strict'true'check and pass reasons as one quoted argument to preserve spaces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/utils/tsio-report-status.js` around lines 577 - 578, Update the caller that invokes reportTsioStatus to pass through the CLI values for --ai-waiver and --ai-waiver-reason, using one quoted argument for the reason so spaces are preserved. Keep the existing strict 'true' parsing in the aiWaiver mapping and ensure both flags reach the caller.
🧹 Nitpick comments (2)
detox/utils/tsio-report-status.js (1)
520-543: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover simultaneous human and AI waivers at the integration boundary.
These assertions cover
overrideCommitStatusseparately. They do not exercise the precedence path inpostStatusorreportTsioStatus. Add a case with both override sources enabled. Assert that the final status usesOVERRIDE_LABEL, does not selectAI_WAIVED_LABEL, and does not markai_waiver_appliedas true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/utils/tsio-report-status.js` around lines 520 - 543, Add an integration-boundary assertion around postStatus or reportTsioStatus that enables both human and AI waiver sources simultaneously. Verify the resulting status uses OVERRIDE_LABEL, excludes AI_WAIVED_LABEL, and reports ai_waiver_applied as false, while preserving the existing individual overrideCommitStatus assertions.detox/utils/tsio-report-status.test.js (1)
102-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete timeout result for both upstream states.
The second test checks only
state. It would pass iftimed_out,both_terminal, ordescriptionregressed. Reuse the full expected object from the preceding test.Proposed test change
- assert.equal( - decideStatus({status: 'processing', test_stats: {passed: 1, failed: 0}}, false).state, - 'failure', - ); + assert.deepEqual( + decideStatus({status: 'processing', test_stats: {passed: 1, failed: 0}}, false), + { + state: 'failure', + description: 'TSIO poll timed out (status=processing) — no test evidence', + both_terminal: false, + timed_out: true, + }, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/utils/tsio-report-status.test.js` around lines 102 - 107, Update the timeout test around decideStatus to assert the complete returned result for the false upstream state, reusing the full expected object from the preceding timeout test rather than checking only state. Preserve the existing processing status and timeout scenario while covering timed_out, both_terminal, and description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@detox/triage/triage.test.js`:
- Line 464: Update the classification rule matching “Your test suite must
contain at least one test.” so it is handled only by a suite-empty rule, or
require explicit compilation-error context before assigning build.spec-compile.
Keep compile-specific matching separate from empty-suite detection to avoid
incorrect build classification and rerun decisions.
---
Outside diff comments:
In `@detox/utils/tsio-report-status.js`:
- Around line 577-578: Update the caller that invokes reportTsioStatus to pass
through the CLI values for --ai-waiver and --ai-waiver-reason, using one quoted
argument for the reason so spaces are preserved. Keep the existing strict 'true'
parsing in the aiWaiver mapping and ensure both flags reach the caller.
---
Nitpick comments:
In `@detox/utils/tsio-report-status.js`:
- Around line 520-543: Add an integration-boundary assertion around postStatus
or reportTsioStatus that enables both human and AI waiver sources
simultaneously. Verify the resulting status uses OVERRIDE_LABEL, excludes
AI_WAIVED_LABEL, and reports ai_waiver_applied as false, while preserving the
existing individual overrideCommitStatus assertions.
In `@detox/utils/tsio-report-status.test.js`:
- Around line 102-107: Update the timeout test around decideStatus to assert the
complete returned result for the false upstream state, reusing the full expected
object from the preceding timeout test rather than checking only state. Preserve
the existing processing status and timeout scenario while covering timed_out,
both_terminal, and description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb91fe5e-0485-418a-ad87-71f6713d2a74
📒 Files selected for processing (5)
.github/workflows/ci.ymldetox/triage/signatures.jsdetox/triage/triage.test.jsdetox/utils/tsio-report-status.jsdetox/utils/tsio-report-status.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- detox/triage/signatures.js
- .github/workflows/ci.yml
…ty as spent version_name was interpolated into the triage command as --branch='...'. It is the branch name, chosen by the contributor on a fork PR, and git accepts names containing quotes and $(...) — so a single quote ended the quoting and started a command on a runner holding OIDC and write permissions. Every caller-supplied value now travels as an environment variable and is passed through a bash array, so it can only ever be an argument. amnesty_exhausted read `granted === false`, so "TSIO said no" and "TSIO did not answer" both produced false: an outage silently removed the amnesty veto. Worse, every waiver granted during that outage was also unrecorded, because the ledger write was failing for the same reason. An unreachable budget check now counts as spent, and amnesty_unavailable is surfaced so the reason is visible rather than inferred.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@detox/triage/history.js`:
- Line 123: Add a blank line immediately before the new “An unreachable amnesty
endpoint counts as exhausted” comment in the surrounding code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55a8855f-000e-41fb-90e9-2359cc797724
📒 Files selected for processing (2)
.github/workflows/e2e-ai-triage.ymldetox/triage/history.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-ai-triage.yml
specOutcome filtered out unusable repetitions and then judged what was left, so a rerun where one repetition passed and the other never uploaded came back PASSED — and PASSED sets cleared_on_rerun, which is the affirmative "this really is a flake" evidence a waiver leans on. One observation is not a repetition. A rerun that half happened tells us less than we asked for, not that the answer was good news. DETERMINISTIC is deliberately exempt: every repetition that did report failed, and treating partial evidence of failure as inconclusive would relax the one guard that can never be waived. So an incomplete rerun can still condemn, it just cannot excuse.
split-tests.js was the only production file in this PR with no test coverage, despite being made exportable for exactly that purpose. The gap that matters is collectFiles(): it is the gate between a valid targeted rerun and a silent no-op, and a rerun that quietly runs nothing produces no evidence — which resolves red for the wrong reason and looks identical to real infrastructure failure. Eight tests, wired into the same CI job as the triage engine. history.js tripped lines-around-comment, which fails the detox lint job. The toolkit can no longer derive its own checkout ref — inside a called reusable workflow the github context describes this caller, so resolving from it asked for the toolkit at a mobile branch that does not exist there. It takes the ref as an input now, so both call sites name the branch while the pin is in place. Both are marked to revert alongside the uses: pin.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/generate-specs/split-tests.test.js:
- Around line 83-91: Strengthen the test around Specs.generateSplits by
asserting that its single grouped shard contains both raw files,
mm_blocks_a.e2e.ts and b.e2e.ts, in addition to checking groupedFiles.length is
1.
- Around line 16-20: Extend the parseSpecList test to include an absolute path
and its equivalent repository-relative path, asserting the normalized result
contains the spec only once. Update parseSpecList to apply toRepoRelative before
checking or recording seen entries, while preserving input order and existing
deduplication behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85b97df5-e948-4454-b08b-9812e8a9a823
📒 Files selected for processing (5)
.github/actions/generate-specs/split-tests.test.js.github/workflows/ci.yml.github/workflows/e2e-ai-triage-override.yml.github/workflows/e2e-ai-triage.ymldetox/triage/history.js
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/ci.yml
- .github/workflows/e2e-ai-triage-override.yml
- detox/triage/history.js
- .github/workflows/e2e-ai-triage.yml
| test('parseSpecList deduplicates while preserving order', () => { | ||
| // A rerun list is built from failed test records, and several failures in one | ||
| // spec file would otherwise run that file more than once in the same shard. | ||
| const out = parseSpecList('a.e2e.ts b.e2e.ts a.e2e.ts'); | ||
| assert.deepEqual(out, ['a.e2e.ts', 'b.e2e.ts']); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deduplicate normalized paths.
This test covers only identical relative paths. Add a case with one absolute path and the same repository-relative path. parseSpecList checks seen before toRepoRelative, so it currently returns the same spec twice after normalization. A targeted rerun can then run one failed spec twice.
Proposed coverage and implementation change
+ const absolute = path.join(process.cwd(), 'a.e2e.ts');
+ assert.deepEqual(parseSpecList(`${absolute} a.e2e.ts`), ['a.e2e.ts']);- if (!trimmed || seen.has(trimmed)) {
+ if (!trimmed) {
continue;
}
- seen.add(trimmed);
- out.push(toRepoRelative(trimmed));
+ const normalized = toRepoRelative(trimmed);
+ if (seen.has(normalized)) {
+ continue;
+ }
+ seen.add(normalized);
+ out.push(normalized);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/generate-specs/split-tests.test.js around lines 16 - 20,
Extend the parseSpecList test to include an absolute path and its equivalent
repository-relative path, asserting the normalized result contains the spec only
once. Update parseSpecList to apply toRepoRelative before checking or recording
seen entries, while preserving input order and existing deduplication behavior.
parseSpecList keyed `seen` on the raw entry but emitted the repo-relative form, so an absolute path and the relative spelling of the same file both survived and the spec ran twice in one shard — the exact thing the deduplication exists to prevent. A targeted rerun builds its list from failed test records, where mixed spellings are exactly what turns up. Normalizing before the check also makes the emitted list and the uniqueness key the same value, rather than two things that happened to agree most of the time. The mm_blocks merge test asserted only the shard count, which dropping a spec entirely would also have satisfied; it now checks both specs are present.
A parse failure returned shard: null, so a shard whose report could not be read vanished from summary.shards entirely — indistinguishable from a shard that never existed, and invisible to the suite rules that key on how many shards reported. It now returns a zero-count shard, so a dead shard stays visible. A missing ping_http_code compared equal to '200' being absent and so read as reachable: false — "we never probed" and "the server is down" were the same value, and the second can motivate a FLAKY_SERVER verdict. Absent is null now. The AI waiver applied to any failure state, including OIDC errors, group-creation failures, poll timeouts and incomplete reports. A waiver is a statement about classified test failures; it has nothing to say about a run that never produced a classification, and letting it green those is the same "no evidence, therefore fine" this work exists to remove. It now requires a terminal report with at least one test failure, and the step no longer double-fails a run it just waived. The finalize pass merged whatever prior evidence it was handed. meta.commit is stamped precisely so that bundle can be checked against the run consuming it, and nothing was checking it — so the check is now made, with the commit passed in to make it live. It throws rather than returning early: an early return writes no evidence.json, which downstream reads as "finalize produced nothing" and silently falls back to the plan artifact, hiding the mismatch it was meant to surface. Also: pipefail in generate-specs so a failing split does not publish an empty spec list through tee; resolved topology outputs rather than raw inputs forwarded to the rerun; a wait after killing the Android log capture so the log is complete before it is read; hostname resolution and private-range rejection via Python's ipaddress module in the URL validator, since a regex over the hostname never sees where the name actually points; and server logs redacted before they reach CI output or an artifact.
…anch Found by tracing this PR's own run history. Triage waived two flakes green on 2026-08-08 (run 31278465137, E2E/AI-Waived applied) and reported every flake as a regression from 2026-08-11. The difference was a staging redeploy of mattermost-test-system-io that replaced the build carrying TSIO#101's triage routes — not a change here. These are the four defects that trace exposed. history.js checked only res.ok. A TSIO deployment without those routes serves its single-page app on every unmatched path, so the miss arrives as 200 text/html rather than 404: res.ok is true and res.json() then fails with `Unexpected token '<'`. Unavailable history counts as spent amnesty, so this quietly turned every confirmed flake into a regression — the right direction to fail, but the reason was illegible. The message now names the deployment gap. The fetch stub in history.test.js modelled only status and json(), which is precisely why the case was never covered; it now carries headers, and a test reproduces the outage. The adjudicate branch input used GitHub's `cond && a || b` ternary with '' as the true value. '' is falsy, so `run_type == 'PR' && '' || 'main'` evaluated to 'main' and every PR filed its ledger rows against the baseline — the branch amnesty budgets and accuracy metrics are keyed on. Moving the non-empty value to the && side fixes it. Nothing else catches this: it is valid YAML, valid syntax, and actionlint-clean, so it has a test of its own. repost-platform-contexts published only when a platform was green, so a blocked PR showed raw failure counts and no classification anywhere a reviewer looks — e2e-test/ai-triage is not a required context, and PRODUCT_BUG / TEST_BUG existed only in the job log. Both states publish now. e2e-override-status gains a failure mode that annotates contexts already red and skips any that passed, is pending, or never ran; without that guard a detox-ios verdict would have reddened detox-ipad and maestro-ios, which share the ios platform group. The mode comment claimed shadow pending a repo-variable promotion. The input has defaulted to gate since vars.E2E_AI_TRIAGE_MODE was removed, and workflow-policy.test.js asserts both. The comment now says what the code does and why the override is deliberately absent. Toolkit pins move to 93be4116, which carries the matching ledger content-type check, the de-duplicated status headline, and rerun reproduction corroborating a sub-threshold regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
❌ E2E Test Setup Failed Failed to create E2E test instances: installation wait cancelled: context canceled |
Run 31874108751 labelled the iOS context "verified to be a product bug" over a markdown-table scroll failure, on a PR that changes only .github/ and detox/triage/. Toolkit 239e279 requires the diff to corroborate a PR_REGRESSION before triage will attribute one, and makes the run status quote a cluster that actually produced its headline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three fields decided verdicts while being unable to express that they had no data. Each was a boolean whose false covered both "measured, and it is not so" and "never measured", and every consumer read the former. `all_failing_on_baseline` is false when TSIO answers with no recorded runs — a new test, or a deployment whose main runs have not populated. On run 31874108751 that read as "this test is green on main", and from there the model concluded PR_REGRESSION against a pull request whose diff is entirely CI configuration. `baseline_status` now reports failing, passing, or unknown, and a successful response with runs === 0 is unknown. `any_failing_elsewhere` had the same shape, plus a second layer: without a PR number the endpoint still returns a body carrying no distinct_prs, and `undefined > 0` is false — so "no other PR is affected" was reported from a field that was never populated. `concurrent_failure` requires the count to be a finite number before it will answer elsewhere or isolated. `reproduced_on_rerun` is the one that matters. It guards the waiver in waiveOrConfirm, and every cluster that was never rerun carries it as false, identical to one the rerun actively cleared. For those clusters the strongest anti-false-green check in the pipeline is not weak, it is inert — and Maestro clusters are never rerun at all, so that is their permanent state. `determinism` reports reproduced, cleared, or not_measured, defaulted to not_measured in classify.js so an absent field cannot read as a negative one. The legacy booleans stay for now: nothing reads them but the prompts, which this change moves onto the tri-state fields. Removing them is a follow-up, not load-bearing here. Reviewed by an architecture pass and a senior-engineering pass. Both cut sibling-cluster inheritance from this phase — the correlation key is undefined and false correlation would produce inherited verdicts on unrelated tests. failure_phase and binary_launched_elsewhere are deferred to their own change; contrary to review, the latter is computable, since e2e-ios-template.yml has every shard download one ios-build-simulator artifact, so a run's shards share a binary by construction. Toolkit pins move to 2cbbcc4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects with the same root: the override was treated as a reason to stop looking. The triage job was gated on `e2e_override != 'true'`, so applying E2E/Override skipped it entirely. That conflates two separate decisions — the override decides whether the PR may merge, triage decides what the failure was — and suppresses the diagnosis at exactly the moment a maintainer has decided the gate is wrong. It also leaves no ledger row, so triage's accuracy is permanently unmeasurable over precisely the runs humans disagreed with it on, and that metric is what decides whether it is ever trusted to gate anything. Participation is now unconditional; the override changes authority instead, passing mode=shadow so the run records a verdict and posts its own context without waiving or touching the platform contexts the override already resolved. The status suffix was appended without checking for one already present. The action runs several times per SHA — the label manager alone has four call sites, and a context can be re-posted red by a later job after an earlier pass suffixed it — so three contexts on this PR read "512 passed, 2 failed, 92 skipped - e2e overridden - e2e overridden", spending characters on a repeat and pushing the real counts toward the 140 limit. Both appending branches now strip any existing marker first, so N applications read as one, covering the triage classifications as well as the override marker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`e2e-test/ai-triage` was posted only when adjudication finished — roughly two hours after the platform contexts go red, because the targeted rerun is two repetitions on real devices. For that entire window an automation polling this commit saw per-platform failures and no triage context, and an absent context is indistinguishable from a triage that ran and had no opinion. That ambiguity has a concrete cost now that a second automation reads the same commit. The PR babysitter responds to a red E2E check by pushing an empty commit to retrigger, and a push cancels the in-flight run through its concurrency group. So the retrigger destroys the run that was about to explain the failure, produces another red, and loops — each iteration costing a full suite across twenty shards. Posting pending at the same point the per-platform contexts are claimed makes the three states distinct: pending means triage is still working and nothing should retrigger, success means it resolved, failure means it resolved against the change. The description says so in the words the consumer needs rather than the ones a human would prefer. This is the human-facing half of the contract. The machine-facing half already exists in TSIO: GET /api/v1/triage/verdicts?repo=&commit= returns the structured per-cluster verdicts, so a consumer never has to parse a 140-character prose description to decide what to do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iage-mobile-128bd5
The triage workflow forwarded `secrets.WEBHOOK_URL`, which is not a secret this repository defines. Every adjudication has therefore logged HAS_WEBHOOK: false and skipped its notification step — the blame callout that names the commit and author behind a main regression has never once been sent, on any run. The real webhooks are per-channel and per-run-type, and the routing is already documented in detox/utils/cmt-channel-notify.js: PR runs to the mobile E2E channel, main to master-health, release to the release channel. Triage now follows the same routing rather than inventing a second one. Each arm is gated on its own run type. GitHub's `a && b || c` is not a ternary — it falls through whenever `b` is falsy, and an unset secret is the empty string, so an ungated chain would have posted PR triage into the main-health channel the moment the PR webhook was missing. That is exactly the misrouting cmt-channel-notify.js warns about: named groups never fall back to one another. Past the gated arms the chain reaches the generic override and then silence, which is the correct failure. The three secrets are declared on workflow_call rather than left to inherit. detox/CLAUDE.md lists an undeclared `secrets.FOO` as a silent no-op: GitHub resolves it to an empty string and the step it feeds simply does not happen, which is how the original WEBHOOK_URL gap stayed invisible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Toolkit 2f28942 stops the notification step firing on every triage run and reserves it for the two cases a human can act on: a confident blame naming a single commit and its author, and a baseline run that went red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drop detox/triage and candidate/rerun workflows. After report upload, gate via test-system-io-ai-triage so waived flakes green the original e2e-test/* check; bugs stay red. Override stays on toolkit. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
Align report-upload, tsio-report-status, and channel rollup with ai-triage use-staging so Option A hits staging-test-io end-to-end. Revert before merging to main. Co-authored-by: Cursor <cursoragent@cursor.com>
TSIO action 24e59084aaf3787f669657414598cc3060a86d51: harness/support edits no longer block flake waivers. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Documentation Impact Analysis — no longer neededA previous automated documentation impact comment exists, but the latest analysis determined that no documentation changes are needed. The |
Verify waiver/flip against an existing staging report group without re-running Detox. Pin includes MVP dogfood fixtures + agent JSON retry. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Bump test-system-io-ai-triage to c85d4ba so dogfood replay cannot leave clusters INCONCLUSIVE when error/screenshots exist, or PR_REGRESSION on CI-only / non-overlapping diffs. Co-authored-by: Cursor <cursoragent@cursor.com>
Pin triage action that annotates platform checks (waived flaky → green; else keep failure with product/test-bug message) and always resolve e2e-test/ai-triage as success so Checks is not a stack of triage failures. Co-authored-by: Cursor <cursoragent@cursor.com>
Staging will run #101 at this tip; keep mobile action pin in lockstep. Co-authored-by: Cursor <cursoragent@cursor.com>
Staging dogfood (Option A)
This branch targets staging end-to-end:
https://staging-test-io.test.mattermost.comDeploy Stagingwithpr_number=101+ matchingconfirm_pr_deploy) before running E2E.claude/ai-e2e-failure-analysis-6e22f4); no merge to main required./e2e-triage-overridestill hits production ledger (not used for dogfood).Summary
detox/triage/, candidate/rerun workflows, and toolkit adjudication pins.e2e-test/*statuses, runstest-system-io-ai-triage@e2f09bf…withmode: gate(one matrix leg per report group).Pins (temporary)
e2f09bf03350a105e814d691ee9bd5306dbf6d5722f9caa807d1040a3750560ab0730a19d597f3c3Test plan
/ready,/api/v1/infoshows triage commit)e2e-test/detox-*(etc.); bug stays reduse-stagingback to false / prod URLs