-
Notifications
You must be signed in to change notification settings - Fork 21.7k
fix(scripts): break pagination when page is not full, not only when empty #68673
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
e3d06ed
a90f11d
4360705
5d0d51a
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 |
|---|---|---|
|
|
@@ -42,11 +42,14 @@ def main(): | |
| # For PreToolUse, we use tool_name to determine "bash" vs "file" event | ||
| tool_name = input_data.get('tool_name', '') | ||
|
|
||
| event = None | ||
| if tool_name == 'Bash': | ||
| event = 'bash' | ||
| elif tool_name in ['Edit', 'Write', 'MultiEdit']: | ||
| event = 'file' | ||
| else: | ||
| # Unknown tool: only evaluate event=all rules to avoid | ||
| # bash/file-specific rules running on unrelated tools. | ||
| event = 'all' | ||
|
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. What does |
||
|
|
||
| # Load rules | ||
| rules = load_rules(event=event) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,19 @@ async function githubRequest<T>( | |
|
|
||
| // -- | ||
|
|
||
| async function fetchAllPages<T>(baseEndpoint: string): Promise<T[]> { | ||
| const all: T[] = []; | ||
| for (let page = 1; ; page++) { | ||
| const sep = baseEndpoint.includes("?") ? "&" : "?"; | ||
| const items = await githubRequest<T[]>(`${baseEndpoint}${sep}page=${page}`); | ||
| all.push(...items); | ||
| if (items.length < 100) break; | ||
|
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. The page size is hardcoded to 100 here, but async function fetchAllPages<T>(baseEndpoint: string, perPage = 100): Promise<T[]> {
...
if (items.length < perPage) break;
}This makes the contract explicit and safe to reuse. |
||
| } | ||
| return all; | ||
| } | ||
|
|
||
| // -- | ||
|
|
||
| async function markStale(owner: string, repo: string) { | ||
| const staleDays = lifecycle.find((l) => l.label === "stale")!.days; | ||
| const cutoff = new Date(); | ||
|
|
@@ -55,7 +68,7 @@ async function markStale(owner: string, repo: string) { | |
| const issues = await githubRequest<any[]>( | ||
| `/repos/${owner}/${repo}/issues?state=open&sort=updated&direction=asc&per_page=100&page=${page}` | ||
| ); | ||
| if (issues.length === 0) break; | ||
| if (issues.length < 100) break; | ||
|
|
||
| for (const issue of issues) { | ||
| if (issue.pull_request) continue; | ||
|
|
@@ -101,7 +114,7 @@ async function closeExpired(owner: string, repo: string) { | |
| const issues = await githubRequest<any[]>( | ||
| `/repos/${owner}/${repo}/issues?state=open&labels=${label}&sort=updated&direction=asc&per_page=100&page=${page}` | ||
| ); | ||
| if (issues.length === 0) break; | ||
| if (issues.length < 100) break; | ||
|
|
||
| for (const issue of issues) { | ||
| if (issue.pull_request) continue; | ||
|
|
@@ -112,7 +125,7 @@ async function closeExpired(owner: string, repo: string) { | |
|
|
||
| const base = `/repos/${owner}/${repo}/issues/${issue.number}`; | ||
|
|
||
| const events = await githubRequest<any[]>(`${base}/events?per_page=100`); | ||
| const events = await fetchAllPages<any>(`${base}/events?per_page=100`); | ||
|
|
||
| const labeledAt = events | ||
| .filter((e) => e.event === "labeled" && e.label?.name === label) | ||
|
|
@@ -124,7 +137,7 @@ async function closeExpired(owner: string, repo: string) { | |
| // Skip if a non-bot user commented after the label was applied. | ||
| // The triage workflow should remove lifecycle labels on human | ||
| // activity, but check here too as a safety net. | ||
| const comments = await githubRequest<any[]>( | ||
| const comments = await fetchAllPages<any>( | ||
| `${base}/comments?since=${labeledAt.toISOString()}&per_page=100` | ||
| ); | ||
| const hasHumanComment = comments.some( | ||
|
|
||
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.
Same concern as
pretooluse.py— need to confirmload_rules(event='all')is a valid path and not a silent no-op for unknown tools.