[TRTLLMINF-188][infra] Require approval for broad post-merge bot runs

[chzblych]

Description

TRTLLMINF-188 reduces accidental consumption of shared CI resources by requiring the ci: post-merge approved label before broad post-merge bot runs are dispatched.

This PR:

• Adds the default-branch Guard Post-Merge Approval Label workflow.
• Validates the latest approval-label event actor against active membership in NVIDIA/trt-llm-ci-approvers.
• Normally removes the label and posts an explanatory PR comment when the actor is unauthorized or membership cannot be verified.
• Serializes Guard runs per PR and binds cleanup to the exact label event, so an older unauthorized run does not remove a newer approval.
• Keeps a Guard job that has already started eligible to attempt cleanup after standard workflow cancellation, with a five-minute job timeout.
• Uses pull_request_target only for static default-branch API logic and never checks out or executes PR-controlled code.
• Keeps the approval label when new commits are pushed.
• Updates /bot help and CI documentation while preserving explicit stage names and limited wildcard behavior such as *PerfSanity*.

The companion Jenkins wrapper requires the approval label for:

• --post-merge
• --stage-list "*" and --extra-stage "*"
• --stage-list "*Post-Merge*" and --extra-stage "*Post-Merge*"
• equivalent escaped, repeated-star, whitespace-trimmed, or comma-separated broad selectors

The companion wrapper reuses the PR snapshot captured when its job starts and performs one label-presence validation during command parsing. It does not synchronously revalidate the label actor or Team membership. Selector matching remains case-sensitive; approval-label identity is case-insensitive.

Scope and limitations

This is a best-effort CI resource-governance guard, not a strict authorization boundary. Queued-before-start cancellation, force-cancel, runner/service failure, and the remaining asynchronous label-to-wrapper-start window are tracked by TRTLLMINF-220.

The canonical .github/workflows/blossom-ci.yml has no diff. Repository-local proxy test files were intentionally not shipped; validation used static checks, isolated live workflow tests, isolated wrapper validation, and the deployed end-to-end path.

Validation

GitHub workflow

• YAML parsing, embedded github-script compilation, focused label-validation/cleanup simulation, and git diff --check passed.
• Closed draft personal-fork PRs #7, #9, #10, and #11 covered commit persistence, same-actor re-label, exact-event binding, and standard-cancellation races. The standard-cancellation driver and Guard run confirmed that a Guard job which had started could still complete fail-closed cleanup.
• The authoritative upstream unauthorized-actor test used the trtllm-agent account with repository triage access: the driver applied the label to [TRTLLMINF-188][test] Validate merged post-merge approval workflow #16575, Guard run 29694025848 removed it and posted the expected denial comment, and the accompanying LLM check caused no recursive bot or Jenkins dispatch.

Deployed end-to-end verification

After both changes merged, closed draft NVIDIA/TensorRT-LLM#16575 exercised the live /bot and Jenkins path:

1. Missing approval: /bot run --post-merge was rejected with the expected actionable guidance before downstream dispatch.
2. Authorized approval: chzblych, an active approver, applied the label and Guard run 29696485505 succeeded. A second /bot run --post-merge passed the gate and produced the expected pipeline-start evidence.
3. Bounded cleanup: /bot kill was issued after the start evidence appeared. The bot acknowledged the kill, and final readback confirmed all test-related CI work was stopped.

The test PR was restored to closed, draft, unmerged, and label-free state.

Companion Jenkins wrapper

• The companion wrapper change is merged and active on its default branch; merged content matched the reviewed change.
• Isolated wrapper validation passed protected/unrestricted selector, missing/approved label, case-insensitive label, fail-closed missing snapshot, and state-persistence cases without a real downstream dispatch.
• The deployed E2E above confirms both rejection before dispatch and successful dispatch when an authorized approval is present.

Deployment status

• Merged into main as 3124bb3525e6e2ad285ba7c22ddc0dbc236521dc.
• The Guard workflow, approval label, membership validation, and companion Jenkins gate are active.
• Final merged-state readback and end-to-end verification passed; no test workload remains active.
• Stronger authorization/cancellation guarantees remain follow-up work under TRTLLMINF-220.

PR Checklist

• PR description clearly explains what and why.
• No PR-controlled code is executed by pull_request_target.
• Documentation and /bot help are updated.
• The reviewers assigned automatically/manually are appropriate for the PR.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Post-merge approval enforcement

Layer / File(s)	Summary
Approval validation and cleanup .github/workflows/post-merge-approval.yml	Adds a label-triggered workflow that verifies the latest labeling actor’s active membership in NVIDIA/trt-llm-ci-approvers, preserves newer approvals during cleanup, and removes invalid labels with a PR comment.
Command guidance and CI documentation .github/workflows/bot-command.yml, docs/source/developer-guide/ci-overview.md	Documents approval-label requirements for post-merge runs and broad wildcard stage selectors, including persistence and wildcard syntax details.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PullRequest
  participant ApprovalWorkflow
  participant TeamsAPI
  participant GitHubAPI
  PullRequest->>ApprovalWorkflow: Apply approval label
  ApprovalWorkflow->>GitHubAPI: Find latest label event
  ApprovalWorkflow->>TeamsAPI: Verify labeling actor
  TeamsAPI-->>ApprovalWorkflow: Return membership status
  ApprovalWorkflow->>GitHubAPI: Remove invalid label and post comment

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title follows the required ticket/type pattern and clearly summarizes the main change.
Description check	✅ Passed	The description includes the required sections and enough detail on the change, testing, and checklist.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 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/scripts/test_post_merge_approval_workflow.js:
- Around line 87-129: Update the runValidation test fixture so context.actor and
payload.sender.login use distinct values when sender is present, then assert the
result uses the sender login (for example, retain the expected label actor while
using a different fallback actor). Preserve the sender=false case to verify
context.actor is used when no sender exists.

In @.github/workflows/post-merge-approval.yml:
- Around line 70-103: Update the post-merge approval workflow to serialize or
debounce runs per pull request, then re-check the latest label event and
authorization state immediately before any removeLabel cleanup. Ensure an older
unauthorized run exits without removing a label re-added by a later valid
approval.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 43c183c7-c921-4987-97dc-214454fce7d4

📥 Commits

Reviewing files that changed from the base of the PR and between 01291f9 and 3e82b91.

📒 Files selected for processing (4)

• .github/scripts/test_post_merge_approval_workflow.js
• .github/workflows/bot-command.yml
• .github/workflows/post-merge-approval.yml
• docs/source/developer-guide/ci-overview.md

[chzblych]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/source/developer-guide/ci-overview.md (1)

1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add the required NVIDIA copyright header.

This modified Markdown file starts directly with the document title and has no NVIDIA copyright header. Add the repository-standard header in a Markdown-safe comment and use the latest meaningful modification year.

As per coding guidelines, all files matching **/* must contain or update the NVIDIA copyright header.

🤖 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 `@docs/source/developer-guide/ci-overview.md` at line 1, Add the
repository-standard NVIDIA copyright header at the beginning of the Markdown
document using a Markdown-safe comment, and set its year to the latest
meaningful modification year. Keep the existing “Continuous Integration
Overview” title and document content unchanged.

Source: Coding guidelines

🧹 Nitpick comments (1)

.github/workflows/post-merge-approval.yml (1)

18-20: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

pull_request_target dangerous-trigger finding is an expected trade-off here, not a defect.

zizmor's dangerous-triggers rule fires on any pull_request_target use, but this workflow never checks out or executes PR code — the script body is static and only reads timeline/label metadata via the API, so the usual fork-RCE/secret-exfiltration vector doesn't apply. Consider documenting this rationale near the trigger (or adding a suppression entry in zizmor's config, if supported) so the finding doesn't need to be re-litigated on every scan.

🤖 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/post-merge-approval.yml around lines 18 - 20, Document the
intentional use of pull_request_target beside the trigger in the workflow,
noting that the job runs only static logic and reads timeline/label metadata
through the API without checking out or executing pull-request code.
Alternatively, add the supported zizmor suppression for this expected finding,
preserving the existing trigger behavior.

Source: Linters/SAST tools

🤖 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/post-merge-approval.yml:
- Around line 22-24: Add issues: write to the permissions block in the
post-merge approval workflow, preserving the existing contents and pull-requests
permissions so github.rest.issues.removeLabel can execute successfully.

---

Outside diff comments:
In `@docs/source/developer-guide/ci-overview.md`:
- Line 1: Add the repository-standard NVIDIA copyright header at the beginning
of the Markdown document using a Markdown-safe comment, and set its year to the
latest meaningful modification year. Keep the existing “Continuous Integration
Overview” title and document content unchanged.

---

Nitpick comments:
In @.github/workflows/post-merge-approval.yml:
- Around line 18-20: Document the intentional use of pull_request_target beside
the trigger in the workflow, noting that the job runs only static logic and
reads timeline/label metadata through the API without checking out or executing
pull-request code. Alternatively, add the supported zizmor suppression for this
expected finding, preserving the existing trigger behavior.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f8a7cfaf-7771-40f5-8315-71b47658331e

📥 Commits

Reviewing files that changed from the base of the PR and between 01291f9 and f39f1da.

📒 Files selected for processing (4)

• .github/scripts/test_post_merge_approval_workflow.js
• .github/workflows/bot-command.yml
• .github/workflows/post-merge-approval.yml
• docs/source/developer-guide/ci-overview.md

[chzblych]

Addressed the latest CodeRabbit review on head 9d79005:

• Added an accurate rationale beside pull_request_target: the workflow runs static default-branch API logic and never checks out or executes PR code.
• Kept least-privilege pull-requests: write; GitHub documents it as sufficient for both label removal and PR timeline comments, and the fork E2E cleanup path already succeeded with that permission only.
• Did not add a copyright header to docs/source/developer-guide/ci-overview.md: the repository's Markdown documentation convention does not use SPDX headers, and the coding-guideline rule applies to compiled/interpreted OSS source files.

Validation: executable workflow contract test passed, git diff --check passed, and the cross-repository policy contract passed.

[ZhanruiSunCh]

One concern regarding the interaction between this workflow and the Jenkins gate:

Potential bypass via workflow cancellation / delay:

The clear step condition if: always() && \!cancelled() && ... means the label cleanup is skipped when the workflow run is cancelled. A user with Write access (but not a trt-llm-ci-approvers member) could:

1. Add the ci: post-merge approved label
2. Cancel the approval workflow run before the clear step executes
3. The label remains on the PR
4. /bot run --post-merge → Jenkins sees the label → dispatches

The same gap exists when the workflow is queued (concurrency group) or delayed (GitHub Actions runner unavailability) — the label is unvalidated during that window.

Since the Jenkins wrapper job already has a GitHub token, consider adding the same membership verification in validatePostMergeApproval() (fetch PR timeline → check label actor membership in trt-llm-ci-approvers) so Jenkins can independently validate regardless of the workflow's execution state.

[chzblych]

@ZhanruiSunCh Thanks for raising the cancellation/delay concern in your review.

I agree that making this a strict authorization boundary would require Jenkins to synchronously verify the label actor/Team membership (or consume another trusted approval record). For TRTLLMINF-188 we are intentionally keeping the lower-complexity, best-effort resource-governance model: the GitHub Action validates the actor, and the companion Jenkins change refreshes current label presence before dispatch.

This update covers the normal cancellation path after a Guard job starts:

• job-level and cleanup-step always() keep fail-closed cleanup eligible;
• a five-minute timeout bounds a stuck Guard;
• label comparisons are case-insensitive;
• isolated E2E driver 29674054263 / Guard 29674059267 verified a standard cancel request, successful cleanup, label removal, and the denial comment. The E2E used the same Guard implementation plus a temporary deterministic pause solely to make the cancellation window reproducible.

Queued-before-start cancellation, force-cancel, runner/service failure, and the immediate asynchronous label-to-dispatch window are not fully covered by this design. Follow-up TRTLLMINF-220 tracks the stronger guarantee and evaluation of a synchronous Jenkins check or another low-complexity approach.

[chzblych]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[chzblych]

/bot skip --comment "Not tested by L0"

[tensorrt-cicd]

PR_Github #60159 [ skip ] triggered by Bot. Commit: ebde870 Link to invocation

[tensorrt-cicd]

PR_Github #60159 [ skip ] completed with state SUCCESS. Commit: ebde870
Skipping testing for commit ebde870

Link to invocation