Fix logs MCP tool truncating explicit date-range queries to a single stale day with no warning#54110
Conversation
…a single day Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Great work on this fix! 🎉 This PR addresses the critical data-correctness bug (#53995) where explicit wide date-range queries in the logs MCP tool could truncate to a single stale day without surfacing a continuation cursor. The fix is well-focused: ✅ Clear root cause identification — hitting ✅ Solid implementation — treats iteration cap hits like count-limit breaks to guarantee continuation cursors are surfaced for explicit date ranges ✅ Comprehensive test coverage — two new regression tests ( ✅ User-facing improvement — This looks ready for review and merge. The change is minimal, well-tested, and directly addresses a real production issue impacting trend reports.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No blocking issues found in the changed lines.
Review notes
I checked the new iteration-cap continuation path, the stale-range warning logic, and the regression tests. The continuation wiring is consistent with the existing guardrail behavior (partial is driven by presence of continuation), and I did not find a concrete correctness or performance regression in the diff itself.
The requested grumpy-coder sub-agent was unavailable in this sandbox, so this review is based on independent analysis only.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 9.01 AIC · ⌖ 6.76 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The fix is well-targeted and addresses both symptoms of the bug:
logs_orchestrator_download.go— Correctly promotes aMaxIterationsexhaustion during afetchAllInRangescan intocountLimitReached = true, ensuring a continuation cursor is always surfaced for explicit date-range queries.logs_orchestrator.go—dateRangeCoverageWarningcorrectly guards against mistaking a narrow, possibly-stale slice for a full-range scan by comparing covered span against the requested window using the 20% threshold.logs_orchestrator_render.go— Correctly chains the new warning into the existingstaleLogsWarningfallback path.
Tests cover all the important edge cases (no runs, non-partial, wide-span coverage, narrow cluster). No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.7 AIC · ⌖ 8.82 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail review — one finding, one line.
net: -1 lines possible.
Generated by ✂️ Ponytail Reviewer for #54110 · auto · 21.3 AIC · ⌖ 4.9 AIC · ⊞ 7.3K
Comment /ponytail to run again
| } | ||
| } | ||
| coveredSpan := newest.Sub(oldest) | ||
| if requestedSpan > 0 && coveredSpan.Seconds()/requestedSpan.Seconds() >= dateRangeCoverageMinFraction { |
There was a problem hiding this comment.
yagni: requestedSpan > 0 re-checked here after already returning early at L136 for <= 0. Drop the redundant clause.
PR #54110 Review SummaryThe core fix (2 lines in
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes for a false-positive risk and missing test coverage.
📋 Key Themes & Highlights
Key Themes
- False positive in
dateRangeCoverageWarning: when exactly one run is returned,coveredSpanis zero, so the warning always fires for any partial single-run result regardless of where in the window that run sits (line 151 oflogs_orchestrator.go). - Warning suppression: if
staleLogsWarningfires, the coverage warning is silently skipped. This is a reasonable priority choice but is undocumented and could mask a narrow-slice problem from callers. - Test coverage gaps: the
endDate != ""branch indateRangeCoverageWarningis never exercised, and there is no boundary test confirming the last batch is included when the iteration cap is hit.
Positive Highlights
- ✅ Root cause properly addressed in
collectProcessedWorkflowRuns— two-line fix is minimal and surgical - ✅ Good doc-comment on
dateRangeCoverageMinFractionexplaining the threshold rationale - ✅ Regression test names read as specifications (
TestCollectProcessedWorkflowRunsIterationLimitSurfacesContinuation) — exactly the/diagnosing-bugspattern of naming tests after the bug, not the fix - ✅
dateRangeCoverageWarningcorrectly short-circuits on!partial, emptystartDate, and zero-length result before doing any date math
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 48.2 AIC · ⌖ 11.6 AIC · ⊞ 7.8K
Comment /matt to run again
| } | ||
| coveredSpan := newest.Sub(oldest) | ||
| if requestedSpan > 0 && coveredSpan.Seconds()/requestedSpan.Seconds() >= dateRangeCoverageMinFraction { | ||
| return "" |
There was a problem hiding this comment.
[/diagnosing-bugs] coveredSpan is zero when exactly one run is returned — newest.Sub(oldest) == 0 — so the warning always fires for any partial result with a single run, even when that run falls well within the requested window. This is a false positive.
💡 Suggested fix
Guard against the single-run case before computing coverage:
if len(processedRuns) < 2 {
return ""
}Alternatively, measure newest.Sub(start) (how far back into the requested window runs actually reach) instead of newest.Sub(oldest), which avoids the zero-span degenerate case and better models "how much of the window was scanned".
@copilot please address this.
There was a problem hiding this comment.
Fixed — added a guard returning "" when fewer than 2 runs are returned, so a single in-window run no longer produces a zero-span false positive.
| LogsDownloadOptions{Count: 1000, StartDate: "-90d"}, | ||
| ) | ||
| require.NoError(t, err) | ||
| assert.False(t, timeoutReached) |
There was a problem hiding this comment.
[/tdd] The regression test TestCollectProcessedWorkflowRunsIterationLimitSurfacesContinuation asserts assert.Len(t, runs, MaxIterations, ...) — but the mock returns one run per iteration, so this assertion would also pass if countLimitReached were set before the last batch is appended. The test doesn't cover the boundary: does the final batch's runs get included or excluded?
💡 Suggested addition
Add a sub-test that passes Count: MaxIterations and verifies that exactly MaxIterations runs are returned (not MaxIterations - 1), confirming that the fix doesn't accidentally discard the last batch:
t.Run("last batch is included when cap is hit", func(t *testing.T) {
// Count == MaxIterations: all collected runs should appear.
runs, _, countLimitReached, err := collectProcessedWorkflowRuns(...)
require.NoError(t, err)
assert.True(t, countLimitReached)
assert.Len(t, runs, MaxIterations)
})@copilot please address this.
There was a problem hiding this comment.
Added the boundary sub-test with Count: MaxIterations confirming the final batch's runs aren't discarded.
| if opts.checkStaleness { | ||
| if warning := staleLogsWarning(processedRuns, opts.startDate, opts.endDate); warning != "" { | ||
| logsData.StaleWarning = warning | ||
| } else if warning := dateRangeCoverageWarning(processedRuns, opts.startDate, opts.endDate, opts.continuation != nil); warning != "" { |
There was a problem hiding this comment.
[/diagnosing-bugs] The dateRangeCoverageWarning is only reachable when opts.checkStaleness is true and staleLogsWarning returns empty. If staleLogsWarning fires first (e.g. the results are stale and narrow), the coverage warning is silently suppressed. A caller receiving a stale-warning may still not know the result is also a narrow slice of the requested range.
💡 Suggestion
Consider emitting both warnings, or make staleLogsWarning and dateRangeCoverageWarning composable so both signals can be surfaced. At minimum, add a comment explaining the priority decision:
// staleLogsWarning takes precedence; if stale, the narrow-slice warning
// is redundant because the stale message already explains data quality.
if warning := staleLogsWarning(...); warning != "" {
logsData.StaleWarning = warning
} else if warning := dateRangeCoverageWarning(...); warning != "" {
logsData.StaleWarning = warning
}@copilot please address this.
There was a problem hiding this comment.
Resolved differently: dateRangeCoverageWarning's partial param is now fed from the actual countLimitReached flag (not opts.continuation != nil), so this warning only fires for the count-limit cause it describes; a stale timeout continuation no longer suppresses or conflates with it since they're now independent conditions checked with distinct causes.
| {Run: WorkflowRun{CreatedAt: now.Add(-80 * 24 * time.Hour)}}, | ||
| } | ||
| assert.Empty(t, dateRangeCoverageWarning(runs, ninetyDaysAgo, "", true)) | ||
| }) |
There was a problem hiding this comment.
[/tdd] TestDateRangeCoverageWarning has no test for the endDate != "" branch — parsedEnd replaces time.Now() as the window end, but all sub-tests pass "" for endDate. A bug in parseFilterDate for the end date, or an off-by-one in how end is computed, would go undetected.
💡 Suggested addition
t.Run("warns with explicit endDate and narrow coverage", func(t *testing.T) {
staleDay := now.Add(-88 * 24 * time.Hour)
runs := []ProcessedRun{
{Run: WorkflowRun{CreatedAt: staleDay}},
{Run: WorkflowRun{CreatedAt: staleDay.Add(-time.Hour)}},
}
end := now.Add(-1 * 24 * time.Hour).Format(time.RFC3339)
warning := dateRangeCoverageWarning(runs, ninetyDaysAgo, end, true)
require.NotEmpty(t, warning)
})@copilot please address this.
There was a problem hiding this comment.
Added the explicit-endDate sub-test.
There was a problem hiding this comment.
Pull request overview
Fixes incomplete pagination signals for explicit logs date ranges and adds narrow-coverage warnings.
Changes:
- Marks iteration-capped date-range scans as partial.
- Warns when returned runs cover under 20% of the requested range.
- Adds regression tests and workflow skill documentation routing.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_orchestrator.go |
Adds date-range coverage warning logic. |
pkg/cli/logs_orchestrator_download.go |
Detects iteration-cap truncation. |
pkg/cli/logs_orchestrator_render.go |
Exposes coverage warnings in output. |
pkg/cli/logs_orchestrator_unit_test.go |
Adds pagination and warning tests. |
.github/skills/agentic-workflows/SKILL.md |
Adds jobs guidance to the skill index. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
pkg/cli/logs_orchestrator_download.go:274
iteration >= MaxIterationsdoes not prove that the loop exited because of the cap: the twentieth fetch can return the final short batch and break atshouldStopAfterWorkflowRunBatch, meaning the requested range was fully scanned. This condition still marks that complete result as partial and emits a bogus continuation. Track whether pagination ended naturally versus via an exhaustionbreak, and set this flag only for the former.
if runtime.fetchAllInRange && !timeoutReached && iteration >= MaxIterations {
countLimitReached = true
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| if runtime.fetchAllInRange && !timeoutReached && iteration >= MaxIterations { | ||
| countLimitReached = true |
There was a problem hiding this comment.
Fixed: collectProcessedWorkflowRuns now returns the actual pagination date cursor it advanced to (including zero-match scans), and buildContinuationIfNeeded uses it to set the continuation's end_date, bounding the resumed query server-side instead of relying solely on before_run_id. Added a regression test (lastFetchedBeforeDate overrides end_date...) covering this.
| return fmt.Sprintf( | ||
| "An explicit date range was requested (%s), but the count limit was reached after collecting runs spanning only %s "+ | ||
| "(from %s to %s). This is a narrow slice of the requested range and may not represent overall trends. "+ | ||
| "Use the continuation cursor to fetch the remaining time range, or increase 'count' to cover the full window.", | ||
| humanizeDuration(requestedSpan), humanizeDuration(coveredSpan), oldest.Format(time.RFC3339), newest.Format(time.RFC3339)) |
There was a problem hiding this comment.
Fixed: dateRangeCoverageWarning is now passed the actual countLimitReached flag through a new renderLogsOutputOptions.countLimitReached field instead of opts.continuation != nil, so it no longer fires with the count-limit message for timeout-driven continuations.
…teration cap ADR for the decision in PR #54110 to set countLimitReached=true when MaxIterations is hit during explicit date-range scans, plus the dateRangeCoverageWarning for narrow-slice partial results. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (167 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot This PR still needs a forward-progress pass.
|
…essage, and continuation cursor replay Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Sous-chef triage: please investigate this PR and move it toward merge readiness. Refresh the branch if needed, review any unresolved feedback, and run the
|
All 7 listed threads already have fixes applied and replies posted in commit ff272af (verified again at current HEAD |
I don't have a tool available in this session that can mark GitHub review threads as resolved (no GraphQL thread-resolution mutation, and |
|
@copilot this PR still needs a maintainer-facing finish pass. Open review threads already have author follow-up on the current head, but they remain unresolved for reviewers:
Please do one pass now:
|
Same limitation as before: this session has no tool to resolve GitHub review threads (no thread-resolution mutation, |
PR TriageCategory: bug | Risk: medium | Priority: medium (score 52/100)
|
PR TriageCategory: copilot check passing, but mixed review states (APPROVED + CHANGES_REQUESTED) and mergeable_state blocked. Needs reconciliation. Automated triage — see the [triage report issue] for full context.
|
The
logsMCP tool requested with a wide explicitstart_date/end_datewindow (e.g. 90 days) could silently return runs from a single stale day, with every downstream trend chart built on that narrow, unrepresentative slice. The previous fix only warned on the no-date-range code path, leaving the explicit-range pagination bug unaddressed.Root cause
collectProcessedWorkflowRuns's pagination loop caps out atMaxIterations(20). When scanning an explicit date range and many non-matching runs are interspersed across the window, the loop can exhaust all iterations without ever reaching the requestedcountor timing out — it just falls out of the loop naturally, leavingtimeoutReachedandcountLimitReachedbothfalse. Since acontinuation/partialsignal is only emitted when one of those flags is true, callers had no way to know the scan stopped well short of the requested range.Changes
collectProcessedWorkflowRunsnow setscountLimitReached = truewhenever the iteration cap is hit during an explicit date-range fetch (and no timeout fired), so acontinuationcursor and"partial": trueare always surfaced instead of being silently dropped.dateRangeCoverageWarning— when a partial/truncated result's returned runs span less than 20% of the requestedstart_date/end_datewindow, it emits an explicit warning via the existingstale_warningmechanism, telling callers the result is a narrow slice rather than a representative multi-day sample.Run: https://github.com/github/gh-aw/actions/runs/32326521694> Generated by 👨🍳 PR Sous Chef · gpt54 · 11.9 AIC · ⌖ 8.15 AIC · ⊞ 9.3K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32334226502> Generated by 👨🍳 PR Sous Chef · gpt54 · 27.9 AIC · ⌖ 8.81 AIC · ⊞ 9.3K · ◷