Skip to content

T8943: feat(mirror): multi-branch support for force_workspace backport companion#145

Closed
andamasov wants to merge 3 commits into
productionfrom
feature/r4-multibranch-force-workspace
Closed

T8943: feat(mirror): multi-branch support for force_workspace backport companion#145
andamasov wants to merge 3 commits into
productionfrom
feature/r4-multibranch-force-workspace

Conversation

@andamasov

Copy link
Copy Markdown
Member

Summary

Adds multi-branch support to the force_workspace backport 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 b2 command (Scenario A), and that the per-branch cherry-pick SHAs can differ. Gold-standard public example: vyos-documentation#2024 — both circinus and sagitta fail 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

  • Parser → (sha, branch) pairs. extract_pairs segments the body by each failed `to branch \`X\ `` bullet and takes the single Cherry-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.
  • Matrix fan-out. 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. Distinct deterministic force-backport/<sha>/<branch> heads mean the parallel matrix jobs never collide.
  • Empty-case semantics (preserved): raw-empty → RED; all-allowlist-dropped → RED; all-idempotent → green no-op (matrix guarded by pairs != '[]'); mixed allowlist → process the allowlisted subset, log the dropped.
  • Hardening: the assignee step now passes the trigger login via env: instead of run: interpolation.

Verification

  • Smoke parser red-first then green on all 6 real fixtures (run exactly as CI extracts the step).
  • Filter decision-tree unit-tested across all six branches (raw-empty, both-allowed, all-idempotent, all-out-of-allowlist, mixed, orphan-recovery).
  • Lockstep block confirmed byte-identical between the two files (34 lines).
  • actionlint clean on both files.
  • Parse step emits the expected matrix-shaped JSON for #2024:
    pairs=[{"sha":"dec7dcb1…00","branch":"circinus"},{"sha":"3c4b0d49…85","branch":"sagitta"}] → two matrix instances.

🤖 Generated by robots

…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)
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: eac79127-41ff-4c22-947d-ea043e9cca40

📥 Commits

Reviewing files that changed from the base of the PR and between c24c52f and f6293b1.

📒 Files selected for processing (2)
  • .github/workflows/force-workspace-backport.yml
  • .github/workflows/force-workspace-parser-smoke.yml
🚧 Files skipped from review as they are similar to previous changes (2)
  • .github/workflows/force-workspace-backport.yml
  • .github/workflows/force-workspace-parser-smoke.yml
📜 Recent review details
🧰 Additional context used
🔍 Remote MCP Context7

Relevant facts to aid review

  • You can emit a JSON matrix from a job by writing a step output into $GITHUB_OUTPUT (e.g. echo 'matrix={"include":[{...}]}' >> "$GITHUB_OUTPUT"), expose it as a job output, and consume it downstream with strategy.matrix: ${{ fromJSON(needs..outputs.) }}. Example and exact syntax shown in the docs (including the echo "matrix=..." >> "$GITHUB_OUTPUT" pattern used in this PR).

  • The documented pattern for setting a step output is: echo "{name}={value}" >> "$GITHUB_OUTPUT" (Bash) or the equivalent for PowerShell; this is the supported mechanism to make values available as needs..outputs..

  • Use strategy.fail-fast: false on matrix jobs to allow all matrix rows to proceed to completion even if some fail (matches the PR's use for independent branch backport attempts).

  • env vs contexts: Values placed in env and referenced via ${{ env.NAME }} are interpolated for steps and become runner environment variables; however, parts of the workflow processed by GitHub Actions (e.g., job/step conditionals or strategy definitions) must use contexts (not runner-only env variables). Passing a value into a step via env (instead of embedding expressions inside run shell interpolation) is the recommended approach when you want the step or an invoked action to receive that value. This supports the PR change that moves TRIGGER_LOGIN into env rather than inline run interpolation.

Sources used: Context7 docs for GitHub Actions (dynamic matrix/fromJSON, GITHUB_OUTPUT output syntax, env/context behavior, fail-fast examples).,


📝 Walkthrough

Summary by CodeRabbit

  • Improvements
    • Backport workflow now parses multiple cherry-pick/backport entries from a single abort comment, processes them in parallel, and skips ambiguous entries.
    • Added allowlist and per-pair idempotency checks to avoid duplicates, skip out-of-scope targets, and recover orphaned branches.
    • Downstream jobs and PR creation are matrix-driven and gated per-pair; audit-trail PRs can set assignee from trigger metadata.
  • Tests
    • Strengthened smoke tests to validate parsing across real-world comment scenarios.

Walkthrough

Workflow refactors abort-comment parsing from single-branch extraction to multi-pair lockstep parsing and runs downstream backport steps as a matrix per {sha, branch} pair; a filter step validates/idempotently prunes pairs. Smoke tests implement and assert the same parser against real abort-comment fixtures.

Changes

Backport Workflow and Smoke Test Refactor

Layer / File(s) Summary
Lockstep parser extraction logic
.github/workflows/force-workspace-backport.yml (lines 70–127), .github/workflows/force-workspace-parser-smoke.yml (lines 30–76)
Core Python parser segments abort-comment text by each "Backport to branch … failed" bullet, requires per-segment context, extracts anchored 40-hex SHAs, enforces one SHA per branch, and emits raw_pairs JSON.
Parse-and-validate output & filtering
.github/workflows/force-workspace-backport.yml (lines 31–34, 50, 162–254)
parse-and-validate now outputs pairs JSON (lines 31–34) and a new "Filter pairs" step (lines 162–254) applies allowlist checks and per-pair idempotency (drop pairs with existing same-repo PRs; keep orphaned deterministic refs). Gating changed to pairs != '[]'. Step name updated to reflect pair parsing (line 50).
Matrix-driven consumer jobs wiring
.github/workflows/force-workspace-backport.yml (lines 279–288, 374–375, 410–417, 455–456)
Downstream jobs run as a matrix over fromJSON(...pairs) and source SHA/TARGET from matrix.pair (lines 279–288, 374–375, 410–417). Audit-trail PR step receives TRIGGER_LOGIN via env and selects --assignee based on its presence (lines 410–417, 455–456).
Smoke test fixtures and assertions
.github/workflows/force-workspace-parser-smoke.yml (lines 21–339)
Smoke-test step runs inline Python extract_pairs() (lines 21–29, 30–76); fixtures replaced with real abort-comment bodies and expected (sha, branch) lists (lines 77–315). Test loop normalizes/sorts results, enforces JSON shape via round-trip, prints per-fixture OK/FAIL, and exits nonzero on mismatch (lines 316–335).

Possibly related PRs

  • vyos/.github#140: Introduced earlier parsing and smoke-test fixtures that this change replaces with the lockstep multi-pair extraction and matrix-driven handling.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the primary change: adding multi-branch support to the force_workspace backport companion workflow.
Description check ✅ Passed The description comprehensively relates to the changeset, detailing the parser logic, matrix fan-out strategy, and empirical validation against six real abort-comment fixtures.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch feature/r4-multibranch-force-workspace
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch feature/r4-multibranch-force-workspace

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@andamasov andamasov marked this pull request as ready for review June 2, 2026 00:49

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 945a1bd and e8db3d4.

📒 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 use workflow_call trigger; avoid non-reusable workflows unless necessary (exception: cla-check.yml uses pull_request_target)
Most GitHub Actions jobs must include a bullfrogsec/bullfrog@v0.8.4 egress-audit step (non-fatal)
Use vyosbot identity for cross-org mutations via org-level PAT and REMOTE_OWNER secrets

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 repos current -> rolling; vyos/.github and other non-release-train repos current -> production. (1) Reusable-workflow refs of the form vyos/.github/.github/workflows/<name>.yml@production are CORRECT and canonical. Do NOT suggest changing @production to @current: current is 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 caller pr-mirror-repo-sync.yml, permissions: contents: read is INTENTIONAL: the central reusable workflow performs all push/PR writes with the vyos-bot GitHub App installation token (minted via the get-token action), not the inherited GITHUB_TOKEN. Do NOT suggest broadening the caller's permissions to contents: 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

  1. Matrix Pattern Correctness: The use of fail-fast: false with dynamic matrix pairs from prior job output is a validated GitHub Actions pattern
  2. Merge Commit Handling: The parser must correctly identify merge commits when extracting cherry-pick SHAs, which the smoke tests verify with real fixtures
  3. 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!

Comment on lines +400 to +402
# 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 }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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).

andamasov added 2 commits June 2, 2026 03:59
…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)
@andamasov

Copy link
Copy Markdown
Member Author

Adversarial review — f6293b1 — Codex + Gemini parallel (Code Reviewer, read-only)

Two iterative rounds on the parser trust model; both providers APPROVE the final head. Findings and their resolutions:

  1. [high · Codex round 1] Unanchored branch boundary → SHA mis-pair. BRANCH_RE matched any to branch \X`text, so a conflicting filename / prose in the embedded git-status output could forge a segment boundary and mis-pair a SHA. Also, duplicate same-branch bullets opened two PRs for one branch. → **Fixed inc24c52f:** BRANCH_REline-anchored to the literal failed bullet^ * Backport to branch `X` failed`; SHAs aggregated per branch and a branch with ≠1 distinct SHA is skipped (fail-closed). Smoke fixtures added (injection-resistance, duplicate-branch).

  2. [high · Codex round 2] Forged failed-bullet could still steal a SHA if a bullet line for another branch appeared inside a real segment before its Cherry-pick of line.
    Fixed in f6293b1: each segment must also contain its own cherry-pick context On branch mergify/bp/<branch>/ matching the bullet branch. Correctness no longer depends on comment-body ordering — a forged bullet fails closed. Smoke fixture "forged-bullet steal attempt" → []. (Analysis: the exploit was not achievable through Mergify's real output, but the security property should not rest on that empirical assumption.)

  3. [low · Gemini] Clarify the src_branch reachability assumption.
    Addressed in f6293b1: comment added noting origin/HEAD is the default branch Mergify backports from, checked per matrix instance.

Final verdict (head f6293b1): Codex APPROVE · Gemini APPROVE · local CodeRabbit 0 findings. Lockstep parser block byte-identical between the reusable and the smoke test; smoke 9/9; actionlint clean.


Each finding above received a fix commit. No open threads.

@andamasov

Copy link
Copy Markdown
Member Author

Closing — obsolete. #144 mothballed Rollout 4 and removed the force_workspace reusable + smoke test from production. Per #144, Mergify's ignore_conflicts: true config already creates the backport workspace PR and applies backport-conflict for path-drift conflicts, so the No backport have been created trigger this companion relied on never fires. Multi-branch support for it is therefore moot.

The implementation (parser → (sha, branch) pairs, fail-fast: false matrix fan-out, line-anchored bullet + On branch mergify/bp/<branch>/ cross-check, 9 real-fixture smoke) is preserved in the local plan should R4 ever be revived. T8943.

🤖 Generated by robots

@andamasov andamasov closed this Jun 2, 2026
@andamasov andamasov deleted the feature/r4-multibranch-force-workspace branch June 2, 2026 07:04
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.

1 participant