fix: propagate tools.startup-timeout to gateway.startupTimeout (prevents safeoutputs eviction on slow runners)#46875
Conversation
…vent safeoutputs timeout Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Propagates MCP startup timeout configuration to the gateway to prevent slow backends from being prematurely evicted.
Changes:
- Adds and renders
gateway.startupTimeout. - Defaults the gateway timeout to 120 seconds.
- Updates tests, golden fixtures, and generated workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/tools_types.go |
Adds runtime startup-timeout state. |
pkg/workflow/mcp_gateway_config.go |
Builds the gateway timeout value. |
pkg/workflow/mcp_renderer.go |
Emits startupTimeout in gateway JSON. |
pkg/workflow/mcp_gateway_config_test.go |
Tests default, literal, and expression handling. |
pkg/workflow/mcp_renderer_test.go |
Tests timeout rendering behavior. |
pkg/workflow/codex_engine_test.go |
Updates expected MCP configuration. |
pkg/workflow/testdata/**/*.golden |
Updates generated-output expectations. |
.github/workflows/*.lock.yml |
Recompiles 260 workflows with the gateway timeout. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 280/280 changed files
- Comments generated: 1
- Review effort level: Medium
| if workflowData.ToolsStartupTimeout != "" { | ||
| if n := templatableIntValue(&workflowData.ToolsStartupTimeout); n > 0 { | ||
| startupTimeout = n | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ 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. ✅ |
🧪 Test Quality Sentinel Report❌ Test Quality Score: 27/100 — Poor
📊 Metrics (16 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two issues need fixing before merge
1. Expression split-brain (high): When tools.startup-timeout is a GitHub Actions expression, the gateway JSON is compiled with startupTimeout: 120 but the engine receives the raw expression string via GH_AW_STARTUP_TIMEOUT and evaluates it at runtime. If that expression resolves to >120s, the gateway will evict backends the engine is still waiting for — silently reproducing the original bug for expression-valued timeouts. No warning is emitted to the user.
2. Fragile > 0 guard in renderer (medium): The renderer only emits startupTimeout when StartupTimeout > 0, which contradicts the PR's own stated invariant ("this field is now always present"). Correctness depends on buildMCPGatewayConfig always producing 120; any future caller that zero-initialises the struct silently drops the field and reintroduces the 30s-eviction bug.
What works well
- Core fix is sound: the 30s default is overridden for all existing compiled workflows.
templatableIntValueis the correct tool for compile-time expression detection.- Test coverage for the new cases is present and meaningful.
- 260 lock files recompiled consistently.
🔎 Code quality review by PR Code Quality Reviewer · 67.7 AIC · ⌖ 4.74 AIC · ⊞ 5.6K
Comment /review to run again
| if workflowData.ToolsStartupTimeout != "" { | ||
| if n := templatableIntValue(&workflowData.ToolsStartupTimeout); n > 0 { | ||
| startupTimeout = n | ||
| } |
There was a problem hiding this comment.
Expression-valued startup-timeout creates a split-brain: gateway gets 120s hardcoded while the engine uses the runtime-evaluated expression. If the expression resolves to >120 at runtime, the gateway will evict backends the engine is still polling, silently reproducing the original bug.
💡 Details and suggested fix
buildMCPGatewayConfig falls back to 120 when templatableIntValue can't evaluate the expression:
if n := templatableIntValue(&workflowData.ToolsStartupTimeout); n > 0 {
startupTimeout = n
}Meanwhile engine_helpers.go passes the raw expression string to the engine via GH_AW_STARTUP_TIMEOUT, which is evaluated at runtime to whatever the user set (e.g. 300s). The gateway JSON is already compiled with "startupTimeout": 120.
Result: a user who sets tools.startup-timeout: 300 via an expression gets a gateway that kills slow-starting backends after 120s while the engine patiently waits up to 300s — the original eviction bug, just with a higher threshold.
At minimum, emit a log warning when the fallback is applied so operators can see it in debug output:
if workflowData.ToolsStartupTimeout != "" {
if n := templatableIntValue(&workflowData.ToolsStartupTimeout); n > 0 {
startupTimeout = n
} else {
mcpGatewayConfigLog.Printf("tools.startup-timeout %q is a GitHub Actions expression and cannot be evaluated at compile time; gateway.startupTimeout will be fixed at %ds", workflowData.ToolsStartupTimeout, startupTimeout)
}
}The only real fix is to document this limitation explicitly (the gateway JSON does not support runtime expressions) and surface it as a compiler warning to the user.
| // Without this field, the gateway evicts safeoutputs backends that start after 30s | ||
| // and permanently caches zero tools, causing silent data loss. The gh-aw default is | ||
| // 120 seconds (constants.DefaultMCPStartupTimeout); users can override via tools.startup-timeout. | ||
| if options.GatewayConfig.StartupTimeout > 0 { |
There was a problem hiding this comment.
StartupTimeout > 0 guard contradicts the stated invariant "this field is now always present" and creates a silent regression footgun.
💡 Details and suggested fix
The PR description says "Because the default is always 120, this field is now always present — preventing the gateway from applying its own 30s default." But the renderer guard:
if options.GatewayConfig.StartupTimeout > 0 {
fmt.Fprintf(&configBuilder, ",\n \"startupTimeout\": %d", options.GatewayConfig.StartupTimeout)
}means the field is only emitted when the caller happens to set a positive value. The safety property depends entirely on the invariant that buildMCPGatewayConfig always produces StartupTimeout = 120. Any future caller that zero-initialises MCPGatewayRuntimeConfig (e.g. in tests, new code paths, or if the default is ever changed to 0 to mean use
There was a problem hiding this comment.
Review: propagate tools.startup-timeout to gateway.startupTimeout
The fix is correct and well-scoped. Key observations:
StartupTimeoutfield onMCPGatewayRuntimeConfigis correctly taggedyaml:"-"(not user-settable, computed-only).buildMCPGatewayConfigalways sets the 120s default and overrides only when the user value parses as a positive integer — GitHub Actions expressions correctly fall back to the default.RenderJSONMCPConfigguards with> 0, ensuring zero is never emitted.- All 260 lock files are uniformly regenerated.
- Unit tests cover the four meaningful renderer cases (default, custom, zero-omit, minimum) and the two new config cases (explicit integer, expression fallback).
No blocking issues. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 46 AIC · ⌖ 4.36 AIC · ⊞ 5K
…eout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (176 new lines in business logic directories) but does not have a linked Architecture Decision Record (ADR). 📄 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. The fix is correct and well-tested — commenting (not blocking) on two low-risk gaps.
Positive Highlights
- Root cause correctly identified:
startupTimeoutwas never emitted, so the gateway silently used its 30s default. - Clean propagation chain:
ToolsStartupTimeoutstring → integer computation → struct field → JSON emission. - Regression tests cover explicit integer, expression fallback, and four renderer cases.
- All 260 lock files recompiled consistently.
yaml:"-"tag correctly prevents the field leaking into serialised YAML.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 62 AIC · ⌖ 4.68 AIC · ⊞ 6.7K
Comment /matt to run again
| // slightly late. GitHub Actions expressions (non-numeric strings) fall back to the default. | ||
| startupTimeout := int(constants.DefaultMCPStartupTimeout / time.Second) | ||
| if workflowData.ToolsStartupTimeout != "" { | ||
| if n := templatableIntValue(&workflowData.ToolsStartupTimeout); n > 0 { |
There was a problem hiding this comment.
[/tdd] Negative integer values (e.g. startup-timeout: -1) silently fall through to the 120s default because n > 0 rejects them. Add a test case for -1 and "0" to document this clamping behaviour explicitly.
💡 Suggested test cases
{
name: "clamps negative startup-timeout to default",
workflowData: &WorkflowData{
ToolsStartupTimeout: "-1",
SandboxConfig: &SandboxConfig{},
},
expected: &MCPGatewayRuntimeConfig{
// ... other fields ...
StartupTimeout: int(constants.DefaultMCPStartupTimeout / time.Second),
},
},
{
name: "clamps zero startup-timeout to default",
workflowData: &WorkflowData{
ToolsStartupTimeout: "0",
SandboxConfig: &SandboxConfig{},
},
expected: &MCPGatewayRuntimeConfig{
StartupTimeout: int(constants.DefaultMCPStartupTimeout / time.Second),
},
},Without these tests, a user who sets startup-timeout: 0 or -1 gets the 120s default with no warning or documentation. The tests make that contract explicit.
@copilot please address this.
| // Without this field, the gateway evicts safeoutputs backends that start after 30s | ||
| // and permanently caches zero tools, causing silent data loss. The gh-aw default is | ||
| // 120 seconds (constants.DefaultMCPStartupTimeout); users can override via tools.startup-timeout. | ||
| if options.GatewayConfig.StartupTimeout > 0 { |
There was a problem hiding this comment.
[/diagnosing-bugs] The guard StartupTimeout > 0 means that if StartupTimeout is explicitly set to 0 (e.g. via a code path that zeroes it out), the field is silently omitted and the gateway falls back to its 30s default — the exact failure mode this fix is meant to prevent. Since buildMCPGatewayConfig always sets this to at least 120, the guard is safe today, but it creates a latent risk for future callers. Consider removing the guard or documenting the invariant clearly.
💡 Options
Option A — always emit (simplest, safest):
fmt.Fprintf(&configBuilder, ",\n \"startupTimeout\": %d", options.GatewayConfig.StartupTimeout)Option B — make the invariant explicit with a comment:
// StartupTimeout is always >= DefaultMCPStartupTimeout (120s) when set via buildMCPGatewayConfig.
// The > 0 guard protects against zero-value configs in tests or future callers.
if options.GatewayConfig.StartupTimeout > 0 {
...
}The renderer test already covers StartupTimeout == 0 (omits the field), so the current contract is tested. The concern is whether future callers might accidentally pass zero expecting 120s behaviour.
@copilot please address this.
|
@copilot run pr-finisher skill |
|
@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.
|
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
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.
Smoke review done. Tools grunt okay.
📰 BREAKING: Report filed by Smoke Copilot · 74.8 AIC · ⌖ 7.29 AIC · ⊞ 19K
Comment /smoke-copilot to run again
Add label smoke to run again
|
🎉 This pull request is included in a new release. Release: |
MCP Gateway's built-in
startupTimeoutis 30s. When asafeoutputsbackend starts later than that (common on busy runners), the gateway evicts it, permanently caches zero tools, and the workflow reports success with no output. gh-aw was not emittinggateway.startupTimeout, so the gateway always used its 30s default instead of gh-aw's documented 120s default.Changes
MCPGatewayRuntimeConfig(tools_types.go): NewStartupTimeout intfield, emitted asgateway.startupTimeoutin the JSON config pipe.buildMCPGatewayConfig(mcp_gateway_config.go): ComputesstartupTimeoutfromtools.startup-timeout(integer seconds); falls back toconstants.DefaultMCPStartupTimeout(120s) when unset or when value is a GitHub Actions expression not evaluable at compile time.timeimport added.RenderJSONMCPConfig(mcp_renderer.go): Emits"startupTimeout": Nin thegatewaysection wheneverStartupTimeout > 0. Because the default is always 120, this field is now always present — preventing the gateway from applying its own 30s default.Every compiled workflow now includes:
.lock.ymlfiles recompiled.buildMCPGatewayConfig(two new cases: explicit integer and expression fallback) andRenderJSONMCPConfig(four cases covering default, custom, zero, and minimum).codex_engine_test.goupdated to reflect the new field in expected output.pr-sous-chef run https://github.com/github/gh-aw/actions/runs/29761732160
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.