Skip to content

[ready to merge] docs(flaky-tests): document branch scope for pass-on-retry monitor#625

Merged
trunk-staging-io[bot] merged 4 commits into
mainfrom
sam-gutentag/por-branch-scope
May 7, 2026
Merged

[ready to merge] docs(flaky-tests): document branch scope for pass-on-retry monitor#625
trunk-staging-io[bot] merged 4 commits into
mainfrom
sam-gutentag/por-branch-scope

Conversation

@samgutentag
Copy link
Copy Markdown
Member

Summary

Documents the new branch scope configuration option for the pass-on-retry monitor, which lets teams restrict detection to specific branches (e.g., stable branches only).

Source

Test plan

  • Preview in GitBook

Generated by Claude Code

…try monitor

trunk2 PRs #3843 and #3909 added backend and UI support for restricting
pass-on-retry detection to configured branches. Documents the new Branch
scope setting and explains when scoping to stable branches is useful.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@trunk-staging-io
Copy link
Copy Markdown

trunk-staging-io Bot commented May 7, 2026

😎 Merged successfully - details.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 21s —— View job


Review: docs(flaky-tests): document branch scope for pass-on-retry monitor

  • Read PR diff
  • Read updated pass-on-retry-monitor.md
  • Cross-check link target failure-rate-monitor.md#branch-pattern-syntax
  • Compare with sibling docs (failure-rate-monitor.md, failure-count-monitor.md) for consistency
  • Post review

Findings

Overall: Clean, well-scoped addition. The new "Branch Scope" subsection mirrors the existing structure (settings table → narrative subsection), the linked anchor #branch-pattern-syntax exists at failure-rate-monitor.md:106, and the motivation paragraph (PR-branch noise → restrict to stable) is concrete and useful.

A few small consistency notes:

1. Missing comma after "By default" — line 47

The intro you added at line 9 uses By default, this monitor evaluates… (with comma). Line 47 drops the comma: By default the pass-on-retry monitor considers…. Minor, but inconsistent with the same file's earlier phrasing.

By default the pass-on-retry monitor considers test runs from every branch.

By default, the pass-on-retry monitor considers test runs from every branch.

Fix this →

2. Pattern list uses literal comma syntax that isn't shown elsewhere — line 54

The example main, release/* reads as the literal string a user types into the field. The sibling failure-rate-monitor.md instead refers to multi-pattern values in prose (e.g., line 104: a monitor scoped to `main` and `release/*` ) and never shows the comma-joined form. Two possibilities:

  • If the UI really does take a single comma-separated string, great — but then it would be worth saying so explicitly (e.g., "Enter multiple patterns as a comma-separated list") and ideally adding the same wording to the failure-rate doc.
  • If the UI takes patterns as separate chips/entries (like the screenshot placeholder in failure-rate-monitor.md:161 suggests: "tag/chip-style UI"), then main, release/* is misleading — readers will paste it as one string.

Worth confirming against the actual UI before merging. My read of the failure-rate doc is that it's chip-style, in which case I'd suggest:

- `main` — only stable branch runs
- `main` and `release/*` — stable plus release branches
- `*` (default) — all branches

3. Consider mentioning the 10-pattern limit

failure-rate-monitor.md:104 says "You can specify up to 10 branch patterns." If the same limit applies here (likely, since the syntax is shared), consider noting it — either inline or by following the leaner failure-count-monitor.md pattern, which just defers to the failure-rate doc:

Branch patterns work the same way as [failure rate monitor branch patterns](failure-rate-monitor.md#branch-pattern-syntax), including glob syntax and merge queue patterns.

That avoids duplicating the example list and keeps the pattern-syntax source of truth in one place. Up to you whether the inline examples are worth keeping for skim-readability vs. the conciseness of a single cross-reference.

4. (Nit) Slight redundancy between lines 9 and 47

Both say roughly "by default, this monitor evaluates all branches." Not a blocker — the intro framing and the section opener serve different purposes — but if you trim, line 47 could drop the "By default…" sentence and start directly with "Use the Branch scope setting to restrict the monitor to a specific set of branches."


Verdict

No blocking issues — the changes are accurate to the documented behavior and link targets resolve. Items above are polish; #2 is the one I'd most want confirmed before merge since it affects whether readers can correctly enter the value.
· Branch: sam-gutentag/por-branch-scope

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Auto-approved: Claude code review passed.

… section

Four small fixes flagged by the Claude review on the same-day open of
PR #625, plus a UI fact-check against the eng PRs that confirmed
Claude's chip-style hypothesis.

* Drop the redundant "By default the pass-on-retry monitor considers
  test runs from every branch" opener at line 47. The intro at line 9
  already establishes the same default; the section opener now leads
  with "Use the **Branch scope** setting to restrict the monitor..."
* Document the chip-style input. The eng PR #3909 uses a `TagInput`
  component (string[] of patterns, max 10, comma-key triggers chip
  creation), not a comma-joined string field. Replaced the misleading
  `main, release/*` example with `main` and `release/*` (prose form,
  matching failure-rate-monitor.md:104) and added a sentence
  describing the chip add gesture.
* Surface the 10-pattern limit inline. The cap is enforced in the UI
  (`max = 10` in the TagInput component); readers should know.
* Cross-reference the failure-rate doc with a slightly tighter phrase
  ("failure rate monitor branch patterns") since the syntax source of
  truth lives there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@samgutentag
Copy link
Copy Markdown
Member Author

Verification status (2026-05-07): live

Verified: customers can use this. Ready to publish.

  • Eng PRs: trunk2#3843 ("Support branch scope for pass-on-retry monitor", merged 2026-05-06) and trunk2#3909 ("Add branch scope UI to pass-on-retry monitor config modal", merged 2026-05-06). Both merge commits intact on main.
  • Linear ticket: none referenced
  • Flag: none (neither eng PR uses useFeatureFlag, useFlag, flags.ts, or LaunchDarkly URLs; branch scope ships unflagged)
  • Signals: pure feature addition, no rollout to chase. Both backend and UI shipped together yesterday.

Review nits applied in commit 5dafc36:

  • Dropped the redundant "By default..." opener at line 47 since the intro at line 9 already establishes the same default.
  • Confirmed Claude's chip-style hypothesis from the eng PR #3909 diff. The UI uses a TagInput component (tags: string[], max = 10, comma-key triggers chip creation), not a comma-joined string field. Replaced the misleading main, release/* example with main and release/* (prose form, matching failure-rate-monitor.md:104) and added a sentence describing the chip add gesture so readers know to type and press Enter or comma.
  • Surfaced the 10-pattern limit inline since it is enforced in the UI.
  • Tightened the cross-reference to the failure-rate doc as the glob syntax source of truth.

trunk check clean.

Suggested next action: flip out of draft and merge.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 54s —— View job


Re-review: docs(flaky-tests): document branch scope for pass-on-retry monitor

  • Read latest PR diff (commit 5dafc36)
  • Verify the four review-nit fixes were applied correctly
  • Cross-check link target and consistency with sibling docs
  • Post review verdict

Findings

Verdict: LGTM — ready to merge.

All four review nits from the prior pass are correctly addressed:

# Original nit Resolution
1 Missing comma after "By default" at line 47 Resolved by removing the redundant "By default…" opener entirely; line 47 now starts with "Use the Branch scope setting…"
2 main, release/* literal-comma example was misleading for a chip-style UI Confirmed against eng PR #3909 (TagInput component). Replaced with prose form `main` and `release/*` and added a chip-add gesture sentence at line 51
3 10-pattern limit not mentioned Added inline at line 51 ("…and accepts up to 10 patterns")
4 Lines 9 and 47 were redundantly stating "by default, all branches" Resolved by the same fix as #1 — the section opener no longer restates the default

Cross-checks I ran:

  • Link target (failure-rate-monitor.md#branch-pattern-syntax) — anchor exists at failure-rate-monitor.md:106 (#### Branch Pattern Syntax). Resolves.
  • Title casing### Branch Scope matches sibling sections at failure-rate-monitor.md:102 and failure-count-monitor.md:83.
  • Default valueAll branches (\`)in the settings table (line 39) is consistent with the` (default) bullet at line 55.
  • Cross-reference style — slightly heavier than failure-count-monitor.md:87 (which fully defers to the failure-rate doc) but lighter than the failure-rate doc itself. The added motivation paragraph (PR-branch noise) and inline 10-pattern note justify the extra prose for skim-readability. Reasonable middle ground.
  • New claim at line 57 ("Changes to branch scope take effect for newly detected events. Previously detected flaky tests are not re-evaluated.") — wasn't in the prior review's scope, but it's a useful operator-facing detail and matches how scope changes typically behave for stateful detection. Author has marked the PR live against the merged eng PRs, so I'll trust it.

Optional polish (non-blocking)

One tiny stylistic note on line 51:

Type a pattern and press **Enter** or **,** to add it as a chip.

The bold comma (**,**) reads a little oddly — a bold punctuation glyph is visually ambiguous (is it the comma key, or a sentence comma?). Two cleaner alternatives if you ever revisit:

Type a pattern and press **Enter** or comma to add it as a chip.

or, if you want to keep the keycap framing:

Type a pattern and press **Enter** (or `,`) to add it as a chip.

Not worth a follow-up commit on its own — flagging only in case it's easier to roll into a future edit.

Suggested next action: agreed with the author — flip out of draft and merge.
· Branch: sam-gutentag/por-branch-scope

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Auto-approved: Claude code review passed.

@samgutentag samgutentag marked this pull request as ready for review May 7, 2026 17:44
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 44s —— View job


Review: docs(flaky-tests): document branch scope for pass-on-retry monitor

  • Read latest PR diff
  • Read updated pass-on-retry-monitor.md
  • Verify typos, grammar, formatting
  • Cross-check link target and sibling-doc consistency
  • Post review verdict

Findings

Verdict: LGTM — no blocking issues.

This is a fresh review against the current state (commit 5dafc36). All prior review nits remain correctly addressed; nothing new surfaced on a focused typo/grammar/formatting pass.

Typos / grammar / formatting:

  • Line 9 intro reads cleanly; comma after "By default" is correct.
  • Line 39 table row: **Branch scope** description and default (All branches (\*`)`) match the bullet at line 55. Consistent.
  • Line 45 heading ### Branch Scope matches sibling sections at failure-rate-monitor.md:102 and failure-count-monitor.md:83. Title-case consistent.
  • Line 49 PR-branch motivation paragraph is concrete and grammatical; semicolon is used correctly.
  • Line 51 cross-reference link target (failure-rate-monitor.md#branch-pattern-syntax) resolves to the anchor at failure-rate-monitor.md:106.
  • Bullet list at lines 53–55 is parallel and the em-dash glossing pattern is consistent.
  • Line 57 caveat about previously detected flaky tests is a useful operator-facing detail.

Documentation best practices:

  • Cross-reference style sits between failure-count-monitor.md (which fully defers to the failure-rate doc) and failure-rate-monitor.md itself. The added motivation + 10-pattern note + chip-gesture sentence justify the extra prose for skim-readability — reasonable middle ground.
  • Default value is surfaced in both the settings table and the bullet list, which is a good pattern for skimmers who land directly in the section.

Optional polish (non-blocking, previously flagged):

Line 51 still has **Enter** or **,**. The bolded comma glyph is visually ambiguous (could read as a sentence comma rather than the comma key). The prior review noted this as non-blocking polish; flagging only because it's the one thing I'd nudge if you ever revisit. Either of these reads cleaner:

Type a pattern and press **Enter** or comma to add it as a chip.
Type a pattern and press **Enter** (or `,`) to add it as a chip.

Fix this →

Not a merge blocker. Agreed with the author's live assessment — ready to merge.
· Branch: sam-gutentag/por-branch-scope

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Auto-approved: Claude code review passed.

… keystroke

- Replace em dashes in the Branch Scope example bullets with colons
- Reword the chip-add hint from bold comma glyph to "or comma" for
  readability
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 2m 5s —— View job


Review: docs(flaky-tests): document branch scope for pass-on-retry monitor

  • Read latest PR diff (commit 14fffdc)
  • Read updated pass-on-retry-monitor.md
  • Verify typos, grammar, formatting
  • Cross-check link targets and sibling-doc consistency
  • Post review verdict

Findings

Verdict: LGTM — ready to merge.

The latest commit 14fffdc cleanly resolves the two prior polish items:

Prior nit Resolution
Bold comma glyph (**,**) was visually ambiguous as a sentence comma vs. the comma key Reworded to "press Enter or comma" at line 51 — reads unambiguously
Bullet glossing style consistency Em dashes in lines 53–55 replaced with colons, matching the bold+colon glossing pattern at failure-rate-monitor.md:264

Fresh checks against the current state:

  • Typos / grammar: None found. Comma after "By default" at line 9 is correct; semicolon at line 49 ("automatically; if those retries…") is used correctly to join two related independent clauses.
  • Formatting: Bullet list at lines 53–55 is parallel (`<pattern>`: <gloss>). The (default) parenthetical on line 55 is consistent with the settings-table default on line 39 (All branches (\*`)`).
  • Headings: ### Branch Scope at line 45 matches the title-case used at failure-rate-monitor.md:102 and failure-count-monitor.md:83.
  • Cross-reference: Link target failure-rate-monitor.md#branch-pattern-syntax resolves to the heading at failure-rate-monitor.md:106.
  • Sibling consistency: The cross-reference style — defer to failure-rate-monitor.md for glob/syntax details, but inline the chip-input gesture, 10-pattern limit, and motivating examples — sits comfortably between the heavier failure-rate-monitor.md and the leaner failure-count-monitor.md:87. The extra prose pays off for skim-readability and is justified.
  • Operational caveat (line 57): "Changes to branch scope take effect for newly detected events. Previously detected flaky tests are not re-evaluated." — useful operator-facing detail, retained from the prior commit.

Documentation best practices:

  • Default value is surfaced in two places (settings table line 39, default-bullet line 55), which helps both skimmers and readers landing directly in the section.
  • The motivation paragraph at line 49 (PR-branch retry noise → restrict to stable branches) is concrete and actionable rather than abstract — good doc discipline.
  • The 10-pattern UI limit is noted inline at line 51 rather than buried, which prevents readers from hitting an unexpected wall.

No blocking issues; no further nits worth flagging. Ready to merge.
· Branch: sam-gutentag/por-branch-scope

@samgutentag samgutentag changed the title docs(flaky-tests): document branch scope for pass-on-retry monitor [ready to merge] docs(flaky-tests): document branch scope for pass-on-retry monitor May 7, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Auto-approved: Claude code review passed.

@trunk-staging-io trunk-staging-io Bot merged commit c5fe9af into main May 7, 2026
7 checks passed
@trunk-staging-io trunk-staging-io Bot deleted the sam-gutentag/por-branch-scope branch May 7, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants