Skip to content

[ready to merge] Update branch rulesets given problems from zipline#652

Merged
trunk-staging-io[bot] merged 2 commits into
mainfrom
phil/update-ruleset-problems
May 13, 2026
Merged

[ready to merge] Update branch rulesets given problems from zipline#652
trunk-staging-io[bot] merged 2 commits into
mainfrom
phil/update-ruleset-problems

Conversation

@pv72895
Copy link
Copy Markdown
Contributor

@pv72895 pv72895 commented May 13, 2026

Essentially update the docs to help address the onboarding problems skyline was seeing with rulesets - context thread - https://trunk-io.slack.com/archives/C09L0GB2771/p1778679381269979

@trunk-staging-io
Copy link
Copy Markdown

trunk-staging-io Bot commented May 13, 2026

😎 Merged successfully - details.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 13, 2026

Claude finished @pv72895's task in 2m 19s —— View job


Claude PR Review

  • Read changed files
  • Check for typos, grammar, formatting issues
  • Check documentation best practices
  • Post review feedback

Summary

Overall a solid documentation addition. The new "Exempt Trunk's temporary branches from other rulesets" section addresses a real onboarding gap — particularly the org-level rulesets case and the trunk-temp/** vs. trunk-temp/**/* GitHub matcher quirk. The cross-link from the Classic section, the two verification checklists, and the Branch-vs-Push ruleset disambiguation are all valuable additions. The new image is appropriate and the alt text is descriptive.

Five inline comments left:

  1. Lines 126, 129e.g. missing the comma after it. The rest of the page uses e.g., (lines 96, 97, 113, 114, 183, 232). Two-character consistency fix.
  2. Line 132 — Slightly ambiguous this in "this is a GitHub ruleset type and is unrelated to…". Minor clarity nit, not a blocker.
  3. Line 162 — The "Trunk Sudo" verification bullet packs two distinct conditional statements into one sentence and could be split for clarity.
  4. Line 220 (and around lines 195–217) — Pattern notation inconsistency: the new content uses trunk-temp/**/* but the existing Classic section still uses trunk-temp/*. This is the most worthwhile issue to address, since the new hint at line 148–150 specifically calls out that the trailing /* matters for GitHub's matcher — leaving trunk-temp/* references in the same section could mislead readers.

None of these are blocking — the page is publishable as-is. The pattern-notation point (#4) is the one I'd most recommend addressing before merge.
· Branch: phil/update-ruleset-problems


##### Exempt Trunk's temporary branches from other rulesets <a href="#exempt-trunk-temporary-branches" id="exempt-trunk-temporary-branches"></a>

The two rulesets above target only your protected branch, so they don't match `trunk-temp/**/*` or `trunk-merge/**/*`. But any **other** Branch ruleset — at the **organization** level or elsewhere on this repository — whose targeting is broader (e.g. **All branches**, or a wildcard include like `**/*`) will match Trunk's temporary branches and block the queue.
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.

Style consistency: This page uses e.g., with a comma everywhere else (lines 96, 97, 113, 114, 183, 232). The two new uses on lines 126 and 129 omit the comma. Worth normalizing for consistency.

Suggested change
The two rulesets above target only your protected branch, so they don't match `trunk-temp/**/*` or `trunk-merge/**/*`. But any **other** Branch ruleset — at the **organization** level or elsewhere on this repository — whose targeting is broader (e.g. **All branches**, or a wildcard include like `**/*`) will match Trunk's temporary branches and block the queue.
The two rulesets above target only your protected branch, so they don't match `trunk-temp/**/*` or `trunk-merge/**/*`. But any **other** Branch ruleset — at the **organization** level or elsewhere on this repository — whose targeting is broader (e.g., **All branches**, or a wildcard include like `**/*`) will match Trunk's temporary branches and block the queue.

The two rulesets above target only your protected branch, so they don't match `trunk-temp/**/*` or `trunk-merge/**/*`. But any **other** Branch ruleset — at the **organization** level or elsewhere on this repository — whose targeting is broader (e.g. **All branches**, or a wildcard include like `**/*`) will match Trunk's temporary branches and block the queue.

{% hint style="danger" %}
**Symptom:** A PR enters the queue and then fails out shortly after testing starts with a GitHub permission error (e.g. "Permission denied on trunk-merge/\* branch"). You'll see this on the **Trunk Merge Queue** status check on the PR, in Trunk's status comment on the PR, and on the PR's detail page in the [Trunk dashboard](https://app.trunk.io/). This almost always means a Branch ruleset is preventing Trunk from creating, pushing to, or deleting `trunk-temp/**/*` or `trunk-merge/**/*`.
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.

Style consistency (same as line 126): missing comma after e.g.. Also worth considering using GitBook's italics or backticks for the error string instead of straight double-quotes, to match the way error strings appear elsewhere (e.g., line 217 uses italics-free plain text but in a different sentence structure).

Suggested change
**Symptom:** A PR enters the queue and then fails out shortly after testing starts with a GitHub permission error (e.g. "Permission denied on trunk-merge/\* branch"). You'll see this on the **Trunk Merge Queue** status check on the PR, in Trunk's status comment on the PR, and on the PR's detail page in the [Trunk dashboard](https://app.trunk.io/). This almost always means a Branch ruleset is preventing Trunk from creating, pushing to, or deleting `trunk-temp/**/*` or `trunk-merge/**/*`.
**Symptom:** A PR enters the queue and then fails out shortly after testing starts with a GitHub permission error (e.g., "Permission denied on trunk-merge/\* branch"). You'll see this on the **Trunk Merge Queue** status check on the PR, in Trunk's status comment on the PR, and on the PR's detail page in the [Trunk dashboard](https://app.trunk.io/). This almost always means a Branch ruleset is preventing Trunk from creating, pushing to, or deleting `trunk-temp/**/*` or `trunk-merge/**/*`.

**Symptom:** A PR enters the queue and then fails out shortly after testing starts with a GitHub permission error (e.g. "Permission denied on trunk-merge/\* branch"). You'll see this on the **Trunk Merge Queue** status check on the PR, in Trunk's status comment on the PR, and on the PR's detail page in the [Trunk dashboard](https://app.trunk.io/). This almost always means a Branch ruleset is preventing Trunk from creating, pushing to, or deleting `trunk-temp/**/*` or `trunk-merge/**/*`.
{% endhint %}

**Branch rulesets vs. Push rulesets.** Only **Branch** rulesets need this exemption — this is a GitHub ruleset type and is unrelated to the [Push-Triggered testing mode](#push-triggered-mode-advanced) above. Push rulesets gate the *content* of pushes (file size limits, secret scanning, restricted file paths, etc.) rather than the branch operations the queue performs, so they can target Trunk's temporary branches without breaking the queue. To tell them apart, open a ruleset's edit page: Branch rulesets have a **Branch targeting criteria** section, while Push rulesets have **Push rules** and target repositories rather than branches. Audit only the Branch rulesets.
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.

Minor clarity nit: in "this is a GitHub ruleset type and is unrelated to the [Push-Triggered testing mode]", the antecedent of this is slightly ambiguous — does it refer to "Branch rulesets", "Push rulesets", or "the distinction"? Consider:

Suggested change
**Branch rulesets vs. Push rulesets.** Only **Branch** rulesets need this exemption — this is a GitHub ruleset type and is unrelated to the [Push-Triggered testing mode](#push-triggered-mode-advanced) above. Push rulesets gate the *content* of pushes (file size limits, secret scanning, restricted file paths, etc.) rather than the branch operations the queue performs, so they can target Trunk's temporary branches without breaking the queue. To tell them apart, open a ruleset's edit page: Branch rulesets have a **Branch targeting criteria** section, while Push rulesets have **Push rules** and target repositories rather than branches. Audit only the Branch rulesets.
**Branch rulesets vs. Push rulesets.** Only **Branch** rulesets need this exemption. Branch vs. Push is a GitHub ruleset type and is unrelated to the [Push-Triggered testing mode](#push-triggered-mode-advanced) above. Push rulesets gate the *content* of pushes (file size limits, secret scanning, restricted file paths, etc.) rather than the branch operations the queue performs, so they can target Trunk's temporary branches without breaking the queue. To tell them apart, open a ruleset's edit page: Branch rulesets have a **Branch targeting criteria** section, while Push rulesets have **Push rules** and target repositories rather than branches. Audit only the Branch rulesets.

Not a blocker — the original is understandable, just a touch less explicit.

* [ ] Ruleset #1 targets only your protected branch and has the Trunk GitHub App on the bypass list as **Exempt**.
* [ ] Ruleset #2 targets only your protected branch and does **not** bypass Trunk.
* [ ] Every other Branch ruleset visible at the organization level and on this repository either does not match `trunk-temp/**/*`/`trunk-merge/**/*`, or explicitly excludes both patterns.
* [ ] (If using [Trunk Sudo](../../setup-and-administration/trunk-sudo-app.md)) Trunk Sudo is on Ruleset #1's bypass list as **Exempt**, and on Ruleset #2's bypass list as **Exempt** if you use Force merge or stacked PRs.
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.

This bullet packs two distinct conditional statements about Trunk Sudo into one sentence, and the second condition (Force merge / stacked PRs) only modifies the second half. It reads as a small parsing hurdle. Consider splitting:

Suggested change
* [ ] (If using [Trunk Sudo](../../setup-and-administration/trunk-sudo-app.md)) Trunk Sudo is on Ruleset #1's bypass list as **Exempt**, and on Ruleset #2's bypass list as **Exempt** if you use Force merge or stacked PRs.
* [ ] (If using [Trunk Sudo](../../setup-and-administration/trunk-sudo-app.md)) Trunk Sudo is on Ruleset #1's bypass list as **Exempt**.
* [ ] (If using [Trunk Sudo](../../setup-and-administration/trunk-sudo-app.md) **and** Force merge or stacked PRs) Trunk Sudo is also on Ruleset #2's bypass list as **Exempt**.

**What happens if these branches are protected:** Merge Queue encounters GitHub permission errors and displays messages like "Permission denied on trunk-merge/\* branch."

{% hint style="warning" %}
**Also check rulesets, even if you only use Classic protection.** Organization-level Branch rulesets and other repository-level Branch rulesets apply on top of Classic rules and can match `trunk-temp/**/*`/`trunk-merge/**/*` independently. See [Exempt Trunk's temporary branches from other rulesets](#exempt-trunk-temporary-branches) for how to audit and fix them.
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.

Pattern notation inconsistency within the Classic section. This new hint (and the new verification checklist on line 232) uses trunk-temp/**/* and trunk-merge/**/*, but the surrounding Classic section still describes the branches as trunk-temp/* and trunk-merge/* (lines 199–200, 210, 217).

Readers cross-referencing this hint with the description at lines 199–200 will see two different patterns and may wonder which is correct. Since the new docs explicitly call out (in the line 148–150 hint) that the trailing /* is required for GitHub's matcher, the older trunk-temp/* references feel out of step.

Two options:

  1. Update the Classic section's existing pattern references to trunk-temp/**/* / trunk-merge/**/* for consistency.
  2. Or explicitly note that trunk-temp/* is conceptual shorthand for the branch namespace and trunk-temp/**/* is the GitHub ruleset matcher.

Option 1 is probably simpler. Not a blocker, but worth resolving to avoid reader confusion.

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.

- Normalize `e.g.,` punctuation in new section to match rest of page
- Disambiguate "this" antecedent in Branch-vs-Push paragraph
- Split the Trunk Sudo verification bullet so the Force-merge/stacked-PR
  condition only modifies the Ruleset #2 check
- Standardize `trunk-temp/**/*` / `trunk-merge/**/*` notation throughout
  the page, including the Classic section, so readers don't see the same
  branches described two different ways near the new hint that calls out
  the trailing `/*` as required for GitHub's ruleset matcher

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

claude Bot commented May 13, 2026

Claude finished @pv72895's task in 1m 10s —— View job


Claude PR Review

  • Read changed files
  • Compare against previous review feedback
  • Check for typos, grammar, formatting issues
  • Check documentation best practices
  • Post review feedback

Summary

All five comments from the prior review pass have been addressed in commit 3812348:

  1. Lines 126, 129e.g., punctuation normalized; consistent with the rest of the page (verified all eight occurrences use the comma form).
  2. Line 132this antecedent disambiguated: the sentence now opens with "Branch vs. Push is a GitHub ruleset type and is unrelated to…" which removes the parsing hesitation.
  3. Lines 162–163 — Trunk Sudo verification bullet split into two items so the Force merge or stacked PRs condition only modifies the Ruleset ok #2 check.
  4. Lines 200–201, 211, 216 — Classic section pattern notation harmonized to trunk-temp/**/* / trunk-merge/**/*, matching the new content and the hint at lines 148–150 about the trailing /* requirement.
  5. Line 25 also bumped to the new notation as part of the same sweep — nice catch.

Also noted as nice incidental polish: line 58 changed Trunk creates a \trunk-merge/*` branchtoTrunk creates a branch under `trunk-merge/`` — that's a clearer phrasing since it describes the namespace rather than mixing in a glob.

Remaining observations

None of these are blocking — the PR is ready to merge.

  • Minor consistency nit (non-blocking): Lines 161 and 234 separate the two patterns with a slash — `trunk-temp/**/*`/`trunk-merge/**/*` — while every other occurrence in the page uses or between them (lines 25, 126, 129, 141, 211, 216, 233). Not a parsing problem, just a small visual inconsistency you could normalize on a future pass.

  • Hint stacking around lines 220–226: A warning hint immediately followed by an info hint in the Classic section. Reads fine, but two stacked hint blocks can feel busy. Acceptable as-is.

Verdict

Ready to merge. The page is materially clearer than before — the new "Exempt Trunk's temporary branches from other rulesets" section, the Branch-vs-Push disambiguation, and the two verification checklists meaningfully close the onboarding gap.
· Branch: phil/update-ruleset-problems

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
Copy link
Copy Markdown
Member

Verification status (2026-05-13): live

Verified: customers can use this. Ready to publish.

  • Eng PR: none (pure docs change describing existing behavior)
  • Flag: none
  • Signals checked:
    • PR body has no engineering PR refs and no Linear ticket; context is a Slack thread about onboarding pain (#team-skyline customer)
    • Diff is documentation only: corrects glob patterns (trunk-temp/* to trunk-temp/**/* to match GitHub's matcher), adds a new "Exempt Trunk's temporary branches from other rulesets" section, and adds two verification checklists
    • No feature flag is being documented. The page describes existing GitHub Branch ruleset behavior and the existing trunk-temp/ and trunk-merge/ branch namespaces, both of which are already live in production
    • Authored by the domain owner for Merge Queue (@pv72895), whose Merge Queue docs PRs are treated as authoritative

Ready to merge once review threads are resolved.

@samgutentag samgutentag changed the title Update branch rulesets given problems from zipline [ready to merge] Update branch rulesets given problems from zipline May 13, 2026
@trunk-staging-io trunk-staging-io Bot merged commit ee7c76e into main May 13, 2026
7 checks passed
@trunk-staging-io trunk-staging-io Bot deleted the phil/update-ruleset-problems branch May 13, 2026 17:36
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