[TRTLLMINF-188][infra] Require approval for broad post-merge bot runs#16318
[TRTLLMINF-188][infra] Require approval for broad post-merge bot runs#16318chzblych wants to merge 5 commits into
Conversation
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
📝 WalkthroughWalkthroughAdds a label-triggered GitHub Actions workflow that validates post-merge approval actors, removes invalid approval labels, and comments on affected pull requests. It also adds workflow tests and updates bot help and CI documentation for approval-label and wildcard requirements. ChangesPost-merge approval enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant ApprovalWorkflow
participant TeamsAPI
participant PullRequestAPI
PullRequest->>ApprovalWorkflow: Apply ci: post-merge approved
ApprovalWorkflow->>TeamsAPI: Validate labeling actor
TeamsAPI-->>ApprovalWorkflow: Return membership status
ApprovalWorkflow->>PullRequestAPI: Remove label if invalid
ApprovalWorkflow->>PullRequestAPI: Post explanatory comment
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (4)
.github/scripts/test_post_merge_approval_workflow.js.github/workflows/bot-command.yml.github/workflows/post-merge-approval.ymldocs/source/developer-guide/ci-overview.md
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
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 winAdd 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_targetdangerous-trigger finding is an expected trade-off here, not a defect.zizmor's
dangerous-triggersrule fires on anypull_request_targetuse, 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
📒 Files selected for processing (4)
.github/scripts/test_post_merge_approval_workflow.js.github/workflows/bot-command.yml.github/workflows/post-merge-approval.ymldocs/source/developer-guide/ci-overview.md
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
|
Addressed the latest CodeRabbit review on head
Validation: executable workflow contract test passed, |
1 participant
Summary by CodeRabbit
New Features
ci: post-merge approvedlabel is applied by an active team member.Documentation
/bot runhelp and developer guidance to require the approval label for broad wildcard stage selections, and clarified label persistence across commits.Tests
Description
TRTLLMINF-188 prevents unapproved broad post-merge bot runs from consuming shared CI resources.
This PR:
Guard Post-Merge Approval Labelworkflow for theci: post-merge approvedlabel.NVIDIA/trt-llm-ci-approvers.pull_request_targetwithout checking out or executing PR code.--post-merge,--stage-list "*",--extra-stage "*",--stage-list "*Post-Merge*",--extra-stage "*Post-Merge*", and equivalent escaped/repeated-star or comma-list forms require approval.*PerfSanity*.The membership check uses the repository secret
TRTLLM_AGENT_SHARED_TOKEN. That token must be able to read active membership inNVIDIA/trt-llm-ci-approvers; label cleanup and PR comments continue to use the scopedGITHUB_TOKEN.The Jenkins-side pre-dispatch label gate is implemented in companion internal MR ftp/infra/trtllm-github-ci!86.
The canonical
.github/workflows/blossom-ci.ymlis unchanged.Test Coverage
node .github/scripts/test_post_merge_approval_workflow.jsTRTLLM_AGENT_SHARED_TOKENpositive membership smoke: workflow run 29241460945 returned HTTP 200 and active membership forchzblychinNVIDIA/trt-llm-ci-approverschzblych/TensorRT-LLM:7a52965662566cdadbf92487da472b3cd6beacb1, the label remained; no synchronize-triggered guard run or comment appeared.379031ee7497bec37975d9f56050cd10609a32acb3b2e9a3317714ac15c93a4e.68651377063abe7cdef8b375bf919c48f2c53ef4cfafba4c1828a24786ab020f.f175430b9cbc023c46f6cf6d7d4fb6c9d77fdf462e7f3bbfc94dd10b15d958e1.SHA256SUMS. An initial D driver run failed only in the test audit/cleanup harness after the product Guard terminal assertion passed; the harness was corrected, reviewed, and the accepted D run above passed end to end.issue_comment/Blossom or production downstream run was triggered.git diff --check.github/workflows/blossom-ci.ymlNo production downstream Blossom pipeline was triggered. Jenkins runtime validation used an isolated controller-only
Sandbox/yanchaoljob with synthetic GitHub and downstream-dispatch fixtures.Deployment Prerequisite
TRTLLM_AGENT_SHARED_TOKENwith organization/team-membership read access. The fork-only positive smoke proves the configured token value can perform the required lookup, but it cannot read or attest the upstream repository's secret configuration.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.