-
Notifications
You must be signed in to change notification settings - Fork 427
Handle failure-issue permission denials and enforce timeout-specific failure messaging #38273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0de3812
bded792
2a8d51d
91015c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1462,6 +1462,47 @@ describe("handle_agent_failure", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("shouldBuildEngineFailureContext", () => { | ||
| const { shouldBuildEngineFailureContext } = require("./handle_agent_failure.cjs"); | ||
|
|
||
| it("returns true for plain failure without timeout or tool-denials-exceeded", () => { | ||
| expect(shouldBuildEngineFailureContext("failure", false, false)).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false when timeout is detected", () => { | ||
| expect(shouldBuildEngineFailureContext("failure", false, true)).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when tool-denials-exceeded is present", () => { | ||
| expect(shouldBuildEngineFailureContext("failure", true, false)).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false for non-failure conclusions", () => { | ||
| expect(shouldBuildEngineFailureContext("timed_out", false, true)).toBe(false); | ||
| expect(shouldBuildEngineFailureContext("success", false, false)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("isIssueWritePermissionError", () => { | ||
| const { isIssueWritePermissionError } = require("./handle_agent_failure.cjs"); | ||
|
|
||
| it("returns true for 403 Resource not accessible by integration", () => { | ||
| expect(isIssueWritePermissionError({ status: 403, message: "Resource not accessible by integration" })).toBe(true); | ||
| }); | ||
|
|
||
| it("returns true for 403 insufficient permissions", () => { | ||
| expect(isIssueWritePermissionError({ status: 403, message: "Insufficient permissions to create issue" })).toBe(true); | ||
| }); | ||
|
|
||
| it("returns true for 403 resource not accessible by personal access token", () => { | ||
| expect(isIssueWritePermissionError({ status: 403, message: "Resource not accessible by personal access token" })).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false for non-403 errors", () => { | ||
| expect(isIssueWritePermissionError({ status: 500, message: "Internal server error" })).toBe(false); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test for "resource not accessible by personal access token" message pattern: 💡 Suggested fixAdd a third test case in the same it("returns true for 403 resource not accessible by personal access token", () => {
expect(
isIssueWritePermissionError({ status: 403, message: "Resource not accessible by personal access token" })
).toBe(true);
});All three listed patterns should be covered so a future edit to the match list produces a test failure rather than a silent regression. |
||
| }); | ||
|
|
||
| // ────────────────────────────────────────────────────── | ||
| // buildEngineFailureContext | ||
| // ────────────────────────────────────────────────────── | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overly broad
"insufficient permissions"match string: this substring appears in many GitHub API 403 responses (labeling, sub-issue linking, repo content writes, etc.), not only issue write failures. BecauseisIssueWritePermissionErroris now exported, a future caller in a different catch block could inadvertently silence warnings for unrelated permission errors.💡 Suggested fix
Either:
"insufficient scopes"is more precise for that class of error, though you should verify the exact strings returned by your Octokit version), orisTokenPermissionErrorand document in its JSDoc that it is deliberately broad across all 403 permission flavors so future callers understand the intent.If the function is truly meant to be a general-purpose 403-classifier for any GitHub write, the current name
isIssueWritePermissionErroris misleading; callers will be surprised when it also matches a 403 on a label API call.