fix(ci): retry transient maintainer workflow approvals - #8168
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe maintainer approval workflow now checks out its helper from the pull request’s exact base SHA. It validates checkout provenance and invokes only the verified helper. Static tests enforce trusted checkout, workflow contracts, pinned actions, and rejection of head-code execution. ChangesTrusted maintainer approval execution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ApprovalWorkflow
participant GitHubCheckout
participant TrustedHelper
ApprovalWorkflow->>GitHubCheckout: Checkout the exact pull request base SHA
GitHubCheckout-->>ApprovalWorkflow: Return the verified helper file
ApprovalWorkflow->>TrustedHelper: Import and validate the exported entry point
ApprovalWorkflow->>TrustedHelper: Invoke approveMaintainerPrWorkflowRuns
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit c5aab66 in the TypeScript / code-coverage/cliThe overall coverage in commit c5aab66 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/approve-maintainer-pr-workflow-runs.yaml (1)
439-441: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExit polling gracefully when the script budget ends.
waitWithinScriptBudgetthrows when the remaining budget is not greater thanPOLL_INTERVAL_MS. Polling itself consumes 60000ms of the 105000ms budget, so slow API attempts reach this path. The job then reports failure and skips the summary log, even though every discovered run was already approved.Budget exhaustion during discovery is not an authority failure or a reconciliation failure. Stop polling and report the summary instead.
♻️ Proposed change to stop polling within budget
if (attempt + 1 < POLL_ATTEMPTS) { - await waitWithinScriptBudget('Workflow-run polling', POLL_INTERVAL_MS); + if (remainingScriptBudgetMs() <= POLL_INTERVAL_MS) { + core.warning( + `Workflow-run polling stopped after ${attempt + 1}/${POLL_ATTEMPTS} attempts; the bounded ${SCRIPT_BUDGET_MS}ms script budget ended`, + ); + break; + } + await waitWithinScriptBudget('Workflow-run polling', POLL_INTERVAL_MS); }Add a test that exhausts the budget during polling, because no current case covers this branch.
🤖 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/approve-maintainer-pr-workflow-runs.yaml around lines 439 - 441, Update the polling loop around waitWithinScriptBudget so budget exhaustion during discovery is handled as a graceful stop rather than a failure: catch the expected budget-exhaustion error, exit polling, and continue to the existing summary/reporting path while preserving genuine API and reconciliation failures. Add coverage for a polling attempt that exhausts the script budget and verifies the summary is still emitted.test/maintainer-pr-workflow-approval.test.ts (1)
261-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the redundant source-text assertions.
Lines 261-267 assert exact declaration text. The behavior tests at lines 328-556 already prove the same facts through the mocked API boundary: retry counts, 250ms and 500ms backoff, a 10000ms request timeout, per-attempt signals, and
getWorkflowRunreconciliation. These text assertions add no confidence and break on a rename or a reformat.Keep lines 268-269 as removal proof for the superseded
request: { timeout: ... }path. Keep lines 270-272, because the budget-versus-timeout-minutesinvariant is not observable through behavior.♻️ Proposed reduction
- expect(script).toContain("const API_RETRY_ATTEMPTS = 3;"); - expect(script).toContain("const API_RETRY_BASE_DELAY_MS = 250;"); - expect(script).toContain("const API_RETRY_MAX_DELAY_MS = 1000;"); - expect(script).toContain("const API_REQUEST_TIMEOUT_MS = 10000;"); - expect(script).toContain("const SCRIPT_BUDGET_MS = 105000;"); - expect(script).toContain("AbortSignal.timeout"); - expect(script).toContain("github.rest.actions.getWorkflowRun"); expect(script).not.toContain("requestTimeoutFor");Based on path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 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 `@test/maintainer-pr-workflow-approval.test.ts` around lines 261 - 267, Remove the redundant exact source-text assertions for API retry constants, request timeout, AbortSignal.timeout, and getWorkflowRun. In the affected test, retain the assertions covering removal of the superseded request timeout path and the budget-versus-timeout-minutes invariant, leaving behavioral coverage to validate the runtime values and reconciliation behavior.Source: Path instructions
🤖 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.
Nitpick comments:
In @.github/workflows/approve-maintainer-pr-workflow-runs.yaml:
- Around line 439-441: Update the polling loop around waitWithinScriptBudget so
budget exhaustion during discovery is handled as a graceful stop rather than a
failure: catch the expected budget-exhaustion error, exit polling, and continue
to the existing summary/reporting path while preserving genuine API and
reconciliation failures. Add coverage for a polling attempt that exhausts the
script budget and verifies the summary is still emitted.
In `@test/maintainer-pr-workflow-approval.test.ts`:
- Around line 261-267: Remove the redundant exact source-text assertions for API
retry constants, request timeout, AbortSignal.timeout, and getWorkflowRun. In
the affected test, retain the assertions covering removal of the superseded
request timeout path and the budget-versus-timeout-minutes invariant, leaving
behavioral coverage to validate the runtime values and reconciliation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: effba14b-ab2a-49cc-91fb-c43a4491c6d3
📒 Files selected for processing (2)
.github/workflows/approve-maintainer-pr-workflow-runs.yamltest/maintainer-pr-workflow-approval.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Changes requested for exact head 248c3dc6e0f896ad2581d1c513b0295405525303.
The retry and reconciliation behavior is useful, but this implementation reverses the executable-workflow extraction pattern completed in #7719 and #7741. .github/workflows/approve-maintainer-pr-workflow-runs.yaml grows from 259 to 464 lines and adds about 200 lines of executable JavaScript inside actions/github-script. This is an actions: write pull_request_target boundary, so the implementation needs a separately reviewable and directly testable trusted source rather than a large program embedded in YAML.
The existing comment says the code must remain inline to avoid checking out pull-request code. That trust requirement is valid, but #7719/#7741 already established the repository pattern for solving it:
- Land the helper separately so it exists on a trusted
mainrevision. - In the consumer workflow, sparse-check out only that helper from the PR base SHA or workflow SHA before any candidate checkout.
- Fail closed when the trusted helper is missing or its provenance is wrong.
- Keep workflow YAML limited to permissions, inputs, trusted bootstrap, and helper invocation.
Please move the retry, timeout, pagination, mutable-authority revalidation, exact-run reconciliation, and approval logic into a TypeScript helper or trusted local action. Keep the current fail-closed behavior and adversarial coverage. Tests should exercise the helper directly and separately assert that the workflow loads only the trusted helper revision.
Because the helper does not yet exist on this PR's base, this likely needs the same two-PR bootstrap and adoption sequence used by #7719 and #7741. Do not execute the helper from the candidate checkout.
wscurran
added
area: ci
<!-- markdownlint-disable MD041 --> ## Summary Add the trusted, directly tested approval helper required before PR #8168 can remove its large inline program from the write-capable workflow. This first stage is dormant: the current workflow does not load the helper, so repository behavior does not change. ## Related Issue Prerequisite for #8168. Related to #7744. ## Changes - Move PR #8168's bounded retry, request timeout, exact-run reconciliation, and mutable-authority validation behavior into `tools/ci/approve-maintainer-pr-workflow-runs.mts`. - Add 28 direct behavioral tests for transient and terminal API errors, hung requests, script-budget exhaustion, exact PR and run identity, permission changes, head movement, external repositories, and ambiguous approval responses. - Preserve Aaron Erickson's transferred contribution with a `Co-authored-by` trailer using the source commit identity. - Leave the consumer workflow unchanged until this helper exists on a trusted `main` revision. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This adds a dormant internal CI helper and direct tests. No workflow consumes it, and no public or contributor-facing behavior changes. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Pi completed the nine-category review at `0d25e53b2` with no findings. The helper validates exact PR, repository, author permission, head SHA, workflow-run identity, pagination, retry, timeout, and reconciliation state. It has no workflow consumer, permissions, checkout, secret access, dependency, or network path beyond the injected GitHub API client. Direct adversarial tests cover each fail-closed boundary. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Independent Pi CLI review confirmed that only the direct test imports the dormant helper. The existing workflow is unchanged, so no API, CLI, configuration, UI, workflow, supported behavior, or agent variant changes. - Agent: Pi CLI <!-- docs-review-head-sha: 0d25e53 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project integration test/maintainer-pr-workflow-approval-helper.test.ts` passed 28 tests after the current-main refresh. `npm run typecheck:cli`, Biome format/lint, repository checks, diff checks, and NUL-byte checks passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated approval handling for eligible pull-request workflow runs, with validation of commit, repository, author, and permissions. * Added safeguards that stop approvals when pull-request details or authorization change. * **Reliability Improvements** * Added bounded polling, timeouts, transient retries, and final status verification for more dependable workflow approvals. * Added comprehensive coverage for approval, timeout, retry, and invalid-response scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
Replace the write-capable inline program with exact-base helper checkout. Validate provenance before invoking the helper. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Use direct trust-boundary assertions so the workflow contract remains within the conditional budget. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/maintainer-pr-workflow-approval.test.ts (2)
80-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative cases for the two bash guard assertions.
Lines 80-113 assert that the validate step contains the base-SHA regex guard and the unused-root guard, and that the verify step contains the
rev-parseprovenance check and the symlink guard. Theitblock mutates only the checkoutref, the sparse-checkout pattern, and the script body. No mutation removes a bash guard, so the test never proves those four conditions can fail.Add mutations that strip each
runguard. The mutations keep the source-text assertions load-bearing, which the path instructions require for tests that assert on implementation text.💚 Proposed additional negative cases
const inlineApi = workflowFixture(); requireStep(inlineApi.jobs.approve!, "Approve exact-head maintainer workflow runs") .with!.script = "await github.rest.actions.approveWorkflowRun({ run_id: 1 });"; expect(trustedBootstrapErrors(inlineApi)).toContain( "workflow must invoke only the trusted helper through the pinned GitHub Script action", ); + + const missingShaGuard = workflowFixture(); + const validateStep = requireStep( + missingShaGuard.jobs.approve!, + "Validate trusted helper revision", + ) as { run?: string }; + validateStep.run = 'set -euo pipefail\necho "no guard"\n'; + expect(trustedBootstrapErrors(missingShaGuard)).toContain( + "workflow must validate the base SHA and unused checkout root before checkout", + ); + + const missingProvenanceGuard = workflowFixture(); + const verifyStep = requireStep( + missingProvenanceGuard.jobs.approve!, + "Verify trusted approval helper", + ) as { run?: string }; + verifyStep.run = 'set -euo pipefail\ntest -e "$TRUSTED_HELPER_ROOT"\n'; + expect(trustedBootstrapErrors(missingProvenanceGuard)).toContain( + "workflow must verify helper provenance and file type before execution", + );As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
Also applies to: 136-162
🤖 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 `@test/maintainer-pr-workflow-approval.test.ts` around lines 80 - 113, The test’s negative cases do not remove any of the four bash guards asserted in the validate and verify steps. Extend the relevant it block to mutate the workflow run text by stripping the base-SHA regex guard, unused checkout-root guard, rev-parse provenance check, and symlink guard individually, while preserving the existing source-text assertions so each mutation fails for the intended condition.Source: Path instructions
39-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the mapping and event-type comparisons order-insensitive.
Lines 39-62 compare parsed YAML mappings with
JSON.stringifydeep equality.JSON.stringifyoutput depends on key insertion order, and the YAML parser preserves the order of keys in the workflow file. A maintainer who reordersactions,contents, andpull-requestsunderpermissions, or reorders thetypeslist, gets the failure "workflow must keep its least-privilege permission contract" for a workflow whose permission set did not change.Compare the sets instead. Keep strict order comparison only where order is a real contract, which is the step-name list at Lines 68-78.
♻️ Proposed order-insensitive comparison
+function sameEntries(actual: unknown, expected: Record<string, unknown>): boolean { + const value = (actual ?? {}) as Record<string, unknown>; + const sort = (entries: [string, unknown][]) => + [...entries].sort(([a], [b]) => a.localeCompare(b)); + return ( + JSON.stringify(sort(Object.entries(value))) === JSON.stringify(sort(Object.entries(expected))) + ); +} + function trustedBootstrapErrors(workflow: ApprovalWorkflow): string[] { const errors: string[] = []; const job = workflow.jobs.approve; if (!job) return ["workflow is missing approve job"]; if ( - JSON.stringify(workflow.on?.pull_request_target) !== - JSON.stringify({ - types: ["opened", "synchronize", "reopened", "edited", "ready_for_review"], - }) + JSON.stringify([...(workflow.on?.pull_request_target?.types ?? [])].sort()) !== + JSON.stringify( + ["opened", "synchronize", "reopened", "edited", "ready_for_review"].sort(), + ) ) { errors.push("workflow must keep the pull_request_target event contract"); } - if ( - JSON.stringify(workflow.permissions) !== - JSON.stringify({ actions: "write", contents: "read", "pull-requests": "read" }) - ) { + if (!sameEntries(workflow.permissions, { actions: "write", contents: "read", "pull-requests": "read" })) { errors.push("workflow must keep its least-privilege permission contract"); } - if ( - JSON.stringify(workflow.concurrency) !== - JSON.stringify({ - group: - "approve-maintainer-pr-workflow-runs-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }}", - "cancel-in-progress": false, - }) - ) { + if ( + !sameEntries(workflow.concurrency, { + group: + "approve-maintainer-pr-workflow-runs-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }}", + "cancel-in-progress": false, + }) + ) { errors.push("workflow must keep exact-head non-cancelling concurrency"); }🤖 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 `@test/maintainer-pr-workflow-approval.test.ts` around lines 39 - 62, Update the assertions for pull_request_target types and permissions in the workflow contract test to compare order-insensitively: treat event types as an unordered set and compare permission mappings by keys and values regardless of YAML insertion order. Preserve exact equality for concurrency and the existing strict ordering for the step-name list.
🤖 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.
Nitpick comments:
In `@test/maintainer-pr-workflow-approval.test.ts`:
- Around line 80-113: The test’s negative cases do not remove any of the four
bash guards asserted in the validate and verify steps. Extend the relevant it
block to mutate the workflow run text by stripping the base-SHA regex guard,
unused checkout-root guard, rev-parse provenance check, and symlink guard
individually, while preserving the existing source-text assertions so each
mutation fails for the intended condition.
- Around line 39-62: Update the assertions for pull_request_target types and
permissions in the workflow contract test to compare order-insensitively: treat
event types as an unordered set and compare permission mappings by keys and
values regardless of YAML insertion order. Preserve exact equality for
concurrency and the existing strict ordering for the step-name list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aac6cdb2-3be6-4906-aa45-5beebf3e652f
📒 Files selected for processing (2)
.github/workflows/approve-maintainer-pr-workflow-runs.yamltest/maintainer-pr-workflow-approval.test.ts
Addressed at c5aab66. PR #8400 first landed the directly tested helper on main. This PR now sparse-checks out only that helper from the exact PR base SHA, disables credential persistence, verifies checkout provenance and file type, and invokes it through pinned GitHub Script. The workflow shrank from 207 to 39 executable lines, direct helper and workflow-boundary tests pass, and current security and documentation reviews report no findings. Final approval remains subject to current-head CI and merge gates.
5 participants
Summary
Retry transient GitHub API failures in the trusted maintainer workflow-run approval job. The write-capable workflow now loads the directly tested helper from the exact PR base SHA, verifies its provenance, and invokes it without checking out or executing PR-head code.
Related Issue
Related to #7744. Uses the helper bootstrapped by #8400.
Changes
actions: writeworkflow YAML limited to permissions, trusted bootstrap, provenance verification, and helper invocation through pinned actions.Type of Change
Quality Gates
23fec2d2dwith no findings. The workflow keeps least privilege, uses pinned checkout and GitHub Script actions, validates a lowercase exact base SHA, checks out onlytools/ci/approve-maintainer-pr-workflow-runs.mts, disables credential persistence, verifies checkout provenance and regular-file type, and never checks out PR-head code. The helper retains exact PR, repository, author permission, head SHA, workflow-run identity, retry, timeout, pagination, and reconciliation validation. Direct and workflow mutation tests cover these fail-closed boundaries.Documentation Writer Review
no-docs-neededc5aab6646confirmed that the change only affects the internal trusted-helper implementation boundary. No CLI, API, configuration, contributor procedure, approval policy, agent variant, code sample, or support claim changes.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project integration test/maintainer-pr-workflow-approval.test.ts test/maintainer-pr-workflow-approval-helper.test.tspassed 29 tests.npm run typecheck:cli,npm run checks:repository,npm run source-shape:check, andnpm run test-conditionals:scan --top 25passed. Normal hooks passed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests