Version issue-creation REST calls in handle_agent_failure#38017
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request updates the failure-reporting path in handle_agent_failure to explicitly version GitHub REST API calls when creating issues, eliminating deprecation warnings caused by unversioned Octokit requests.
Changes:
- Added a
GITHUB_API_VERSIONconstant (2022-11-28) to the handler module. - Plumbed
headers: { "X-GitHub-Api-Version": GITHUB_API_VERSION }into allgithub.rest.issues.create(...)call sites inhandle_agent_failure.cjs. - Extended unit tests to assert that the API version header is included for representative issue-creation paths (including cascade rollup creation).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/handle_agent_failure.cjs | Adds an API version constant and includes the version header in all issue creation REST calls in the failure handler. |
| actions/setup/js/handle_agent_failure.test.cjs | Adds assertions verifying the version header is passed through on issue creation calls (including cascade rollup). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38017 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (well under the 100-line threshold). Neither Condition A nor Condition B is met. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on test coverage gaps across the four patched call sites.
📋 Key Themes & Highlights
Key Themes
- Incomplete test coverage: 2 of 4 patched
issues.createcall sites (parent issue creation and daily-cap rollup) have no assertion verifying that the newX-GitHub-Api-Versionheader is passed. The PR description acknowledges this as "representative" coverage, but full coverage would close the regression surface completely. - Minor style inconsistency: the two added assertions use different patterns (
toHaveBeenCalledWith(expect.objectContaining(...))vs direct property access on the extracted call object).
Positive Highlights
- ✅ Extracting
GITHUB_API_VERSIONas a module-level constant is clean — a single future version bump rather than four scattered string updates. - ✅ The fix is purely additive and scoped tightly; no existing logic was mutated.
- ✅ The cascade rollup header assertion at line 3168 correctly validates the propagated value.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 135.7 AIC · ⌖ 13.3 AIC
| expect.objectContaining({ | ||
| headers: { "X-GitHub-Api-Version": "2022-11-28" }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
[/tdd] This assertion covers one of the four patched call sites, leaving two paths — parent issue creation (findOrCreateParentIssue, impl line ~575) and daily-cap rollup (findOrCreateDailyCapRollupIssue, impl line ~2138) — without header assertions. If either path accidentally drops the X-GitHub-Api-Version header in a future refactor, no test will catch it.
💡 Suggested additions
In the test for the parent issue creation path:
expect(createIssueMock).toHaveBeenCalledWith(
expect.objectContaining({
headers: { "X-GitHub-Api-Version": "2022-11-28" },
})
);And similarly in the findOrCreateDailyCapRollupIssue test block. The PR description says coverage was added for "representative" paths, but since the fix touches four distinct call sites, all four warrant explicit assertions to fully close the regression surface.
There was a problem hiding this comment.
Addressed in 6c00eee. Added explicit header coverage for the missing parent-issue and daily-cap create paths in actions/setup/js/handle_agent_failure.test.cjs.
| expect(createCall.title).toBe(CASCADE_ROLLUP_TITLE); | ||
| expect(createCall.labels).toContain(CASCADE_ROLLUP_LABEL); | ||
| expect(createCall.labels).toContain("agentic-workflows"); | ||
| expect(createCall.headers).toEqual({ "X-GitHub-Api-Version": "2022-11-28" }); |
There was a problem hiding this comment.
[/tdd] Minor style inconsistency: this assertion uses direct property access (createCall.headers), while the companion assertion at line 428 uses toHaveBeenCalledWith(expect.objectContaining(...)). Both are functionally correct, but aligning on one pattern across all four call-site tests would improve readability and make the intent — verify header presence on every issues.create invocation — more scannable.
💡 Consistent pattern
If the test already has access to the call arguments via createCall, the direct style used here (expect(createCall.headers).toEqual(...)) is actually more readable. Consider applying it to the test at line 428 too, replacing the toHaveBeenCalledWith(expect.objectContaining(...)) form with the same destructured-call pattern for uniformity.
There was a problem hiding this comment.
Addressed in 6c00eee. The header assertions in actions/setup/js/handle_agent_failure.test.cjs now use the same direct-call pattern for readability.
There was a problem hiding this comment.
Non-blocking feedback on two test coverage gaps.
### Findings summary
2 medium gaps — both non-blocking
-
ensureParentIssuecreate path is untested (handle_agent_failure.cjs:575): Every existing test mockssearch.issuesAndPullRequeststo return a pre-existing parent issue, so theissues.createbranch insideensureParentIssueis never reached. The header added here has no test coverage at all. -
findOrCreateDailyCapRollupIssueheader not asserted (handle_agent_failure.test.cjs:426): The daily-cap rollup test (line 743) exercises this callsite and checks the issuetitle, but skips theheadersfield. A silent regression is possible.
The implementation change itself is correct — all four issues.create callsites are updated consistently. The two informational inline comments above suggest minimal additions to close the test gaps.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 13.3 AIC
| expect(createIssueMock).toHaveBeenCalledOnce(); | ||
| expect(createIssueMock).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| headers: { "X-GitHub-Api-Version": "2022-11-28" }, |
There was a problem hiding this comment.
Missing header assertion for the findOrCreateDailyCapRollupIssue create path: the existing daily-cap test (around line 743) exercises findOrCreateDailyCapRollupIssue and asserts on the issue title, but does not check headers. A regression that removes the version header from that callsite would go undetected.
💡 Suggested addition
In the daily-cap test block (the one that asserts title: "[aw] Daily failure issue cap exceeded"), add:
expect(createIssueMock).toHaveBeenCalledWith(
expect.objectContaining({
headers: { "X-GitHub-Api-Version": "2022-11-28" },
})
);This mirrors the pattern already used at line 424 for the main issue creation path.
There was a problem hiding this comment.
Addressed in 6c00eee. The daily-cap rollup test now asserts the X-GitHub-Api-Version header on the create call.
| title: parentTitle, | ||
| body: parentBody, | ||
| labels: [parentLabel], | ||
| headers: { "X-GitHub-Api-Version": GITHUB_API_VERSION }, |
There was a problem hiding this comment.
ensureParentIssue create path has zero header test coverage: the header is added here, but no test ever reaches this branch — every existing test mocks search.issuesAndPullRequests to return an existing parent issue, which causes ensureParentIssue to short-circuit before reaching issues.create. The header change at this callsite is functionally unverified.
💡 Suggested fix
Add a test where the search mock returns an empty result for the parent-issue query, forcing ensureParentIssue to hit the create path, and assert:
expect(createIssueMock).toHaveBeenCalledWith(
expect.objectContaining({
headers: { "X-GitHub-Api-Version": "2022-11-28" },
})
);A minimal setup is a search mock that returns { total_count: 0, items: [] } only when the query contains "[aw] Failed runs".
There was a problem hiding this comment.
Addressed in 6c00eee. Added a test that enables grouped reports, forces the [aw] Failed runs create path, and verifies the version header on that parent issue creation.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). Both modified tests enforce observable API-versioning contracts against the GitHub REST interface. See the detailed report in the comment above for improvement suggestions.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The failure-reporting path in
handle_agent_failurewas emitting unversioned Octokit REST calls (e.g.POST /repos/{owner}/{repo}/issues), which now surface deprecation warnings. This change updates the generated calls to use the current API-versioned contract.Problem scope
actions/setup/js/handle_agent_failure.cjsusedgithub.rest.issues.create(...)without explicit API version headers, leading to deprecated endpoint usage warnings in workflow runs.Code changes
GITHUB_API_VERSIONconstant (2022-11-28).headers: { "X-GitHub-Api-Version": GITHUB_API_VERSION }to all issue-creation call sites in this module:Test updates
handle_agent_failure.test.cjsassertions to verify header propagation on representativeissues.createcall paths (including cascade rollup creation).