Warn callers when logs MCP tool returns stale data with no date range specified - #53719
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
logs MCP tool returns stale data with no date range specified
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
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: comment
I don't see a changed-line bug here that should block merge.
Review notes
- The new stale-data warning is correctly gated off when
start_dateorend_dateis explicitly supplied, which avoids false positives for intentionally bounded queries. - The warning is threaded into both rendered logs output and the MCP guardrail response, so callers can see it without opening the cached file.
- I did not find a correctness regression in the message-merging logic or the added tests.
- The requested
grumpy-codersub-agent was unavailable in this environment, so this review relies on direct analysis of the diff.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 13.2 AIC · ⌖ 8.62 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Review: Warn callers when logs MCP tool returns stale data
The implementation is clean and well-tested. One non-blocking issue to consider:
Hint messages mixed with WARNING prefix
In renderLogsOutput, both the stale-data warning and the usage-only artifact hint are joined into logsData.Message with a single space. Then in buildLogsFileResponse, extractLogsMessage pulls the whole combined string and unconditionally prefixes it with "WARNING: ".
This means a response that includes both messages would render as:
WARNING: No start_date/end_date was specified ... When only the usage artifact was downloaded, add a hint...
The usage-only hint text gets a misleading WARNING: prefix. Consider keeping the stale-data warning and informational hints in separate fields, or only prepending WARNING: when the message originated from the staleness check rather than for the whole combined string.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.9 AIC · ⌖ 8.81 AIC · ⊞ 5.7K
PR #53719 Review SummaryThe staleness-detection approach is solid and well-structured. Five issues are flagged:
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Pull request overview
Adds stale-result warnings for date-unbounded workflow-log queries and surfaces them through MCP responses.
Changes:
- Detects results whose newest run exceeds the 48-hour threshold.
- Threads date filters through rendering and combines output hints.
- Adds unit coverage for detection and MCP response surfacing.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_orchestrator.go |
Implements staleness detection. |
pkg/cli/logs_orchestrator_types.go |
Adds rendering date options. |
pkg/cli/logs_orchestrator_render.go |
Adds warnings to rendered messages. |
pkg/cli/mcp_logs_guardrail.go |
Surfaces messages in MCP responses. |
pkg/cli/logs_orchestrator_unit_test.go |
Tests staleness conditions. |
pkg/cli/mcp_logs_guardrail_test.go |
Tests MCP warning surfacing. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| // result is unexpectedly old, warn the caller so stale data is never served | ||
| // silently (see issue: logs MCP tool returns stale data without date params). | ||
| var hints []string | ||
| if warning := staleLogsWarning(processedRuns, opts.startDate, opts.endDate); warning != "" { |
| // Surface any top-level warning (e.g. stale-data warning when no date range was | ||
| // requested) directly in the tool response so callers see it without having to | ||
| // open the file. | ||
| if warning := extractLogsMessage(outputStr); warning != "" { |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on a few targeted issues before merging.
📋 Key Themes & Highlights
Key Themes
- Duration formatting:
age.Round(time.Hour)produces"264h0m0s"not"11 days old"— the warning message will confuse callers and agents. - Test variable naming:
oldestin the stale-warning test actually holds the newest timestamp; a confusing mismatch. - Message assembly pattern: two different composition styles (
fmt.Sprintfvs if/else) exist side-by-side inbuildLogsFileResponse; thehints []string+strings.Joinpattern used inrenderLogsOutputshould be applied here too. - Silent failure in
extractLogsMessage: parse errors are swallowed with no debug log, making issues hard to diagnose. - Test assertion style: the new guardrail test uses
t.Errorfwhere the rest of the file usestestifyassertions.
Positive Highlights
- ✅ Clean separation of staleness detection into its own pure function — easy to test in isolation.
- ✅ The staleness threshold constant is well-documented with the rationale comment.
- ✅ Warning is threaded all the way to the MCP tool response so callers see it without opening the file.
- ✅ Good test coverage: the four
staleLogsWarningsub-tests cover the key decision tree branches.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42.7 AIC · ⌖ 11.7 AIC · ⊞ 7.8K
Comment /matt to run again
| return "" | ||
| } | ||
| return fmt.Sprintf( | ||
| "No start_date/end_date was specified, and the most recent run in this result is %s old (created %s). "+ |
There was a problem hiding this comment.
[/diagnosing-bugs] age.Round(time.Hour) formats as "264h0m0s" — callers see a raw Go duration string rather than "11 days old" as shown in the PR description. A human-readable form is far more actionable for agents and users.
💡 Suggested fix
Add a small helper and use it in the Sprintf:
func humanizeDuration(d time.Duration) string {
days := int(d.Hours()) / 24
if days >= 1 {
return fmt.Sprintf("%d day(s)", days)
}
return fmt.Sprintf("%d hour(s)", int(d.Hours()))
}Also add a test that asserts the stale-data warning for 11-day-old data contains "11 day" to prevent format regressions.
@copilot please address this.
| } | ||
| assert.Empty(t, staleLogsWarning(runs, "", "")) | ||
| }) | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] The staleLogsWarning in the test for "warns when no dates given and newest run is old" uses a variable named oldest but assigns it the newest timestamp (the other run is oldest.Add(-time.Hour)). The naming mismatch makes the test harder to follow and could mask a logic inversion.
💡 Suggested rename
newest := time.Now().Add(-11 * 24 * time.Hour)
runs := []ProcessedRun{
{Run: WorkflowRun{CreatedAt: newest}},
{Run: WorkflowRun{CreatedAt: newest.Add(-time.Hour)}},
}While here, assert the warning contains "11 day" to lock in the human-readable format.
@copilot please address this.
| if warning := extractLogsMessage(outputStr); warning != "" { | ||
| if response.Message == "" { | ||
| response.Message = "WARNING: " + warning | ||
| } else { |
There was a problem hiding this comment.
[/codebase-design] buildLogsFileResponse now contains two separate message-assembly patterns: one for the continuation case (inline fmt.Sprintf) and one for the stale-data warning (if/else append). This makes it hard to see the final message shape at a glance and will grow messier as more warning types are added.
💡 Suggested refactor
Collect all message fragments into a []string and strings.Join them at the end — the same pattern used in renderLogsOutput:
var msgs []string
if continuation != nil {
msgs = append(msgs, fmt.Sprintf("PARTIAL RESULTS: ... '%s'.", filePath))
}
if warning := extractLogsMessage(outputStr); warning != "" {
msgs = append(msgs, "WARNING: "+warning)
}
if len(msgs) > 0 {
response.Message = strings.Join(msgs, " ")
}This also removes the duplicated "WARNING: " prefix logic.
@copilot please address this.
|
|
||
| var response MCPLogsGuardrailResponse | ||
| if err := json.Unmarshal([]byte(result), &response); err != nil { | ||
| t.Fatalf("Response should be valid JSON: %v", err) |
There was a problem hiding this comment.
[/tdd] TestBuildLogsFileResponse_SurfacesStaleDataWarning uses t.Errorf for assertions but the test above it (TestBuildLogsFileResponse_CompleteResultsNotPartial) uses assert/require helpers. Mixing styles makes the test suite harder to scan. The t.Errorf path also continues running after the first failure, which can produce misleading secondary failures.
💡 Suggested fix
Switch to require/assert from the existing testify import:
require.NoError(t, json.Unmarshal([]byte(result), &response))
assert.Contains(t, response.Message, "WARNING:")
assert.Contains(t, response.Message, "No start_date/end_date was specified")@copilot please address this.
| var parsed struct { | ||
| Message string `json:"message"` | ||
| } | ||
| if err := json.Unmarshal([]byte(outputStr), &parsed); err != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] extractLogsMessage silently returns "" on JSON parse error, meaning a malformed outputStr will cause the stale-data warning to be silently dropped rather than surfaced. This makes the guardrail harder to debug in production.
💡 Suggested fix
Log the parse error at debug level so it's visible under DEBUG=cli:*:
func extractLogsMessage(outputStr string) string {
var parsed struct {
Message string `json:"message"`
}
if err := json.Unmarshal([]byte(outputStr), &parsed); err != nil {
mcpLogsGuardrailLog.Printf("extractLogsMessage: failed to parse output JSON: %v", err)
return ""
}
return parsed.Message
}@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (144 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
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@copilot run pr-finisher skill |
…d, humanized age Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The
logsMCP tool's default code path (nostart_date/end_date) can silently settle on an old pagination window in high-activity repos and return stale runs with no indication anything is wrong—leading deep-report and audit workflows to draw false conclusions about current fleet health.Changes
pkg/cli/logs_orchestrator.go): addedstaleLogsWarning(), which checks whether no date range was requested and the newest run in the result set is older than 48 hours; if so, produces a warning advising the caller to retry with an explicitstart_date.startDate/endDatetorenderLogsOutputOptionssorenderLogsOutputcan invoke the staleness check and fold the warning into the output'smessagefield alongside existing hints.pkg/cli/mcp_logs_guardrail.go):buildLogsFileResponsenow extracts this warning and appends it directly to the tool's top-level response text, so it's visible immediately without opening the cached output file.staleLogsWarningbehavior (explicit dates, recent data, no runs, stale data) and confirming the guardrail response surfaces the warning.Example: calling the tool as
agenticworkflows logs --count 30when the newest returned run is 11 days old now returns a response message like: