Repository navigation
Retry transient slash-command provenance checks - #59313
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis complete: 90/100 Excellent. 2 new tests (retry & provenance) with 100% design contract coverage and 100% error path testing. No violations. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has <=100 new lines of code in business logic directories.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
test
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 23.4 AIC · ⌖ 7.31 AIC · ⊞ 21.8K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix is targeted (retries only transient 5xx/network/rate-limit failures via the existing withRetry/isTransientError utilities) and is backed by two focused regression tests covering both recovery and fail-closed-after-exhaustion.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Reuses the existing
error_recovery.cjsretry utility instead of inventing bespoke retry logic, consistent with usage elsewhere in the codebase (add_labels.cjs,create_check_run.cjs). - ✅
shouldRetrycorrectly combines an explicit 5xx status check withisTransientErrorrather than retrying blindly, preserving fail-closed security semantics on permanent errors (404/403 without rate-limit headers, etc.). - ✅ Regression tests exercise both the happy recovery path (transient 500 then success) and the fail-closed path (exhausted retries →
is_team_member: false,result: api_error) using fake timers, which is the right approach for testing backoff without slowing down the suite.
Minor Suggestion
- One inline comment suggests adding a test for a permanent API failure (e.g. 404) to explicitly lock in the "do not retry permanent failures" claim from the PR description — currently only the transient path is asserted for retry count, so a regression that widens
shouldRetrywouldn't be caught by these tests alone.
No blocking issues found.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 33.4 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "false"); | ||
| expect(mockCore.setOutput).toHaveBeenCalledWith("result", "api_error"); | ||
| } finally { | ||
| vi.useRealTimers(); |
There was a problem hiding this comment.
[/diagnosing-bugs] Both new tests cover the transient-500 path, but the PR description also claims "Do not retry permanent API failures" — that invariant isn't exercised by a test.
💡 Suggested regression test
Add a case where mockGithub.rest.pulls.get rejects with a non-retryable error (e.g. status: 404) and assert it's called exactly once (no retries) and still fails closed with result: "api_error". This locks in the "preserve security behavior" claim and guards against a future change to shouldRetry accidentally widening the retry surface to permanent failures.
it("should not retry a permanent (404) provenance failure", async () => {
const apiError = Object.assign(new Error("Not Found"), { status: 404 });
mockGithub.rest.pulls.get.mockRejectedValue(apiError);
await runScript();
expect(mockGithub.rest.pulls.get).toHaveBeenCalledTimes(1);
expect(mockCore.setOutput).toHaveBeenCalledWith("result", "api_error");
});@copilot please address this.
There was a problem hiding this comment.
Added the requested 404 provenance regression test in actions/setup/js/check_membership.test.cjs. It asserts pulls.get is called once and the check fails closed with result: "api_error"; targeted tests pass.
Test Quality Sentinel ReportPR #59313 — Retry transient slash-command provenance checks Score: 90/100 ✅ ExcellentSummary: Added 2 behavioral tests covering critical security contracts for retry resilience and fail-closed behavior. Both tests are high-value design tests with 100% edge-case coverage. No violations detected. Details:
Metrics:
Status: Approve Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Reviewed with Impeccable harden/audit lenses (bug-fix / error-handling change).
Findings: No blocking issues.
- Retry is correctly scoped to the PR provenance lookup only (
github.rest.pulls.get), with a customshouldRetrythat retries 5xx/transient errors but leaves 4xx (e.g. 404 missing PR) to fail immediately — preserving fail-closed behavior for permanent errors. - Worst-case added latency is bounded (~3 retries, initialDelay 2500ms, backoff x2, capped at 30000ms) — acceptable for a pre-activation gate step.
- Test coverage validates both the retry-then-success path (2 calls) and retry-exhaustion fail-closed path (4 calls →
api_error), matchingmaxRetries: 3. - Test harness correctly loads the real
error_recovery.cjsviacreateRequiresowithRetry/isTransientErrorbehavior isn't mocked away, keeping the test meaningful.
Nothing actionable to request changes on.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 35.8 AIC · ⌖ 13.2 AIC · ⊞ 8.3K
There was a problem hiding this comment.
🟡 Changes recommended
Permanent API failure behavior needs direct regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds bounded retries to centralized slash-command PR provenance checks while preserving fail-closed authorization behavior.
Changes:
- Retries transient GitHub API failures with exponential backoff.
- Adds recovery and retry-exhaustion tests.
File summaries
| File | Description |
|---|---|
actions/setup/js/check_membership.cjs |
Applies retry handling to provenance lookup. |
actions/setup/js/check_membership.test.cjs |
Tests transient recovery and exhaustion. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
@copilot Please take the next forward-progress pass on PR #59313.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Centralized slash-command workflows treated transient GitHub API failures during PR provenance checks as authorization denials, silently skipping downstream execution and leaving status comments unresolved.
Run: https://github.com/github/gh-aw/actions/runs/34178092363