Skip to content

fix(sync): only PRs merged into the default branch transmit - #308

Merged
mmcky merged 2 commits into
mainfrom
fix/sync-default-branch-only
Sep 14, 2026
Merged

mmcky merged 2 commits into
mainfrom
fix/sync-default-branch-only

Conversation

@mmcky

@mmcky mmcky commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

What happened

QuantEcon/lecture-python-programming#629 (a theme v3.0.0 migration) was merged into the long-lived jb2 branch on 2026-09-12. All three sync workflows fired and opened translation PRs for content the English edition does not publish: lecture-python-programming.fr#79, lecture-python-programming.fa#166 and lecture-python-programming.zh-cn#105.

Why

The published sync workflow triggers on pull_request: types: [closed] with no branches: filter, so it fires for a PR closed against any base branch. The job's if: only checks github.event.pull_request.merged == true, which says nothing about which branch it merged into. The action itself never looked at the base branch either.

The change

Two layers, both needed:

Layer Change
Workflow trigger Every published copy of the sync trigger now carries branches: [main] under pull_request: (README, examples/, quickstart, action-reference, the three tutorials, the scaffolder in src/cli/commands/setup.ts, and the E2E harness template). The run never starts.
Action backstop A merged PR whose base branch is not the repository's default branch is skipped, on both the pull_request path (validatePREvent) and the \translate-resync path (runSync, via pulls.get). An issue_comment trigger cannot carry a branch filter, and deployed workflows copied before this change carry none.

The comparison is against repository.default_branch, not a hard-coded main, so a master edition is unaffected. An unreadable default branch or base ref warns and proceeds rather than silencing production on a payload shape this code has not seen.

The workflow-template sweep in workflow-templates.test.ts now requires the filter on every sync job it finds, so a doc page added later is covered the day it lands. Unit tests cover the guard for the jb2 case, a master default branch, a payload with no default branch, and a closed-unmerged PR.

Docs updated: README, action-reference, quickstart, FAQ (new entry), and examples/README.md explain why the filter is load-bearing. CHANGELOG entry under Unreleased. dist-action/ rebuilt.

Still to do outside this repo

The deployed workflows in the source repos (lecture-python-programming, lecture-python.myst, and the others) predate this change and need the one-line branches: [main] edit. Until this ships in a release and the @v0 alias advances, the action-side check is not live for them either. The three downstream PRs listed above are still open and should be closed without merging.

Verification

npm run build, npm run lint, npm run format:check and npm test all pass (67 suites, 1574 tests).

🤖 Generated with Claude Code

The published sync workflow triggered on `pull_request: types: [closed]`
with no `branches:` filter, and the job's `merged == true` guard never
looked at the base branch. A theme-migration PR merged into the long-lived
`jb2` branch (surfaced by QuantEcon/lecture-python-programming#629) fired
every deployed sync workflow and opened translation PRs in the fr, fa and
zh-cn target repos for content the English edition does not publish.

Two layers, both needed:

- Every published copy of the sync trigger (README, examples/, quickstart,
  action-reference, the three tutorials, the scaffolder in setup.ts, and
  the E2E harness template) now carries `branches: [main]`, which stops the
  run before it starts.
- The action skips a merged PR whose base branch is not the repository's
  default branch, on both the pull_request path (validatePREvent) and the
  \translate-resync path (runSync, via pulls.get). An issue_comment trigger
  cannot carry a branch filter, and deployed workflows copied before this
  change carry none, so the in-action check is the backstop for both.

The comparison is against repository.default_branch, not a hard-coded
`main`; an unreadable default branch or base ref warns and proceeds rather
than silencing production on an unfamiliar payload. The workflow-template
sweep now requires the filter on every sync job it finds.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 14, 2026 01:36

Copilot AI left a comment

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.

🟡 Changes recommended

A couple of newly updated docs/log messages are misleading (notably “merged into main” wording and an unconditional “merged PR” production log), and should be corrected to match the default-branch semantics.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Prevents translation sync runs from triggering for pull requests merged into long-lived non-published branches by adding an explicit workflow branch filter and enforcing the same rule inside the action (including the \translate-resync path).

Changes:

  • Add on.pull_request.branches: [main] to published sync-workflow examples/templates so the workflow doesn’t start for non-main base branches.
  • Add an action-side backstop (mergedIntoDefaultBranch) to skip merged PRs whose base ref isn’t the repository default branch (covers issue_comment resync and older deployed workflows).
  • Extend tests to enforce the presence of the branch filter and cover the new base-branch guard logic.
File summaries
File Description
tool-test-action-on-github/test-action-on-github-data/sync-workflow-template.yml Adds branches: [main] to the E2E harness sync workflow fixture.
src/inputs.ts Adds mergedIntoDefaultBranch and uses it in validatePREvent to skip non-default-branch merges.
src/index.ts Applies the same base-branch guard on the \translate-resync (issue_comment) path after fetching PR details.
src/cli/commands/setup.ts Updates the scaffolded sync workflow to include branches: [main].
src/cli/tests/workflow-templates.test.ts Extracts and asserts on.pull_request.branches is present and equals ['main'] for all documented/scaffolded sync jobs.
src/tests/inputs.test.ts Adds unit tests for default-branch merge guarding behavior and exports usage.
README.md Updates the workflow example + narrative to explain the branches: [main] filter and the action backstop.
examples/README.md Updates example workflows and adds documentation explaining why the branches: [main] filter is required.
docs/user/tutorials/fresh-setup.md Updates tutorial workflow snippet to include branches: [main].
docs/user/tutorials/connect-existing.md Updates tutorial workflow snippet to include branches: [main].
docs/user/tutorials/add-language.md Updates tutorial workflow snippet to include branches: [main].
docs/user/quickstart.md Updates quickstart workflow snippet to include branches: [main] and explains why it matters.
docs/user/faq.md Adds an FAQ entry clarifying that sync does not run for merges into non-main branches.
docs/user/action-reference.md Updates reference docs to include the branch filter and explains the trigger semantics.
dist-action/index.js Rebuilds the bundled action to include the new guard logic.
CHANGELOG.md Adds an Unreleased “Fixed” entry describing the incident and the two-layer fix.
Review details
  • Files reviewed: 16/17 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread README.md
Comment thread docs/user/quickstart.md Outdated
Comment thread src/inputs.ts
- README summary and quickstart say "default branch" where the action's
  guard is described, since the check follows repository.default_branch
  and only the template's filter is literally `main`.
- validatePREvent returns as soon as a closed PR is found unmerged, so the
  "Running in PRODUCTION mode" line no longer prints after "Skipping sync".

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mmcky
mmcky merged commit aae38d1 into main Sep 14, 2026
1 check passed
@mmcky
mmcky deleted the fix/sync-default-branch-only branch September 14, 2026 02:08
mmcky added a commit to QuantEcon/lecture-python-programming that referenced this pull request Sep 14, 2026
The sync workflows triggered on `pull_request: types: [closed]` with no
`branches:` filter, so a PR merged into any branch fired them. A merge into
the long-lived `jb2` branch opened translation PRs in every target repo for
content the published edition does not carry. Mirrors the template change
in QuantEcon/action-translation#308 (v0.28.1).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Sep 14, 2026
Patch release: sync mode fires only for PRs merged into the default
branch (#308). CHANGELOG promoted, version bumped, .dev/STATE.md and a
log entry record the jb2 incident and the tidy-up.

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Sep 14, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Sep 14, 2026
…9024 (#310)

* dev: STATE.md + log — v0.28.1 released, §4a gate completed, @v0 = 3de9024

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* dev: STATE.md — label #308 as the fix PR and #309 as the release PR

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
mmcky added a commit to QuantEcon/lecture-python-programming that referenced this pull request Sep 14, 2026
The sync workflows triggered on `pull_request: types: [closed]` with no
`branches:` filter, so a PR merged into any branch fired them. A merge into
the long-lived `jb2` branch opened translation PRs in every target repo for
content the published edition does not carry. Mirrors the template change
in QuantEcon/action-translation#308 (v0.28.1).

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
mmcky added a commit to QuantEcon/lecture-python-intro that referenced this pull request Sep 14, 2026
* ci: sync translations only for PRs merged into main

The sync workflows triggered on `pull_request: types: [closed]` with no
`branches:` filter, so a PR merged into any branch fired them. A merge into
the long-lived `jb2` branch opened translation PRs in every target repo for
content the published edition does not carry. Mirrors the template change
in QuantEcon/action-translation#308 (v0.28.1).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* ci: clearer wording on the branch-filter comment

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
mmcky added a commit to QuantEcon/lecture-python.myst that referenced this pull request Sep 14, 2026
* ci: sync translations only for PRs merged into main

The sync workflows triggered on `pull_request: types: [closed]` with no
`branches:` filter, so a PR merged into any branch fired them. A merge into
the long-lived `jb2` branch opened translation PRs in every target repo for
content the published edition does not carry. Mirrors the template change
in QuantEcon/action-translation#308 (v0.28.1).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* ci: clearer wording on the branch-filter comment

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.

2 participants