Remove 120s MCP bridge cap for logs calls to prevent partial daily audit datasets#46901
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
logs calls to prevent partial daily audit datasets
🤖 PR Triage
Score breakdown: impact 28 + urgency 20 + quality 10 Daily audit datasets are being truncated due to the 120s MCP bridge timeout. Directly impacts data quality of all daily reports. Draft — needs undraft + CI before merge.
|
|
✅ 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 #46901 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Updates the MCP CLI bridge so logs calls can exceed the previous 120-second transport timeout.
Changes:
- Resolves
logstransport timeouts from tool arguments with a buffer. - Adds focused timeout-resolution tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/mcp_cli_bridge.cjs |
Adds per-tool timeout resolution. |
actions/setup/js/mcp_cli_bridge.test.cjs |
Tests default, explicit, fractional, and invalid timeouts. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| } | ||
|
|
||
| const timeoutCandidate = Number(toolArgs?.timeout); | ||
| const timeoutMinutes = Number.isFinite(timeoutCandidate) && timeoutCandidate > 0 ? timeoutCandidate : LOGS_TOOL_DEFAULT_TIMEOUT_MINUTES; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is correct and well-tested. getToolCallTimeoutMs cleanly isolates the per-tool timeout logic, the Math.max guard prevents regressions for short calls, and all five new test cases verify the expected millisecond values. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.5 AIC · ⌖ 7.61 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (5 tests)
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one minor observation, otherwise a well-executed fix.
📋 Key Themes & Highlights
Key Themes
- Lower-bound semantics:
Math.max(TOOL_CALL_TIMEOUT_MS, resolvedTimeoutMs)means a tiny positivetimeout(e.g.0.001min → ~75ms resolved) silently falls back to 120 s rather than the 5-minute logs default. Whether this is intentional deserves a clarifying comment (see inline comment).
Positive Highlights
- ✅ Clean extraction of
getToolCallTimeoutMs— single-responsibility, easy to test in isolation - ✅ Well-named constants with clear units in comments
- ✅
Math.ceilprevents premature timeout on fractional values - ✅ Comprehensive test suite covering 5 distinct paths — reads like a specification
- ✅
module.exportsupdated so the function is directly testable without side effects
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 32 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
| const timeoutCandidate = Number(toolArgs?.timeout); | ||
| const timeoutMinutes = Number.isFinite(timeoutCandidate) && timeoutCandidate > 0 ? timeoutCandidate : LOGS_TOOL_DEFAULT_TIMEOUT_MINUTES; | ||
| const resolvedTimeoutMs = Math.ceil(timeoutMinutes * 60 * 1000) + TOOL_CALL_TIMEOUT_BUFFER_MS; | ||
| return Math.max(TOOL_CALL_TIMEOUT_MS, resolvedTimeoutMs); |
There was a problem hiding this comment.
[/diagnosing-bugs] The Math.max(TOOL_CALL_TIMEOUT_MS, resolvedTimeoutMs) lower-bound guard is correct, but a very small explicit timeout (e.g. 0.001 minutes → ~75ms resolved) will silently fall through to 120 s rather than the logs-specific 5-minute default — potentially masking misconfigured callers.
💡 Consider clamping to the default floor instead
const resolvedTimeoutMs = Math.ceil(timeoutMinutes * 60 * 1000) + TOOL_CALL_TIMEOUT_BUFFER_MS;
// Clamp to the logs default floor so tiny values behave consistently
const logsDefaultMs = Math.ceil(LOGS_TOOL_DEFAULT_TIMEOUT_MINUTES * 60 * 1000) + TOOL_CALL_TIMEOUT_BUFFER_MS;
return Math.max(logsDefaultMs, resolvedTimeoutMs);If the intent is "never go below the global cap", the current code is correct; but if the intent is "honour the logs default when no useful timeout is provided", a tiny positive value still bypasses it. A comment clarifying which invariant is desired would also help future readers.
@copilot please address this.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 42.4 AIC · ⌖ 4.65 AIC · ⊞ 5.6K
Comment /review to run again
| } | ||
|
|
||
| const timeoutCandidate = Number(toolArgs?.timeout); | ||
| const timeoutMinutes = Number.isFinite(timeoutCandidate) && timeoutCandidate > 0 ? timeoutCandidate : LOGS_TOOL_DEFAULT_TIMEOUT_MINUTES; |
There was a problem hiding this comment.
Unbounded caller-supplied timeout allows indefinite bridge process hold: no upper cap is enforced, so timeout: 99999999 yields ~115 days of HTTP timeout.
💡 Suggested fix
Add a reasonable maximum before the buffer calculation:
const MAX_LOGS_TIMEOUT_MINUTES = 60;
const timeoutMinutes = Number.isFinite(timeoutCandidate) && timeoutCandidate > 0
? Math.min(timeoutCandidate, MAX_LOGS_TIMEOUT_MINUTES)
: LOGS_TOOL_DEFAULT_TIMEOUT_MINUTES;Without this cap, a misbehaving agent or adversarial tool argument can hold an HTTP connection open for an arbitrarily long time, potentially exhausting process/socket resources in the bridge runner.
| const timeoutCandidate = Number(toolArgs?.timeout); | ||
| const timeoutMinutes = Number.isFinite(timeoutCandidate) && timeoutCandidate > 0 ? timeoutCandidate : LOGS_TOOL_DEFAULT_TIMEOUT_MINUTES; | ||
| const resolvedTimeoutMs = Math.ceil(timeoutMinutes * 60 * 1000) + TOOL_CALL_TIMEOUT_BUFFER_MS; | ||
| return Math.max(TOOL_CALL_TIMEOUT_MS, resolvedTimeoutMs); |
There was a problem hiding this comment.
Math.max silently inflates explicitly-short user-supplied timeouts with no feedback: if a caller passes timeout: 1 intending a 1-minute bound, the function quietly returns 120 000 ms (2 minutes) instead.
💡 Details
The Math.max(TOOL_CALL_TIMEOUT_MS, resolvedTimeoutMs) lower-bound is useful as a defensive floor, but when the resolved timeout is below that floor the caller receives no signal that their requested value was ignored.
If the intent is "never go below 120s for protocol safety", consider logging a debug warning when the floor kicks in, or document this behavior in the JSDoc so callers know the effective minimum:
/**
* ...
* `@returns` {number} Timeout in ms. Always at least TOOL_CALL_TIMEOUT_MS (120s).
*/The silent override means a test that passes timeout: 1 (e.g., to speed up a test) will still wait 120 s on network failure, which is surprising.
| return TOOL_CALL_TIMEOUT_MS; | ||
| } | ||
|
|
||
| const timeoutCandidate = Number(toolArgs?.timeout); |
There was a problem hiding this comment.
Number() accepts whitespace-padded strings silently: Number(' 5 ') === 5 passes the validity check, so a model-generated timeout: " 5 " is accepted as valid without any trimming or type check.
💡 Details
This is benign today because whitespace-padded numbers work correctly at the math level. However, the guard's intent is to validate that the caller passed a real numeric timeout, and Number() is a blunt instrument — it also accepts null (→ 0, caught by > 0), true (→ 1), and [] (→ 0).
A more explicit check avoids surprising implicit coercions:
const timeoutCandidate = typeof toolArgs?.timeout === 'number' ? toolArgs.timeout : NaN;
const timeoutMinutes = Number.isFinite(timeoutCandidate) && timeoutCandidate > 0
? timeoutCandidate
: LOGS_TOOL_DEFAULT_TIMEOUT_MINUTES;This makes the function's validation contract explicit: only a numeric timeout is honoured.
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…logs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All remaining unresolved review threads addressed in the latest push:
Tests updated: |
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
All review threads addressed in prior commits; no further code changes were needed this pass.
Hand-off: CI must be re-triggered by a maintainer (close/reopen the PR, |
|
@copilot please run the Unresolved review threads to close out:
Run: https://github.com/github/gh-aw/actions/runs/29788028039 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
All 5 threads are outdated (
Local |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29789600762 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
No remaining code changes needed. Current state on HEAD
|
🔍 PR Triage — Run §29791496271
Rationale: Fixed 120s MCP bridge timeout truncating daily audit log fetches. Small targeted change with test coverage. Already labeled fast_track — awaiting review.
|
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
No code changes were needed this pass.
Next concrete maintainer action: The merge gate is the 5 unresolved (but outdated) review threads in GitHub's UI. A maintainer needs to:
|
|
🎉 This pull request is included in a new release. Release: |
Daily audits were frequently computing on incomplete run sets because the MCP CLI bridge enforced a fixed 120s
tools/calltimeout, truncating longlogsfetches. This change aligns bridge timeout behavior withlogsworkload duration so fleet-health and API-cost reporting can consume complete daily data.Bridge timeout behavior
TOOL_CALL_TIMEOUT_MSusage fortools/callwith per-tool timeout resolution.logscalls now derive transport timeout fromarguments.timeout(minutes) with a response buffer, instead of hard-failing at 120s.logstools retain existing 120s behavior.logs-specific fallback semanticslogstimeout floor in the bridge whentimeoutis missing/invalid.Math.max(...)to avoid regressions for short calls.Targeted unit coverage
logstimeout unchanged,logstimeout path,logstimeout path,Run: https://github.com/github/gh-aw/actions/runs/29781538011
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
branch update requested from https://github.com/github/gh-aw/actions/runs/29788028039
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.