Repository navigation
chore(agents): teach the test and merge-queue skills about clock-granularity flakes - #93157
Conversation
|
😎 Merged successfully - details. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🤖 CI report✅ Trunk lane — non-backend lane (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df08cf4068
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — escalated to a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Documentation-only change with no production risk, but Codex left two unresolved current-head inline comments — one flagging that the new bisection-triage heuristic could wrongly absolve a PR that broke a test via shared code/config rather than an environment race, the other flagging an unverified gh api pagination detail — and neither has been addressed or rebutted.
- chatgpt-codex-connector[bot] reviewed the current head.
- Unresolved P1 Codex comment on triaging-merge-queue-failures/SKILL.md: the bisection-triage rule may incorrectly clear a PR that deterministically broke a test through shared code/fixtures rather than an environment race.
- Unresolved P2 Codex comment on fixing-flaky-tests/SKILL.md: the suggested gh api check-runs command lacks pagination and may miss the relevant shard on SHAs with many checks.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 0L, 0F substantive, 20L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T0 auto-approve: T0-deterministic (20L, 4F, single-area, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ df08cf4 · reviewed head df08cf4 |
…ularity flakes A warehouse-sources test that pinned a Delta snapshot at the previous write's history() timestamp failed 3/3 attempts on the queue runner, passed on master at the same base SHA, and Trunk's bisection blamed an unrelated PR (#92922, fixed in #93155). The skills covering that path had three gaps: no rule against deriving a cutoff from an earlier write's recorded time, no recognition that an environment-dependent race fails every rerun yet passes elsewhere, and no account of Trunk's bisection branches or of the warehouse-sources exclusion from the quarantine gate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8566f1d to
8676a09
Compare
# Conflicts: # .agents/skills/fixing-flaky-tests/SKILL.md
There was a problem hiding this comment.
Approved.
Pure documentation edit to four skill guide files describing CI/flaky-test triage heuristics — no code, config, or CI behavior changes, so no risky territory is touched.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 0L, 0F substantive, 20L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T0 auto-approve: T0-deterministic (20L, 4F, single-area, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ ad11944 · reviewed head ad11944 |
Problem
Anyone triaging a merge-queue failure, or writing a test that pins "as of" a timestamp, can repeat what happened to #92922 this morning: a warehouse-sources test that depends on two back-to-back writes having distinguishable timestamps failed 3/3 attempts on the queue runner, passed on master at the same base SHA, and Trunk's bisection blamed a cohorts-only PR. The fix is #93155; this PR captures what the skills were missing so the next agent (or person) recognises the shape faster.
Three gaps showed up:
/writing-testswarns about frozen clocks but not about deriving a cutoff from a previous write's recorded time, or about libraries that read a different clock than the one you sampled (delta-rs time travel uses commit-file mtimes, nothistory()timestamps)./fixing-flaky-teststreats "fails every attempt" as a deterministic regression and "doesn't reproduce locally" as the end of empirical validation. An environment-dependent race (Linux coarse mtime clock vs macOS nanoseconds) does both, and can usually be forced locally./triaging-merge-queue-failuresand/debugging-ci-failuresdescribe batch branches as "master plus every PR ahead" and tell the reader the other merge commits are the first suspects. A-bisectionbranch carries only master plus the one PR, so that heuristic points at the wrong thing there. Neither mentions thatproducts/warehouse_sources/junit-product.xmlbypasses the Trunk quarantine gate, so a known flake in that product still reds the shard.Changes
Skill text only; no code.
writing-tests: new determinism rule, "Two writes in a row are not ordered in time", with the delta-rs example and the fix shape (stamp the times explicitly, minutes apart, cutoff between them).fixing-flaky-tests: step 1 gains the "every rerun fails here, master passes the same code" case and the free control atrunk-merge/**PR body provides (its base SHA); step 3 says to ask what the runner has that the laptop lacks and to force the condition before giving up on empirical validation; the step 4 table and the pytest cause list get the clock-granularity row.triaging-merge-queue-failures: Trunk behaviour notes on bisection (and why "failed every attempt" is not evidence either way) and on the warehouse-sources quarantine exclusion.debugging-ci-failures: the bisection exception to "other merge commits are the first suspects".How did you test this code?
Ran
markdownlint-cli2with the repo config andprettier --checkon the four edited files. The guidance itself is what I followed to diagnose #92922, written down after the fact; the evidence for each claim is linked from #93155.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Fable 5.1), same session as #93155. After the fix, the user asked whether any repo skills needed updating to avoid a repeat; I read the four skills that cover this path and added only what the incident showed was missing. No repo skills were invoked.
🤖 Generated with Claude Code