Paginate daily performance data through its 90-day window - #54679
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for the effort on fixing the analysis window collapse in the daily regulatory reports! 🚀 This PR is still marked as a draft
Once tests are added and the description is filled out, this should be in good shape for review! 🎯
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54679 does not have the implementation label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
The new windowed query path fixes the record-cap problem, but it also introduces a silent schema break in github-pr-query: callers get different PR objects when since is set than when it is not.
Blocking theme
github-pr-querydropsreviewDecisionandreviewRequestsin thesincecode path while still advertising them as available fields.- That makes downstream jq filters and workflow logic return wrong results only for the new 90-day mode, which is exactly the path this PR wants consumers to adopt.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 10.3 AIC · ⌖ 6.98 AIC · ⊞ 7K
Comment /review to run again
| fi | ||
| SINCE=$(date -u -d "$SINCE" '+%Y-%m-%dT%H:%M:%SZ') | ||
| if [[ -n "$REPO" ]]; then | ||
| API_PATH="repos/${REPO}/pulls" |
There was a problem hiding this comment.
The new since code path for github-pr-query no longer returns reviewDecision or reviewRequests, even though this tool still documents those fields as part of its output contract. Any workflow that switches to since and filters on review state or requested reviewers will silently get incomplete data and make wrong decisions from it.
💡 Why this blocks merge
The old path uses gh pr list --json with:
JSON_FIELDS="...,reviewDecision,...,reviewRequests,..."but the new REST/jq projection drops both fields:
OUTPUT=$(jq --arg since "$SINCE" --arg state "$STATE" '[.[] | ... | {
number, title, state: (.state | ascii_upcase), author: .user, createdAt: .created_at,
updatedAt: .updated_at, mergedAt: .merged_at, closedAt: .closed_at,
headRefName: .head.ref, baseRefName: .base.ref, isDraft: .draft, labels, assignees,
additions, deletions, changedFiles: .changed_files, url: .html_url
}]' <<< "$OUTPUT")That is a silent schema regression: callers that worked before now receive different objects only when since is set.
At minimum, either:
# keep the same output contract in the since-path
reviewDecision: ...
reviewRequests: ...or explicitly remove those fields from the documented schema and every consumer that depends on them.
There was a problem hiding this comment.
Ponytail review (over-engineering only).
net: -8 lines possible.
Generated by ✂️ Ponytail Reviewer for #54679 · auto · 32.2 AIC · ⌖ 4.45 AIC · ⊞ 7.3K
Comment /ponytail to run again
| # updated time, so stop only after reaching the requested boundary. | ||
| if [[ -n "$SINCE" ]]; then | ||
| if ! date -d "$SINCE" --iso-8601=seconds >/dev/null 2>&1; then | ||
| echo "Error: since must be an ISO 8601 date or timestamp" >&2 |
There was a problem hiding this comment.
yagni: same since-validation + date-normalize block (date -d ... --iso-8601 check + date -u -d ... +%Y-%m-%dT%H:%M:%SZ) repeated verbatim in issue, PR, and discussion tools (L44-48, L168-172, L306-309). Factor into one shared shell function/step.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — requesting changes on correctness issues.
📋 Key Themes & Highlights
Key Themes
- Silent failure on API errors: all three pagination loops call
gh apiwithout checking the exit code. A transient failure produces an error JSON object;jq 'length'counts it as 1 item and the loop continues, accumulating corrupted data. - Schema mismatch on PR pagination: the
SINCE-path jq transform dropsreviewDecisionandreviewRequeststhat thegh pr listfallback path returns. - Placeholder date risk: the prompt instructs the agent to substitute a concrete 90-day timestamp but gives no concrete format example. A literal-string pass-through causes the
date -dguard to exit 1 and stops data collection entirely. - Loop exit asymmetry (discussions): the boundary check and
hasNextPagecheck read from two separate variables derived from the same response, making the exit conditions harder to audit.
Positive Highlights
- ✅ Good use of
sort=updated&direction=desc— enables early loop termination when the oldest item in a page is before$SINCE. - ✅ REST and GraphQL paths are cleanly separated; the non-
sincefallback is preserved for existing callers. - ✅ Input validation with
date -dbefore the loop prevents silent clock-skew issues. - ✅ The
merged→closedREST state translation is correct.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.9 AIC · ⌖ 10.2 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
.github/workflows/shared/github-queries-mcp-script.md:665
[/codebase-design] The paginated PR path silently drops reviewDecision and reviewRequests fields that the gh pr list path returns, creating a schema mismatch between the two code paths.
<details>
<summary>💡 Details & fix</summary>
JSON_FIELDS includes reviewDecision and reviewRequests, but the jq transform used in the SINCE branch omits them. Any caller relying on those fields gets null from the paginated path but real values from the limit path.
Add the missing fields t…
.github/workflows/shared/github-queries-mcp-script.md:599
[/diagnosing-bugs] gh api failures inside the pagination loop are not checked — a transient error returns an error JSON object, jq 'length' evaluates it as 1, the loop continues, and corrupted data accumulates silently.
<details>
<summary>💡 Suggested guard</summary>
PAGE_OUTPUT=$(gh api "${API_PATH}?..." 2>&1) || { echo "Error: gh api failed: $PAGE_OUTPUT" >&2; exit 1; }
# Also verify it's an array before processing:
if [[ "$(jq 'type' <<< "$PAGE_OUTPUT")" != '"array"' ]]; th…
</details>
<details><summary>.github/workflows/daily-performance-summary.md:266</summary>
**[/diagnosing-bugs]** `since: "<UTC timestamp exactly 90 days before this run>"` is a natural-language placeholder the executing AI agent must compute and substitute. If the agent passes the literal string, the `date -d` validation exits with an error and data collection fails entirely.
<details>
<summary>💡 Suggestion</summary>
Add a concrete format example in the prompt to reduce ambiguity:
github-pr-query with state: "all",
since: "YYYY-MM-DDT00:00:00Z", -- e.g. 2025-11-23T00:00:0…
.github/workflows/shared/github-queries-mcp-script.md:737
[/codebase-design] The hasNextPage check reads from $GRAPHQL_OUTPUT while the boundary check reads from $PAGE_NODES — two variables derived from the same source but used asymmetrically, making the loop exit conditions harder to reason about.
<details>
<summary>💡 Cleaner approach</summary>
Extract all decision values from one pass and check them together:
HAS_NEXT=$(jq -r '.data.repository.discussions.pageInfo.hasNextPage' <<< "$GRAPHQL_OUTPUT")
NEXT_CURSOR=$(jq -r '.data.r…
</details>There was a problem hiding this comment.
Pull request overview
Adds date-window pagination to GitHub query tools so daily performance reports cover the full 90-day period.
Changes:
- Adds
sincepagination for PRs, issues, and discussions. - Updates daily performance instructions to request a 90-day UTC window.
- Regenerates dependent workflow lock files.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/shared/github-queries-mcp-script.md |
Implements windowed pagination. |
.github/workflows/daily-performance-summary.md |
Uses 90-day query boundaries. |
.github/workflows/daily-performance-summary.lock.yml |
Regenerates the performance workflow. |
.github/workflows/daily-regulatory.lock.yml |
Regenerates the regulatory workflow. |
.github/workflows/smoke-copilot.lock.yml |
Regenerates smoke workflow tooling. |
.github/workflows/smoke-copilot-arm.lock.yml |
Regenerates ARM smoke workflow tooling. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Regenerates Entra smoke workflow tooling. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Regenerates API-key smoke workflow tooling. |
Review details
Suppressed comments (3)
.github/workflows/shared/github-queries-mcp-script.md:185
- This accumulation also passes the complete PR history and current page through argv. Once either JSON argument exceeds the OS per-argument limit, pagination aborts with
Argument list too longbefore reaching the requested boundary. Stream the arrays tojqinstead.
OUTPUT=$(jq -cn --argjson all "$OUTPUT" --argjson page "$PAGE_OUTPUT" '$all + $page')
.github/workflows/shared/github-queries-mcp-script.md:364
- Passing the growing discussion array via
--argjsonmakes the new pagination fail once the argument reaches the OS size limit, which is likely during a busy 90-day window. Stream both arrays tojqrather than embedding them in argv.
OUTPUT=$(jq -cn --argjson all "$OUTPUT" --argjson page "$PAGE_NODES" '$all + $page')
.github/workflows/shared/github-queries-mcp-script.md:316
- This changes every limit-only discussion query from newest-created to most-recently-updated, despite the code below promising to preserve existing behavior when
sinceis absent.daily-regulatory.md:121-127depends onlimit: 100returning newly created reports, so active old discussions can now displace the reports it needs. SelectUPDATED_ATonly for windowed calls and retainCREATED_ATotherwise.
discussions(first: \$first, after: \$after, orderBy: {field: UPDATED_AT, direction: DESC}) {
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
| while :; do | ||
| PAGE_OUTPUT=$(gh api "${API_PATH}?state=${STATE}&sort=updated&direction=desc&per_page=100&page=${PAGE}") | ||
| [[ "$(jq 'length' <<< "$PAGE_OUTPUT")" -eq 0 ]] && break | ||
| OUTPUT=$(jq -cn --argjson all "$OUTPUT" --argjson page "$PAGE_OUTPUT" '$all + $page') |
| number, title, state: (.state | ascii_upcase), author: .user, createdAt: .created_at, | ||
| updatedAt: .updated_at, mergedAt: .merged_at, closedAt: .closed_at, | ||
| headRefName: .head.ref, baseRefName: .base.ref, isDraft: .draft, labels, assignees, | ||
| additions, deletions, changedFiles: .changed_files, url: .html_url | ||
| }]' <<< "$OUTPUT") |
| 2. Set `since` to the exact UTC timestamp 90 days before the run; the tools paginate by `updatedAt` through that boundary rather than using a record cap | ||
| 3. Each tool supports jq-based filtering for efficient data querying | ||
| 4. Tools are authenticated with `GITHUB_TOKEN` for GitHub API access | ||
| 5. Call tools with parameters like: `github-pr-query with state: "all", since: "YYYY-MM-DDT00:00:00Z", jq: "."` |
There was a problem hiding this comment.
Review: Paginate daily performance data through its 90-day window
The pagination approach is well-structured and the discussion query correctly uses GraphQL with cursor-based pagination. However, there are two correctness issues in the PR/issue REST paths:
Blocking:
-
additions,deletions,changedFilesare alwaysnullfrom the REST list endpoint (see inline comment). The GitHub RESTGET /repos/{owner}/{repo}/pullslist endpoint omits these fields; they require a per-PR detail call. The daily report uses these values for churn/velocity metrics — they will be silently wrong for allsince-based runs. -
reviewDecisionandreviewRequestsare missing from thesince-pathjqprojection, causing schema inconsistency with the legacy path.
Suggestion: Consider using GraphQL for the PR and issue queries too (as done for discussions), since GraphQL returns the full field set including additions, deletions, changedFiles, and reviewDecision in list responses.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 74.6 AIC · ⌖ 9.21 AIC · ⊞ 6.2K
| number, title, state: (.state | ascii_upcase), author: .user, createdAt: .created_at, | ||
| updatedAt: .updated_at, mergedAt: .merged_at, closedAt: .closed_at, | ||
| headRefName: .head.ref, baseRefName: .base.ref, isDraft: .draft, labels, assignees, | ||
| additions, deletions, changedFiles: .changed_files, url: .html_url |
There was a problem hiding this comment.
Bug: additions, deletions, and changedFiles are always null from the REST list endpoint.
GitHub's GET /repos/{owner}/{repo}/pulls (list) endpoint does not return additions, deletions, or changed_files — those fields are only available on the individual PR detail endpoint. As a result, the since-based code path silently produces null for these three fields while the legacy gh pr list path returns real values.
The daily performance report depends on additions/deletions for velocity/churn metrics, so this causes incorrect analysis for any run that uses since.
Fix options (pick one):
- Fetch each PR individually in a second pass (slow).
- Use the GraphQL API — like the discussion query in this PR — which returns
additions,deletions,changedFiles,reviewDecision, andreviewRequestsin list responses. - Document the limitation and return
nullexplicitly.
Option 2 is most consistent with the discussion query approach already in this PR.
@copilot please address this.
| done | ||
| OUTPUT=$(jq --arg since "$SINCE" --arg state "$STATE" '[.[] | select(.updated_at >= $since and ($state != "merged" or .merged_at != null)) | { | ||
| number, title, state: (.state | ascii_upcase), author: .user, createdAt: .created_at, | ||
| updatedAt: .updated_at, mergedAt: .merged_at, closedAt: .closed_at, |
There was a problem hiding this comment.
reviewDecision and reviewRequests are missing from the since-path transformation.
The JSON_FIELDS variable includes reviewDecision and reviewRequests, but the jq transformation in the since branch omits both. Callers that relied on these fields for review-readiness analysis will silently get null/missing keys.
Either include them in the jq projection (even if null from the REST list endpoint), or document the schema difference clearly.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
Daily performance reports could exhaust fixed record caps during high activity, reducing a stated 90-day analysis to only recent days. This makes velocity and resolution metrics unrepresentative of their labeled window.
Windowed MCP queries
sinceto shared PR, issue, and discussion query tools.updatedAtorder until the normalized UTC boundary instead of stopping at a record limit.Daily performance report
limit: 1000guidance with an exact UTC timestamp 90 days before the run.