fix: remove label filter from issue dedup query#1180
Conversation
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for the fix — the root cause analysis is correct. I have one concern about the new dedup query that I think should be addressed before merging.
The repo currently has ~85 open issues and ~44 open PRs (≈129 total). Since listForRepo returns issues and PRs (PRs count as issues in this endpoint) and this call fetches only the first page with per_page: 100, the CI failure issue being deduped against will fall off the page as soon as there are >100 newer open issues/PRs. The previous labels: 'high-severity,ci' filter kept the result set small enough that a single page was always sufficient; removing it makes the existing single-page limitation a real correctness problem right now, not a theoretical one.
See the inline comment for options.
f7e45c1 to
2ec1065
Compare
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
The switch to the search API addresses the >100-issues pagination concern from my previous review — thanks for iterating on this.
I have one remaining concern: the GitHub search API has a well-known indexing delay (typically seconds to a minute or more) between when an issue is created and when it becomes findable via search. This reintroduces a duplicate-issue risk that the previous listForRepo approach did not have, because listForRepo reads from the primary datastore and reflects newly-created issues immediately.
See inline comment for details and a couple of options.
2ec1065 to
ee8c7b6
Compare
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
The hybrid approach addresses both previous concerns cleanly:
- The search API handles the general case efficiently regardless of repo size, solving the >100 issues/PRs pagination problem from the first review.
- The
listForRepofallback sorted bycreated descwithper_page: 30covers the search-indexing-delay race window from the second review — a freshly-created issue will be at the top of the recently-created list, well within 30 items even accounting for concurrent PR activity.
The exact-title check i.title === title on both paths correctly compensates for the substring behavior of search's in:title and also makes the fallback robust against PRs with similar titles (since listForRepo returns both issues and PRs).
workflowName is validated against /^[A-Za-z0-9 _()\-]+$/ before being interpolated into the search query, so no injection concern.
LGTM.
Description
The dedup logic in
ci-failure-issue.ymlfiltered by labels (high-severity,ci) when checking for existing issues vialistForRepo. If the token creating the issue does not have permission to apply labels, the issue is created without labels. On subsequent runs, the dedup query cannot find it (because it filters on labels that were never applied), causing duplicate issues.This PR removes the
labelsparameter from thelistForRepocall so dedup relies solely on title matching.Related Issue
N/A — discovered during CI workflow re-runs.
Documentation PR
N/A
Type of Change
Testing
Re-run the CI failure workflow and verify it does not create a duplicate issue when one already exists (regardless of whether labels were applied).
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsNo source code changes — workflow-only fix. No unit/integ tests apply.
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.