T8943: feat(mirror): multi-branch support for force_workspace backport companion#145
T8943: feat(mirror): multi-branch support for force_workspace backport companion#145andamasov wants to merge 3 commits into
Conversation
…t companion Mergify posts ONE combined "No backport have been created" comment per multi-branch backport command (Scenario A; confirmed live — vyos/vyos-documentation#2024, where both branches fail with DIFFERENT per-branch SHAs). The prior parser required exactly one distinct SHA + one branch and fail-closed otherwise, opening no PR for any branch. The parser now extracts (sha, branch) PAIRS: segment the abort comment by each failed `to branch \`X\`` bullet and take the single `Cherry-pick of <40hex>` SHA within each segment. BRANCH_RE/SHA_RE are byte-identical to #143; a new lockstep extract_pairs wrapper is shared byte-identically with the smoke test (verified by diff). Success bullets use "for branch" (excluded), and merge-commit / branch-not-found failures carry no Cherry-pick anchor (0 pairs → fail-closed per branch). parse-and-validate emits a compact-JSON `pairs` array after per-pair allowlist + idempotency filtering (folded into Python); cherry-pick-and-force-stage runs a `fail-fast: false` matrix over the pairs (one PR per branch, independent outcomes). Empty-case semantics preserved: raw-empty → RED; all-allowlist-dropped → RED; all-idempotent → green no-op; mixed allowlist → process the allowlisted subset. Smoke test rewritten with SIX real captured abort-comment bodies (verbatim) + a shape assertion on the {sha,branch} contract that fromJSON+matrix consume. Also hardens the assignee step (trigger login via env, not run: interpolation). Plan: ~/.claude/plans/2026-06-02-force-workspace-multibranch-backport.md Plan-reviewed by Codex + Gemini (both APPROVE). 🤖 Generated by [robots](https://vyos.io)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details🧰 Additional context used🔍 Remote MCP Context7Relevant facts to aid review
Sources used: Context7 docs for GitHub Actions (dynamic matrix/fromJSON, GITHUB_OUTPUT output syntax, env/context behavior, fail-fast examples)., 📝 WalkthroughSummary by CodeRabbit
WalkthroughWorkflow refactors abort-comment parsing from single-branch extraction to multi-pair lockstep parsing and runs downstream backport steps as a matrix per ChangesBackport Workflow and Smoke Test Refactor
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/force-workspace-backport.yml:
- Around line 400-402: The TRIGGER_LOGIN env currently uses
github.event.issue.user.login (the issue/PR author) but should use the commenter
login for issue_comment events; change TRIGGER_LOGIN to source
github.event.comment.user.login so the user who posted the backport command is
used, and ensure any callers via workflow_call preserve that comment.user.login
field when forwarding inputs (the context key remains comment.user.login in an
issue_comment payload and should be passed through to the reusable workflow).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 91bddc65-5bf3-4706-93ff-e0dd231e39c9
📒 Files selected for processing (2)
.github/workflows/force-workspace-backport.yml.github/workflows/force-workspace-parser-smoke.yml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
.github/workflows/**/*.yml
📄 CodeRabbit inference engine (AGENTS.md)
.github/workflows/**/*.yml: Reusable GitHub Actions workflows must useworkflow_calltrigger; avoid non-reusable workflows unless necessary (exception:cla-check.ymlusespull_request_target)
Most GitHub Actions jobs must include abullfrogsec/bullfrog@v0.8.4egress-audit step (non-fatal)
Usevyosbotidentity for cross-org mutations via org-levelPATandREMOTE_OWNERsecrets
Files:
.github/workflows/force-workspace-parser-smoke.yml.github/workflows/force-workspace-backport.yml
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: Rollout 1c (Phorge T8943, 2026-05-30) renamed default branches fleet-wide: release-train reposcurrent->rolling;vyos/.githuband other non-release-train reposcurrent->production. (1) Reusable-workflow refs of the formvyos/.github/.github/workflows/<name>.yml@productionare CORRECT and canonical. Do NOT suggest changing@productionto@current:currentis the OLD name, retained only as a temporary bake-period alias and slated for removal. Any AGENTS.md still saying "reusables pinned to@current" or "current (rolling)" is stale post-1c and must not drive review suggestions. (2) In the mirror callerpr-mirror-repo-sync.yml,permissions: contents: readis INTENTIONAL: the central reusable workflow performs all push/PR writes with thevyos-botGitHub App installation token (minted via theget-tokenaction), not the inheritedGITHUB_TOKEN. Do NOT suggest broadening the caller'spermissionstocontents: write/pull-requests: write.
Files:
.github/workflows/force-workspace-parser-smoke.yml.github/workflows/force-workspace-backport.yml
🧠 Learnings (1)
📚 Learning: 2026-05-30T19:50:14.606Z
Learnt from: andamasov
Repo: vyos/.github PR: 132
File: .github/workflows/pr-mirror-repo-sync.yml:41-43
Timestamp: 2026-05-30T19:50:14.606Z
Learning: When reviewing GitHub Actions workflows in the vyos/.github repository, do not require SHA-pinning for `uses:` action references that intentionally follow the repo’s Rollout 1a conventions. In particular, internal central-infra actions may be referenced via mutable tag/branch refs (e.g., `vyos/.github/.github/actions/get-tokenproduction`) so that all workflow consumers pick up updates from a single central `vyos/.githubproduction` change. Similarly, third-party actions should follow the existing repo convention (e.g., version tags like `bullfrogsec/bullfrogv0.8.4`). SHA-pinning fleet-wide is tracked separately (e.g., T8850) and is out of scope for individual PRs unless a referenced action clearly deviates from the established convention.
Applied to files:
.github/workflows/force-workspace-parser-smoke.yml.github/workflows/force-workspace-backport.yml
🔍 Remote MCP
I'll search for relevant technical context to support the review of this multi-branch force-workspace backport implementation.## Summary of Relevant Technical Context
Based on my research, here's the relevant context that would help in reviewing this multi-branch force-workspace backport implementation:
GitHub Actions Matrix Strategy
Setting fail-fast to false ensures that all jobs complete, providing a full set of results for analysis, which aligns with the PR's use of fail-fast: false to process all branch pairs even if some fail. Dynamic matrices can be used to adapt to code changes and dynamically update matrix variables based on previous job outputs, which is the foundation of the PR's approach to fan-out multiple pairs from the parser output.
A matrix can use fromJSON() to parse dynamic output from previous jobs, which is the pattern the PR uses to pass the pairs array from the parse-and-validate step to the matrix-driven cherry-pick step. This pattern is well-established for combining matrix outputs across job dependencies.
Git Cherry-Pick and Backporting Context
Cherry-picking between two publicly visible branches (e.g., backporting a fix to a maintenance branch for an older release from a development branch) adds information about the original commit that can be useful. The PR's approach to deterministic branch naming (force-backport/<sha>/<branch>) aligns with backport best practices.
When cherry-picking a merge commit, you need to specify the parent branch number using the -m flag, which is relevant since the PR's smoke tests mention "merge-commit error" scenarios, indicating the parser handles merge commit edge cases.
Parser Idempotency Considerations
The PR's per-pair idempotency logic (skipping when a deterministic PR already exists, recovering when an orphaned branch exists) aligns with standard backport patterns. The use of deterministic branch names (force-backport/<sha>/<branch>) ensures that repeated backport attempts for the same SHA and branch combination are idempotent and avoid collisions when processing multiple branches.
Key Review Points from Technical Context
- Matrix Pattern Correctness: The use of
fail-fast: falsewith dynamic matrix pairs from prior job output is a validated GitHub Actions pattern - Merge Commit Handling: The parser must correctly identify merge commits when extracting cherry-pick SHAs, which the smoke tests verify with real fixtures
- Branch Naming Collision Prevention: Deterministic naming prevents collisions across multiple concurrent branches in the matrix
🔇 Additional comments (2)
.github/workflows/force-workspace-backport.yml (1)
32-34: LGTM!Also applies to: 50-115, 150-225, 230-242, 267-268, 359-360, 395-399, 440-442
.github/workflows/force-workspace-parser-smoke.yml (1)
21-259: LGTM!
| # Pass the trigger login via env (NOT interpolated into run:) — avoids a workflow-injection | ||
| # surface even though a GitHub login is a constrained charset. | ||
| TRIGGER_LOGIN: ${{ github.event.issue.user.login }} |
There was a problem hiding this comment.
Use the commenter login here, not the PR author.
Line 402 reads github.event.issue.user.login, which is the issue/PR author on issue_comment. If this env is meant to carry the trigger login, the generated PR will be assigned to the wrong user whenever someone else posts the backport command.
Suggested fix
- TRIGGER_LOGIN: ${{ github.event.issue.user.login }}
+ TRIGGER_LOGIN: ${{ github.event.comment.user.login }}For a GitHub Actions workflow triggered by issue_comment, which context field contains the commenter login: github.event.comment.user.login or github.event.issue.user.login? If this event payload is forwarded into a reusable workflow via workflow_call, does that distinction remain the same?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/force-workspace-backport.yml around lines 400 - 402, The
TRIGGER_LOGIN env currently uses github.event.issue.user.login (the issue/PR
author) but should use the commenter login for issue_comment events; change
TRIGGER_LOGIN to source github.event.comment.user.login so the user who posted
the backport command is used, and ensure any callers via workflow_call preserve
that comment.user.login field when forwarding inputs (the context key remains
comment.user.login in an issue_comment payload and should be passed through to
the reusable workflow).
…ranch SHA dedup Adversarial review (Codex) flagged that BRANCH_RE matched any `to branch \`X\`` text, not just Mergify's real failed bullet. A conflicting filename / prose containing `to branch \`sagitta\`` inside the git-status output Mergify embeds in its abort comment could forge a spurious segment boundary and mis-pair a SHA to the wrong branch. - BRANCH_RE is now LINE-ANCHORED to the literal failed bullet `^ * Backport to branch \`X\` failed` (multiline). Non-bullet text — filenames, prose — can no longer create a boundary. Verified: all six real fixtures still parse identically. - extract_pairs aggregates SHAs PER BRANCH; a branch with != 1 distinct SHA (0 = no anchor; >=2 = duplicate bullets or an injected fake `Cherry-pick of <sha>`) is skipped — fail-closed, never mis-staged and never two PRs for one branch. Two synthetic smoke fixtures added: injection-resistance (filename contains `to branch \`sagitta\``) and duplicate-branch-conflicting-SHAs. Lockstep block stays byte-identical between the reusable and the smoke test (verified by diff); actionlint clean. 🤖 Generated by [robots](https://vyos.io)
…text (adversarial round-2) Adversarial round-2 (Codex) noted that segment-by-bullet pairing still trusts comment-body ordering: a forged failed bullet could in principle capture another branch's SHA. The exploit is not achievable through Mergify's real output (attacker content lands only inside the git-status fence, after the SHA line; per-branch SHA dedup already fail-closes injected SHAs), but the security property should not depend on that empirical assumption. extract_pairs now requires each segment to contain the cherry-pick context "On branch mergify/bp/<branch>/" matching ITS bullet branch. This couples each SHA to its own context, so correctness no longer depends on comment ordering — a forged bullet fails-closed rather than mis-pairing. Two synthetic fixtures added (forged-bullet steal attempt; duplicate fixture now carries real On-branch context to exercise the dedup); injection fixture updated with the realistic On-branch line. Also clarifies the reachability-check src_branch assumption (Gemini recommendation). Lockstep block stays byte-identical between the two files; smoke 9/9; actionlint clean. 🤖 Generated by [robots](https://vyos.io)
Adversarial review —
|
|
Closing — obsolete. #144 mothballed Rollout 4 and removed the The implementation (parser → 🤖 Generated by robots |
Summary
Adds multi-branch support to the
force_workspacebackport companion (Mirror Pipeline Rollout 4). Previously the parser required exactly one distinct SHA + one branch in a Mergify abort comment and fail-closed otherwise — so a single abort comment naming two failed branches opened no force-staged PR for either, forcing a manual cherry-pick.Empirical prerequisite (discharged)
Confirmed against live fleet data that Mergify posts ONE combined comment per multi-branch
@Mergifyio backport b1 b2command (Scenario A), and that the per-branch cherry-pick SHAs can differ. Gold-standard public example: vyos-documentation#2024 — bothcircinusandsagittafail in one comment with different SHAs.Six real comment shapes were captured and are now the smoke-test fixtures (verbatim): both-fail-conflict (#2024), both-fail-merge-error (#1890), partial 1-ok-1-fail (#2016), partial-typo-branch (#1878), both-success (#2005), single-fail (#2116).
Design
extract_pairssegments the body by each failed`to branch \`X\`` bullet and takes the singleCherry-pick of <40hex>SHA in each segment. `BRANCH_RE`/`SHA_RE` are unchanged from the prior anchoring fix; the new pairing wrapper is byte-identical between the reusable and the smoke test (verified by `diff`). Success bullets use "for branch" (excluded); merge-commit / branch-not-found failures have no `Cherry-pick of` anchor → 0 pairs → fail-closed per branch.parse-and-validateemits a compact-JSONpairsarray after per-pair allowlist + idempotency filtering (folded into Python).cherry-pick-and-force-stageruns afail-fast: falsematrix over the pairs → one PR per branch, independent outcomes. Distinct deterministicforce-backport/<sha>/<branch>heads mean the parallel matrix jobs never collide.pairs != '[]'); mixed allowlist → process the allowlisted subset, log the dropped.env:instead ofrun:interpolation.Verification
actionlintclean on both files.pairs=[{"sha":"dec7dcb1…00","branch":"circinus"},{"sha":"3c4b0d49…85","branch":"sagitta"}]→ two matrix instances.🤖 Generated by robots