Repository navigation
Retry MCP gateway startup and identify pre-agent checkout failures - #72970
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The retry and diagnostics changes contain correctness issues that can misreport failures and substantially extend startup time.
Details
ERR_SAFE_OUTPUTS_GATEWAY_CONFIGis retried despite being deterministic configuration failure.- Each retry gets a fresh full health/readiness timeout, potentially doubling startup duration.
- Generic
Executestep matching can misattribute pre-agent failures when user-defined steps share that prefix.
The specialist pass completed; its advisory findings were adjudicated before publication.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~5 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| activeGatewayPid = null; | ||
| clearGatewayStartupMarker(getGatewayStartupMarkerPath()); | ||
| const message = redactGatewayDiagnostics(getErrorMessage(err)); | ||
| if (message.startsWith(`${ERR_CONFIG}:`)) throw err; |
There was a problem hiding this comment.
This retries deterministic safe-outputs gateway configuration failures, contradicting the fail-fast behavior and producing a misleading startup-retry error.
💡 Why this matters
waitForSafeOutputsReadiness throws ERR_SAFE_OUTPUTS_GATEWAY_CONFIG, but this guard only exempts ERR_CONFIG:. The closure is therefore relaunched and the final diagnostic says the gateway failed after two startup attempts even though configuration cannot recover by restarting. Treat this configuration error code as non-retryable and add a regression test.
There was a problem hiding this comment.
ERR_SAFE_OUTPUTS_GATEWAY_CONFIG now fails fast through the outer retry wrapper, with regression coverage preserving its original code. Commit 0858968.
| } | ||
| child.stdin.write(JSON.stringify(configObj)); | ||
| child.stdin.end(); | ||
| const { gatewayPid, stderrPath } = await startGatewayWithRetry(async () => { |
There was a problem hiding this comment.
The retry gives each full startup attempt a fresh health/readiness budget, so a broken gateway can consume roughly twice the configured startup timeout before the job fails.
💡 Why this matters
startupDeadline is derived from the new gatewayStartTime inside the retried closure. A health failure can already consume the complete backend startup budget, then the second launch receives another complete budget and its own 5-second initialization wait. On workflows near their job timeout this turns a bounded startup failure into a substantially longer timeout. Share one overall deadline across attempts or limit the retry to the launch/liveness phase, and test the bound.
There was a problem hiding this comment.
Health and readiness now share one startup deadline across outer retries, and initialization/retry waits are capped by the remaining budget. Regression tests cover the shared deadline. Commit 0858968.
| } | ||
| const steps = agentJob.steps?.slice(); | ||
| const steps = agentJob.steps?.slice() || []; | ||
| const setupBoundary = steps.findIndex(step => step.name === "Start MCP Gateway" || (typeof step.name === "string" && step.name.startsWith("Execute "))); |
There was a problem hiding this comment.
Using any step beginning with Execute as the setup boundary can misattribute failures when a workflow contains an unrelated user step with that name.
💡 Why this matters
The classifier stops its pre-agent search at the first matching display name, so a custom Execute migrations step before the generated checkout can prevent a real checkout failure from being recognized, or mark the wrong step as pre-agent. Match the generated engine step names (or a stable identifier) rather than the generic prefix, and add a regression case for an unrelated Execute ... step.
There was a problem hiding this comment.
Failure attribution now recognizes generated Execute … CLI steps rather than every Execute … label; a regression test covers an unrelated Execute migrations step preceding checkout. Commit 0858968.
There was a problem hiding this comment.
🟡 Changes recommended
The outer retry restarts the container on permanent configuration and authentication/protocol failures, obscuring configuration error codes and repeating non-retryable handshakes.
1 open issue
What changed in this PR
This PR hardens gh-aw’s pre-agent setup for #72887 by adding gateway startup recovery and more specific failure diagnostics.
Changes:
- Retries gateway startup once after cleanup.
- Identifies pre-agent gateway and checkout failures using Actions metadata, with merge-ref guidance for confirmed trigger-ref checkouts.
- Adds regression tests and model-catalog fields to audit and log schemas.
| File | Description |
|---|---|
schemas/logs-jsonl.schema.json |
Adds model-catalog metadata to log records. |
schemas/audit.schema.json |
Adds model-catalog metadata to audit output. |
actions/setup/js/start_mcp_gateway.test.cjs |
Tests retry limits, recovery, and configuration fail-fast behavior. |
actions/setup/js/start_mcp_gateway.cjs |
Adds startup retry, cleanup, and failure context. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests pre-agent attribution and merge-ref diagnostics. |
actions/setup/js/handle_agent_failure.cjs |
Classifies setup failures and renders targeted guidance. |
Review effort: Balanced (auto)
| activeGatewayPid = null; | ||
| clearGatewayStartupMarker(getGatewayStartupMarkerPath()); | ||
| const message = redactGatewayDiagnostics(getErrorMessage(err)); | ||
| if (message.startsWith(`${ERR_CONFIG}:`)) throw err; |
There was a problem hiding this comment.
The readiness helper now propagates an explicit non-retryable signal for authentication/protocol failures, and the outer wrapper preserves the original error without repeating the handshake. Tests cover both cases. Commit 0858968.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — the fix itself is correct and well covered by tests (merge-ref checkout, post-agent checkout exclusion, config-error fail-fast, single-retry success/exhaustion). Two non-blocking maintainability suggestions below.
📋 Key Themes & Highlights
Key Themes
- Readability debt: the
setupFailureboundary-detection expression inhandle_agent_failure.cjspacks several boolean conditions and string predicates into one unnamed ternary, making it hard to verify by inspection. - Duplicated literal: the
"Start MCP Gateway"step-name string is repeated acrossstart_mcp_gateway.cjsandhandle_agent_failure.cjswith no shared constant, so a future step rename could silently break pre-agent attribution.
Positive Highlights
- ✅
startGatewayWithRetrycleanly separates config errors (fail-fast) from transient errors (retry once), and resource cleanup (stopGatewayProcess, marker clearing, fd closing) is handled consistently across both failure paths. - ✅ New
handle_agent_failure.test.cjscases cover the tricky edge cases that matter most for correctness: merge-ref checkout failures, non-default checkout/gateway failures without assuming a ref, and (critically) not misclassifying a post-agent checkout as pre-agent. - ✅ Redaction-failure priority is preserved —
preAgentFailuredoes not override it sincesetupFailureis only consulted after the!startupFailure && redactionStepcheck.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~103 AIC · ⌖ ~16 AIC · ⊞ 10.2K
Comment /matt to run again
| const steps = agentJob.steps?.slice(); | ||
| const steps = agentJob.steps?.slice() || []; | ||
| const setupBoundary = steps.findIndex(step => step.name === "Start MCP Gateway" || (typeof step.name === "string" && step.name.startsWith("Execute "))); | ||
| const setupFailure = |
There was a problem hiding this comment.
[/codebase-design] The setupFailure ternary (lines 2017-2025) packs five conditions and three string-matching predicates (startsWith("Execute "), startsWith("Checkout "), literal "Start MCP Gateway") into a single unnamed expression. This is hard to verify by inspection and easy to subtly break on the next step rename.
💡 Suggested refactor
Extract named predicates/constants so the logic reads as a spec:
const isExecuteStep = step => typeof step.name === "string" && step.name.startsWith("Execute ");
const isCheckoutStep = step => typeof step.name === "string" && step.name.startsWith("Checkout ");
const isSetupStep = step => step.name === "Start MCP Gateway" || isCheckoutStep(step);
const setupBoundary = steps.findIndex(step => step.name === "Start MCP Gateway" || isExecuteStep(step));
const agentNeverRan = setupBoundary >= 0 && !steps.some(step => isExecuteStep(step) && step.conclusion !== "skipped");
const boundaryFailed = setupBoundary >= 0 && ["skipped", "failure", "timed_out"].includes(steps[setupBoundary].conclusion);
const anyExecuteSkipped = steps.some(step => isExecuteStep(step) && step.conclusion === "skipped");
const setupFailure =
boundaryFailed && agentNeverRan && (startupFailure || steps[setupBoundary].conclusion === "skipped" || anyExecuteSkipped)
? steps.slice(0, setupBoundary + (steps[setupBoundary].name === "Start MCP Gateway" ? 1 : 0)).find(step => ["failure", "timed_out"].includes(step.conclusion) && isSetupStep(step))
: undefined;This reduces the amount of boolean algebra a reviewer must hold in their head at once and makes the step-name matching reusable with the literal already duplicated at start_mcp_gateway.cjs:746 and handle_agent_failure.cjs:4150.
@copilot please address this.
There was a problem hiding this comment.
I extracted named step predicates and split the setup-boundary logic into explicit conditions while preserving redaction-failure priority. Commit 0858968.
| const message = redactGatewayDiagnostics(getErrorMessage(err)); | ||
| if (message.startsWith(`${ERR_CONFIG}:`)) throw err; | ||
| if (attempt === 2) { | ||
| throw codedError(`Pre-agent failure in step "Start MCP Gateway" after 2 startup attempts: ${message}`, ERR_SYSTEM, { cause: err }); |
There was a problem hiding this comment.
[/diagnosing-bugs] The string "Start MCP Gateway" is duplicated here and in handle_agent_failure.cjs line 2016/2023 (step-name matching) with no shared constant. If this Action step is ever renamed, pre-agent failure attribution silently stops matching while this error message keeps saying the same thing, producing a misleading-but-plausible diagnostic instead of a loud failure.
💡 Suggested fix
Extract a shared constant (e.g. in constants.cjs or a small module both files import) such as STEP_NAME_START_MCP_GATEWAY = "Start MCP Gateway", and use it in both files. Add a cheap regression test that fails if the literal drifts between the two files (e.g. assert the generated YAML step name from checkout_step_generator.go/mcp_setup_generator.go matches the literal used here).
@copilot please address this.
There was a problem hiding this comment.
Both startup diagnostics and failure attribution now use the shared STEP_NAME_START_MCP_GATEWAY constant from constants.cjs. Commit 0858968.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Smoke Copilot Auto completed successfully!
|
|
✨ Gemini awakens... Smoke Gemini begins its journey on this pull request... |
|
🔧 Starting tool validation... Agent Container Smoke Test is checking the agent container tools... |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟
|
|
🌅 Haiku Gateway stumbles once, Inspired by PR #72970: retrying MCP gateway startup and clearer pre-agent checkout failures.
|
|
Commit pushed:
|
Codex smoke test: FAIL✅ GitHub MCP · ✅ Serena (3 symbols) · ❌ Playwright (policy blocked) · ❌ native web fetch (HTTP 400)
|
|
Smoke Copilot: partial. ✅ PRs, mcpscripts, curl, file, build, artifact, memory, sub-agent, check run; ❌ Serena CLI, Playwright, discussions, dispatch, review, LSP not run. Overall: FAIL. cc Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "host.docker.internal"
- "update.googleapis.com"See Network Configuration for more information.
|
Comment MemoryPeek at saved memory (smoke)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "host.docker.internal"
- "update.googleapis.com"See Network Configuration for more information.
|
This comment has been minimized.
This comment has been minimized.
💥 Smoke Test Run 41084447691Core Tests: ✅✅✅✅✅✅✅✅✅✅✅✅
PR Review Tests (#72970):
Overall: PARTIAL (2 skipped, all others passed) Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "update.googleapis.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
update.googleapis.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "update.googleapis.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · claude · sonnet46 · ~78 AIC · ⌖ ~8 AIC · ⊞ 926
Comment /smoke-claude to run again
| * @param {number} [startupDeadline] | ||
| * @returns {Promise<T>} | ||
| */ | ||
| async function startGatewayWithRetry(start, startupDeadline = Infinity) { |
There was a problem hiding this comment.
👍 Nice addition of startGatewayWithRetry — the single retry approach strikes a good balance between resilience and avoiding infinite loops. Consider documenting the rationale for exactly one retry in the JSDoc to help future maintainers.
| const ALLOWED_AGENT_CONCLUSIONS = new Set(["success", "failure", "cancelled", "skipped", "neutral", "timed_out", "action_required", "stale"]); | ||
| const EXECUTE_AGENT_STEP_NAME_RE = /^Execute .+ CLI$/; | ||
|
|
||
| function isExecuteAgentStep(step) { |
There was a problem hiding this comment.
Good decomposition into small, focused predicate functions (isExecuteAgentStep, isGatewayStartupStep, isCheckoutStep). This makes the failure attribution logic much easier to follow and test in isolation. ✅
Gateway and checkout failures before agent execution can surface as generic agent failures. This change adds one gateway startup retry and step-specific diagnostics, including pull-request merge refs.
<!-- START COPILOT CODING AGENT SUFFIX -->
Changeset
✨ PR Review Safe Output Test - Run 41084447691
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
update.googleapis.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.