Skip to content

feat(ci): fix flaky tests daily with claude-code-action - #11742

Open
basvandijk wants to merge 55 commits into
masterfrom
bas/fix-flaky-tests-workflow
Open

basvandijk wants to merge 55 commits into
masterfrom
bas/fix-flaky-tests-workflow

Conversation

@basvandijk

@basvandijk basvandijk commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Adds a daily workflow that lets Claude fix the tests that flaked since its previous run by following .claude/skills/fix-flaky-tests/SKILL.md, and opens each fix as a draft PR.

How it works

  • schedule-fix-flaky-tests.yml lists the tests that flaked since the previous daily run with bazel run //ci/githubstats:query -- top 100 flaky% --gt 0 --since <28 hours ago> --columns=label,last_flaky_at (ci/scripts/find-flaky-tests.sh): a day, plus the up to 3 hours that the github-stats DB lags behind CI and an hour of cron delay. It groups the variants of a test (_local, _farm, _head_nns_*, ...), which run the same test binary, and drops tests that already have an open deflake PR, or one merged into master after their last flaky run and not reverted (in both cases of claude[bot] or an author with write access), or whose last attempt in the past week ended as not_reproducible or needs_human (the deflake-result-* artifacts of the schedule's own runs and dispatches, kept 7 days: the next attempt would end the same way), or that match the FIX_FLAKY_TESTS_SKIP_PATTERNS variable. The first MAX_FLAKY_TESTS_TO_FIX (a repository variable) of the remaining tests, by flakiness, get a matrix job each, at most 4 at a time, since an incident can make many tests flaky at once (60 on 2026-10-02).
  • fix-flaky-test.yml runs anthropics/claude-code-action with Opus 5.5 at max effort on a dind-large runner. Claude first checks whether the test is already fixed or a fix PR is open, reading only PRs and comments of trusted authors. A fix that leaves a cause for later counts too, as long as the test hasn't flaked since. Otherwise it follows the skill, verifies the fix and commits it locally. The job only has a read-only token: the fix is squashed into one commit and uploaded as a git bundle. Claude has 200 minutes: the prompt states the deadline, and a PreToolUse hook (ci/scripts/deflake-deadline-hook.sh, installed root-owned in Claude Code's managed settings) cuts the timeout of each command to the time left and, past the deadline, denies every tool but the one that returns the result, so the session always ends with a report. A session that ends past the deadline without a result counts as needs_human, and up to 10 000 characters of a reason are kept for whoever picks the test up.
  • open-deflake-pr.yml, dispatched on master, validates the bundle (ci/scripts/validate-deflake-bundle.sh): a single commit that only changes rs/, packages/ and ic-os/, no .bzl or Bazel module files, symlinks or binaries, at most 1000 lines and 1 MB, and no dependency changes besides adding or removing workspace dependencies with a matching Cargo.lock (ci/scripts/validate-deflake-dependencies.py), so no .cargo/config, requirements.txt or other dependency files. It recommits the fix as claude[bot] and opens it as a draft PR (ci/scripts/open-deflake-pr.sh), whose description shows all that Claude wrote as text: the validator neutralizes HTML, links, tables, math, code block languages and mentions, so that nothing in it is hidden from reviewers. PRs for long_test targets get the CI_ALL_BAZEL_TARGETS label. //ci/scripts:validate_deflake_bundle_test tests the validation on temporary git repositories, with a workspace whose lockfiles are the ones Cargo writes.

Why it's built this way:

  • claude[bot] authors the PRs because the CI runs of PRs opened with GITHUB_TOKEN are held for approval (github-actions[bot] has no write access) and PRs of the PR creation bot that touch rs/ fail Check Bot Policies. claude-code-action gets its token through an OIDC exchange that requires the running workflow file to equal the one on master and that expires after an hour, hence the separately dispatched workflow. The e2e PRs (fix: deflake //rs/tests/message_routing/xnet:xnet_slo_3_subnets_test_farm_colocate #11796, fix: deflake //rs/tests/nns:delete_subnet_test_local #11797) were opened with GITHUB_TOKEN only because the exchange can't run from a branch, so the first run on master is the first run of the exchange for this repository (public runs show it issuing claude[bot] tokens to runs dispatched by github-actions[bot]); if it fails, dispatching open-deflake-pr.yml by hand within the 3 days of the fix artifact still yields a claude[bot] PR. Failures and cancellations of the daily run and of the open workflow are posted to eng-idx-alerts, since neither has a person to notify.
  • Ultracode isn't enabled: claude-code-action stops at Claude's first result message (Automation mode stops at the first result; background subagents leave the review silently empty anthropics/claude-code-action#1852), so work that finishes in a later turn, like workflows, background agents or Monitor, would be lost. Those tools are disabled.
  • The fix job drops sudo before Claude starts, so it isn't root in the privileged container.
  • The deadline is enforced by a hook because Claude Code has no wall-clock budget (--max-turns and --max-budget-usd end the session without a result), and claude-code-action writes its outputs only after Claude's result: in the pre-merge test, a leg that ran into the job's 4-hour timeout lost its whole session.
  • CI isn't gated on these PRs: like a member's draft PR, each one runs CI before review, so its fix is already verified when someone looks at it. To limit what a prompt-injected Claude could do, it only reads the PR text and comments of authors with write access, it has no write token, and the bundle validator limits what a fix can change and keeps its description from hiding text.

Setup

  1. Create the fix-flaky-tests environment with the ANTHROPIC_API_KEY secret, limited to the master branch so that workflows on other branches can't read the key (done).
  2. Set the MAX_FLAKY_TESTS_TO_FIX repository variable to the number of tests to fix per run, like 10, and optionally FIX_FLAKY_TESTS_SKIP_PATTERNS to regexes, one per line, of tests that shouldn't be fixed.
  3. Try it with gh workflow run schedule-fix-flaky-tests.yml -f label=<label> -f dry_run=true, then without dry_run.
  4. Set the FIX_FLAKY_TESTS_SCHEDULE repository variable to true to run it daily.

🤖 Generated with Claude Code

Adds a daily workflow that lets Claude fix every test that flaked in the
last week, following .claude/skills/fix-flaky-tests/SKILL.md:

* schedule-fix-flaky-tests.yml lists the flaky tests with
  `//ci/githubstats:query -- top 100 flaky% --gt 0 --week`, groups the
  variants of each test (_local, _farm, _head_nns_*, ...) and drops tests
  with an open deflake PR or matching FIX_FLAKY_TESTS_SKIP_PATTERNS.
* fix-flaky-test.yml runs anthropics/claude-code-action (Opus 5.5, max
  effort) per test with a read-only token. Claude skips tests that are
  already fixed or have a fix PR open and otherwise commits its fix
  locally, which is bundled as an artifact.
* open-deflake-pr.yml, dispatched from master, validates the bundle,
  squashes it into one commit and opens it as a draft PR authored by
  claude[bot].

The daily schedule only runs once the repository variable
FIX_FLAKY_TESTS_SCHEDULE is 'true'.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Dependency validation can be bypassed, and older rejected fixes may be proposed again.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Adds daily automation to identify flaky Bazel tests, generate fixes with Claude, validate them, and open draft PRs.

Changes:

  • Adds scheduled flaky-test discovery and repair workflows.
  • Validates and publishes generated fixes through a separate trusted workflow.
  • Updates flaky-test guidance and variant grouping.
File Description
.github/​workflows/​schedule-fix-flaky-tests.yml Schedules and summarizes repair jobs.
.github/​workflows/​fix-flaky-test.yml Runs Claude and packages fixes.
.github/​workflows/​open-deflake-pr.yml Validates artifacts and opens draft PRs.
ci/​scripts/​find-flaky-tests.sh Finds and groups flaky targets.
ci/​scripts/​validate-deflake-bundle.sh Validates generated Git bundles.
ci/​scripts/​open-deflake-pr.sh Pushes fixes and creates PRs.
.claude/​skills/​fix-flaky-tests/​SKILL.md Updates flaky-test selection guidance.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ci/scripts/validate-deflake-bundle.sh Resolved
Comment thread .github/workflows/fix-flaky-test.yml Resolved
…ed fixes

Addresses the Copilot review:

* validate-deflake-bundle.sh only allowed Cargo.lock without dependency
  changes, but any Cargo.toml under rs/ could change dependencies, e.g. their
  features. Cargo.toml changes may now only add or remove dependencies as
  `name = { workspace = true }`, like the deflake fix in #11518 did.
* Closed deflake PRs of the test are collected at any age, not just from the
  last 30 days, so that Claude doesn't propose a rejected fix again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The manifest trust boundary is bypassable, and PR conflict discovery is incomplete.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment thread .github/workflows/fix-flaky-test.yml Resolved
Addresses the Copilot review: the prompt tells Claude to skip a test when an
open PR modifies its source files, but only PRs matched by title or branch
were collected. Now all open PRs updated in the last 30 days that touch the
package of the test are collected too, e.g. #11738 for
subnet_splitting_v2_test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The bundle validator permits dependency-scope and lockfile dependency changes outside its stated security boundary.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread ci/scripts/validate-deflake-bundle.sh Resolved
Addresses the Copilot review: the line-based check accepted any table header
change, like renaming [dev-dependencies] to [dependencies], which moves the
dependencies below it to another table. validate-deflake-bundle.sh now parses
both versions of each changed Cargo.toml and requires them to be equal apart
from dependencies that only say `workspace = true`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Bundle validation has lockfile and symlink bypasses, and generated PR links use incomplete branch names.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Generated branch suffix makes PR link target nonexistent branch

.github/​workflows/​schedule-fix-flaky-tests.yml:171

GitHub's head: qualifier matches a branch name, not a prefix. The generated branch is ai/deflake-$slug-<date> (and can have a run-ID suffix), so this URL searches for the nonexistent exact branch ai/deflake-$slug and the “fixed” link returns no PR. Build the link from the full branch/URL returned by the opener, or search for the generated deflake $base title instead.

Comment thread ci/scripts/validate-deflake-bundle.sh Resolved
Comment thread ci/scripts/validate-deflake-bundle.sh Resolved
Addresses the Copilot review of validate-deflake-bundle.sh:

* Deleting an existing symlink or submodule passed because only the new mode
  was checked.
* Nested Cargo.lock files under rs/ were allowed and unchecked; they are now
  rejected.
* The root Cargo.lock may now only add or remove the names of workspace
  dependencies that a changed Cargo.toml added or removed, so replacing or
  dropping other entries is rejected too.

Also anchors the `head:ai/deflake-<slug>-` searches with the dash that follows
the slug, so they don't match tests whose names extend it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@basvandijk

Copy link
Copy Markdown
Collaborator Author

Re the "previously missed" finding that head: isn't a prefix match: it is one, in both the search API and the web UI. head:ai/deflake-icp_rosetta finds #11648, #9805 and #9210, whose branches are ai/deflake-icp_rosetta_system_tests-<date>. 059e1f7 appends the - that follows the slug in the generated branch names, so the summary link and the context searches don't match tests whose names extend the slug.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Untrusted PR logs can reach an unrestricted agent, and case-sensitive PR detection can schedule duplicate fixes.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Case-sensitive marker parsing can schedule duplicate fixes

ci/​scripts/​find-flaky-tests.sh:60

The GitHub search used above is case-insensitive, but this capture is case-sensitive. An open internal PR titled, for example, Fix: Deflake //rs/... is returned by gh pr list but contributes nothing to $pr_bases; unless it also uses the generated branch prefix, the scheduler starts a duplicate fix. Parse the marker case-insensitively to match the search semantics.

Comment thread .github/workflows/fix-flaky-test.yml Resolved
Addresses the Copilot review: GitHub's search is case-insensitive but the
title parsing in find-flaky-tests.sh wasn't, so an open PR titled like
`Fix: Deflake //...` didn't drop its test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@basvandijk

Copy link
Copy Markdown
Collaborator Author

Re the "previously missed" case-sensitivity finding: fixed in c47e8e9, the title capture in find-flaky-tests.sh now uses the "i" flag, so e.g. Fix: Deflake //rs/... drops its test too.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Bundle validation permits no-op fixes and unrelated Cargo.lock graph modifications.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject no-op fixes with an empty net diff

ci/​scripts/​validate-deflake-bundle.sh:42

The bundle can contain one or more commits whose final tree is identical to BASE_SHA; all current checks pass, and commit-tree then opens (or attempts to open) a no-op “fixed” PR. Since the scheduler already records the result as fixed, reject an empty net diff before preparing the PR commit.

Comment thread ci/scripts/validate-deflake-bundle.sh Resolved
Addresses the Copilot review of validate-deflake-bundle.sh:

* A workspace dependency added to one crate's Cargo.toml could be added to
  any package in Cargo.lock. Both versions of Cargo.lock are now parsed: they
  must have the same packages, and only the crates whose manifests changed
  may add or remove those dependencies.
* A fix whose commits don't change anything in total is rejected instead of
  being opened as an empty PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@basvandijk

Copy link
Copy Markdown
Collaborator Author

Re the "previously missed" no-op finding: fixed in 8999724, validate-deflake-bundle.sh now rejects a fix whose commits don't change anything in total.

@basvandijk
basvandijk requested a balanced review from Copilot October 2, 2026 13:18
The bundle's paths can have any character, like a newline that starts a
workflow command in the job log, so the message of disallowed files quotes
them for bash. The byte count drops the padding that some wc implementations
add, and validate-deflake-dependencies.py prints its usage instead of a
traceback when it doesn't get two revisions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The security-sensitive workflow executes AI-generated changes and triggers privileged CI, warranting final human review despite its extensive safeguards.

Review effort: Balanced
Findings: None

Resolved since last review (4)

Only runs on master can use the secrets of the fix-flaky-tests environment, while anyone with write
access can read a repository secret from a workflow on their own branch. The jobs set
`deployment: false` because they only use the secret and deploy nothing.

schedule-fix-flaky-tests.yml still passes ANTHROPIC_API_KEY to fix-flaky-test.yml, although it's
empty there: a called workflow only gets the environment secrets whose names its caller passes
(actions/runner#4453).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The model can execute unrestricted networked Bash while holding a long-lived Anthropic API key, allowing prompt-injected credential exfiltration.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread .github/workflows/fix-flaky-test.yml Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The multi-workflow security boundary and automated publication of AI-generated code warrant final human review.

Review effort: Balanced
Findings: None

Resolved since last review (1)

The MAX_FLAKY_TESTS_TO_FIX repository variable sets how many of the tests that flaked since the
previous run get fixed, 10 by default instead of 20.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Untrusted oversized dependency files are parsed before the configured resource limits are enforced.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread ci/scripts/validate-deflake-bundle.sh Resolved
basvandijk and others added 2 commits October 6, 2026 11:02
…s own name

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The dependency checks and the diff limits read whole files, which a bundle can make arbitrarily
large. Their sizes from the object headers come first now: the diff has at least as many bytes as
the files grow, so a fix whose files grow by more than its 1 MB limit would fail it anyway.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The fixed lookback window can permanently miss flakes after delayed or failed daily runs.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Fixed 28-hour cutoff can miss flakes after delayed or failed runs

ci/​scripts/​find-flaky-tests.sh:25

This fixed 28-hour cutoff can leave gaps after a scheduled run is delayed by more than the assumed hour or after any daily run fails before completing this query. The next successful run still looks back only 28 hours, so flakes between the last successful watermark and that cutoff are never considered, despite the workflow's promise to cover flakes since its previous run. Derive since from the last successful daily discovery run (with the DB-lag overlap), or persist a successful-run watermark rather than assuming every cron invocation completed on time.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The cross-workflow automation processes model-generated code and triggers CI, warranting final human security and operational review.

Review effort: Balanced
Findings: None

…flaky run

find-flaky-tests.sh now also reads the time of each test's last flaky run, lists the PRs merged into master
within its window, and drops a group when a deflake PR for it, matched by its `deflake <label>` title or
`ai/deflake-<slug>-` branch, merged after that run. As for open PRs, only same-repository PRs of claude[bot] and
of authors with write access count, since authors can still edit titles after the merge. Reverts and the PRs
they revert don't count. A test that flakes again after the merge is picked again. Manually dispatched labels
have no such time and aren't dropped this way.

The fix job's prompt also skips a test when a PR merged or a commit made in the last week addressed its
flakiness and none of its flaky labels flaked since, even if that fix leaves a cause for later: its author may
be on it.

The e2e test of 2026-10-08 picked xnet_slo_3_subnets_test_farm_colocate although #11790 had merged after its
last flaky run, and Claude then fixed the cause that #11790 had left for later (#11796).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The dependency validator can emit attacker-controlled manifest paths as GitHub Actions workflow commands.

1 open finding

🧠 Review effort: Balanced

…r's messages

validate-deflake-dependencies.py printed paths and dependency names from the fix unescaped, so an added manifest
like `rs/x\n::error::y/Cargo.toml` could start a workflow command in the log of open-deflake-pr.yml, which the
shell validator already prevents with `%q`. They are now printed with their non-printable characters and
backslashes escaped, so ordinary paths read as before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The fixed query window can permanently miss flakes after a delayed, failed, or cancelled daily run.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Fixed 28-hour window can miss flakes after missed or delayed runs

ci/​scripts/​find-flaky-tests.sh:22

The fixed 28-hour window does not actually guarantee coverage since the previous successful run. If one daily run fails/is cancelled, or the next scheduled run starts more than an hour late, flakes that had not reached the lagging database at the prior run can be older than 28 hours by the next query and are never considered. Use a persisted last-successful watermark (with the database-lag overlap), or a window large enough to cover missed runs plus explicit deduplication.

🧠 Review effort: Balanced

basvandijk and others added 3 commits October 9, 2026 10:21
The sr_app leg of the second e2e ran until the job's 240-minute timeout
cancelled the Claude step, and everything was lost: claude-code-action
writes its outputs only after Claude's result, and the prompt never
mentioned a limit. The test's p90 is 10 minutes, so the hours went into
reruns, which can neither show nor refute a flake seen once in 75 runs.

Now a step sets a deadline 200 minutes ahead, the prompt states it and
tells Claude to find the cause in the logs and verify once, and a
PreToolUse hook enforces it: it clamps every Bash timeout to the time
left and, past the deadline, denies all tools but StructuredOutput, so
the session always ends with a result. The hook is installed as root in
the managed settings, which Claude can't edit once sudo is dropped,
unlike its own ~/.claude/settings.json whose hooks reload on change.

Package reads a missing structured output and a final prose turn from
the execution file, records a result after the deadline as needs_human,
and keeps up to 10 000 characters of the reason: the canister_http
report was cut mid-sentence at 1000.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… a human

find-flaky-tests.sh had no memory of attempts, so a test that keeps
flaking and that the job can't fix would cost up to 200 minutes every
day. It now reads the deflake-result-* artifacts of the last week's
scheduled runs and dispatches on its branch, and drops a test whose last
attempt ended as not_reproducible or needs_human until that artifact
expires (fix-flaky-test.yml keeps it 7 days). fixed is left to the PR
rules, and failed, no_logs and skipped are cheap or depend on others, so
they are retried. A dispatched label skips the rule: whoever dispatches
it wants the attempt. Only this branch's own runs count, because a fork
PR that may run workflows could upload a result under the same name.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Late results can bypass deadline classification, and all-missing artifacts prevent the intended summary.

2 open findings

🧠 Review effort: Balanced

Comment thread .github/workflows/fix-flaky-test.yml Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Failed-action recovery can skip valid fix uploads and dispatches, while attempt discovery omits paginated results.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread .github/workflows/fix-flaky-test.yml Resolved
Comment thread ci/scripts/find-flaky-tests.sh Resolved
… lookups

Copilot: "Upload the fix" and request-pr carried an implicit success(),
so a fixed result that Package read from the execution file after the
action failed was recorded but never opened as a PR, and find's lookups
read one page, so a run with more than 30 artifacts could hide a result
and let a parked test be retried. Both steps now run unless the job was
cancelled, and both listings paginate.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The security-sensitive autonomous CI workflow and validation boundary warrant final human review.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

The dropped tests and their reasons only went to the step summary, which
the API doesn't expose and local runs don't have.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Large-PR conflict context and retry-time label application remain incomplete.

0 open findings

Previously missed (2)

In code that hasn't changed since last review

Medium severity Preserve paginated PR filenames in deflake context

.github/​workflows/​fix-flaky-test.yml:145

For PRs with more than 100 files, the paginated API response is used only to decide that the PR touches this package and is then discarded. The subsequent gh pr view --json files still contributes only its first 100 paths to deflake-context.json, so when the matching source file is on a later page Claude cannot enforce the prompt's conflict check. Preserve the paginated filenames (or an explicit touches_package marker) and include them in the context written below.

Medium severity Apply long-test label idempotently to existing PRs

ci/​scripts/​open-deflake-pr.sh:51

The long-test label is applied only while creating the PR. gh pr create --label must create the PR before it can add the label, so a transient labeling failure can leave an open PR; the documented retry then finds that PR, skips this block, and permanently omits CI_ALL_BAZEL_TARGETS. Apply the label idempotently after resolving url, including for an existing PR.

🧠 Review effort: Balanced

… the claude[bot] recovery

The daily run's actor is the merge queue and the open workflow is dispatched with GITHUB_TOKEN, so a
failed run notifies nobody, and a fix job that hits its timeout cancels its run, which the Slack
workflow never posts. Both workflows are now posted to eng-idx-alerts on failure, timeout and
cancellation.

The header of open-deflake-pr.yml says to recover with a new dispatch, not a re-run: a re-run keeps
github-actions[bot] as the actor, while a person's dispatch is accepted by the claude[bot] token
exchange even if it refused the bot.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The security-sensitive workflow spans privileged CI, external credentials, automated code generation, and custom validation boundaries that warrant final human review.

0 open findings

🧠 Review effort: Balanced

…ps are introduced

The deadline paragraph referred to steps 2 and 6 of the fix-flaky-tests skill before the prompt named
the skill, and its advice against reruns read as unconditional. The three sentences now follow the
paragraph that introduces the skill, and the advice is conditional on a flake seen once in dozens of
runs, which reruns can neither reproduce nor refute a fix for.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The security-sensitive unattended workflow, validator, credential handling, and privileged CI integration warrant final human review.

0 open findings

🧠 Review effort: Balanced

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants