Scale MCP logs timeout for larger fetch windows#42295
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the MCP logs tool to choose a larger implicit timeout for larger log fetch windows, reducing default 60s deadline hits on fleet-wide queries while keeping explicit user-provided timeouts authoritative.
Changes:
- Introduces a count-aware default timeout calculation for MCP
logsrequests and uses it whentimeoutis omitted. - Updates the MCP tool schema defaults/description to reflect the new implicit timeout behavior.
- Adds/updates unit tests to cover the new implicit-timeout logic and schema defaults.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/mcp_tools_privileged.go | Adds count-based implicit timeout selection and updates the MCP logs tool schema defaults/description. |
| pkg/cli/mcp_server_defaults_test.go | Aligns schema-default assertions with the updated MCP logs tool defaults. |
| pkg/cli/logs_timeout_test.go | Adds focused unit coverage for the new implicit MCP logs timeout calculation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| cmdArgs = appendRepoFlagFromEnv(cmdArgs) | ||
|
|
||
| // Set timeout to 1 minute for MCP server if not explicitly specified | ||
| timeoutValue := args.Timeout | ||
| if timeoutValue == 0 { | ||
| timeoutValue = 1 | ||
| } | ||
| // Scale the implicit MCP timeout with the requested fetch window so | ||
| // larger fleet-wide requests do not hit the 60s server deadline by default. | ||
| timeoutValue := effectiveMCPLogsToolTimeoutMinutes(args.Timeout, args.Count) | ||
| cmdArgs = append(cmdArgs, "--timeout", strconv.Itoa(timeoutValue)) |
| AfterRunID int64 `json:"after_run_id,omitempty" jsonschema:"Filter runs with database ID after this value (exclusive)"` | ||
| BeforeRunID int64 `json:"before_run_id,omitempty" jsonschema:"Filter runs with database ID before this value (exclusive)"` | ||
| Timeout int `json:"timeout,omitempty" jsonschema:"Maximum time in minutes to spend downloading logs (default: 1 for MCP server)"` | ||
| Timeout int `json:"timeout,omitempty" jsonschema:"Maximum time in minutes to spend downloading logs (default: auto-scales with count in the MCP server: 1 minute for up to 40 runs, 2 minutes for 41-80 runs, and 3 minutes for the default 100-run window)"` |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42295 does not have the 'implementation' label and has only 84 new lines of code in business logic directories (threshold: 100). ADR gate skipped. |
|
@copilot please run the
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Scale MCP logs timeout for larger fetch windows
The change is correct and well-structured. The math checks out across all documented tiers, the previous tautological test is replaced with a proper table-driven suite, and the schema/test constants are now derived from the source-of-truth rather than hardcoded magic numbers.
Two non-blocking suggestions are filed as inline comments:
- Missing tier-boundary test — the
80 -> 81boundary (2 to 3 min) is not covered; only the40 -> 41boundary is tested. - Schema default vs. runtime default — the elicitation default for
timeout(3 min) reflects the 100-run tier, but callers who pass a smallercountwill get a lower effective timeout at runtime. A short clarifying comment would help.
Neither is blocking. The core logic, constants, and formula are correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 36.6 AIC · ⌖ 6.63 AIC · ⊞ 4.9K
| requestedTimeout: 0, | ||
| count: 0, | ||
| want: 3, | ||
| }, |
There was a problem hiding this comment.
The test suite covers the 40 → 41 boundary (1 → 2 min) but misses the 80 → 81 boundary (2 → 3 min). Consider adding test cases for count=80 (expect 2 min) and count=81 (expect 3 min) to fully exercise both tier transitions and guard against accidental formula changes.
@copilot please address this.
| mcpLog.Printf("Failed to add default for count: %v", err) | ||
| } | ||
| if err := AddSchemaDefault(logsSchema, "timeout", 1); err != nil { | ||
| if err := AddSchemaDefault(logsSchema, "timeout", defaultMCPLogsToolTimeoutMinutesForCount(defaultMCPLogsToolCount)); err != nil { |
There was a problem hiding this comment.
The schema default for timeout is pinned to the 100-run tier (3 min). A caller that provides an explicit count=10 but no timeout will get a 1-minute effective timeout at runtime, despite the schema hint suggesting 3. The jsonschema tag description on the field already documents the tiers, but the schema default could mislead callers who assume it applies to any count value.
Consider adding a small comment here like:
// Schema default corresponds to defaultMCPLogsToolCount; actual runtime value
// scales down for smaller count values — see defaultMCPLogsToolTimeoutMinutesForCount.@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Go: 2 ( Score breakdown
🔍 Per-test analysis
The old test was a compile-time no-op: it set a local variable then immediately checked its own set value — it could never fail. The replacement is a table-driven test with 5 rows that exercise the real
Assertions use
Two changes: replaces hardcoded magic numbers ( Verdict
References: §28389435456
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Test boundary gap: Tier 1→2 (count 40/41) is tested; tier 2→3 (count 80/81) is not — a future constant change could silently regress.
- Schema description drift:
Timeoutstruct tag hardcodes tier numbers that should mirrormcpLogsRunsPerDefaultTimeoutMinute.
Positive Highlights
- ✅ Correct ceiling-division formula — verified manually against all four tier boundaries.
- ✅ Replaces a tautological mock test (
timeout == 50 → want 50) with real table-driven tests that exercise the live function. - ✅ Clean two-function split:
defaultMCPLogsToolTimeoutMinutesForCountfor the policy andeffectiveMCPLogsToolTimeoutMinutesfor the override logic. - ✅ Named constants for every magic number; no bare literals left in the logic path.
- ✅
mcp_server_defaults_test.gois now self-consistent — schema defaults reference the same constants as the implementation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 62.1 AIC · ⌖ 7.43 AIC · ⊞ 6.6K
Comment /matt to run again
| count: 0, | ||
| want: 3, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[/tdd] The tier 1→2 boundary (count=40/41) is tested but the tier 2→3 boundary (count=80/81) is not. A future off-by-one in the ceiling-division formula would only be caught at the tested boundaries.
💡 Two additional cases to add before line 152
{
name: "top of tier 2 keeps two minutes",
requestedTimeout: 0,
count: 80,
want: 2,
},
{
name: "bottom of tier 3 gets three minutes",
requestedTimeout: 0,
count: 81,
want: 3,
},These pin both sides of the second tier boundary, matching the depth of coverage already present for the first boundary (40/41).
@copilot please address this.
| AfterRunID int64 `json:"after_run_id,omitempty" jsonschema:"Filter runs with database ID after this value (exclusive)"` | ||
| BeforeRunID int64 `json:"before_run_id,omitempty" jsonschema:"Filter runs with database ID before this value (exclusive)"` | ||
| Timeout int `json:"timeout,omitempty" jsonschema:"Maximum time in minutes to spend downloading logs (default: 1 for MCP server)"` | ||
| Timeout int `json:"timeout,omitempty" jsonschema:"Maximum time in minutes to spend downloading logs (default: auto-scales with count in the MCP server: 1 minute for up to 40 runs, 2 minutes for 41-80 runs, and 3 minutes for the default 100-run window)"` |
There was a problem hiding this comment.
[/grill-with-docs] The jsonschema description hardcodes the tier boundaries ("1 minute for up to 40 runs, 2 minutes for 41-80 runs, and 3 minutes for the default 100-run window"), but the real boundaries are derived from mcpLogsRunsPerDefaultTimeoutMinute. If that constant changes the description will silently drift, misleading MCP callers.
💡 Suggestion
Add a short comment above the field pointing readers at the authoritative constants, or use a generate step to embed the computed strings. At minimum, note in a comment that the inline numbers shadow mcpLogsRunsPerDefaultTimeoutMinute:
// Tier thresholds mirror mcpLogsRunsPerDefaultTimeoutMinute; update this
// string if that constant changes.
Timeout int `json:"timeout,omitempty" jsonschema:"..."`This keeps the description honest without requiring a runtime approach.
@copilot please address this.
There was a problem hiding this comment.
Two correctness issues need fixing before merge — plus two supporting issues already flagged in earlier comments.
### Blocking issues (this review)
1. timeout: 0 cannot reach the CLI's "unlimited" mode (line 71, effectiveMCPLogsToolTimeoutMinutes)
The guard if requestedTimeout > 0 silently treats an explicit 0 identically to an omitted value, so an MCP caller who intends --timeout 0 (no timeout, CLI-documented contract) always gets an auto-computed budget. The PR description says "Keep explicit timeout values authoritative" — this code doesn't deliver that. Either pass 0 through as-is, or explicitly document and enforce that unlimited mode is intentionally blocked.
2. No upper bound on computed timeout (line 62, defaultMCPLogsToolTimeoutMinutesForCount)
count=1000 → 25-minute timeout; count=10000 → ~4-hour hang. The schema count field has no maximum constraint, making the handler susceptible to over-large inputs. A hard cap (e.g., 10 minutes) should be applied and exposed in the schema.
### Supporting issues (also flagged in this review)
- Test
"unspecified count falls back to default window size"encodes the count=0 / 10-run inconsistency (identified by a prior comment) as expected behavior, making it harder to fix. - Stale
"seconds"inmcp_server_defaults_test.go:59— test fixture out of sync with the reallogsArgs.Timeoutfield which now says"minutes".
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 95.8 AIC · ⌖ 9.09 AIC · ⊞ 5.2K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/mcp_tools_privileged.go:71
timeout: 0 cannot request unlimited execution — contradicts the PR's stated design goal: if requestedTimeout > 0 silently treats an explicit 0 the same as an omitted value, so an MCP caller who passes timeout: 0 to opt into unlimited mode receives an auto-computed budget instead.
<details>
<summary>💡 Details and suggested fix</summary>
The CLI contract is explicit: --timeout 0 means "no timeout" (logs_command.go flag: "Download timeout in minutes (0 = no timeout)"). The PR …
pkg/cli/mcp_tools_privileged.go:62
No upper bound on computed timeout — a large count can produce arbitrarily long hangs: count=1000 yields a 25-minute timeout; there is no schema maximum on count and no cap in this function.
<details>
<summary>💡 Details and suggested fix</summary>
The formula (count + 39) / 40 scales linearly without a ceiling:
| count | computed timeout |
|---|---|
| 120 | 3 min |
| 121 | 4 min |
| 200 | 5 min |
| 1000 | 25 min … |
pkg/cli/logs_timeout_test.go:150
This test case encodes a known bug as expected behavior: the test asserts count=0 → 3-minute timeout because defaultMCPLogsToolTimeoutMinutesForCount substitutes defaultMCPLogsToolCount=100. But when args.Count==0, the handler doesn't pass -c to the CLI, which then fetches only 10 runs (its own default). The timeout is over-provisioned by 3× and the test name implies this is correct.
<details>
<summary>💡 Why this matters</summary>
An existing review comment on this PR already i…
pkg/cli/mcp_server_defaults_test.go:59
Stale unit descriptor in test fixture: this local logsArgs struct still says "Maximum time in seconds" while the real logsArgs.Timeout field (changed in this PR) says "minutes". Because the test redefines the struct locally instead of importing the real type, schema-documentation drift goes undetected.
<details>
<summary>💡 Suggested fix</summary>
Change the local field tag to match production:
Timeout int `json:"timeout,omitempty" jsonschema:"Maximum time in minutes to spen…
</details>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in I ran the pr-finisher workflow for this branch: applied/validated the remaining timeout-review fixes, ran local validation ( |
The MCP
logstool was still defaulting to a 1-minute timeout, which caused fleet-wide queries to hit the 60s server deadline once the requested window grew beyond ~40 runs. This change raises the effective default budget for larger requests without changing explicit user-provided timeouts or the existing continuation behavior.MCP logs timeout behavior
timeoutvalues authoritative.Default timeout tiers
Schema/default alignment
timeoutto match the new count-scaled behavior.Targeted test updates