Add reusable E2E AI failure-triage adjudication workflow - #3
Open
yasserfaraazkhan wants to merge 21 commits into
Open
Add reusable E2E AI failure-triage adjudication workflow#3yasserfaraazkhan wants to merge 21 commits into
yasserfaraazkhan wants to merge 21 commits into
Conversation
Takes a normalized evidence.json from a caller repo, adjudicates only the failure clusters the caller's deterministic rules could not decide, and posts e2e-test/ai-triage. The split is deliberate: clustering needs a repo's spec layout, artifact names, and failure-signature catalogue, while adjudication and policy are repo-agnostic. One JSON file is the whole contract, so any framework that can produce it can use this. The model never decides its own authority. It emits a verdict; the deterministic, unit-tested policy engine decides what that means for the merge button, and the model never touches the status API. Policy rules that make an automated green trustworthy: - Fail closed. No evidence, unparseable output, unknown verdict, low confidence, API error, timeout — all resolve red. - Asymmetric bars: 0.85 to waive, 0.7 to keep red. A false red costs a rerun; a false green ships a bug. - Two independent citations minimum, or the verdict is downgraded to INCONCLUSIVE before policy sees it. - One unwaived cluster keeps the whole run red. - MAIN and RELEASE runs never auto-waive — baseline health has to reflect reality, and it is the comparison every PR verdict is drawn from. - A MAIN_REGRESSION excuses a PR only when the PR does not touch the failing area; overlap means ambiguity, and ambiguity is red. - AI waivers use E2E/AI-Waived, never the human E2E/Override. Conflating them makes the false-green metric uncomputable. Ships in shadow mode: posts a verdict, never waives. Promotion to assist is a repo-variable change, gated on false_greens being zero over a real sample, so rollback is instant. 21 unit tests cover the policy engine and verdict assembly.
End-to-end trace of the PR and main paths found four defects, one of which would have made the check actively harmful. A passing E2E run was posting RED. decideRun treated an empty decision list as "nothing decided → fail closed", but there are three very different reasons the list can be empty and they were collapsed into one: the suite passed (green — there was nothing to triage), no reports were produced (red — nothing can be concluded), or failures exist that nothing explained (red — fail closed). Only the middle two are failures. As written, every green PR would have gained a red e2e-test/ai-triage, which is the fastest way to train everyone to ignore the check. A stale E2E/AI-Waived label stayed applied across pushes. The status reporter honours the label unconditionally, so a waiver granted for one commit would have kept greening every later commit on the branch — including one introducing a real regression. Any run that does not waive now clears it. The ledger write could never have succeeded. It sent a static secret as a bearer, but TSIO accepts only an X-API-Key or an OIDC bearer it verifies against the GitHub Actions issuer. It now mints an OIDC token the same way tsio-report-status.js does, so no shared secret is needed at all. A missing evidence artifact killed the job before any status was posted. The download step had no continue-on-error, so the documented "post red when there is no evidence" path was unreachable and the check would simply never appear — worse than a red, because a required check that never arrives blocks forever. Also: the run-level decision now carries its confidence (statuses read "(?)" without it), the waiver sentence only appears on an actual waiver, and a clean run clears a stale comment instead of posting a new one. 26 policy and assembly tests, including one per defect above.
The policy engine decides whether an E2E failure blocks a merge for every repo that calls the reusable workflow. Without CI a regression in "when is a red allowed to turn green" would reach consumers silently — the exact failure mode the design exists to prevent. Also lints the workflows: the reusable workflow is consumed by SHA/ref from other repos, so a syntax error here breaks their pipelines, not this one's.
The rerun stage is the one experiment that turns "flaky" from an inference into a measurement, so its result has to outrank the model's reading of the error text. A cluster that failed every repetition is deterministic by definition; no confidence score makes it flakiness. decideCluster now takes reproducedOnRerun and rejects every waivable verdict when it is set. Red verdicts are unaffected — only waivers are blocked, and only by evidence, never by interpretation. This is the strongest single guard against a false green in the design: the other guards are thresholds and heuristics, this one is a repeated measurement.
Two gaps that made the design incomplete in opposite directions. Every triage comment advertised `/e2e-triage-override <verdict> <reason>`, and nothing implemented it. Maintainers would have typed it and watched nothing happen. It now records the correction in the ledger and brings the checks into line with the human's decision. Corrections are the only ground truth this system gets — everything else is triage grading its own homework — so the ledger write comes first and its failure is reported loudly rather than swallowed. Correcting away from a waivable verdict also withdraws the E2E/AI-Waived label, because the label is sticky and would otherwise keep greening later commits on the branch. MAIN_REGRESSION told the PR author "not your fault" and told the person who actually broke main nothing at all. Blame closes that: TSIO already knows the last passing and first failing commit, so the suspect range is whatever landed between, which is usually one commit and costs one compare call rather than a bisect. One commit is attribution and the author is named; two to eight are listed as candidates; more than that is not attributed at all. Naming the wrong author would burn the only thing the callout needs, which is being trusted enough to read. 24 new tests (55 total), covering the parse forms people actually type, the label withdrawal, merge-commit exclusion, and the refusal to blame a flake.
The ledger credential was renamed to TSIO_API_KEY when the write moved to a minted OIDC token; the copy-pasteable example still said TSIO_TOKEN, which would have silently resolved to an empty string in every consumer that copied it.
|
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:
📝 WalkthroughWalkthroughThis change adds fail-closed E2E triage policy and automation. It evaluates model verdicts, attributes baseline regressions, updates GitHub and TSIO, supports human overrides, documents operation, and adds CI validation. ChangesE2E triage automation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant E2EWorkflow
participant Claude
participant TriageApply
participant TriagePolicy
participant GitHub
participant TSIO
E2EWorkflow->>Claude: Submit normalized evidence
Claude-->>E2EWorkflow: Return model verdicts
E2EWorkflow->>TriageApply: Pass evidence and model output
TriageApply->>TriagePolicy: Evaluate cluster and run decisions
TriageApply->>GitHub: Update status, label, and PR comment
TriageApply->>TSIO: Record verdict batch
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
.github/workflows/e2e-ai-triage.md (1)
161-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a comma after the condition clause.
Line 161 reads "If the ledger write fails the checks are still updated". Place a comma after "fails" so the condition and the action separate clearly.
🤖 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-ai-triage.md at line 161, Update the sentence near the ledger write discussion to insert a comma after “fails” in “If the ledger write fails, the checks are still updated.”Source: Linters/SAST tools
scripts/triage-override.js (1)
27-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
VERDICTSandWAIVABLEfromtriage-policy.js.These two sets are duplicated from
scripts/triage-policy.js, which exports both. The comment at line 42 records the coupling but does not enforce it. If a verdict class is added to the policy engine and not here, the override command rejects a verdict the engine accepts, and the two waivable sets can diverge silently.
AI_WAIVED_LABELandSTATUS_CONTEXTare also duplicated fromscripts/triage-apply.js, which exports both.♻️ Proposed refactor
-const AI_WAIVED_LABEL = 'E2E/AI-Waived'; -const STATUS_CONTEXT = 'e2e-test/ai-triage'; - -const VERDICTS = new Set([ - 'PR_REGRESSION', - 'MAIN_REGRESSION', - 'FLAKY_TEST', - 'FLAKY_INFRA', - 'FLAKY_SERVER', - 'BUILD_OR_ENV_ERROR', - 'TEST_DEBT', - 'INCONCLUSIVE', -]); - -// Verdicts whose meaning is "not attributable to this change", i.e. the ones that -// justify a green. Mirrors WAIVABLE in triage-policy.js. -const WAIVABLE = new Set(['FLAKY_TEST', 'FLAKY_INFRA', 'FLAKY_SERVER', 'MAIN_REGRESSION']); +const {VERDICTS, WAIVABLE} = require('./triage-policy'); +const {AI_WAIVED_LABEL, STATUS_CONTEXT} = require('./triage-apply');Note:
scripts/triage-apply.jsguards its entry point withrequire.main === module, so importing it has no side effect.🤖 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 `@scripts/triage-override.js` around lines 27 - 43, Import VERDICTS and WAIVABLE from triage-policy.js and remove their local Set definitions in scripts/triage-override.js, preserving all existing validation and waiving behavior. Also import AI_WAIVED_LABEL and STATUS_CONTEXT from triage-apply.js and remove the duplicated local constants, relying on the existing guarded entry point to avoid side effects.scripts/triage-apply.js (1)
47-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a 30-second timeout to all outbound requests.
Apply
signal: AbortSignal.timeout(30000)togh,mintOidcToken, andrecordLedger. Without a timeout, an unresponsive endpoint can block the workflow until the job timeout, including the last-ditch status update.🤖 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 `@scripts/triage-apply.js` around lines 47 - 62, Update the outbound request implementations in gh, mintOidcToken, and recordLedger to pass signal: AbortSignal.timeout(30000) in each fetch request’s options, including the final status-update request, so every request aborts after 30 seconds.
🤖 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/workflows/ci.yml:
- Line 54: Update the actionlint installation command in the workflow to fetch
the installer from a reviewed immutable commit instead of the mutable main
branch, and verify the fixed release archive’s checksum before extracting or
executing it. Preserve the existing actionlint installation behavior while
ensuring both the installer and downloaded archive are integrity-pinned.
In @.github/workflows/e2e-ai-triage-override.yml:
- Around line 34-37: Update the reusable workflow around the sender input and
override execution to query sender’s collaborator permission for target_repo,
then stop before posting the commit status or applying E2E/AI-Waived when the
permission is below write. Use the workflow’s existing authentication and
repository context, and preserve the current behavior for authorized senders.
In @.github/workflows/e2e-ai-triage.md:
- Around line 196-198: Update the documented Node test command in the E2E AI
triage instructions to include scripts/triage-blame.test.js and
scripts/triage-override.test.js alongside the existing policy and apply suites,
so all four tests run.
In @.github/workflows/e2e-ai-triage.yml:
- Around line 285-298: Replace direct GitHub Actions expression interpolation in
all four command sites with step-level env variables and quoted shell-variable
references: in .github/workflows/e2e-ai-triage.yml lines 285-298 pass
target_repo, commit_sha, pr_number, branch, run_type, mode, tsio_url,
evidence_run_id, and diff_overlaps_failure via env; lines 300-314 pass mode,
run_type, gate reason, AI outcome, apply state, and apply description via env;
lines 331-333 pass pr_number, target_repo, commit_sha, and github.server_url via
env. In .github/workflows/e2e-ai-triage-override.yml lines 77-83 pass
target_repo, pr_number, sender, comment_id, tsio_url, and the run URL via env,
following the existing COMMENT_BODY pattern and preserving the commands’
behavior.
- Around line 143-164: Configure the ci/decide-whether-to-adjudicate step to
continue on error, matching the protection on ci/download-evidence, so malformed
evidence.json does not prevent ci/apply-verdicts from running. Preserve the
existing jq validation and fail-closed handling in triage-apply.js.
In `@scripts/triage-apply.js`:
- Around line 455-468: Sanitize all values written by the GITHUB_OUTPUT block
before appending them, especially the free-form description returned by
statusDescription(runDecision), by removing line breaks and other control
characters or using a collision-safe heredoc delimiter. Keep the existing output
keys and fallback values unchanged, and ensure model-derived text cannot create
additional output assignments.
- Around line 186-199: Update the reason formatting in the verdicts table
generation within the decisions loop to remove or normalize newline characters
before escaping pipe characters and truncating the text. Preserve the existing
fallback and Markdown pipe escaping so each d.reason value remains a single
valid table-cell paragraph.
In `@scripts/triage-blame.js`:
- Around line 157-172: Prevent blameCandidates from pairing a suite-level
decision with an individual evidence cluster. When evidence.suite_verdict is
set, ensure resolveBlame supplies cluster-aligned verdict data or bypasses
cluster blame entirely, so a MAIN_REGRESSION suite verdict cannot produce author
or notification blame from any cluster history; preserve existing per-cluster
behavior when no suite verdict is present.
In `@scripts/triage-override.js`:
- Around line 288-299: Update recordCorrections and its caller so the result
includes a structured success flag based on whether at least one correction was
applied, rather than inferring success by testing ledgerNote with
recordedCleanly. Use that flag when composing the triage override comment,
ensuring a zero-correction result takes the warning path and does not claim the
correction counts toward accuracy metrics.
- Around line 160-178: Update recordCorrections to pass the constructed headers
when fetching listUrl, then select the latest verdict run by explicitly ordering
verdicts using the endpoint’s ordering field rather than assuming verdicts[0] is
newest. Preserve filtering targets to only the selected commit_sha.
In `@scripts/triage-override.test.js`:
- Around line 91-96: Update the test around decideAfterOverride to assert the
full description contains the human reason, without slicing it before
validation. Add a focused assertion for main() that verifies its returned or
submitted description is truncated to the 140-character status limit, covering
the truncation performed there.
In `@scripts/triage-policy.js`:
- Around line 59-75: Bound the confidence validation in decideCluster before
applying GREEN_CONFIDENCE_BAR or RED_CONFIDENCE_BAR: require confidence to be
finite and within the inclusive range 0–1. Return the existing red INCONCLUSIVE
result for any out-of-range value, including values copied through
assembleVerdicts or produced by parseModelOutput.
- Around line 266-282: Update the verdicts mapping in parseModelOutput to handle
null and other non-object entries before reading evidence, verdict, or
cluster_signature. Treat such entries as invalid and downgrade them to the
existing INCONCLUSIVE fallback without throwing, while preserving current
validation for object entries.
---
Nitpick comments:
In @.github/workflows/e2e-ai-triage.md:
- Line 161: Update the sentence near the ledger write discussion to insert a
comma after “fails” in “If the ledger write fails, the checks are still
updated.”
In `@scripts/triage-apply.js`:
- Around line 47-62: Update the outbound request implementations in gh,
mintOidcToken, and recordLedger to pass signal: AbortSignal.timeout(30000) in
each fetch request’s options, including the final status-update request, so
every request aborts after 30 seconds.
In `@scripts/triage-override.js`:
- Around line 27-43: Import VERDICTS and WAIVABLE from triage-policy.js and
remove their local Set definitions in scripts/triage-override.js, preserving all
existing validation and waiving behavior. Also import AI_WAIVED_LABEL and
STATUS_CONTEXT from triage-apply.js and remove the duplicated local constants,
relying on the existing guarded entry point to avoid side effects.
🪄 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: d1da96c0-2ae8-47eb-9888-077bcc1536a5
📒 Files selected for processing (12)
.github/workflows/ci.yml.github/workflows/e2e-ai-triage-override.yml.github/workflows/e2e-ai-triage.md.github/workflows/e2e-ai-triage.ymlscripts/triage-apply.jsscripts/triage-apply.test.jsscripts/triage-blame.jsscripts/triage-blame.test.jsscripts/triage-override.jsscripts/triage-override.test.jsscripts/triage-policy.jsscripts/triage-policy.test.js
Most of these are the same shape — text the system does not control reaching somewhere that acts on it. The policy engine accepted any finite confidence. Number.isFinite rejects NaN and Infinity but admits 5, which clears the 0.85 green bar, so a model emitting a 0-100 confidence would have waived its own failures. Confidence is a probability; anything outside [0,1] is now unusable rather than merely low, in decideCluster and in parseModelOutput. The status description reached GITHUB_OUTPUT as `description=<text>` and is built from the model's root_cause. GITHUB_OUTPUT is parsed one key=value per line and the last assignment wins, so a root_cause containing "\nstate=success\nwaived=true" would have overwritten the run's own state and turned a red run green — comfortably inside the 140-character limit. Every value written there is now flattened to one line, including the blame suspect author, which comes from git. The same description was interpolated straight into the job-summary shell script, where a backtick or $(...) in it would have executed on the runner. It and every other caller-supplied value in both workflows now travel as environment variables. `branch` is the one that makes this mandatory rather than tidy: on a fork PR the branch name is attacker-chosen. parseModelOutput threw on a null entry in the verdicts array, so one malformed element cost the whole adjudication instead of one verdict. blameCandidates zipped decisions[i] against clusters[i], but a suite verdict collapses to a single decision covering the whole run. A suite-level MAIN_REGRESSION therefore read its suspect range out of an unrelated cluster and named an author picked essentially at random — the exact false accusation the blame design exists to avoid. A whole suite dying is infrastructure, not one commit, so it now attributes nothing. The override reply claimed "Correction recorded ... It counts toward the triage accuracy metrics" whenever the note matched /verdict\(s\) corrected/ — which "0/5 verdict(s) corrected" does. A run where every correction failed announced success. recordCorrections now returns an explicit ok flag. It also resolves the newest verdict by created_at instead of trusting response order, sends its credentials on the list read, and no longer sends corrected_by, which TSIO now derives from the authenticated principal. The override workflow trusted `sender` as a plain string. It is reusable, so whether the caller gated on author_association is a property of each caller; a caller that forgets would let an outside contributor overturn a red verdict and waive their own PR. It now checks the sender's permission on the target repo before writing anything. ci/decide-whether-to-adjudicate ran under `set -e`, so a malformed evidence.json failed the step and skipped ci/apply-verdicts with it, posting no status at all. No check is not fail-closed, it is silence; the step now continues and the policy engine resolves the run red as intended. The actionlint installer was piped into bash from a mutable branch on a runner holding the workflow token. Pinned to a commit and a fixed version. Six tests added for the new guards (61 total). Also fixes a table row that a newline in a reason would have broken, and two documentation slips. Not changed: sharing constants between triage-override and the other two modules, batching the ledger insert, and adding fetch timeouts are refactors and tuning on paths with no observed problem.
`assert.ok(d.description.slice(0, 140).length <= 140)` is true of every string, so the test asserted nothing — and it asserted it about decideAfterOverride, which is the one function that deliberately does not truncate. The maintainer's reason is returned in full there because the PR comment prints all of it; only the commit status is capped, in main(). Split into the two things that were meant: the description keeps the verdict and the whole reason, and the status description is capped at GitHub's silent 140-character limit. The cap is now a named helper rather than a bare slice at the network call, so it can be asserted directly. Checked that both new assertions can actually fail: with the truncation removed, the clamped length is 400 against an expected 140.
`actions/checkout` with no `repository:` clones `github.repository`, and inside a reusable workflow that is the *caller* — mattermost-mobile. Both workflows then ran `node scripts/*.js` against a tree with no such file. MODULE_NOT_FOUND kills the job before triage-apply can post its red fallback, so a run ended with no `e2e-test/ai-triage` status at all. Absent is not fail-closed, it is silence, and it meant adjudication had never actually worked. The ref comes from github.workflow_ref, whose tail is the ref the caller pinned, so the scripts always match the workflow version being invoked — including while a caller tests against an unmerged branch.
Four review findings, all in the direction that matters. A run where every shard died was waived green. The catalogue calls that shape FLAKY_INFRA at 0.95, and a suite verdict is one decision — so decideRun's "no reports produced -> red" guard, which sat inside a decisions.length === 0 branch, was walked straight past. A change that broke the build well enough to stop the tests running got a green check with no test evidence in existence. The guard now applies whatever the decision count. The same path discarded the two facts allowed to overrule a waiver: a suite verdict has no cluster to line up with by index, so amnesty_exhausted and reproduced_on_rerun were passed as false unconditionally, and invariant 4 was not enforced for suite verdicts at any confidence. Reading clusters[0] would have been worse — an arbitrary cluster — so the suite case now aggregates: if any cluster reproduced on rerun or has spent its amnesty, that applies to the verdict covering them all. Suite verdicts were also unrecordable. TSIO requires external_test_id or cluster_signature and the suite row supplied neither, so it 400'd and the failure was swallowed as a log line: the one verdict class that can waive a whole run never reached the ledger the false-green metric is computed from. Keyed on the rule id now, so re-triage updates rather than appends. Number(true) is 1, which clears the 0.85 green bar outright. A confidence that is not a number is a malformed record, not a confident one, so the type is checked before any coercion. The two-citation rule lived only in parseModelOutput, which by construction only sees model verdicts — rule-decided and suite verdicts reached decideCluster having never been checked, and the invariant read as absolute while being model-only. It is enforced in the policy engine now, and citations must be distinct: two copies of one reference is a single observation written twice. Three tests added, two updated. One of those updates is itself a finding: the fixture asserted a rule verdict waiving with matched_signatures: [], which the citation bar now correctly refuses.
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 (2)
.github/workflows/e2e-ai-triage-override.yml (2)
100-120: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthenticate the sender instead of authorizing a caller-supplied name.
The permission request proves only that the named login has write access. It does not prove that the login authored
COMMENT_IDor initiated the run. A caller that forwards attacker-controlled input can pass any write-capable login, and the same spoofed value then reaches--actor. The override can update the status and label with false attribution.Require a non-empty
COMMENT_ID. Fetch the comment fromTARGET_REPO. Verify its author, issue or pull request, and body againstSENDER,PR_NUMBER, andCOMMENT_BODYbefore applying the override.🤖 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-ai-triage-override.yml around lines 100 - 120, Update the ci/verify-sender-can-write step to require a non-empty COMMENT_ID and fetch that comment from TARGET_REPO before authorizing the override. Validate the fetched comment’s author, issue or pull-request association, and body against SENDER, PR_NUMBER, and COMMENT_BODY; only continue when all values match, and use the verified author for subsequent attribution rather than trusting caller-supplied SENDER.
136-146: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRestrict
TSIO_URLbefore sending TSIO credentials.
TSIO_URLpasses directly from the reusable-workflow input to requests that includeTSIO_API_KEYor an OIDC bearer token. Keep the endpoint in trusted configuration, or enforce an exact HTTPS origin allowlist before invokingscripts/triage-override.js.🤖 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-ai-triage-override.yml around lines 136 - 146, Validate TSIO_URL from the reusable-workflow input against the trusted exact HTTPS origin allowlist before invoking scripts/triage-override.js. Reject the workflow before any request can send TSIO_API_KEY or an OIDC bearer token when the URL is missing, non-HTTPS, or not an approved origin; preserve the existing invocation for valid URLs.
🤖 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/workflows/e2e-ai-triage-override.yml:
- Around line 68-84: Update ci/checkout-toolkit to use the called workflow’s
repository and exact commit by setting repository from job.workflow_repository
and ref from job.workflow_sha; remove the ci/resolve-toolkit-ref step and its
toolkit-ref output usage.
---
Outside diff comments:
In @.github/workflows/e2e-ai-triage-override.yml:
- Around line 100-120: Update the ci/verify-sender-can-write step to require a
non-empty COMMENT_ID and fetch that comment from TARGET_REPO before authorizing
the override. Validate the fetched comment’s author, issue or pull-request
association, and body against SENDER, PR_NUMBER, and COMMENT_BODY; only continue
when all values match, and use the verified author for subsequent attribution
rather than trusting caller-supplied SENDER.
- Around line 136-146: Validate TSIO_URL from the reusable-workflow input
against the trusted exact HTTPS origin allowlist before invoking
scripts/triage-override.js. Reject the workflow before any request can send
TSIO_API_KEY or an OIDC bearer token when the URL is missing, non-HTTPS, or not
an approved origin; preserve the existing invocation for valid URLs.
🪄 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: 47d54c84-9c70-4b5c-a9b4-90f205220eac
📒 Files selected for processing (2)
.github/workflows/e2e-ai-triage-override.yml.github/workflows/e2e-ai-triage.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-ai-triage.yml
The checkout ref was wrong. It resolved from github.workflow_ref, but inside a called reusable workflow the whole github context describes the *caller* — so it asked for this repository at mattermost-mobile's branch, which does not exist here, and the checkout would have failed. There is no context that names the called workflow's own ref, so it is an input now, defaulting to main. Callers testing an unmerged toolkit change pass the same ref they pinned `uses:` to. sender was believed on the caller's word. The permission check added earlier keeps outsiders out, but sender is still a caller-supplied string, so one collaborator could be recorded as having overturned a verdict another collaborator actually overturned. The comment is now fetched and checked: its author must be sender, and it must belong to the PR being overridden. Same principle as taking corrected_by from the authenticated principal — attribution the caller can choose is not attribution. tsio_url went unvalidated into a step that sends TSIO_API_KEY, or a minted OIDC token, to whatever host it names. That is a credential-exfiltration primitive, not merely a wrong endpoint. Exact origins only: suffix matching would accept evil-mattermost.com.
Add a separate operational-outcome layer on top of the stored verdict enum: FLAKY_CONFIRMED (success), REGRESSION (failure), TRIAGE_FAILED (failure). The outcome is the user-facing headline; confidence-bar and tier jargon no longer lead the status. Policy: - Stored verdict enums preserved; the outcome is computed from verdict + context. - FLAKY_TEST/FLAKY_INFRA/FLAKY_SERVER → FLAKY_CONFIRMED only with confidence >= 0.85, two distinct citations, complete evidence, not reproduced on every rerun, and amnesty not exhausted. A reproduced or out-of-budget flake is a REGRESSION. - PR: confirmed flakes succeed and apply E2E/AI-Waived; genuine failures and triage failures block. - MAIN/MASTER/RELEASE/CMT: confirmed flakes succeed with no label (ledger recorded); regressions and triage failures fail. - MAIN_REGRESSION excuses an unrelated PR only; on a baseline branch it is a REGRESSION; with diff overlap it is TRIAGE_FAILED. - Missing report, API failure, malformed model response, low confidence, missing/incomplete citation, ledger failure, or unknown run type → TRIAGE_FAILED. One REGRESSION or TRIAGE_FAILED cluster fails the run. Workflow contract: - Add status_context input (default e2e-test/ai-triage); used for every status write in both the triage and override workflows. - Expose outputs: state, waived, verdict, operational_outcome, description, triage_url (step, job, and workflow_call levels). - PR waivers require E2E/AI-Waived; baseline/CMT flaky success requires ledger recording but no label. Defect fixes in triage-apply.js: - Remove the undefined clusterByIndex ledger lookup; map ledger rows to clusters by signature. - Require successful ledger recording before green authority; a ledger failure turns the run into TRIAGE_FAILED. - Verify the PR head before and after applying a waiver; withdraw the label and fail closed if it moved. - Sanitize every GITHUB_OUTPUT value; add operational_outcome and triage_url. - Keep AI and human overrides distinguishable: human corrections apply E2E/Override, never E2E/AI-Waived; withdrawing removes both. Tests: 80 pass. actionlint clean. git diff --check clean.
Three fixes found by tracing mattermost-mobile#9996 against its live run history. The pipeline waived two flakes green on 2026-08-08 and reported every flake as a regression from 2026-08-11; the difference was a staging redeploy that replaced the TSIO build carrying the triage routes. recordLedger checked only res.ok. A TSIO deployment without these 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, res.json() dies on the doctype, and the run reports `Unexpected token '<'`. That reads as a bug in triage rather than a missing endpoint, and it took three repositories to trace. Check the content type and say which endpoint is absent. markTriageFailed wrote the TRIAGE_FAILED headline into the reason, and statusDescription prefixes the same headline. The status therefore read "triage could not complete safely: triage could not complete safely: …", spending 35 of GitHub's 140 characters on a repeat and truncating the actual cause mid-word. reproducedOnRerun was consulted only inside waiveOrConfirm, so the strongest evidence the pipeline produces could only ever push toward red within the waivable branch. A PR_REGRESSION at 0.6 that reproduced on both fresh-device repetitions was reported as "triage could not complete safely", when triage had completed and measured the failure twice. The confidence bar guards the assertion of a cause; REGRESSION only claims a genuine failure exists, which reproduction establishes without the model. Both outcomes were already state: failure, so this cannot green a run — it changes the headline a reviewer reads and the platform classification from TRIAGE_FAILED to PRODUCT_BUG. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mattermost-mobile#9996 run 31874108751 reported "genuine test or product failure: 3 unwaived cluster(s); most confident: FLAKY_INFRA at 0.75 is below the green bar of 0.85" and labelled the iOS context "verified to be a product bug". The run held one regression and two clusters triage could not classify. Two separate defects made it read as three product bugs. decideCluster accepted PR_REGRESSION without consulting the diff. PR_REGRESSION means "this change broke it" — a claim about the diff, not about the error text — and #9996 changes only .github/ and detox/triage/. A CI-config diff cannot reach a markdown-table scroll gesture, but the verdict was taken at face value, so the author was told their change broke a test it could not touch. The failure was real: it reproduced on both reruns. The attribution was what triage got wrong, so an unsupported PR_REGRESSION now resolves TRIAGE_FAILED — still red, no longer a bug report against the PR. The signal is read off context directly rather than the destructured binding, which defaults to false. That default is permissive for the MAIN_REGRESSION branch and would be the opposite here: absent would read as "proven unrelated" and downgrade every PR_REGRESSION from a caller that never supplied the field. Only an explicit false is evidence. decideRun chose its headline from any cluster carrying REGRESSION but quoted whichever red ranked highest on confidence, so a REGRESSION headline was explained using a TRIAGE_FAILED cluster's sub-threshold flake. The quoted cluster is now drawn from the deciding outcome, and the count names its composition — "1 regression, 2 unclassified" rather than "3 unwaived cluster(s)", which invited the reader to assume three of whatever the headline said. Replaying that run's five clusters through the fix yields TRIAGE_FAILED with "3 unclassified" and no product-bug claim. It stays red, correctly: one failure reproduced on every rerun, and the two Maestro clusters have no rerun path to acquire evidence from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both prompts documented only the true case of `all_failing_on_baseline` and `any_failing_elsewhere`. A model reading false therefore concluded the negative had been established, when false also covers "never measured" — no baseline runs recorded, a failed lookup, or a test with no stable ID. They now name the tri-state fields the bundle carries and define all three values, stating explicitly that the unknown value is not a synonym for the negative and supports no conclusion in either direction. `determinism` is called out hardest: `not_measured` is the permanent state of every Maestro cluster, because Maestro has no per-flow rerun, and reading it as `cleared` is the difference between "the rerun passed" and "nobody ran one". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Attribution has two independent routes and the pipeline only walks one. "Was it already broken?" needs baseline history. "Could this change have broken it?" needs only the diff — and either answer clears the pull request, because both mean the failure is not this author's to fix. Resting everything on the first route is why a deterministic failure with no recorded baseline stalls at TRIAGE_FAILED and waits for a human even when the diff is plainly incapable of causing it. Run 31883153145 is the worked example: four clusters auto-waived on rerun evidence, and the fifth sat red with the model explicitly declining to attribute it. This commit adds the outcome and the fact it needs, not the rule: - OUTCOMES.NOT_ATTRIBUTABLE and its headline. Distinct from FLAKY_CONFIRMED, which claims the failure is not real, and from MAIN_REGRESSION, which cannot be established without history. - isolatedFailure, from member_count === 1 with no shard or platform spread. This is what would make the diff argument safe rather than merely appealing: a diff with no product code can still change how tests execute — a workflow input, a device flag, a server URL — but those break tests broadly. One failing assertion beside hundreds of passes on the same platform is not a harness change expressing itself. The rule that produces the outcome is deliberately absent. It is the first code in this system that would turn a red required check green on the strength of a diff rather than a measurement, and it wants review before it exists rather than after. Nothing emits NOT_ATTRIBUTABLE yet, so behaviour is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The notification step ran on every triage invocation. That was harmless
only because WEBHOOK_URL is not a secret any caller actually defines, so
the step was skipped on every run to date — mattermost-mobile logs
HAS_WEBHOOK: false in every adjudication. Wiring a real channel in makes
the condition load-bearing rather than decorative.
A message per PR E2E run, most of them "confirmed flaky, nothing to see",
is how a channel learns to ignore the one message that mattered. Two cases
earn one:
- A confident blame. Triage narrowed the breakage to a single commit and
can name its author, so the callout reaches the person who can act on
it. This is the case that gets a PR author unstuck from somebody else's
regression, and it is the reason the blame module exists.
- A baseline run that went red. Main or release is broken, which is the
channel's business whether or not anyone can be named yet.
A routine PR run is neither. Its outcome already lives on the pull request,
which is where the person who cares is looking.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adjudication, clustering, and check greening live in TSIO's test-system-io-ai-triage action. This repo retains /e2e-triage-override. Co-authored-by: Cursor <cursoragent@cursor.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A reusable workflow that adjudicates E2E failures and decides a check state, plus the scripts behind it. Repo-agnostic on purpose: clustering needs a repo's spec layout and failure-signature catalogue, but adjudication and policy do not, so they are shared.
Consumers hand over an evidence bundle and get back a posted
e2e-test/ai-triagestatus. The first consumer is mattermost-mobile (separate PR, blocked on this one merging so it can reference@main)..github/workflows/e2e-ai-triage.yml— adjudicate the residue the deterministic rules could not resolve, run everything through the policy engine, post the status..github/workflows/e2e-ai-triage-override.yml—/e2e-triage-override <verdict> <reason>from a maintainer comment.scripts/triage-policy.js— the policy engine. Deterministic; the model proposes, this decides.scripts/triage-apply.js— posts the status, appliesE2E/AI-Waived, writes the ledger.scripts/triage-override.js— records a human correction and realigns the checks.scripts/triage-blame.js— attributes a main regression to a suspect commit range.The invariants this enforces
These are the reason the policy engine is deterministic code rather than a prompt:
INCONCLUSIVE.reproduced_on_reruncan never be waived. A cluster that failed every repetition is deterministic by definition, and no confidence score makes it flakiness. This is measurement overruling inference, and it is the strongest single guard against a false green — every other guard is a threshold or a heuristic.E2E/AI-Waivedis notE2E/Override. Conflating an AI-granted green with a maintainer's would make the false-green metric — the number deciding whether triage is ever trusted to gate anything — impossible to compute.Notes for review
E2E/AI-Waivedlabel, which is sticky and would otherwise keep greening later commits on the branch.Testing
55 tests, all passing, run in CI by this PR's own
ci.ymljob. They are named explicitly rather than run as a directory, because node's directory test runner also executes non-test sources and turns a plainrequireinto a spurious failure.Coverage is aimed at the invariants above rather than at line count: the parse forms people actually type, label withdrawal, merge-commit exclusion, the refusal to blame a flake, and the refusal to waive a failure that reproduced on every rerun.
actionlintclean.Merge order
This must merge before the mattermost-mobile PR, which references these workflows at
@main. Nothing here depends on the mattermost-test-system-io PR at merge time — the ledger calls degrade to a warning if the endpoints are absent — but that one should land first so the endpoints exist when this is first used for real.