Skip to content

Disable reopen wontfix#234

Open
kzhou314 wants to merge 4 commits into
mainfrom
disable-reopen-wontfix
Open

Disable reopen wontfix#234
kzhou314 wants to merge 4 commits into
mainfrom
disable-reopen-wontfix

Conversation

@kzhou314

Copy link
Copy Markdown

Closes https://github.com/github/accessibility/issues/10756.

Closing a scanner-filed issue without merging a fix used to cause it to reopen on the next run. Adding the wontfix label now keeps it closed

@kzhou314 kzhou314 requested a review from a team as a code owner June 20, 2026 00:06
GitHub Advanced Security started work on behalf of kzhou314 June 20, 2026 00:06 View session
GitHub Advanced Security finished work on behalf of kzhou314 June 20, 2026 00:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the “file” GitHub Action so that previously-closed scanner-filed issues are not reopened on subsequent runs when the issue is labeled wontfix, and documents the behavior for users.

Changes:

  • Add isWontfixIssue() helper and integrate it into the repeated-filing (reopen) path so wontfix issues stay closed.
  • Add Vitest coverage for the new label-check behavior, plus regression adjustments to existing tests.
  • Document how to keep an issue closed using the wontfix label.
Show a summary per file
File Description
README.md Documents the wontfix label behavior for preventing reopen.
.github/actions/file/src/index.ts Skips reopening repeated filings when the issue is labeled wontfix.
.github/actions/file/src/isWontfixIssue.ts New helper to fetch issue labels and detect wontfix.
.github/actions/file/tests/wontfixReopen.test.ts New end-to-end-ish test ensuring wontfix issues are not reopened.
.github/actions/file/tests/isWontfixIssue.test.ts Unit tests for label detection and request routing.
.github/actions/file/tests/dryRun.test.ts Updates regression guard to account for the new label-check GET request when not in dry-run.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 6/6 changed files
  • Comments generated: 1

Comment thread .github/actions/file/src/index.ts
GitHub Advanced Security started work on behalf of kzhou314 June 20, 2026 00:10 View session
GitHub Advanced Security finished work on behalf of kzhou314 June 20, 2026 00:12
GitHub Advanced Security started work on behalf of kzhou314 June 20, 2026 00:15 View session
GitHub Advanced Security finished work on behalf of kzhou314 June 20, 2026 00:16
@kzhou314 kzhou314 requested a review from JoyceZhu June 20, 2026 00:21
repository,
issue_number: issueNumber,
})
const labels = ((response.data as {labels?: IssueLabel[]}).labels ?? []) as IssueLabel[]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if TS will let you do this, shoving these labels/label names in a set so the containment check is constant time would be nice. (some repos / organizations really pile on the labels!)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! Correct me if I'm wrong, but I thought converting an array to a set in TS would be O(n) time, which would asymptotically be the same as iterating through the array to find the correct label. Since we only do a single containment check here, I'm not sure if the set has much benefit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's basically O(n) vs O(n) + O(n) simplifying to O(n) but you go through twice, and that'll further scale per-filing since I believe this is called in an outer loop for every filing


type IssueLabel = string | {name?: string}

export async function isWontfixIssue(octokit: Octokit, {owner, repository, issueNumber}: Issue): Promise<boolean> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about naming here; it might be more maintainable to call isWontfixIssue a more generic shouldReopenIssue or something similar. The latter expresses the intent of this feature more broadly. Right now its implementation is totally coupled to the existence of a wontfix label, but e.g. if we add other supported labels or change the fingerprinting algorithm (deciding if a finding is effectively a duplicate of another one), there won't be logic unrelated to wontfix at all also buried inside isWontfixIssue.

@JoyceZhu

Copy link
Copy Markdown
Contributor

🤔 I wonder if instead of making one API request per issue, it would work to attempt to grab all wontfix issue IDs in the repository in a single request (based on these docs we'd need to say "issues with a closed state and the label wontfix"), then we can just do a quick set check.

Another possibility: the repository in question may not even have wontfix as a possible label, in which case we're unnecessarily sending additional network traffic. Perhaps checking if the label even exists for the repository could help us avoid continuing with requests which won't do anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants