Skip to content

docs(ci): say why fold is push-only too, and name the fork case - #1222

Open
munkhorgil wants to merge 1 commit into
mainfrom
orgil/fold-comment-parity
Open

docs(ci): say why fold is push-only too, and name the fork case#1222
munkhorgil wants to merge 1 commit into
mainfrom
orgil/fold-comment-parity

Conversation

@munkhorgil

Copy link
Copy Markdown
Contributor

Comment-only follow-up to #1221, which merged before review notes landed. Opened here so this repo and sdk-roark-analytics-python (roarkhq/sdk-roark-analytics-python#523) keep an identical fold-next-into-main.yml.

Two review notes from the python side

1. The two gates documented themselves unequally. #1221 gave stalled a long comment explaining the push-only filter, and gave fold the same condition with no explanation — its comment still only said "only fold when CI actually passed on next". fold now says why as well, briefly, pointing at stalled rather than restating it. The consequence genuinely differs: on stalled the second run pages somebody, whereas on fold it only ever wasted a clone and a token mint to discover main already contains next.

2. The one excluded case that is not a blank run. event == 'push' would also filter a fork pull request whose head branch is itself named next — that matches branches: [next], and Stainless's guard is push || fork, so its jobs would really execute. next is an internal release branch and these repositories take no external contributions, so that run does not exist today. Recorded rather than handled, so whoever weighs an external-contribution change finds it already written down instead of rediscovering it.

Scope

Comments only. Both if: conditions are byte-identical to what merged in #1221; git diff touches nothing but comment lines. YAML re-parsed.

🤖 Generated with Claude Code

Review note on the alarm fix: `stalled` gained a long comment explaining the
push-only filter and `fold` gained the same condition with none, so the two
gates documented themselves unequally. `fold` now says why as well - briefly,
pointing at `stalled` rather than restating it, because the consequence differs:
there the second run pages somebody, here it only ever wasted a clone and a
token mint to discover `main already contains next`.

Also records the one case `event == 'push'` excludes that is not a blank run: a
FORK pull request whose head branch is itself named `next` would match
`branches: [next]`, and Stainless's guard is `push || fork`, so its jobs would
really execute. `next` is an internal release branch and these repositories take
no external contributions, so that run does not exist today. Noted rather than
handled, so the next person weighing an external-contribution change finds it
already written down instead of rediscovering it.

Comments only; both `if:` conditions are unchanged.

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

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @munkhorgil's task in 1m 22s —— View job


Review complete

Code Review Summary

Category Score Notes
Stainless Parity N/A (5/5) No Stainless-generated SDK/CLI/MCP code touched; this is hand-authored CI infra (.github/workflows/fold-next-into-main.yml)
Code Quality 5/5 Comment-only change; clear, well-placed, no drift from surrounding style
Security 5/5 No behavioral change; nothing to assess
Performance 5/5 N/A — no code paths changed
Testing 5/5 N/A — comment-only; no test surface to cover
Documentation 5/5 This is the documentation improvement; both notes are accurate and useful
Overall 5/5

Baseline used

This repo has no Stainless-generated commits touching CI workflow files — fold-next-into-main.yml is a hand-written internal automation file, not codegen output, so Stainless parity doesn't apply here. The relevant baseline is the prior state of this same file from #1221 (d4015d4c's parent), which this PR only adds comments to.

Parity differences

None — confirmed by diff inspection: the if: blocks for both fold (lines 45–49) and stalled (lines 163–167) are byte-for-byte unchanged from what merged in #1221. git diff origin/main...HEAD touches only #-prefixed comment lines. This matches the PR description's claim exactly.

Correctness check of the added comments

  1. fold job comment (lines 38–44): Correctly states the job only proceeds on the push-triggered workflow_run (matches github.event.workflow_run.event == 'push' in the unchanged if:), and accurately summarizes why — pointing to stalled's longer explanation rather than duplicating it. Consistent with the actual condition.
  2. Fork case note (lines 156–161): The reasoning holds: workflow_run's branches: filter matches against the head branch of the triggering event, so a fork PR with head branch literally named next would satisfy branches: [next], and if the referenced Stainless CI guard is event == 'push' || fork, that fork's pull_request run would execute jobs (unlike a same-repo PR run, which is skipped). Correctly scoped as "recorded, not handled," since next is described as an internal-only branch with no external contributions today.

Blocking findings

None.

Other notes

  • Could not execute python3 -c "import yaml..." to mechanically re-verify YAML parses, due to sandbox restrictions in this environment (no shell command approval available). Visual inspection confirms indentation and comment (#) prefixes are consistent with the surrounding block — no structural YAML risk from this diff.
  • Nice touch: the new fold comment avoids restating stalled's rationale and instead cross-references it, keeping the two comments from drifting out of sync in the future — matches the PR's own stated goal ("briefly, pointing at stalled rather than restating it").

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant