Skip to content

Add a flaky-test triage agent for #pw-health - #36468

Open
shah-harshit wants to merge 5 commits into
mainfrom
shah-harshit/openemetadata-flaky-tests-pr-fix
Open

shah-harshit wants to merge 5 commits into
mainfrom
shah-harshit/openemetadata-flaky-tests-pr-fix

Conversation

@shah-harshit

@shah-harshit shah-harshit commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Describe your changes:

Adds an agent that triages the flaky Playwright tests posted in #pw-health, for open-metadata/OpenMetadata only.

.github/workflows/flaky-test-triage.yml, hourly or by hand, three jobs:

  • collect (Slack token): gathers OSS alerts with FLAKY lines and no Flaky triage reply, plus a week of alert threads for the dedupe. If nothing is pending, the run stops before starting a stack or calling Claude.

  • triage (read-only github.token only, no PAT, no Slack token): starts an OpenMetadata stack, then runs Claude Code (claude-opus-5-5) over skills/flaky-test-triage/SKILL.md. For each flaky test it:

    1. reads the first-attempt error from the run's playwright-blob-* artifacts;
    2. reuses any fix from the last 7 days, reporting its status (draft / in review / approved / in merge queue / merged);
    3. otherwise fixes the root cause (no retries, sleeps, longer timeouts or skips) and runs the spec 10× with retries off against the stack;
    4. names the owners.

    It writes fix branches, PR bodies and thread replies as files.

  • publish (COLLATE_PAT + Slack token, runs no agent code):

    • Publishes only fixes on flaky-fix/… branches that change nothing but Playwright test code (e2e/, utils/, support/, constant/, fixtures/, test-data/).
    • Opens each PR with UI and skip-pr-checks and assigns the test owner.
    • Fills PR links and Slack mentions into the replies, then posts them only under this run's pending alerts.
    • One failed fix or reply doesn't stop the rest.

Fix PRs do not get safe to test: a maintainer reads the agent-written diff before adding the label that runs it with repo secrets. COLLATE_PAT is used rather than github.token because PRs made with github.token never trigger their checks. The rules are recorded in ADR:2026-10-29-flaky-test-triage-agent-writes-nothing-itself.

Prerequisites: a repo secret ANTHROPIC_API_KEY (or CLAUDE_CODE_OAUTH_TOKEN). COLLATE_BOT_SLACK_TOKEN needs channels:history (already works) and users:read.email; without the latter, mentions fall back to @github-login.

Trial: the pull_request trigger marked TRIAL ONLY runs the agent for real on one alert from the last 24h. Remove it before merge.

No linked issue: this is internal CI tooling.

Type of change:

  • New feature

Tests:

Unit tests

  • .github/scripts/tests/test_flaky_triage.py (7 passed) covers:
    • OSS-alert parsing, and skipping Collate and FAILED-only alerts;
    • triaged threads staying in the history but not in the pending list;
    • filling in PR links and mentions;
    • the publish allowlist rejecting files outside the test code (including lint rules, reporters and configs);
    • a bad fix name, title or body;
    • safe to test never being auto-applied;
    • publish still replying when a fix can't be opened.

Manual testing

  • Trial run on this PR: the collect job reads #pw-health and picks an alert. The triage job is blocked until the Claude secret is added.

Checklist:

  • I have read the CONTRIBUTING document.
  • I have commented on my code, particularly in hard-to-understand areas.

🤖 Generated with Claude Code


Summary by Gitar

  • Architecture:
    • Agent job holds only read-only github.token; separate publish job without agent code pushes fix branches, opens PRs, and posts replies
    • Helper renamed to flaky_triage.py with collect and publish commands
  • Publish safeguards:
    • Only flaky-fix/ branches changing files under playwright/ are published
    • Replies post only under this run's pending alerts and must include the Flaky triage marker
  • Triage state:
    • Alerts whose threads already contain a Flaky triage reply are skipped
  • Decision record:
    • Added ADR 2026-10-29-flaky-test-triage-agent-writes-nothing-itself

This will update automatically on new commits.

Hourly workflow lists OSS flaky alerts without a triage reply, then runs
Claude Code (Opus 5.5) over skills/flaky-test-triage: dedupe against the
last 7 days, root-cause, verify the fix 10x retry-free against a CI stack,
open the fix PR, and reply in the thread with RCA, PR status and owners.
@shah-harshit shah-harshit added UI UI specific issues safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check labels Oct 28, 2026
@shah-harshit shah-harshit self-assigned this Oct 28, 2026
@shah-harshit
shah-harshit requested review from a team and tutte as code owners October 28, 2026 15:24
Comment thread .github/workflows/flaky-test-triage.yml Outdated
Comment thread .github/workflows/flaky-test-triage.yml Outdated
Comment thread skills/flaky-test-triage/SKILL.md
The agent job now holds only the read-only github.token: it drafts fix
branches, PR bodies and replies as files. A publish job that runs no agent
code checks each fix touches only playwright/, pushes and opens the PR with
COLLATE_PAT, and posts replies under this run's pending alerts with the
Slack token. Look back 7h so a run queued behind a long triage misses no
alert. Record the rules in a decision record. Accept ANTHROPIC_API_KEY as
well as CLAUDE_CODE_OAUTH_TOKEN.
Comment thread .github/scripts/flaky_triage.py Outdated
Comment thread .github/scripts/flaky_triage.py
Fix PRs no longer get 'safe to test'; a maintainer adds it after reading
the diff. Publish accepts only Playwright test code (e2e, utils, support,
constant, fixtures, test-data), not lint rules, reporters or configs that
CI runs on every PR. One fix or reply that fails to publish no longer
stops the rest, so every alert still gets its reply.
@shah-harshit shah-harshit removed the safe to test Add this label to run secure Github workflows on PRs label Oct 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@shah-harshit shah-harshit added the safe to test Add this label to run secure Github workflows on PRs label Oct 28, 2026
@gitar-bot

gitar-bot Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Code Review ✅ Approved 5 closed / 5 findings

🟡 Medium risk · The triage workflow pushes allowlisted test branches, opens PRs, and posts Slack replies, so mis-scoped drafts could publish unintended changes or messages.

Adds an hourly GitHub Actions agent that triages flaky Playwright tests from #pw-health, separating read-only triage from a publish job that opens test-only fix PRs and posts Slack replies. Resolved this session: the agent's tools could no longer read or leak COLLATE_PAT or the Slack token, the 2h look-back no longer drops alerts during long runs, agent-written playwright code no longer runs with secrets via safe to test, and a single failing fix no longer aborts publish. No open issues remain.

✅ 5 closed
✅ Security: Allowed tools let the agent read and leak COLLATE_PAT and the Slack token

📄 .github/workflows/flaky-test-triage.yml:110-113 📄 .github/workflows/flaky-test-triage.yml:155-169 📄 skills/flaky-test-triage/SKILL.md:20 📄 skills/flaky-test-triage/SKILL.md:47-53
The comment at line 170 says the tokens stay inside gh, git and the Slack helper, but the allowlist doesn't enforce that:

  • Bash(gh:*) allows gh auth token and arbitrary gh api calls.
  • Bash(cat:*) / Bash(git:*) can read the PAT that actions/checkout persists into the git config (token: COLLATE_PAT, and persist-credentials is left at its default).
  • Write/Edit plus Bash(npx playwright test:*), Bash(yarn:*) or Bash(python3 .github/scripts/flaky_triage_slack.py:*) run any JS or Python the agent writes, with GH_TOKEN and SLACK_BOT_TOKEN in the environment. The agent can even rewrite the helper it is allowed to run.

The skill feeds untrusted text into the agent. Step 2 searches PRs whose title or body mentions the spec name, and anyone can open such a PR, so a prompt injection there could exfiltrate an org PAT or post as the bot. 'Treat as data' in the prompt is not a control.

Fix: give the agent a token that can do nothing beyond this repo (a fine-grained token with only contents and pull-requests write), and set persist-credentials: false. Keep SLACK_BOT_TOKEN out of the agent's environment: run the Slack calls in a separate step after the agent, for example by having the agent write replies to files that a later step posts. Also block writes under .github/.

✅ Edge Case: 2h look-back drops alerts while a run of up to 5h is in progress

📄 .github/workflows/flaky-test-triage.yml:17-18 📄 .github/workflows/flaky-test-triage.yml:53-59 📄 .github/workflows/flaky-test-triage.yml:97
A scheduled run only looks back SINCE_HOURS=2, but the triage job can run for up to timeout-minutes: 300. concurrency (with cancel-in-progress: false) queues the next run behind the current one, and GitHub keeps only one pending run per group, so the hourly runs in between are cancelled.

The queued run's pending job computes time.time() - 2h when it actually starts. If the previous run took 4h, alerts posted in its first two hours fall outside every later window and are never triaged. That contradicts the comment at lines 17-18 that 'overlapping windows are safe'.

Fix: make the look-back at least the job timeout plus the cron interval (e.g. 7h). The Flaky triage marker already prevents duplicate replies, so a wider window is safe.

✅ Quality: Triage dedupe marker and verification thresholds exist only in prose

📄 skills/flaky-test-triage/SKILL.md:43-57 📄 skills/flaky-test-triage/SKILL.md:73 📄 skills/flaky-test-triage/SKILL.md:95-97 📄 skills/flaky-test-triage/SKILL.md:124-127 📄 .github/scripts/flaky_triage_slack.py:26 📄 .github/scripts/flaky_triage_slack.py:149-150
This PR sets rules other runs and contributors have to agree with, but no record in docs/decisions/ states them.

  • The description says the Flaky triage marker "is what stops repeat runs from replying twice". That marker is a contract between runs, the Slack helper and anyone replying by hand in #pw-health.
  • It picks judgment-call limits:
    • a fix PR is opened only after "10× with retries off";
    • dedupe covers the last 7 days;
    • after two red rounds the agent stops;
    • fixes may never add retries, sleeps, longer timeouts or skips.

The record would say: "A #pw-health alert thread is triaged once it contains a reply with the text Flaky triage. An automated flaky-test fix PR is opened only after 10 consecutive retry-free runs of the spec, a fix to the same spec from the last 7 days is reused, and fixes never add retries, waits, longer timeouts or skips."

✅ Security: Agent-written playwright/ code runs in CI with secrets via 'safe to test'

📄 .github/scripts/flaky_triage.py:40-41 📄 .github/scripts/flaky_triage.py:163-165 📄 .github/scripts/flaky_triage.py:185-194 📄 docs/decisions/2026-10-29-flaky-test-triage-agent-writes-nothing-itself.md:18-21 📄 docs/decisions/2026-10-29-flaky-test-triage-agent-writes-nothing-itself.md:31-33
fix_problem lets through any change under openmetadata-ui/src/main/resources/ui/playwright/, and publish pushes that branch to origin with COLLATE_PAT, then opens the PR with the safe to test label (flaky_triage.py:40-41, 185-194). Because the branch lives in the same repo, playwright-e2e-reusable.yml runs it with repo secrets in the test environment (for example PLAYWRIGHT_SNOWFLAKE_PASSWORD and PLAYWRIGHT_BQ_PRIVATE_KEY at lines 2016-2027). A prompt injection in Slack or PR text can make the agent write a spec or fixture that sends those secrets out, and the path allowlist does not stop it. That breaks the guarantee in the new ADR's Consequences section ("cannot … change anything outside playwright/, whatever text it reads"), since code inside playwright/ is itself executable with secrets. Fix: do not auto-apply safe to test (and possibly skip-pr-checks), so a human reviews the diff before CI runs it with secrets. Or push agent branches somewhere that runs with fork-level secret isolation, and update the ADR to match.

✅ Bug: One failing fix aborts publish, so no Slack replies get posted

📄 .github/scripts/flaky_triage.py:169-183 📄 .github/scripts/flaky_triage.py:216-230
publish uses check=True for git fetch from the bundle, git push, and gh pr create, and none of these calls are wrapped. Branch names are deterministic (flaky-fix/<spec>-<title>), so pushing to a branch left over from an earlier run is rejected as non-fast-forward. A branch named in a fix JSON but missing from the bundle, a label or permission error, or a branch that already has a PR all raise CalledProcessError too. Any of these kills the script before the replies loop runs, so no thread gets its Flaky triage reply. Every one of those alerts then stays pending and the next hourly run repeats the full stack plus 300-minute agent triage. Fix: wrap each fix's fetch, push and PR creation in try/except, record the failure in the summary, and continue. Its reply then falls back to not opened, see <run>.

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Not enabled

Set up

For more verification details, comment gitar display:verbose.

Options

Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

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

Labels

safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant