Fail fast on empty safeoutputs schema and add Copilot soft-time convergence guard#43057
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
|
|
There was a problem hiding this comment.
Pull request overview
Hardens scheduled agent runs by (1) preventing silent degraded execution when the safeoutputs tool schema is unexpectedly empty and (2) adding a Copilot harness soft-time budget to stop retry thrash early and emit structured report_incomplete output before the GitHub Actions hard timeout.
Changes:
- Enforce non-empty
safeoutputsschema when mounting MCP servers as CLIs, with fallback recovery fromGH_AW_SAFE_OUTPUTS_TOOLS_PATH/ generatedsafeoutputs/tools.json. - Enforce non-empty
safeoutputsschema in the CLI bridge help-path (fail fast if tools remain empty). - Add a Copilot harness soft timeout guard derived from
GH_AW_TIMEOUT_MINUTES, with tests for deadline computation.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/mount_mcp_as_cli.cjs | Adds safeoutputs empty-schema recovery + fail-fast behavior when tools remain empty. |
| actions/setup/js/mount_mcp_as_cli.test.cjs | Adds unit tests for safeoutputs fallback recovery and fail-fast behavior. |
| actions/setup/js/mcp_cli_bridge.cjs | Ensures safeoutputs CLI has a non-empty tools schema (fallback + fail-fast). |
| actions/setup/js/mcp_cli_bridge.test.cjs | Adds unit tests covering safeoutputs schema fallback and fail-fast behavior. |
| actions/setup/js/copilot_harness.cjs | Adds a soft timeout guard to exit retry loops early and emit structured incomplete output. |
| actions/setup/js/copilot_harness.test.cjs | Adds tests validating soft timeout guard behavior and deadline calculation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Low
| const core = global.core; | ||
| const fallbackPath = process.env.GH_AW_SAFE_OUTPUTS_TOOLS_PATH || `${process.env.RUNNER_TEMP}/gh-aw/safeoutputs/tools.json`; | ||
| if (fallbackPath && fallbackPath !== toolsFile) { | ||
| const fallbackTools = loadTools(fallbackPath); | ||
| if (fallbackTools.length > 0) { | ||
| core.warning(`[${serverName}] tools cache ${toolsFile} is empty; recovered ${fallbackTools.length} tool(s) from ${fallbackPath}`); | ||
| return fallbackTools; | ||
| } | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (6 tests)
Verdict
Inflation check: test:prod ratios — Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting on testability gaps and a few defensive-coding improvements. No blocking issues; the overall direction is solid.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps (most important):
buildSoftTimeoutGuardtests use a hardcoded magic number and miss edge cases (0,NaN, sub-buffer timeouts). The loop-level behaviour (guard fires →lastExitCode=1→emitInfrastructureIncomplete) is tested only indirectly by the existing harness tests. global.corecoupling inmcp_cli_bridge.cjs: Unlikemount_mcp_as_cli.cjs,ensureSafeOutputsToolsin the bridge usesglobal.corewithout a null guard. This makes it harder to test and can produce a crypticTypeErrorin unexpected environments.RUNNER_TEMPnot guarded in bridge fallback path: If the env var is unset, the fallback path becomes'undefined/...and fails silently rather than with an actionable error.- Duplicate
emitInfrastructureIncompletecall-sites: Two identical message strings; easy to diverge on future edits.
Positive Highlights
- ✅ Excellent fail-fast semantics: errors are thrown rather than swallowing failures silently
- ✅ Two-layer recovery strategy (MCP response empty → fallback file → hard error) is clean and progressive
- ✅ Both new modules export their helpers for testing, making them properly testable
- ✅
mount_mcp_as_cli.cjsalready follows the preferred pattern of injectingcoreas a parameter — worth bringingmcp_cli_bridge.cjsinto line - ✅ PR description is thorough and links each code change to a concrete observed failure mode
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 112.3 AIC · ⌖ 8.16 AIC · ⊞ 6.6K
Comment /matt to run again
| if (serverName !== SAFEOUTPUTS_SERVER_NAME || tools.length > 0) { | ||
| return tools; | ||
| } | ||
| const core = global.core; |
There was a problem hiding this comment.
[/diagnosing-bugs] global.core is accessed without a null guard; if it is undefined the recovery-warning path throws a cryptic TypeError instead of the intended fail-fast error.
💡 Suggestion
Consider accepting core as an injected parameter — matching the pattern in mount_mcp_as_cli.cjs — rather than accessing global.core directly:
function ensureSafeOutputsTools(tools, serverName, toolsFile, core = global.core) {
if (serverName !== SAFEOUTPUTS_SERVER_NAME || tools.length > 0) return tools;
if (!core) throw new Error('[' + serverName + '] global.core is not set');
// ...
}This also makes the function independently testable without mutating global.
@copilot please address this.
| // Unified retry loop for CLI and driver modes. | ||
| // --continue is a CLI concept; in SDK mode retries always restart the session fresh. | ||
| for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { | ||
| if (softTimeoutGuard && Date.now() >= softTimeoutGuard.softDeadlineMs) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The soft-timeout guard is checked at loop entry and after backoff sleep, but not during a long-running attempt — if a single Copilot invocation runs past the soft deadline, the guard will only fire on the next iteration, leaving fewer than 90 seconds to emit safe-output.
💡 Suggestion
If Copilot invocations can individually outlast the buffer, consider passing an AbortSignal that fires at softDeadlineMs to the subprocess spawn (or using a Promise.race), so the guard can preempt a running attempt rather than waiting for it to complete.
This is an inherent limitation of a polling guard; document the assumption that individual attempts complete within the buffer window.
@copilot please address this.
| const guard = buildSoftTimeoutGuard(10_000, { GH_AW_TIMEOUT_MINUTES: "15" }); | ||
| expect(guard).toEqual({ | ||
| timeoutMinutes: 15, | ||
| softDeadlineMs: 820000, |
There was a problem hiding this comment.
[/tdd] The test hardcodes softDeadlineMs: 820000 — a magic number derived from 10_000 + (15*60*1000 - 90*1000). If SOFT_TIMEOUT_BUFFER_MS is changed the test will break silently with a misleading failure message.
💡 Suggestion
Express the expected value using the same arithmetic so the test documents intent and tracks refactors automatically:
const EXPECTED_BUFFER_MS = 90 * 1000; // matches SOFT_TIMEOUT_BUFFER_MS
const guard = buildSoftTimeoutGuard(10_000, { GH_AW_TIMEOUT_MINUTES: '15' });
expect(guard.softDeadlineMs).toBe(10_000 + 15 * 60 * 1000 - EXPECTED_BUFFER_MS);@copilot please address this.
|
|
||
| describe("soft timeout guard", () => { | ||
| it("returns null when GH_AW_TIMEOUT_MINUTES is missing", () => { | ||
| expect(buildSoftTimeoutGuard(1_000, {})).toBeNull(); |
There was a problem hiding this comment.
[/tdd] The test suite for buildSoftTimeoutGuard is missing edge cases that could cause hard-to-diagnose field failures: GH_AW_TIMEOUT_MINUTES=0, a non-numeric string (e.g. 'NaN'), and a value so small that hardTimeoutMs - SOFT_TIMEOUT_BUFFER_MS < 1000 (the Math.max floor).
💡 Missing tests to add
it('returns null for GH_AW_TIMEOUT_MINUTES=0', () => {
expect(buildSoftTimeoutGuard(0, { GH_AW_TIMEOUT_MINUTES: '0' })).toBeNull();
});
it('returns null for non-numeric GH_AW_TIMEOUT_MINUTES', () => {
expect(buildSoftTimeoutGuard(0, { GH_AW_TIMEOUT_MINUTES: 'NaN' })).toBeNull();
});
it('floors softDeadlineMs to at least 1000ms after start when timeout < buffer', () => {
// 1 minute = 60000ms < SOFT_TIMEOUT_BUFFER_MS (90000ms) → Math.max floor
const guard = buildSoftTimeoutGuard(0, { GH_AW_TIMEOUT_MINUTES: '1' });
expect(guard.softDeadlineMs).toBe(1000);
});@copilot please address this.
| } | ||
| const core = global.core; | ||
| const fallbackPath = process.env.GH_AW_SAFE_OUTPUTS_TOOLS_PATH || `${process.env.RUNNER_TEMP}/gh-aw/safeoutputs/tools.json`; | ||
| if (fallbackPath && fallbackPath !== toolsFile) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The condition if (fallbackPath && fallbackPath !== toolsFile) silently skips recovery when the fallback resolves to the same path as the primary tools file — but if the primary file was empty, the fallback would also be empty, so the skip is correct. However this also means when RUNNER_TEMP is unset, fallbackPath could become 'undefined/gh-aw/safeoutputs/tools.json', which will silently fail to load rather than failing fast.
💡 Suggestion
Guard against an unset RUNNER_TEMP explicitly:
const runnerTemp = process.env.RUNNER_TEMP;
if (!runnerTemp) {
throw new Error('[' + serverName + '] RUNNER_TEMP is not set; cannot locate fallback tools.json');
}
const fallbackPath = process.env.GH_AW_SAFE_OUTPUTS_TOOLS_PATH || `${runnerTemp}/gh-aw/safeoutputs/tools.json`;mount_mcp_as_cli.cjs already has a hard RUNNER_TEMP assignment at the top, so the root fix could also be adding that same constant here.
@copilot please address this.
| // --continue is a CLI concept; in SDK mode retries always restart the session fresh. | ||
| for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { | ||
| if (softTimeoutGuard && Date.now() >= softTimeoutGuard.softDeadlineMs) { | ||
| emitInfrastructureIncomplete(`Copilot harness reached soft retry budget before the ${softTimeoutGuard.timeoutMinutes}-minute step timeout. ` + "Stopping retries early to preserve structured failure output."); |
There was a problem hiding this comment.
[/diagnosing-bugs] emitInfrastructureIncomplete is called twice with identical message strings (one at loop entry, one after backoff sleep). A future wording change would need to be applied in both places.
💡 Suggestion
Extract the message to a constant or a small helper so the two call-sites stay consistent:
function softTimeoutMessage(guard) {
return `Copilot harness reached soft retry budget before the ${guard.timeoutMinutes}-minute step timeout. Stopping retries early to preserve structured failure output.`;
}@copilot please address this.
| // Query tools from the server using the host-accessible URL (mount step runs on host) | ||
| const tools = await fetchMCPTools(url, apiKey, core); | ||
| let tools = await fetchMCPTools(url, apiKey, core); | ||
| if (name === SAFEOUTPUTS_SERVER_NAME) { |
There was a problem hiding this comment.
[/codebase-design] The safeoutputs-specific recovery logic is inlined inside main() via a string comparison (name === SAFEOUTPUTS_SERVER_NAME). This makes main() responsible for knowing which servers need special validation, which will be hard to extend if other servers need similar hardening.
💡 Suggestion
Consider a plugin/callback pattern that lets each server registration declare its own post-fetch validator:
const SERVER_VALIDATORS = {
safeoutputs: (tools, core) => recoverSafeOutputsToolsIfNeeded(tools, core),
};
// In loop:
const validator = SERVER_VALIDATORS[name];
tools = validator ? validator(tools, core) : tools;This keeps the main loop free of server-specific knowledge and makes future additions obvious.
@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("recovers empty safeoutputs schema from fallback tools path", () => { |
There was a problem hiding this comment.
[/tdd] The recovery test doesn't assert that a warning was emitted — it only checks the returned tool list. Without asserting on core.warning, a silent regression (e.g. the warning call being removed) would go undetected.
💡 Suggestion
Capture the warning side-effect in the test. Because ensureSafeOutputsTools currently reads global.core directly, you'd need to either inject core as a parameter (see the first comment) or assert on global.core.warning calls:
const warnings = [];
global.core = { warning: msg => warnings.push(msg) };
const recovered = ensureSafeOutputsTools([], 'safeoutputs', path.join(tempDir, 'empty.json'));
expect(warnings).toHaveLength(1);
expect(warnings[0]).toMatch(/recovered 1 tool/);@copilot please address this.
| // This prevents a broken --continue recovery from resurrecting --continue on the next attempt. | ||
| let continueDisabledPermanently = false; | ||
| const driverStartTime = Date.now(); | ||
| const softTimeoutGuard = buildSoftTimeoutGuard(driverStartTime); |
There was a problem hiding this comment.
[/tdd] There are no integration/behaviour tests verifying that the soft-timeout guard actually causes the retry loop to break with lastExitCode = 1. The unit test only validates the guard's deadline arithmetic — the loop integration (that a guard hit exits the loop and calls emitInfrastructureIncomplete) is untested.
💡 Suggestion
Add a test that mocks Date.now to return a value past the soft deadline and verifies the process exits with code 1 and that the incomplete output was emitted. This is the most important regression test for the feature's observable behaviour.
@copilot please address this.
There was a problem hiding this comment.
Review: Fail fast on empty safeoutputs schema + soft-time convergence guard
Overall this is a solid reliability fix. The two mechanisms (schema fail-fast and soft-time budget) address the root causes of silent timeout failures cleanly, and the test coverage is good.
Three non-blocking suggestions are attached as inline comments:
- Hardcoded magic number in test —
softDeadlineMs: 820000in the soft-timeout guard test silently breaks ifSOFT_TIMEOUT_BUFFER_MSchanges. global.corenull-dereference —ensureSafeOutputsToolsinmcp_cli_bridge.cjsaccessesglobal.corewithout a null guard; a defensive fallback would protect untested call sites.- Duplicated soft-timeout emit — the two identical
emitInfrastructureIncompleteblocks (before attempt and after backoff sleep) could be extracted into a shared helper to avoid drift.
None of these are blocking — the logic is correct and the tests pass. Addressing them would improve maintainability.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 129.7 AIC · ⌖ 6.85 AIC · ⊞ 4.9K
| const guard = buildSoftTimeoutGuard(10_000, { GH_AW_TIMEOUT_MINUTES: "15" }); | ||
| expect(guard).toEqual({ | ||
| timeoutMinutes: 15, | ||
| softDeadlineMs: 820000, |
There was a problem hiding this comment.
The expected value 820000 is a magic number derived from driverStartTime(10_000) + hardTimeoutMs(15×60×1000) − SOFT_TIMEOUT_BUFFER_MS(90×1000). Since SOFT_TIMEOUT_BUFFER_MS is not exported, a future change to the buffer will silently break this test with an opaque mismatch.\n\nConsider either:\n1. Exporting SOFT_TIMEOUT_BUFFER_MS and computing the expected value inline: driverStartTime + 15 * 60 * 1000 - SOFT_TIMEOUT_BUFFER_MS\n2. Or replacing the exact match with range assertions.\n\n@copilot please address this.
| if (serverName !== SAFEOUTPUTS_SERVER_NAME || tools.length > 0) { | ||
| return tools; | ||
| } | ||
| const core = global.core; |
There was a problem hiding this comment.
ensureSafeOutputsTools uses global.core directly after the early-return guard, which means any call for a non-safeoutputs server returns safely, but a call for safeoutputs with an empty tools array will dereference global.core — this will throw a TypeError: Cannot read properties of undefined if the function is called before global.core is set (e.g., in unit tests that don't set up the global, or in a path where bridge is invoked before core initialisation).\n\nThe test in mcp_cli_bridge.test.cjs sets global.core in beforeEach, so test coverage is fine, but adding a guard like const core = global.core ?? { warning: () => {} } would make the function defensively safe.\n\n@copilot please address this.
| // --continue is a CLI concept; in SDK mode retries always restart the session fresh. | ||
| for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { | ||
| if (softTimeoutGuard && Date.now() >= softTimeoutGuard.softDeadlineMs) { | ||
| emitInfrastructureIncomplete(`Copilot harness reached soft retry budget before the ${softTimeoutGuard.timeoutMinutes}-minute step timeout. ` + "Stopping retries early to preserve structured failure output."); |
There was a problem hiding this comment.
There are two identical calls to emitInfrastructureIncomplete + break (one at line ~897 before the attempt, one at line ~912 after backoff sleep). Both share the same message string. If the message ever changes, it needs to be updated in two places.\n\nConsider extracting this into a small helper or labelled-break to deduplicate:\n\njs\nfunction triggerSoftTimeout(guard) {\n emitInfrastructureIncomplete(\n `Copilot harness reached soft retry budget before the ${guard.timeoutMinutes}-minute step timeout. ` +\n "Stopping retries early to preserve structured failure output."\n );\n log(`soft-timeout guard reached: timeoutMinutes=${guard.timeoutMinutes} bufferMs=${SOFT_TIMEOUT_BUFFER_MS}`);\n lastExitCode = 1;\n}\n\n\n@copilot please address this.
|
@copilot please run the
|
Triage Result
Rationale: Critical reliability fix for systemic scheduled-run timeouts. Fails fast on empty safeoutputs schema and adds soft-time convergence guard before the 15-min hard cap. High impact (prevents silent run losses), high urgency (active prod issue), 6 focused files. Prioritize for expedited human review.
|
Fixed in c75f85c — |
|
Great work on this one! 🎉 The fix targets a real and painful failure mode — scheduled runs hitting the hard 15-minute step cap without producing any safe-output artifacts is exactly the kind of silent failure that's hard to diagnose after the fact. The PR looks ready for review:
This looks aligned and well-executed. 🚀
|
|
@copilot please run the Outstanding review follow-ups:
Run: https://github.com/github/gh-aw/actions/runs/28632917404
|
…, deduplicate soft-timeout block Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in ce1a77d:
|
|
@copilot refactor as a general purpose harness helper/functionality that all harnesses can reuse |
…se across harnesses Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. |
|
@copilot apply soft timeouts to other agentic engines harnesses |
|
@copilot please run the
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — |
…g assertion, soft-timeout test Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done — addressed in the latest commit:
|
|
🎉 This pull request is included in a new release. Release: |
Scheduled Copilot runs were hitting the 15-minute hard step timeout after thrashing, often without emitting any safe-output. A key contributor was intermittent empty
safeoutputstool schema resolution, which left the agent unable to discover safe-output tools and spiraling into retry loops.Bridge/schema hardening (
safeoutputs)safeoutputsin MCP CLI mounting and bridge startup paths.tools/listreturns empty forsafeoutputs, the code now re-derives fromGH_AW_SAFE_OUTPUTS_TOOLS_PATH(safeoutputs/tools.json).Copilot convergence guard before hard timeout
copilot_harness.cjsderived fromGH_AW_TIMEOUT_MINUTESwith a pre-timeout buffer.report_incomplete(reason: infrastructure_error) so runs degrade explicitly instead of being killed by Actions hard timeout.Failure signal quality